Merge pull request #27728 from xedin/port-extraneous-args

[Diagnostics] Diagnose extraneous argument(s) via fixes

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -1122,13 +1122,20 @@ ERROR(extra_argument_named,none,
       "extra argument %0 in call", (Identifier))
 ERROR(extra_argument_positional,none,
       "extra argument in call", ())
+ERROR(extra_arguments_in_call,none,
+      "extra arguments at positions %0 in call", (StringRef))
 ERROR(extra_argument_to_nullary_call,none,
       "argument passed to call that takes no arguments", ())
 ERROR(extra_trailing_closure_in_call,none,
       "extra trailing closure passed in call", ())
 ERROR(trailing_closure_bad_param,none,
       "trailing closure passed to parameter of type %0 that does not "
       "accept a closure", (Type))
+NOTE(candidate_with_extraneous_args,none,
+      "candidate %0 requires %1 argument%s1, "
+      "but %2 %select{were|was}3 %select{provided|used in closure body}4",
+      (Type, unsigned, unsigned, bool, bool))
+
 ERROR(no_accessible_initializers,none,
       "%0 cannot be constructed because it has no accessible initializers",
       (Type))

lib/Sema/CSDiag.cpp

@@ -2155,56 +2155,6 @@ class ArgumentMatcher : public MatchCallArgumentListener {
         ParamInfo(paramInfo), Arguments(args), CandidateInfo(CCI),
         IsSubscript(isSubscript) {}
 
-  void extraArgument(unsigned extraArgIdx) override {
-    auto name = Arguments[extraArgIdx].getLabel();
-    Expr *arg = ArgExpr;
-
-    auto tuple = dyn_cast<TupleExpr>(ArgExpr);
-    if (tuple)
-      arg = tuple->getElement(extraArgIdx);
-
-    auto loc = arg->getLoc();
-    if (tuple && extraArgIdx == tuple->getNumElements() - 1 &&
-        tuple->hasTrailingClosure())
-      TC.diagnose(loc, diag::extra_trailing_closure_in_call)
-          .highlight(arg->getSourceRange());
-    else if (Parameters.empty()) {
-      auto Paren = dyn_cast<ParenExpr>(ArgExpr);
-      Expr *SubExpr = nullptr;
-      if (Paren) {
-        SubExpr = Paren->getSubExpr();
-      }
-
-      if (SubExpr && CandidateInfo.CS.getType(SubExpr) &&
-          CandidateInfo.CS.getType(SubExpr)->isVoid()) {
-        TC.diagnose(loc, diag::extra_argument_to_nullary_call)
-            .fixItRemove(SubExpr->getSourceRange());
-      } else {
-        TC.diagnose(loc, diag::extra_argument_to_nullary_call)
-            .highlight(ArgExpr->getSourceRange());
-      }
-    } else if (name.empty())
-      TC.diagnose(loc, diag::extra_argument_positional)
-          .highlight(arg->getSourceRange());
-    else
-      TC.diagnose(loc, diag::extra_argument_named, name)
-          .highlight(arg->getSourceRange());
-
-    Diagnosed = true;
-  }
-
-  bool missingLabel(unsigned paramIdx) override {
-    return false;
-  }
-
-  bool extraneousLabel(unsigned paramIdx) override {
-    return false;
-  }
-
-  bool incorrectLabel(unsigned paramIdx) override {
-    return false;
-  }
-
   bool outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
     auto &cs = CandidateInfo.CS;
     OutOfOrderArgumentFailure failure(nullptr, cs, argIdx, prevArgIdx, Bindings,

lib/Sema/CSDiagnostics.cpp

@@ -4219,7 +4219,137 @@ bool OutOfOrderArgumentFailure::diagnoseAsError() {
     addFixIts(emitDiagnostic(diagLoc, diag::argument_out_of_order_named_named,
                              first, second));
   }
+  return true;
+}
+
+bool ExtraneousArgumentsFailure::diagnoseAsError() {
+  // Simplified anchor would point directly to the
+  // argument in case of contextual mismatch.
+  auto *anchor = getAnchor();
+  if (auto *closure = dyn_cast<ClosureExpr>(anchor)) {
+    auto fnType = ContextualType;
+    auto params = closure->getParameters();
 
+    auto diag = emitDiagnostic(
+        params->getStartLoc(), diag::closure_argument_list_tuple, fnType,
+        fnType->getNumParams(), params->size(), (params->size() == 1));
+
+    bool onlyAnonymousParams =
+        std::all_of(params->begin(), params->end(),
+                    [](ParamDecl *param) { return !param->hasName(); });
+
+    // If closure expects no parameters but N was given,
+    // and all of them are anonymous let's suggest removing them.
+    if (fnType->getNumParams() == 0 && onlyAnonymousParams) {
+      auto inLoc = closure->getInLoc();
+      auto &sourceMgr = getASTContext().SourceMgr;
+
+      if (inLoc.isValid())
+        diag.fixItRemoveChars(params->getStartLoc(),
+                              Lexer::getLocForEndOfToken(sourceMgr, inLoc));
+    }
+    return true;
+  }
+
+  if (isContextualMismatch()) {
+    auto *locator = getLocator();
+    emitDiagnostic(anchor->getLoc(),
+                   locator->isLastElement<LocatorPathElt::ContextualType>()
+                       ? diag::cannot_convert_initializer_value
+                       : diag::cannot_convert_argument_value,
+                   getType(anchor), ContextualType);
+    return true;
+  }
+
+  if (ExtraArgs.size() == 1) {
+    return diagnoseSingleExtraArgument();
+  }
+
+  if (ContextualType->getNumParams() == 0) {
+    if (auto argExpr = getArgumentListExprFor(getLocator())) {
+      emitDiagnostic(anchor->getLoc(), diag::extra_argument_to_nullary_call)
+          .highlight(argExpr->getSourceRange())
+          .fixItRemove(argExpr->getSourceRange());
+      return true;
+    }
+  }
+
+  if (ExtraArgs.size() < 2)
+    return false;
+
+  llvm::SmallString<64> positions;
+  llvm::raw_svector_ostream OS(positions);
+
+  interleave(
+      ExtraArgs,
+      [&](const std::pair<unsigned, AnyFunctionType::Param> &arg) {
+        OS << "#" << (arg.first + 1);
+      },
+      [&] { OS << ", "; });
+
+  emitDiagnostic(anchor->getLoc(), diag::extra_arguments_in_call, OS.str());
+
+  if (auto overload = getChoiceFor(getLocator())) {
+    if (auto *decl = overload->choice.getDeclOrNull()) {
+      emitDiagnostic(decl, diag::decl_declared_here, decl->getFullName());
+    }
+  }
+
+  return true;
+}
+
+bool ExtraneousArgumentsFailure::diagnoseAsNote() {
+  auto overload = getChoiceFor(getLocator());
+  if (!(overload && overload->choice.isDecl()))
+    return false;
+
+  auto *decl = overload->choice.getDecl();
+  auto *anchor = getAnchor();
+  auto numArgs = getTotalNumArguments();
+  emitDiagnostic(decl, diag::candidate_with_extraneous_args, ContextualType,
+                 ContextualType->getNumParams(), numArgs, (numArgs == 1),
+                 isa<ClosureExpr>(anchor));
+  return true;
+}
+
+bool ExtraneousArgumentsFailure::diagnoseSingleExtraArgument() const {
+  auto *arguments = getArgumentListExprFor(getLocator());
+  if (!arguments)
+    return false;
+
+  const auto &e = ExtraArgs.front();
+  auto index = e.first;
+  auto argument = e.second;
+
+  auto tuple = dyn_cast<TupleExpr>(arguments);
+  auto argExpr = tuple ? tuple->getElement(index)
+                       : cast<ParenExpr>(arguments)->getSubExpr();
+
+  auto loc = argExpr->getLoc();
+  if (tuple && index == tuple->getNumElements() - 1 &&
+      tuple->hasTrailingClosure()) {
+    emitDiagnostic(loc, diag::extra_trailing_closure_in_call)
+        .highlight(argExpr->getSourceRange());
+  } else if (ContextualType->getNumParams() == 0) {
+    auto *PE = dyn_cast<ParenExpr>(arguments);
+    Expr *subExpr = nullptr;
+    if (PE)
+      subExpr = PE->getSubExpr();
+
+    if (subExpr && argument.getPlainType()->isVoid()) {
+      emitDiagnostic(loc, diag::extra_argument_to_nullary_call)
+          .fixItRemove(subExpr->getSourceRange());
+    } else {
+      emitDiagnostic(loc, diag::extra_argument_to_nullary_call)
+          .highlight(argExpr->getSourceRange());
+    }
+  } else if (argument.hasLabel()) {
+    emitDiagnostic(loc, diag::extra_argument_named, argument.getLabel())
+        .highlight(argExpr->getSourceRange());
+  } else {
+    emitDiagnostic(loc, diag::extra_argument_positional)
+        .highlight(argExpr->getSourceRange());
+  }
   return true;
 }
 

lib/Sema/CSDiagnostics.h

@@ -1329,6 +1329,36 @@ class MissingArgumentsFailure final : public FailureDiagnostic {
                                          ConstraintLocator *locator);
 };
 
+class ExtraneousArgumentsFailure final : public FailureDiagnostic {
+  FunctionType *ContextualType;
+  SmallVector<std::pair<unsigned, AnyFunctionType::Param>, 4> ExtraArgs;
+
+public:
+  ExtraneousArgumentsFailure(
+      Expr *root, ConstraintSystem &cs, FunctionType *contextualType,
+      ArrayRef<std::pair<unsigned, AnyFunctionType::Param>> extraArgs,
+      ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator),
+        ContextualType(resolveType(contextualType)->castTo<FunctionType>()),
+        ExtraArgs(extraArgs.begin(), extraArgs.end()) {}
+
+  bool diagnoseAsError() override;
+  bool diagnoseAsNote() override;
+
+private:
+  bool diagnoseSingleExtraArgument() const;
+
+  unsigned getTotalNumArguments() const {
+    return ContextualType->getNumParams() + ExtraArgs.size();
+  }
+
+  bool isContextualMismatch() const {
+    auto *locator = getLocator();
+    return locator->isLastElement<LocatorPathElt::ContextualType>() ||
+           locator->isLastElement<LocatorPathElt::ApplyArgToParam>();
+  }
+};
+
 class OutOfOrderArgumentFailure final : public FailureDiagnostic {
   using ParamBinding = SmallVector<unsigned, 1>;
 

lib/Sema/CSFix.cpp

@@ -518,6 +518,42 @@ AddMissingArguments::create(ConstraintSystem &cs,
   return new (mem) AddMissingArguments(cs, synthesizedArgs, locator);
 }
 
+bool RemoveExtraneousArguments::diagnose(Expr *root, bool asNote) const {
+  ExtraneousArgumentsFailure failure(root, getConstraintSystem(),
+                                     ContextualType, getExtraArguments(),
+                                     getLocator());
+  return failure.diagnose(asNote);
+}
+
+bool RemoveExtraneousArguments::isMinMaxNameShadowing(
+    ConstraintSystem &cs, ConstraintLocatorBuilder locator) {
+  auto *anchor = dyn_cast_or_null<CallExpr>(locator.getAnchor());
+  if (!anchor)
+    return false;
+
+  if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor->getFn())) {
+    if (auto *baseExpr = dyn_cast<DeclRefExpr>(UDE->getBase())) {
+      auto *decl = baseExpr->getDecl();
+      if (baseExpr->isImplicit() && decl &&
+          decl->getFullName() == cs.getASTContext().Id_self) {
+        auto memberName = UDE->getName();
+        return memberName.isSimpleName("min") || memberName.isSimpleName("max");
+      }
+    }
+  }
+
+  return false;
+}
+
+RemoveExtraneousArguments *RemoveExtraneousArguments::create(
+    ConstraintSystem &cs, FunctionType *contextualType,
+    llvm::ArrayRef<IndexedParam> extraArgs, ConstraintLocator *locator) {
+  unsigned size = totalSizeToAlloc<IndexedParam>(extraArgs.size());
+  void *mem = cs.getAllocator().Allocate(size, alignof(RemoveExtraneousArguments));
+  return new (mem)
+      RemoveExtraneousArguments(cs, contextualType, extraArgs, locator);
+}
+
 bool MoveOutOfOrderArgument::diagnose(Expr *root, bool asNote) const {
   OutOfOrderArgumentFailure failure(root, getConstraintSystem(), ArgIdx,
                                     PrevArgIdx, Bindings, getLocator());
@@ -761,8 +797,15 @@ bool AllowTupleSplatForSingleParameter::attempt(
     //
     // We'd want to suggest argument list to be `x: (0, 1)` instead
     // of `(x: 0, 1)` which would be incorrect.
-    if (index == 0 && param.getLabel() == label)
-      label = Identifier();
+    if (param.hasLabel() && label == param.getLabel()) {
+      if (index == 0) {
+        label = Identifier();
+      } else {
+        // If label match anything other than first argument,
+        // this can't be a tuple splat.
+        return true;
+      }
+    }
 
     // Tuple can't have `inout` elements.
     if (flags.isInOut())

lib/Sema/CSFix.h

@@ -150,6 +150,10 @@ enum class FixKind : uint8_t {
   /// by adding new arguments to the list represented as type variables.
   AddMissingArguments,
 
+  /// If there are more arguments than parameters, let's fix that up
+  /// by removing extraneous arguments.
+  RemoveExtraneousArguments,
+
   /// Allow single tuple closure parameter destructuring into N arguments.
   AllowClosureParameterDestructuring,
 
@@ -1019,6 +1023,55 @@ class AddMissingArguments final
   }
 };
 
+class RemoveExtraneousArguments final
+    : public ConstraintFix,
+      private llvm::TrailingObjects<
+          RemoveExtraneousArguments,
+          std::pair<unsigned, AnyFunctionType::Param>> {
+  friend TrailingObjects;
+
+  using IndexedParam = std::pair<unsigned, AnyFunctionType::Param>;
+
+  FunctionType *ContextualType;
+  unsigned NumExtraneous;
+
+  RemoveExtraneousArguments(ConstraintSystem &cs, FunctionType *contextualType,
+                            llvm::ArrayRef<IndexedParam> extraArgs,
+                            ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::RemoveExtraneousArguments, locator),
+        ContextualType(contextualType), NumExtraneous(extraArgs.size()) {
+    std::uninitialized_copy(extraArgs.begin(), extraArgs.end(),
+                            getExtraArgumentsBuf().begin());
+  }
+
+public:
+  std::string getName() const override { return "remove extraneous argument(s)"; }
+
+  ArrayRef<IndexedParam> getExtraArguments() const {
+    return {getTrailingObjects<IndexedParam>(), NumExtraneous};
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  /// FIXME(diagnostics): Once `resolveDeclRefExpr` is gone this
+  /// logic would be obsolete.
+  ///
+  /// Determine whether presence of extraneous arguments indicates
+  /// potential name shadowing problem with local `min`/`max` shadowing
+  /// global definitions with different number of arguments.
+  static bool isMinMaxNameShadowing(ConstraintSystem &cs,
+                                    ConstraintLocatorBuilder locator);
+
+  static RemoveExtraneousArguments *
+  create(ConstraintSystem &cs, FunctionType *contextualType,
+         llvm::ArrayRef<IndexedParam> extraArgs, ConstraintLocator *locator);
+
+private:
+  MutableArrayRef<IndexedParam> getExtraArgumentsBuf() {
+    return {getTrailingObjects<IndexedParam>(), NumExtraneous};
+  }
+};
+
 class MoveOutOfOrderArgument final : public ConstraintFix {
   using ParamBinding = SmallVector<unsigned, 1>;
 

lib/Sema/CSGen.cpp

@@ -1281,8 +1281,9 @@ namespace {
       SmallVector<AnyFunctionType::Param, 8> params;
       AnyFunctionType::decomposeInput(constrParamType, params);
 
+      auto funcType = constr->getMethodInterfaceType()->castTo<FunctionType>();
       ::matchCallArguments(
-          CS, args, params, ConstraintKind::ArgumentConversion,
+          CS, funcType, args, params, ConstraintKind::ArgumentConversion,
           CS.getConstraintLocator(expr, ConstraintLocator::ApplyArgument));
 
       Type result = tv;