diff --git a/lib/Basic/StringExtras.cpp b/lib/Basic/StringExtras.cpp index 4103fef57e9..e58a54e5bb3 100644 --- a/lib/Basic/StringExtras.cpp +++ b/lib/Basic/StringExtras.cpp @@ -820,26 +820,18 @@ StringRef camel_case::toLowercaseInitialisms(StringRef string, return scratch.copyString(scratchStr); } -/// Determine whether the given word preceding "with" indicates that -/// "with" should be retained. -static bool wordPairsWithWith(StringRef word) { - return camel_case::sameWordIgnoreFirstCase(word, "compatible"); -} - /// Determine whether the given word occurring before the given /// preposition results in a conflict that suppresses preposition /// splitting. static bool wordConflictsBeforePreposition(StringRef word, StringRef preposition) { - if (camel_case::sameWordIgnoreFirstCase(preposition, "with")) { - if (camel_case::sameWordIgnoreFirstCase(word, "encode")) - return true; + if (camel_case::sameWordIgnoreFirstCase(preposition, "with") && + camel_case::sameWordIgnoreFirstCase(word, "compatible")) + return true; - if (camel_case::sameWordIgnoreFirstCase(word, "copy")) - return true; - - return false; - } + if (camel_case::sameWordIgnoreFirstCase(preposition, "of") && + camel_case::sameWordIgnoreFirstCase(word, "kind")) + return true; return false; } @@ -855,27 +847,78 @@ static bool wordConflictsAfterPreposition(StringRef word, return true; } - if (camel_case::sameWordIgnoreFirstCase(word, "visible") && - camel_case::sameWordIgnoreFirstCase(preposition, "to")) + if (camel_case::sameWordIgnoreFirstCase(preposition, "to") && + camel_case::sameWordIgnoreFirstCase(word, "visible")) return true; return false; } -/// Determine whether there is a preposition in the given name. -static bool containsPreposition(StringRef name) { - for (auto word : camel_case::getWords(name)) { - if (camel_case::sameWordIgnoreFirstCase(word, "with")) continue; - if (getPartOfSpeech(word) == PartOfSpeech::Preposition) return true; +/// When splitting based on a preposition, whether we should place the +/// preposition on the argument label (vs. on the base name). +static bool shouldPlacePrepositionOnArgLabel(StringRef beforePreposition, + StringRef preposition, + StringRef afterPreposition) { + // X/Y/Z often used as coordinates and should be the labels. + if (afterPreposition == "X" || + afterPreposition == "Y" || + afterPreposition == "Z") + return false; + + return true; +} + +/// Determine whether the preposition in a split is "vacuous", and +/// should be removed. +static bool isVacuousPreposition(StringRef beforePreposition, + StringRef preposition, + StringRef afterPreposition, + const OmissionTypeName ¶mType) { + // Only consider "with" or "using" to be potentially vacuous. + if (!camel_case::sameWordIgnoreFirstCase(preposition, "with") && + !camel_case::sameWordIgnoreFirstCase(preposition, "using")) + return false; + + // If the preposition is "with", check for special cases. + if (camel_case::sameWordIgnoreFirstCase(preposition, "with")) { + // Some words following the preposition indicate that "with" is + // not vacuous. + auto following = camel_case::getFirstWord(afterPreposition); + if (camel_case::sameWordIgnoreFirstCase(following, "coder") || + camel_case::sameWordIgnoreFirstCase(following, "zone")) + return false; + + // If the last word of the argument label looks like a past + // participle (ends in "-ed"), the preposition is not vacuous. + auto lastWord = camel_case::getLastWord(afterPreposition); + if (lastWord.endswith("ed")) + return false; + + if (camel_case::sameWordIgnoreFirstCase(following, "delegate") || + camel_case::sameWordIgnoreFirstCase(following, "frame")) + return true; } + + // If the parameter has a default argument, it's vacuous. + if (paramType.hasDefaultArgument()) return true; + + // If the parameter is of function type, it's vacuous. + if (paramType.isFunction()) return true; + + // If the first word of the name is a verb, the preposition is + // likely vacuous. + if (getPartOfSpeech(camel_case::getFirstWord(beforePreposition)) + == PartOfSpeech::Verb) + return true; + return false; } /// Split the base name after the last preposition, if there is one. -static bool splitBaseNameAfterLastPreposition(StringRef &baseName, - StringRef &argName, - const OmissionTypeName ¶mType, - StringRef secondArgName) { +static bool splitBaseNameAfterLastPreposition( + StringRef &baseName, + StringRef &argName, + const OmissionTypeName ¶mType) { // Scan backwards for a preposition. auto nameWords = camel_case::getWords(baseName); auto nameWordRevIter = nameWords.rbegin(), @@ -917,36 +960,21 @@ static bool splitBaseNameAfterLastPreposition(StringRef &baseName, return false; // Determine whether we should drop the preposition. - bool dropPreposition = false; + StringRef beforePreposition(baseName.begin(), + preposition.begin() - baseName.begin()); + StringRef afterPreposition(preposition.end(), + baseName.end() - preposition.end()); + bool dropPreposition = isVacuousPreposition(beforePreposition, + preposition, + afterPreposition, + paramType); - // If the first parameter has a default, drop the preposition. - if (paramType.hasDefaultArgument()) { - dropPreposition = true; - - // If the preposition is "with" and the base name starts with a - // verb, assume "with" is a separator and remove it. - } else if (nameWordRevIter.base().getPosition() > 4 && - camel_case::sameWordIgnoreFirstCase(preposition, "with") && - !wordPairsWithWith(*std::next(nameWordRevIter)) && - getPartOfSpeech(camel_case::getFirstWord(baseName)) - == PartOfSpeech::Verb) { - dropPreposition = true; - - // If the preposition is "using" and the parameter is a function or - // block, assume "using" is a separator and remove it. - } else if (nameWordRevIter.base().getPosition() > 5 && - paramType.isFunction() && - camel_case::sameWordIgnoreFirstCase(preposition, "using")) { - dropPreposition = true; - } - - // If we have a preposition in the second argument label, pull the - // first preposition into the first argument label to balance them. - bool prepositionOnArgLabel = false; - if (containsPreposition(secondArgName)) { - prepositionOnArgLabel = true; + // By default, put the prposition on the argument label. + bool prepositionOnArgLabel = + shouldPlacePrepositionOnArgLabel(beforePreposition, preposition, + afterPreposition); + if (prepositionOnArgLabel) ++nameWordRevIter; - } unsigned startOfArgumentLabel = nameWordRevIter.base().getPosition(); unsigned endOfBaseName = startOfArgumentLabel; @@ -990,8 +1018,7 @@ static bool splitBaseNameAfterLastPreposition(StringRef &baseName, /// Split the base name, if it makes sense. static bool splitBaseName(StringRef &baseName, StringRef &argName, const OmissionTypeName ¶mType, - StringRef paramName, - StringRef secondArgName) { + StringRef paramName) { // If there is already an argument label, do nothing. if (!argName.empty()) return false; @@ -1014,20 +1041,12 @@ static bool splitBaseName(StringRef &baseName, StringRef &argName, return false; // Try splitting after the last preposition. - if (splitBaseNameAfterLastPreposition(baseName, argName, paramType, - secondArgName)) + if (splitBaseNameAfterLastPreposition(baseName, argName, paramType)) return true; return false; } -// Retrieve the second "real" argument label. -static StringRef getSecondArgumentName(ArrayRef names) { - if (names.empty()) return ""; - if (names[0] == "error") return getSecondArgumentName(names.slice(1)); - return names[0]; -} - bool swift::omitNeedlessWords(StringRef &baseName, MutableArrayRef argNames, StringRef firstParamName, @@ -1102,8 +1121,7 @@ bool swift::omitNeedlessWords(StringRef &baseName, // If needed, split the base name. if (!argNames.empty() && - splitBaseName(baseName, argNames[0], paramTypes[0], firstParamName, - getSecondArgumentName(argNames.slice(1)))) + splitBaseName(baseName, argNames[0], paramTypes[0], firstParamName)) anyChanges = true; // Omit needless words based on parameter types. diff --git a/test/IDE/Inputs/custom-modules/OmitNeedlessWords.h b/test/IDE/Inputs/custom-modules/OmitNeedlessWords.h index f2ea2be4cfe..bcaefa166de 100644 --- a/test/IDE/Inputs/custom-modules/OmitNeedlessWords.h +++ b/test/IDE/Inputs/custom-modules/OmitNeedlessWords.h @@ -9,4 +9,5 @@ -(void)removeWithNoRemorse:(nonnull id)object; -(void)bookmarkWithURLs:(nonnull NSArray *)urls; -(void)saveToURL:(nonnull NSURL *)url forSaveOperation:(NSInteger)operation; +-(void)indexWithItemNamed:(nonnull NSString *)name; @end diff --git a/test/IDE/print_omit_needless_words.swift b/test/IDE/print_omit_needless_words.swift index 3f75d085b27..8a21a415081 100644 --- a/test/IDE/print_omit_needless_words.swift +++ b/test/IDE/print_omit_needless_words.swift @@ -35,7 +35,7 @@ // CHECK-FOUNDATION: func makeObjectsPerform(_: Selector, withObject: AnyObject?, withObject: AnyObject?) // Note: id -> "Object". -// CHECK-FOUNDATION: func indexOf(_: AnyObject) -> Int +// CHECK-FOUNDATION: func index(of _: AnyObject) -> Int // Note: Class -> "Class" // CHECK-OBJECTIVEC: func isKindOf(aClass: AnyClass) -> Bool @@ -43,11 +43,11 @@ // Note: Pointer-to-struct name matching; "with" splits the first // piece, then the "with" is dropped. // -// CHECK-FOUNDATION: func copyWith(_: Zone = nil) -> AnyObject! +// CHECK-FOUNDATION: func copy(withZone _: Zone = nil) -> AnyObject! // Note: Objective-C type parameter names. -// CHECK-FOUNDATION: func objectFor(_: Copying) -> AnyObject? -// CHECK-FOUNDATION: func removeObjectFor(_: Copying) +// CHECK-FOUNDATION: func object(forKey _: Copying) -> AnyObject? +// CHECK-FOUNDATION: func removeObject(forKey _: Copying) // Note: Don't drop the name of the first parameter in an initializer entirely. // CHECK-FOUNDATION: init(array: [AnyObject]) @@ -112,8 +112,8 @@ // Note: By --> . // CHECK-FOUNDATION: func withString(_: String) -> String -// Note: Not splitting on "With". -// CHECK-FOUNDATION: func urlWith(addedString _: String) -> URL? +// Note: Noun phrase puts preposition inside. +// CHECK-FOUNDATION: func url(withAddedString _: String) -> URL? // Note: CalendarUnits is not a set of "Options". // CHECK-FOUNDATION: class func forCalendarUnits(_: CalendarUnit) -> String! @@ -207,19 +207,19 @@ // CHECK-APPKIT: func getRGBAComponents(_: UnsafeMutablePointer) // Note: Skipping over "3D" -// CHECK-APPKIT: func drawInAirAt(_: Point3D) +// CHECK-APPKIT: func drawInAir(at _: Point3D) // Note: with -> -// CHECK-APPKIT: func drawAt(_: Point3D, withAttributes: [String : AnyObject]? = [:]) +// CHECK-APPKIT: func draw(at _: Point3D, withAttributes: [String : AnyObject]? = [:]) // Note: Don't strip names that aren't preceded by a verb or preposition. // CHECK-APPKIT: func setTextColor(_: NSColor?) // Note: Splitting with default arguments. -// CHECK-APPKIT: func drawIn(_: NSView?) +// CHECK-APPKIT: func draw(in _: NSView?) // Note: NSDictionary default arguments for "options" -// CHECK-APPKIT: func drawAnywhereIn(_: NSView?, options: [Object : AnyObject] = [:]) +// CHECK-APPKIT: func drawAnywhere(in _: NSView?, options: [Object : AnyObject] = [:]) // CHECK-APPKIT: func drawAnywhere(options _: [Object : AnyObject] = [:]) // Note: no lowercasing of initialisms when there might be a prefix. @@ -245,7 +245,7 @@ // CHECK-APPKIT: func dismiss(animated _: Bool) -// CHECK-APPKIT: func shouldCollapseAutoExpandedItemsFor(deposited _: Bool) -> Bool +// CHECK-APPKIT: func shouldCollapseAutoExpandedItems(forDeposited _: Bool) -> Bool // Introducing argument labels and pruning the base name. // CHECK-APPKIT: func rectForCancelButtonWhenCentered(_: Bool) @@ -256,25 +256,26 @@ // CHECK-APPKIT: func setContentHuggingPriority(_: NSLayoutPriority) // Look through typedefs of pointers. -// CHECK-APPKIT: func layoutAt(_: NSPointPointer) +// CHECK-APPKIT: func layout(at _: NSPointPointer) // The presence of a property prevents us from stripping redundant // type information from the base name. // CHECK-APPKIT: func addGestureRecognizer(_: NSGestureRecognizer) // CHECK-APPKIT: func removeGestureRecognizer(_: NSGestureRecognizer) -// CHECK-APPKIT: func favoriteViewFor(_: NSGestureRecognizer) -> NSView? +// CHECK-APPKIT: func favoriteView(forGestureRecognizer _: NSGestureRecognizer) -> NSView? // CHECK-APPKIT: func addLayoutConstraints(_: Set) // CHECK-APPKIT: func add(_: Rect) // CHECK-APPKIT: class func conjureRect(_: Rect) -// CHECK-OMIT-NEEDLESS-WORDS: func jumpTo(_: URL) +// CHECK-OMIT-NEEDLESS-WORDS: func jump(to _: URL) // CHECK-OMIT-NEEDLESS-WORDS: func objectIsCompatibleWith(_: AnyObject) -> Bool // CHECK-OMIT-NEEDLESS-WORDS: func insetBy(x _: Int, y: Int) // CHECK-OMIT-NEEDLESS-WORDS: func setIndirectlyToValue(_: AnyObject) // CHECK-OMIT-NEEDLESS-WORDS: func jumpToTop(_: AnyObject) // CHECK-OMIT-NEEDLESS-WORDS: func removeWithNoRemorse(_: AnyObject) -// CHECK-OMIT-NEEDLESS-WORDS: func bookmarkWith(_: [URL]) +// CHECK-OMIT-NEEDLESS-WORDS: func bookmark(withURLs _: [URL]) // CHECK-OMIT-NEEDLESS-WORDS: func save(to _: URL, forSaveOperation: Int) +// CHECK-OMIT-NEEDLESS-WORDS: func index(withItemNamed _: String) // Don't drop the 'error'. // CHECK-ERRORS: func tryAndReturnError(_: ()) throws diff --git a/test/Sema/omit_needless_words.swift b/test/Sema/omit_needless_words.swift index aa432853535..23d8f292359 100644 --- a/test/Sema/omit_needless_words.swift +++ b/test/Sema/omit_needless_words.swift @@ -3,7 +3,7 @@ class C1 { init(tasteString: String) { } // expected-warning{{'init(tasteString:)' could be named 'init(taste:)'}}{{8-8=taste }} func processWithString(string: String, toInt: Int) { } // expected-warning{{'processWithString(_:toInt:)' could be named 'process(withString:to:)'}}{{8-25=process}}{{42-42=to }} - func processWithInt(value: Int) { } // expected-warning{{'processWithInt' could be named 'processWith'}}{{8-22=processWith}} + func processWithInt(value: Int) { } // expected-warning{{'processWithInt' could be named 'process(withInt:)'}}{{8-22=process}} } extension String { @@ -14,7 +14,7 @@ extension String { func callSites(s: String) { let c1 = C1(tasteString: "blah") // expected-warning{{'init(tasteString:)' could be named 'init(taste:)'}}{{15-26=taste}} c1.processWithString("a", toInt: 1) // expected-warning{{'processWithString(_:toInt:)' could be named 'process(withString:to:)'}}{{6-23=process}}{{29-34=to}} - c1.processWithInt(5) // expected-warning{{'processWithInt' could be named 'processWith'}}{{6-20=processWith}} + c1.processWithInt(5) // expected-warning{{'processWithInt' could be named 'process(withInt:)'}}{{6-20=process}} _ = String.randomString // expected-warning{{'randomString' could be named 'random'}}{{14-26=random}} _ = s.wonkycasedString // expected-warning{{'wonkycasedString' could be named 'wonkycased'}}{{9-25=wonkycased}} }