Merge pull request #28630 from xedin/dynamic-member-diagnostics

[Diagnostics] Port/Improve diagnostics for `@dynamicCallable` and `callAsFunction`

Author: xedin

docs/TypeChecker.rst

@@ -993,7 +993,6 @@ The things in the queue yet to be ported are:
 
 - Problems related to calls and operator applications e.g.
 
-  - ``@dynamicCallable`` related diagnostics
   - Missing explicit ``Self.`` and ``self.``
   - Logic related to overload candidate ranking (``CalleeCandidateInfo``)
   - ``diagnoseParameterErrors``

lib/Sema/CSDiag.cpp

@@ -2017,25 +2017,21 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
   if (!isUnresolvedOrTypeVarType(fnType) &&
       !fnType->is<AnyFunctionType>() && !fnType->is<MetatypeType>()) {
     auto arg = callExpr->getArg();
-    auto isDynamicCallable =
-        CS.DynamicCallableCache[fnType->getCanonicalType()].isValid();
 
-    auto hasCallAsFunctionMethods = fnType->isCallableNominalType(CS.DC);
-
-    // Diagnose specific @dynamicCallable errors.
-    if (isDynamicCallable) {
-      auto dynamicCallableMethods =
-        CS.DynamicCallableCache[fnType->getCanonicalType()];
-
-      // Diagnose dynamic calls with keywords on @dynamicCallable types that
-      // don't define the `withKeywordArguments` method.
-      if (auto tuple = dyn_cast<TupleExpr>(arg)) {
-        bool hasArgLabel = llvm::any_of(
-          tuple->getElementNames(), [](Identifier i) { return !i.empty(); });
-        if (hasArgLabel &&
-            dynamicCallableMethods.keywordArgumentsMethods.empty()) {
-          diagnose(callExpr->getFn()->getStartLoc(),
-                   diag::missing_dynamic_callable_kwargs_method, fnType);
+    // If the argument is a trailing ClosureExpr (i.e. {....}) and it is on
+    // the line after the callee, then it's likely the user forgot to
+    // write "do" before their brace stmt.
+    // Note that line differences of more than 1 are diagnosed during parsing.
+    if (auto *PE = dyn_cast<ParenExpr>(arg)) {
+      if (PE->hasTrailingClosure() && isa<ClosureExpr>(PE->getSubExpr())) {
+        auto *closure = cast<ClosureExpr>(PE->getSubExpr());
+        auto &SM = CS.getASTContext().SourceMgr;
+        if (closure->hasAnonymousClosureVars() &&
+            closure->getParameters()->size() == 0 &&
+            1 + SM.getLineNumber(callExpr->getFn()->getEndLoc()) ==
+                SM.getLineNumber(closure->getStartLoc())) {
+          diagnose(closure->getStartLoc(), diag::brace_stmt_suggest_do)
+              .fixItInsert(closure->getStartLoc(), "do ");
           return true;
         }
       }
@@ -2046,7 +2042,8 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
       auto diag = diagnose(arg->getStartLoc(),
                            diag::missing_init_on_metatype_initialization);
       diag.highlight(fnExpr->getSourceRange());
-    } else if (!isDynamicCallable) {
+      return true;
+    } else {
       auto diag = diagnose(arg->getStartLoc(),
                            diag::cannot_call_non_function_value, fnType);
       diag.highlight(fnExpr->getSourceRange());
@@ -2059,30 +2056,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
           diag.fixItRemove(arg->getSourceRange());
         }
       }
-    }
-
-    // If the argument is a trailing ClosureExpr (i.e. {....}) and it is on
-    // the line after the callee, then it's likely the user forgot to
-    // write "do" before their brace stmt.
-    // Note that line differences of more than 1 are diagnosed during parsing.
-    if (auto *PE = dyn_cast<ParenExpr>(arg)) {
-      if (PE->hasTrailingClosure() && isa<ClosureExpr>(PE->getSubExpr())) {
-        auto *closure = cast<ClosureExpr>(PE->getSubExpr());
-        auto &SM = CS.getASTContext().SourceMgr;
-        if (closure->hasAnonymousClosureVars() &&
-            closure->getParameters()->size() == 0 &&
-            1 + SM.getLineNumber(callExpr->getFn()->getEndLoc()) ==
-            SM.getLineNumber(closure->getStartLoc())) {
-          diagnose(closure->getStartLoc(), diag::brace_stmt_suggest_do)
-            .fixItInsert(closure->getStartLoc(), "do ");
-        }
-      }
-    }
-
-    // Use the existing machinery to provide more useful diagnostics for
-    // @dynamicCallable calls, rather than cannot_call_non_function_value.
-    if ((isExistentialMetatypeType || !isDynamicCallable) &&
-    	  !hasCallAsFunctionMethods) {
       return true;
     }
   }

lib/Sema/CSDiagnostics.cpp

@@ -121,6 +121,12 @@ Expr *FailureDiagnostic::getBaseExprFor(Expr *anchor) const {
     return SE->getBase();
   else if (auto *MRE = dyn_cast<MemberRefExpr>(anchor))
     return MRE->getBase();
+  else if (auto *call = dyn_cast<CallExpr>(anchor)) {
+    auto fnType = getType(call->getFn());
+    if (fnType->isCallableNominalType(getDC())) {
+      return call->getFn();
+    }
+  }
 
   return nullptr;
 }
@@ -3205,6 +3211,9 @@ bool MissingMemberFailure::diagnoseAsError() {
   if (!anchor || !baseExpr)
     return false;
 
+  if (diagnoseForDynamicCallable())
+    return true;
+
   auto baseType = resolveType(getBaseType())->getWithoutSpecifierType();
 
   DeclNameLoc nameLoc(anchor->getStartLoc());
@@ -3376,6 +3385,26 @@ bool MissingMemberFailure::diagnoseAsError() {
   return true;
 }
 
+bool MissingMemberFailure::diagnoseForDynamicCallable() const {
+  auto *locator = getLocator();
+  if (!locator->isLastElement<LocatorPathElt::DynamicCallable>())
+    return false;
+
+  auto memberName = getName();
+  auto arguments = memberName.getArgumentNames();
+  assert(arguments.size() == 1);
+
+  auto &ctx = getASTContext();
+  if (arguments.front() == ctx.Id_withKeywordArguments) {
+    auto anchor = getAnchor();
+    emitDiagnostic(anchor->getLoc(),
+                   diag::missing_dynamic_callable_kwargs_method, getBaseType());
+    return true;
+  }
+
+  return false;
+}
+
 bool InvalidMemberRefOnExistential::diagnoseAsError() {
   auto *anchor = getRawAnchor();
 

lib/Sema/CSDiagnostics.h

@@ -1148,6 +1148,12 @@ class MissingMemberFailure final : public InvalidMemberRefFailure {
   bool diagnoseAsError() override;
 
 private:
+  /// Tailored diagnostics for missing special `@dynamicCallable` methods
+  /// e.g. if caller expects `dynamicallyCall(withKeywordArguments:)`
+  /// overload to be present, but a class marked as `@dynamicCallable`
+  /// defines only `dynamicallyCall(withArguments:)` variant.
+  bool diagnoseForDynamicCallable() const;
+
   static DeclName findCorrectEnumCaseName(Type Ty,
                                           TypoCorrectionResults &corrections,
                                           DeclName memberName);

lib/Sema/CSSimplify.cpp

@@ -3167,6 +3167,12 @@ bool ConstraintSystem::repairFailures(
     // for this call, we can consider overload unrelated.
     if (llvm::any_of(getFixes(), [&](const ConstraintFix *fix) {
           auto *locator = fix->getLocator();
+          // Since arguments to @dynamicCallable form either an array
+          // or a dictionary and all have to match the same element type,
+          // let's allow multiple invalid arguments.
+          if (locator->findFirst<LocatorPathElt::DynamicCallable>())
+            return false;
+
           return locator->findLast<LocatorPathElt::ApplyArgToParam>()
                      ? locator->getAnchor() == anchor
                      : false;
@@ -7819,7 +7825,35 @@ ConstraintSystem::simplifyDynamicCallableApplicableFnConstraint(
     choices.push_back(
       OverloadChoice(type2, candidate, FunctionRefKind::SingleApply));
   }
-  if (choices.empty()) return SolutionKind::Error;
+
+  if (choices.empty()) {
+    if (!shouldAttemptFixes())
+      return SolutionKind::Error;
+
+    // TODO(diagnostics): This is not going to be necessary once
+    // `@dynamicCallable` uses existing `member` machinery.
+
+    auto memberName = DeclName(
+        ctx, ctx.Id_dynamicallyCall,
+        {useKwargsMethod ? ctx.Id_withKeywordArguments : ctx.Id_withArguments});
+
+    auto *fix = DefineMemberBasedOnUse::create(
+        *this, desugar2, memberName,
+        getConstraintLocator(loc, ConstraintLocator::DynamicCallable));
+
+    if (recordFix(fix))
+      return SolutionKind::Error;
+
+    recordPotentialHole(tv);
+
+    Type(func1).visit([&](Type type) {
+      if (auto *typeVar = type->getAs<TypeVariableType>())
+        recordPotentialHole(typeVar);
+    });
+
+    return SolutionKind::Solved;
+  }
+
   addOverloadSet(tv, choices, DC, loc);
 
   // Create a type variable for the argument to the `dynamicallyCall` method.
@@ -7852,14 +7886,20 @@ ConstraintSystem::simplifyDynamicCallableApplicableFnConstraint(
   addConstraint(ConstraintKind::Defaultable, argumentType,
                 ctx.TheAnyType, locator);
 
+  auto *baseArgLoc = getConstraintLocator(
+      loc->getAnchor(),
+      {ConstraintLocator::DynamicCallable, ConstraintLocator::ApplyArgument},
+      /*summaryFlags=*/0);
+
   // All dynamic call parameter types must be convertible to the argument type.
   for (auto i : indices(func1->getParams())) {
     auto param = func1->getParams()[i];
     auto paramType = param.getPlainType();
-    auto locatorBuilder =
-        locator.withPathElement(LocatorPathElt::TupleElement(i));
-    addConstraint(ConstraintKind::ArgumentConversion, paramType,
-                  argumentType, locatorBuilder);
+
+    addConstraint(
+        ConstraintKind::ArgumentConversion, paramType, argumentType,
+        getConstraintLocator(baseArgLoc, LocatorPathElt::ApplyArgToParam(
+                                             i, 0, param.getParameterFlags())));
   }
 
   return SolutionKind::Solved;

lib/Sema/ConstraintLocator.cpp

@@ -113,6 +113,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
   case ConstraintLocator::KeyPathValue:
   case ConstraintLocator::KeyPathComponentResult:
   case ConstraintLocator::Condition:
+  case ConstraintLocator::DynamicCallable:
     return 0;
 
   case ConstraintLocator::FunctionArgument:
@@ -453,6 +454,10 @@ void ConstraintLocator::dump(SourceManager *sm, raw_ostream &out) const {
     case Condition:
       out << "condition expression";
       break;
+
+    case DynamicCallable:
+      out << "implicit call to @dynamicCallable method";
+      break;
     }
   }
   out << ']';

lib/Sema/ConstraintLocatorPathElts.def

@@ -167,6 +167,8 @@ CUSTOM_LOCATOR_PATH_ELT(Witness)
 /// The condition associated with 'if' expression or ternary operator.
 SIMPLE_LOCATOR_PATH_ELT(Condition)
 
+SIMPLE_LOCATOR_PATH_ELT(DynamicCallable)
+
 #undef LOCATOR_PATH_ELT
 #undef CUSTOM_LOCATOR_PATH_ELT
 #undef SIMPLE_LOCATOR_PATH_ELT

lib/Sema/ConstraintSystem.cpp

@@ -3066,6 +3066,11 @@ void constraints::simplifyLocator(Expr *&anchor,
       break;
     }
 
+    case ConstraintLocator::DynamicCallable: {
+      path = path.slice(1);
+      continue;
+    }
+
     case ConstraintLocator::ApplyFunction:
       // Extract application function.
       if (auto applyExpr = dyn_cast<ApplyExpr>(anchor)) {

test/Sema/call_as_function_simple.swift

@@ -102,9 +102,7 @@ struct Mutating {
   }
 }
 func testMutating(_ x: Mutating, _ y: inout Mutating) {
-  // TODO(SR-11378): Improve this error to match the error using a direct `callAsFunction` member reference.
-  // expected-error @+2 {{cannot call value of non-function type 'Mutating'}}
-  // expected-error @+1 {{cannot invoke 'x' with no arguments}}
+  // expected-error @+1 {{cannot use mutating member on immutable value: 'x' is a 'let' constant}}
   _ = x()
   // expected-error @+1 {{cannot use mutating member on immutable value: 'x' is a 'let' constant}}
   _ = x.callAsFunction()

test/attr/attr_dynamic_callable.swift

@@ -52,10 +52,21 @@ func testCallable(
 func testCallableDiagnostics(
   a: Callable, b: DiscardableResult, c: Throwing, d: KeywordArgumentCallable
 ) {
-  a("hello", "world") // expected-error {{cannot invoke 'a' with an argument list of type '(String, String)'}}
-  b("hello", "world") // expected-error {{cannot invoke 'b' with an argument list of type '(String, String)'}}
-  try? c(1, 2, 3, 4) // expected-error {{cannot invoke 'c' with an argument list of type '(Int, Int, Int, Int)'}}
-  d(x1: "hello", x2: "world") // expected-error {{cannot invoke 'd' with an argument list of type '(x1: String, x2: String)'}}
+  a("hello", "world")
+  // expected-error@-1:5  {{cannot convert value of type 'String' to expected argument type 'Int'}}
+  // expected-error@-2:14 {{cannot convert value of type 'String' to expected argument type 'Int'}}
+  b("hello", "world")
+  // expected-error@-1:5  {{cannot convert value of type 'String' to expected argument type 'Double'}}
+  // expected-error@-2:14 {{cannot convert value of type 'String' to expected argument type 'Double'}}
+  try? c(1, 2, 3, 4)
+  // expected-error@-1:10 {{cannot convert value of type 'Int' to expected argument type 'String'}}
+  // expected-error@-2:13 {{cannot convert value of type 'Int' to expected argument type 'String'}}
+  // expected-error@-3:16 {{cannot convert value of type 'Int' to expected argument type 'String'}}
+  // expected-error@-4:19 {{cannot convert value of type 'Int' to expected argument type 'String'}}
+
+  d(x1: "hello", x2: "world")
+  // expected-error@-1:9  {{cannot convert value of type 'String' to expected argument type 'Float'}}
+  // expected-error@-2:22 {{cannot convert value of type 'String' to expected argument type 'Float'}}
 }
 
 func testIUO(