Omit needless words: remove arguments that match the default arguments.

For cases where the Clang importer provides a defaulted argument,
e.g., "[]" for option sets and "nil" for optionals, remove the
corresponding arguments at any call sites that simply specify "[]" or
"nil". Such arguments are basically noise, and tend to harm
readability when there are low-content argument labels like "with:" or
"for".

Some examples from Lister:

  self.updateUserActivity(AppConfiguration.UserActivity.watch,
                          userInfo: userInfo, webpageURL: nil)

becomes

  self.updateUserActivity(AppConfiguration.UserActivity.watch,
                          userInfo: userInfo)

and

  contentView.hitTest(tapLocation, with: nil)

becomes

  contentView.hitTest(tapLocation)

and

  document.closeWithCompletionHandler(nil)

becomes simply

  document.close()

and a whole pile of optional "completion handler" arguments go away.

Swift SVN r31978

include/swift/AST/DiagnosticsSema.def

@@ -2633,6 +2633,10 @@ WARNING(variable_never_read, sema_varusage, none,
 WARNING(omit_needless_words, sema_tcd, none,
         "%0 could be named %1 [-Womit-needless-words]", (DeclName, DeclName))
 
+WARNING(extraneous_default_args_in_call, sema_tcd, none,
+        "call to %0 has extraneous arguments that could use defaults", 
+        (DeclName))
+
 #ifndef DIAG_NO_UNDEF
 # if defined(DIAG)
 #  undef DIAG

lib/AST/ASTPrinter.cpp

@@ -1475,10 +1475,10 @@ namespace swift {
     // For optional types, the default is "nil".
     if (type->getOptionalObjectType()) return StringRef("nil");
 
-    // For option sets, the default is "[]".
     if (auto structType = type->getAs<StructType>()) {
       auto structDecl = structType->getDecl();
       if (structDecl->getClangDecl()) {
+        // For option sets, the default is "[]".
         for (auto attr : structDecl->getAttrs()) {
           if (auto synthesizedProto = dyn_cast<SynthesizedProtocolAttr>(attr)) {
             if (synthesizedProto->getProtocolKind()

lib/Basic/PartsOfSpeech.def

@@ -432,7 +432,6 @@ VERB(lie)
 VERB(lie)
 VERB(lighten)
 VERB(like)
-VERB(list)
 VERB(listen)
 VERB(live)
 VERB(load)

lib/Basic/StringExtras.cpp

@@ -444,8 +444,9 @@ StringRef swift::omitNeedlessWords(StringRef name, OmissionTypeName typeName,
   if ((role == NameRole::FirstParameter ||
        role == NameRole::SubsequentParameter) &&
       (name == "usingBlock" || name == "withBlock") &&
-      typeName.Name == "Block")
+      typeName.Name == "Block") {
     return "body";
+  }
 
   // Get the camel-case words in the name and type name.
   auto nameWords = camel_case::getWords(name);

lib/Sema/MiscDiagnostics.cpp

@@ -1977,6 +1977,217 @@ void TypeChecker::checkOmitNeedlessWords(VarDecl *var) {
     .fixItReplace(var->getLoc(), newName->str());
 }
 
+namespace swift {
+// FIXME: Hackily defined in AST/ASTPrinter.cpp
+  Optional<StringRef> getFakeDefaultArgForType(Type type);
+}
+
+/// Determine the "fake" default argument provided by the given expression.
+static Optional<StringRef> getFakeDefaultArgForExpr(Expr *expr) {
+  // Empty array literals, [].
+  if (auto arrayExpr = dyn_cast<ArrayExpr>(expr)) {
+    if (arrayExpr->getElements().empty())
+      return StringRef("[]");
+
+    return None;
+  }
+
+  // nil.
+  if (auto call = dyn_cast<CallExpr>(expr)) {
+    if (auto ctorRefCall = dyn_cast<ConstructorRefCallExpr>(call->getFn())) {
+      if (auto ctorRef = dyn_cast<DeclRefExpr>(ctorRefCall->getFn())) {
+        if (auto ctor = dyn_cast<ConstructorDecl>(ctorRef->getDecl())) {
+          if (ctor->getFullName().getArgumentNames().size() == 1 &&
+              ctor->getFullName().getArgumentNames()[0]
+                == ctor->getASTContext().Id_nilLiteral)
+            return StringRef("nil");
+        }
+      }
+    }
+  }
+
+  return None;
+}
+
+namespace {
+  struct CallEdit {
+    enum {
+      RemoveDefaultArg,
+      Rename,
+    } Kind;
+
+    // The source range affected by this change.
+    SourceRange Range;
+
+    // The replacement text, for a rename.
+    std::string Name;
+  };
+
+}
+
+/// Find the source ranges of extraneous default arguments within a
+/// call to the given function.
+static bool hasExtraneousDefaultArguments(
+              AbstractFunctionDecl *afd,
+              Expr *arg,
+              DeclName name,
+              SmallVectorImpl<SourceRange> &ranges,
+              SmallVectorImpl<unsigned> &removedArgs) {
+  if (!afd->getClangDecl())
+    return false;
+
+  if (auto shuffle = dyn_cast<TupleShuffleExpr>(arg))
+    arg = shuffle->getSubExpr()->getSemanticsProvidingExpr();
+    
+  TupleExpr *argTuple = dyn_cast<TupleExpr>(arg);
+  ParenExpr *argParen = dyn_cast<ParenExpr>(arg);
+
+  ArrayRef<Pattern *> bodyPatterns = afd->getBodyParamPatterns();
+
+  // Skip over the implicit 'self'.
+  if (afd->getImplicitSelfDecl()) {
+    bodyPatterns = bodyPatterns.slice(1);
+  }
+    
+  ASTContext &ctx = afd->getASTContext();
+  Pattern *bodyPattern = bodyPatterns[0];
+  if (auto *tuple = dyn_cast<TuplePattern>(bodyPattern)) {
+    Optional<unsigned> firstRemoved;
+    Optional<unsigned> lastRemoved;
+    unsigned numElementsInParens;
+    if (argTuple) {
+      numElementsInParens = (argTuple->getNumElements() -
+                             argTuple->hasTrailingClosure());
+    } else if (argParen) {
+      numElementsInParens = 1 - argParen->hasTrailingClosure();
+    } else {
+      numElementsInParens = 0;
+    }
+
+    for (unsigned i = 0; i != numElementsInParens; ++i) {
+      auto &elt = tuple->getElements()[i];
+      auto defaultArg = getFakeDefaultArgForType(elt.getPattern()->getType());
+      if (!defaultArg)
+        continue;
+
+      // Never consider removing the first argument for a "set" method
+      // with an unnamed first argument.
+      if (i == 0 &&
+          !name.getBaseName().empty() &&
+          camel_case::getFirstWord(name.getBaseName().str()) == "set" &&
+          name.getArgumentNames().size() > 0 &&
+          name.getArgumentNames()[0].empty())
+        continue;
+
+      SourceRange removalRange;
+      if (argTuple && i < argTuple->getNumElements()) {
+        // Check whether we have a default argument.
+        auto exprArg = getFakeDefaultArgForExpr(argTuple->getElement(i));
+        if (!exprArg || *defaultArg != *exprArg)
+          continue;
+
+        // Figure out where to start removing this argument.
+        if (i == 0) {
+          // Start removing right after the opening parenthesis.
+          removalRange.Start = argTuple->getLParenLoc();
+        } else {
+          // Start removing right after the preceding argument, so we
+          // consume the comma as well.
+          removalRange.Start = argTuple->getElement(i-1)->getEndLoc();
+        }
+
+        // Adjust to the end of the starting token.
+        removalRange.Start
+          = Lexer::getLocForEndOfToken(ctx.SourceMgr, removalRange.Start);
+
+        // Figure out where to finish removing this element.
+        if (i == 0 && i < numElementsInParens - 1) {
+          // We're the first of several arguments; consume the
+          // following comma as well.
+          removalRange.End = argTuple->getElementNameLoc(i+1);
+          if (removalRange.End.isInvalid())
+            removalRange.End = argTuple->getElement(i+1)->getStartLoc();
+        } else if (i < numElementsInParens - 1) {
+          // We're in the middle; consume through the end of this
+          // element.
+          removalRange.End
+            = Lexer::getLocForEndOfToken(ctx.SourceMgr,
+                                         argTuple->getElement(i)->getEndLoc());
+        } else {
+          // We're at the end; consume up to the closing parentheses.
+          removalRange.End = argTuple->getRParenLoc();
+        }
+      } else if (argParen) {
+        // Check whether we have a default argument.
+        auto exprArg = getFakeDefaultArgForExpr(argParen->getSubExpr());
+        if (!exprArg || *defaultArg != *exprArg)
+          continue;
+
+        removalRange = SourceRange(argParen->getSubExpr()->getStartLoc(),
+                                   argParen->getRParenLoc());
+      } else {
+        continue;
+      }
+
+      if (removalRange.isInvalid())
+        continue;
+
+      // Note that we're removing this argument.
+      removedArgs.push_back(i);
+
+      // If we hadn't removed anything before, this is the first
+      // removal.
+      if (!firstRemoved) {
+        ranges.push_back(removalRange);
+        firstRemoved = i;
+        lastRemoved = i;
+        continue;
+      }
+
+      // If the previous removal range was the previous argument,
+      // combine the ranges.
+      if (*lastRemoved == i - 1) {
+        ranges.back().End = removalRange.End;
+        lastRemoved = i;
+        continue;
+      }
+
+      // Otherwise, add this new removal range.
+      ranges.push_back(removalRange);
+      lastRemoved = i;
+    }
+
+    // If there is a single removal range that covers everything but
+    // the trailing closure at the end, also zap the parentheses.
+    if (ranges.size() == 1 &&
+        *firstRemoved == 0 && *lastRemoved == tuple->getNumElements() - 2 &&
+        argTuple && argTuple->hasTrailingClosure()) {
+      ranges.front().Start = argTuple->getLParenLoc();
+      ranges.front().End
+        = Lexer::getLocForEndOfToken(ctx.SourceMgr, argTuple->getRParenLoc());
+    }
+  } else if (argParen) {
+    // Parameter must have a default argument.
+    auto defaultArg = getFakeDefaultArgForType(bodyPattern->getType());
+    if (!defaultArg)
+      return false;
+
+    // Argument must be a default value.
+    auto argExpr = getFakeDefaultArgForExpr(argParen->getSubExpr());
+    if (!argExpr || *argExpr != *defaultArg)
+      return false;
+    
+    SourceRange removalRange(argParen->getSubExpr()->getStartLoc(),
+                             argParen->getRParenLoc());
+    if (!removalRange.isInvalid()) {
+      ranges.push_back(removalRange);
+      removedArgs.push_back(0);
+    }
+  }
+
+  return !ranges.empty();
+}
+
 void TypeChecker::checkOmitNeedlessWords(ApplyExpr *apply) {
   if (!Context.LangOpts.WarnOmitNeedlessWords)
     return;
@@ -1989,6 +2200,8 @@ void TypeChecker::checkOmitNeedlessWords(ApplyExpr *apply) {
     innermostApply = fnApply;
     ++numApplications;
   }
+  if (numApplications != 1)
+    return;
 
   DeclRefExpr *fnRef
     = dyn_cast<DeclRefExpr>(innermostApply->getFn()->getValueProvidingExpr());
@@ -2001,26 +2214,45 @@ void TypeChecker::checkOmitNeedlessWords(ApplyExpr *apply) {
 
   // Determine whether the callee has any needless words in it.
   auto newName = ::omitNeedlessWords(afd);
-  if (!newName)
+
+  bool renamed;
+  if (!newName) {
+    newName = afd->getFullName();
+    renamed = false;
+  } else {
+    renamed = true;
+  }
+
+  // Determine whether there are any extraneous default arguments to be zapped.
+  SmallVector<SourceRange, 2> removedDefaultArgRanges;
+  SmallVector<unsigned, 2> removedArgs;
+  bool anyExtraneousDefaultArgs
+    = hasExtraneousDefaultArguments(afd, apply->getArg(), *newName,
+                                    removedDefaultArgRanges,
+                                    removedArgs);
+
+  if (!renamed && !anyExtraneousDefaultArgs)
     return;
 
   // Make sure to apply the fix at the right application level.
   auto name = afd->getFullName();
   bool argNamesChanged = newName->getArgumentNames() != name.getArgumentNames();
-  if (argNamesChanged && numApplications != 1)
-    return;
-  else if (!argNamesChanged && numApplications != 0)
-    return;
 
   // Dig out the argument tuple.
-  Expr *arg = apply->getArg()->getSemanticsProvidingExpr();
+  Expr *arg = apply->getArg();
   if (auto shuffle = dyn_cast<TupleShuffleExpr>(arg))
     arg = shuffle->getSubExpr()->getSemanticsProvidingExpr();
   TupleExpr *argTuple = dyn_cast<TupleExpr>(arg);
+  ParenExpr *argParen = dyn_cast<ParenExpr>(arg);
 
-  InFlightDiagnostic diag = diagnose(fnRef->getLoc(),
+  if (argParen && !argTuple)
-                                     diag::omit_needless_words,
+    arg = argParen->getSubExpr();
-                                     name, *newName);
+
+  InFlightDiagnostic diag
+    = renamed ? diagnose(fnRef->getLoc(), diag::omit_needless_words,
+                         name, *newName)
+              : diagnose(fnRef->getLoc(), diag::extraneous_default_args_in_call,
+                         name);
 
   // Fix the base name.
   if (newName->getBaseName() != name.getBaseName()) {
@@ -2028,35 +2260,50 @@ void TypeChecker::checkOmitNeedlessWords(ApplyExpr *apply) {
   }
 
   // Fix the argument names.
-  if (argNamesChanged) {
+  auto oldArgNames = name.getArgumentNames();
-    auto oldArgNames = name.getArgumentNames();
+  auto newArgNames = newName->getArgumentNames();
-    auto newArgNames = newName->getArgumentNames();
+  unsigned currentRemovedArg = 0;
-    if (argTuple) {
+  if (argTuple) {
-      for (unsigned i = 0, n = newArgNames.size(); i != n; ++i) {
+    for (unsigned i = 0, n = newArgNames.size(); i != n; ++i) {
-        auto newArgName = newArgNames[i];
+      // If this argument was completely removed, don't emit any
-        if (oldArgNames[i] == newArgName) continue;
+      // Fix-Its for it.
-
+      if (currentRemovedArg < removedArgs.size() &&
-        if (i > argTuple->getNumElements()) break;
+          removedArgs[currentRemovedArg] == i) {
-        if (argTuple->getElementName(i) != oldArgNames[i]) continue;
+        ++currentRemovedArg;
-
+        continue;
-        auto nameLoc = argTuple->getElementNameLoc(i);
+      }
-        if (nameLoc.isInvalid()) {
+
-          // Add the argument label.
+      // Check whether the name changed.
-          diag.fixItInsert(argTuple->getElement(i)->getStartLoc(),
+      auto newArgName = newArgNames[i];
-                           (newArgName.str() + ": ").str());
+      if (oldArgNames[i] == newArgName) continue;
-        } else if (newArgName.empty()) {
+
-          // Delete the argument label.
+      if (i >= argTuple->getNumElements()) break;
-          diag.fixItRemoveChars(nameLoc, argTuple->getElement(i)->getStartLoc());
+      if (argTuple->getElementName(i) != oldArgNames[i]) continue;
-        } else {
+
-          // Fix the argument label.
+      auto nameLoc = argTuple->getElementNameLoc(i);
-          diag.fixItReplace(nameLoc, newArgName.str());
+      if (nameLoc.isInvalid()) {
-        }
+        // Add the argument label.
+        diag.fixItInsert(argTuple->getElement(i)->getStartLoc(),
+                         (newArgName.str() + ": ").str());
+      } else if (newArgName.empty()) {
+        // Delete the argument label.
+        diag.fixItRemoveChars(nameLoc, argTuple->getElement(i)->getStartLoc());
+      } else {
+        // Fix the argument label.
+        diag.fixItReplace(nameLoc, newArgName.str());
       }
-    } else if (newArgNames.size() > 0 && !newArgNames[0].empty()) {
-      // Add the argument label.
-      auto newArgName = newArgNames[0];
-      diag.fixItInsert(arg->getStartLoc(), (newArgName.str() + ": ").str());
     }
+  } else if (newArgNames.size() > 0 && !newArgNames[0].empty() &&
+             (!argParen || !argParen->hasTrailingClosure()) &&
+             removedArgs.empty()) {
+    // Add the argument label.
+    auto newArgName = newArgNames[0];
+    diag.fixItInsert(arg->getStartLoc(), (newArgName.str() + ": ").str());
+  }
+
+  // Remove all of the defaulted arguments.
+  for (auto extraneous : removedDefaultArgRanges) {
+    diag.fixItRemoveChars(extraneous.Start, extraneous.End);
   }
 }
 

test/ClangModules/omit_needless_words.swift

@@ -0,0 +1,30 @@
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -parse -verify -Womit-needless-words %s
+
+// REQUIRES: objc_interop
+
+import Foundation
+import AppKit
+
+func dropDefaultedNil(array: NSArray, sel: Selector,
+       body: ((AnyObject!, Int, UnsafeMutablePointer<ObjCBool>) -> Void)?) {
+  array.makeObjectsPerformSelector(sel, withObject: nil) // expected-warning{{'makeObjectsPerformSelector(_:withObject:)' could be named 'makeObjectsPerform(_:with:)'}}{{9-35=makeObjectsPerform}}{{39-56=}}
+  array.makeObjectsPerformSelector(sel, withObject: nil, withObject: nil) // expected-warning{{'makeObjectsPerformSelector(_:withObject:withObject:)' could be named 'makeObjectsPerform(_:with:with:)'}}{{9-35=makeObjectsPerform}}{{39-73=}}
+  array.enumerateObjectsRandomlyWithBlock(nil) // expected-warning{{'enumerateObjectsRandomlyWithBlock' could be named 'enumerateObjectsRandomly(withBlock:)'}}{{9-42=enumerateObjectsRandomly}}{{43-46=}}
+  array.enumerateObjectsRandomlyWithBlock(body) // expected-warning{{'enumerateObjectsRandomlyWithBlock' could be named 'enumerateObjectsRandomly(withBlock:)'}}{{9-42=enumerateObjectsRandomly}}{{43-43=withBlock: }}
+}
+
+func dropDefaultedOptionSet(array: NSArray) {
+  array.enumerateObjectsWithOptions([]) { obj, idx, stop in print("foo") } // expected-warning{{'enumerateObjectsWithOptions(_:usingBlock:)' could be named 'enumerateObjects(with:usingBlock:)'}}{{9-36=enumerateObjects}}{{36-40=}}
+  array.enumerateObjectsWithOptions([], usingBlock: { obj, idx, stop in print("foo") }) // expected-warning{{'enumerateObjectsWithOptions(_:usingBlock:)' could be named 'enumerateObjects(with:usingBlock:)'}}{{9-36=enumerateObjects}}{{37-41=}}
+  array.enumerateObjectsWhileOrderingPizza(true, withOptions: [], usingBlock: { obj, idx, stop in print("foo") }) // expected-warning{{'enumerateObjectsWhileOrderingPizza(_:withOptions:usingBlock:)' could be named 'enumerateObjectsWhileOrderingPizza(_:with:usingBlock:)'}}{{48-65=}}
+}
+
+func dropDefaultedWithoutRename(domain: String, code: Int, array: NSArray) {
+  let _ = NSError(domain: domain, code: code, userInfo: nil) // expected-warning{{call to 'init(domain:code:userInfo:)' has extraneous arguments that could use defaults}}{{45-60=}}
+  array.enumerateObjectsHaphazardly(nil) // expected-warning{{call to 'enumerateObjectsHaphazardly' has extraneous arguments that could use defaults}}{{37-40=}}
+  array.optionallyEnumerateObjects([], usingBlock: { obj, idx, stop in print("foo") }) // expected-warning{{call to 'optionallyEnumerateObjects(_:usingBlock:)' has extraneous arguments that could use defaults}}{{36-40=}}
+}
+
+func dontDropUnnamedSetterArg(str: NSString) {
+  str.setTextColor(nil) // don't drop this
+}

test/IDE/print_omit_needless_words.swift

@@ -98,7 +98,7 @@
 // CHECK-FOUNDATION: func enumerateObjects(with _: NSEnumerationOptions = [], body: ((AnyObject!, Int, UnsafeMutablePointer<ObjCBool>) -> Void)!)
 
 // Note: WithBlock -> body
-// CHECK-FOUNDATION: func enumerateObjectsRandomly(body _: ((AnyObject!, Int, UnsafeMutablePointer<ObjCBool>) -> Void)!)
+// CHECK-FOUNDATION: func enumerateObjectsRandomly(body _: ((AnyObject!, Int, UnsafeMutablePointer<ObjCBool>) -> Void)? = nil)
 
 // Note: class method name stripping result type.
 // CHECK-APPKIT: class func red() -> NSColor
@@ -110,7 +110,7 @@
 // CHECK-APPKIT: func drawInAir(at _: Point3D)
 
 // Note: Don't strip names that aren't preceded by a verb or preposition.
-// CHECK-APPKIT: func setTextColor(_: NSColor)
+// CHECK-APPKIT: func setTextColor(_: NSColor?)
 
 // Note: Skipping over "Ref"
 // CHECK-CORECOOLING: func replace(_: CCPowerSupply!)

test/Inputs/clang-importer-sdk/usr/include/AppKit.h

@@ -249,5 +249,5 @@ extern NSString *NSViewFocusDidChangeNotification;
 struct Point3D { double x, y, z; };
 @interface NSString (Drawing)
 -(void)drawInAirAtPoint:(struct Point3D)point;
--(void)setTextColor:(nonnull NSColor *)color;
+-(void)setTextColor:(nullable NSColor *)color;
 @end

test/Inputs/clang-importer-sdk/usr/include/Foundation.h

@@ -96,6 +96,7 @@ __attribute__((availability(ios,introduced=8.0)))
 + (instancetype)arrayWithObjects:(const ObjectType __nonnull [__nullable])objects count:(NSUInteger)count;
 - (void)makeObjectsPerformSelector:(nonnull SEL)aSelector;
 - (void)makeObjectsPerformSelector:(nonnull SEL)aSelector withObject:(nullable ObjectType)anObject;
+- (void)makeObjectsPerformSelector:(nonnull SEL)aSelector withObject:(nullable ObjectType)anObject withObject:(nullable ObjectType)anotherObject;
 @end
 
 @interface NSArray (AddingObject)
@@ -912,6 +913,21 @@ typedef NS_OPTIONS(NSUInteger, NSEnumerationOptions) {
                          usingBlock:(void (^)(id obj, NSUInteger idx,
                                               BOOL *stop))block;
 
-- (void)enumerateObjectsRandomlyWithBlock:(void (^)(id obj, NSUInteger idx,
+- (void)enumerateObjectsRandomlyWithBlock:(void (^ __nullable)(
-                                                    BOOL *stop))block;
+                                                  id obj,
+                                                  NSUInteger idx,
+                                                  BOOL *stop))block;
+
+- (void)enumerateObjectsHaphazardly:(void (^ __nullable)(id obj,
+                                                         NSUInteger idx,
+                                                         BOOL *stop))block;
+
+- (void)optionallyEnumerateObjects:(NSEnumerationOptions)opts
+                        usingBlock:(void (^)(id obj, NSUInteger idx,
+                                             BOOL *stop))block;
+
+- (void)enumerateObjectsWhileOrderingPizza:(BOOL)pizza
+                         withOptions:(NSEnumerationOptions)opts
+                         usingBlock:(void (^)(id obj, NSUInteger idx,
+                                              BOOL *stop))block;
 @end