[Omit needless words] Split before last preposition in most cases.

Splitting *before* the last preposition tends to keep the
prepositional phrase together. Only leave the preposition on the base
name in rare cases where we would end up with weird argument labels
(e.g., prefer "moveTo(x:y:)" to "move(toX:y:)").

Also, refine our heuristics for when we can remove the preposition
entirely.

Author: DougGregor

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(word, "copy"))
-      return true;
+  if (camel_case::sameWordIgnoreFirstCase(preposition, "with") &&
+      camel_case::sameWordIgnoreFirstCase(word, "compatible"))
+    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 &paramType) {
+  // 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 &paramType,
-                                              StringRef secondArgName) {
+static bool splitBaseNameAfterLastPreposition(
+    StringRef &baseName,
+    StringRef &argName,
+    const OmissionTypeName &paramType) {
   // 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;
-
-  // 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;
+  StringRef beforePreposition(baseName.begin(),
+                              preposition.begin() - baseName.begin());
+  StringRef afterPreposition(preposition.end(),
+                             baseName.end() - preposition.end());
+  bool dropPreposition = isVacuousPreposition(beforePreposition,
+                                              preposition,
+                                              afterPreposition,
+                                              paramType);
+
+  // 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 &paramType,
-                          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<StringRef> names) {
-  if (names.empty()) return "";
-  if (names[0] == "error") return getSecondArgumentName(names.slice(1));
-  return names[0];
-}
-
 bool swift::omitNeedlessWords(StringRef &baseName,
                               MutableArrayRef<StringRef> 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.

test/IDE/Inputs/custom-modules/OmitNeedlessWords.h

@@ -9,4 +9,5 @@
 -(void)removeWithNoRemorse:(nonnull id)object;
 -(void)bookmarkWithURLs:(nonnull NSArray<NSURL *> *)urls;
 -(void)saveToURL:(nonnull NSURL *)url forSaveOperation:(NSInteger)operation;
+-(void)indexWithItemNamed:(nonnull NSString *)name;
 @end

test/IDE/print_omit_needless_words.swift

@@ -35,19 +35,19 @@
 // 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
 
 // 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: <context type>By<gerund> --> <gerund>.
 // 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<Int8>)
 
 // Note: Skipping over "3D"
-// CHECK-APPKIT: func drawInAirAt(_: Point3D)
+// CHECK-APPKIT: func drawInAir(at _: Point3D)
 
 // Note: with<something> -> <something>
-// 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<NSLayoutConstraint>)
 // 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

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}}
 }