Merge pull request #63904 from LucianoPAlmeida/ambiguous-overload

LucianoPAlmeida · web-flow · commit 4d981e3c0b89 · 2023-03-15T23:18:00.000-03:00
[Sema] Diagnose function coercion ambiguity
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1403,10 +1403,13 @@ WARNING(unlabeled_trailing_closure_deprecated,Deprecation,
 NOTE(decl_multiple_defaulted_closure_parameters,none,
      "%0 contains defaulted closure parameters %1 and %2",
      (DeclName, Identifier, Identifier))
+NOTE(candidate_with_extraneous_args_closure,none,
+     "candidate %0 requires %1 argument%s1, "
+     "but %2 %select{were|was}3 used in closure body",
+     (Type, unsigned, unsigned, bool))
 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))
+     "candidate %0 has %1 parameter%s1, but context %2 has %3",
+     (Type, unsigned, Type, unsigned))
 
 ERROR(no_accessible_initializers,none,
       "%0 cannot be constructed because it has no accessible initializers",
diff --git a/include/swift/Sema/ConstraintLocatorPathElts.def b/include/swift/Sema/ConstraintLocatorPathElts.def
@@ -265,6 +265,9 @@ ABSTRACT_LOCATOR_PATH_ELT(PatternDecl)
 /// A function type global actor.
 SIMPLE_LOCATOR_PATH_ELT(GlobalActorType)
 
+/// A type coercion operand.
+SIMPLE_LOCATOR_PATH_ELT(CoercionOperand)
+
 #undef LOCATOR_PATH_ELT
 #undef CUSTOM_LOCATOR_PATH_ELT
 #undef SIMPLE_LOCATOR_PATH_ELT
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -4349,7 +4349,8 @@ namespace {
       // If we weren't explicitly told by the caller which disjunction choice,
       // get it from the solution to determine whether we've picked a coercion
       // or a bridging conversion.
-      auto *locator = cs.getConstraintLocator(expr);
+      auto *locator =
+          cs.getConstraintLocator(expr, ConstraintLocator::CoercionOperand);
       auto choice = solution.getDisjunctionChoice(locator);
 
       // Handle the coercion/bridging of the underlying subexpression, where
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -282,6 +282,9 @@ ValueDecl *RequirementFailure::getDeclRef() const {
     return getAffectedDeclFromType(contextualTy);
   }
 
+  if (getLocator()->isFirstElement<LocatorPathElt::CoercionOperand>())
+    return getAffectedDeclFromType(getOwnerType());
+
   if (auto overload = getCalleeOverloadChoiceIfAvailable(getLocator())) {
     // If there is a declaration associated with this
     // failure e.g. an overload choice of the call
@@ -786,7 +789,8 @@ bool GenericArgumentsMismatchFailure::diagnoseAsError() {
   //
   // `value` has to get implicitly wrapped into 2 optionals
   // before pointer types could be compared.
-  auto path = getLocator()->getPath();
+  auto locator = getLocator();
+  auto path = locator->getPath();
   unsigned toDrop = 0;
   for (const auto &elt : llvm::reverse(path)) {
     if (!elt.is<LocatorPathElt::OptionalPayload>())
@@ -802,7 +806,7 @@ bool GenericArgumentsMismatchFailure::diagnoseAsError() {
   if (path.empty()) {
     if (isExpr<AssignExpr>(anchor)) {
       diagnostic = getDiagnosticFor(CTP_AssignSource);
-    } else if (isExpr<CoerceExpr>(anchor)) {
+    } else if (locator->isForCoercion()) {
       diagnostic = getDiagnosticFor(CTP_CoerceOperand);
     } else {
       return false;
@@ -887,6 +891,11 @@ bool GenericArgumentsMismatchFailure::diagnoseAsError() {
       break;
     }
 
+    case ConstraintLocator::CoercionOperand: {
+      diagnostic = getDiagnosticFor(CTP_CoerceOperand);
+      break;
+    }
+
     default:
       break;
     }
@@ -1219,6 +1228,14 @@ ASTNode InvalidCoercionFailure::getAnchor() const {
   return anchor;
 }
 
+SourceLoc InvalidCoercionFailure::getLoc() const {
+  if (getLocator()->isForCoercion()) {
+    auto *CE = castToExpr<CoerceExpr>(getRawAnchor());
+    return CE->getAsLoc();
+  }
+  return FailureDiagnostic::getLoc();
+}
+
 bool InvalidCoercionFailure::diagnoseAsError() {
   auto fromType = getFromType();
   auto toType = getToType();
@@ -2487,7 +2504,7 @@ bool ContextualFailure::diagnoseAsError() {
     diagnostic = diag::cannot_convert_condition_value;
     break;
   }
-      
+  case ConstraintLocator::CoercionOperand:
   case ConstraintLocator::InstanceType: {
     if (diagnoseCoercionToUnrelatedType())
       return true;
@@ -2809,7 +2826,7 @@ bool ContextualFailure::diagnoseConversionToNil() const {
         emitDiagnostic(diag::unresolved_nil_literal);
         return true;
       }
-    } else if (isa<CoerceExpr>(parentExpr)) {
+    } else if (locator->isForCoercion()) {
       // `nil` is passed as a left-hand side of the coercion
       // operator e.g. `nil as Foo`
       CTP = CTP_CoerceOperand;
@@ -2891,23 +2908,23 @@ bool ContextualFailure::diagnoseExtraneousAssociatedValues() const {
 }
 
 bool ContextualFailure::diagnoseCoercionToUnrelatedType() const {
-  auto anchor = getAnchor();
+  auto anchor = getRawAnchor();
+  auto *coerceExpr = getAsExpr<CoerceExpr>(anchor);
+  if (!coerceExpr) {
+    return false;
+  }
 
-  if (auto *coerceExpr = getAsExpr<CoerceExpr>(anchor)) {
-    const auto fromType = getType(coerceExpr->getSubExpr());
-    const auto toType = getType(coerceExpr->getCastTypeRepr());
+  const auto fromType = getType(coerceExpr->getSubExpr());
+  const auto toType = getType(coerceExpr->getCastTypeRepr());
 
-    auto diagnostic = getDiagnosticFor(CTP_CoerceOperand, toType);
+  auto diagnostic = getDiagnosticFor(CTP_CoerceOperand, toType);
 
-    auto diag = emitDiagnostic(*diagnostic, fromType, toType);
-    diag.highlight(getSourceRange());
+  auto diag = emitDiagnostic(*diagnostic, fromType, toType);
+  diag.highlight(getSourceRange());
 
-    (void)tryFixIts(diag);
-    
-    return true;
-  }
+  (void)tryFixIts(diag);
 
-  return false;
+  return true;
 }
 
 bool ContextualFailure::diagnoseConversionToBool() const {
@@ -3178,9 +3195,6 @@ bool ContextualFailure::trySequenceSubsequenceFixIts(
   if (getFromType()->isSubstring()) {
     if (getToType()->isString()) {
       auto *anchor = castToExpr(getAnchor())->getSemanticsProvidingExpr();
-      if (auto *CE = dyn_cast<CoerceExpr>(anchor)) {
-        anchor = CE->getSubExpr();
-      }
 
       if (auto *call = dyn_cast<CallExpr>(anchor)) {
         auto *fnExpr = call->getFn();
@@ -5860,9 +5874,15 @@ bool ExtraneousArgumentsFailure::diagnoseAsNote() {
 
   auto *decl = overload->choice.getDecl();
   auto numArgs = getTotalNumArguments();
-  emitDiagnosticAt(decl, diag::candidate_with_extraneous_args, ContextualType,
-                   ContextualType->getNumParams(), numArgs, (numArgs == 1),
-                   isExpr<ClosureExpr>(getAnchor()));
+  if (isExpr<ClosureExpr>(getAnchor())) {
+    emitDiagnosticAt(decl, diag::candidate_with_extraneous_args_closure,
+                     ContextualType, ContextualType->getNumParams(), numArgs,
+                     (numArgs == 1));
+  } else {
+    emitDiagnosticAt(decl, diag::candidate_with_extraneous_args,
+                     overload->adjustedOpenedType, numArgs, ContextualType,
+                     ContextualType->getNumParams());
+  }
   return true;
 }
 
@@ -8878,12 +8898,7 @@ GlobalActorFunctionMismatchFailure::getDiagnosticMessage() const {
   auto path = locator->getPath();
 
   if (path.empty()) {
-    auto anchor = getAnchor();
-    if (isExpr<CoerceExpr>(anchor)) {
-      return diag::cannot_convert_global_actor_coercion;
-    } else {
-      return diag::cannot_convert_global_actor;
-    }
+    return diag::cannot_convert_global_actor;
   }
 
   auto last = path.back();
@@ -8901,6 +8916,9 @@ GlobalActorFunctionMismatchFailure::getDiagnosticMessage() const {
   case ConstraintLocator::TernaryBranch: {
     return diag::ternary_expr_cases_global_actor_mismatch;
   }
+  case ConstraintLocator::CoercionOperand: {
+    return diag::cannot_convert_global_actor_coercion;
+  }
   default:
     break;
   }
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -2177,6 +2177,8 @@ class InvalidCoercionFailure final : public ContextualFailure {
 
   ASTNode getAnchor() const override;
 
+  SourceLoc getLoc() const override;
+
   bool diagnoseAsError() override;
 };
 
diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp
@@ -3307,7 +3307,8 @@ namespace {
       if (repr) CS.setType(repr, toType);
 
       auto fromType = CS.getType(expr->getSubExpr());
-      auto locator = CS.getConstraintLocator(expr);
+      auto locator =
+          CS.getConstraintLocator(expr, ConstraintLocator::CoercionOperand);
 
       // Literal initialization (e.g. `UInt32(0)`) doesn't require
       // a conversion because the literal is supposed to assume the
@@ -3327,8 +3328,11 @@ namespace {
 
       // If the result type was declared IUO, add a disjunction for
       // bindings for the result of the coercion.
-      if (repr && repr->getKind() == TypeReprKind::ImplicitlyUnwrappedOptional)
-        return createTypeVariableAndDisjunctionForIUOCoercion(toType, locator);
+      if (repr &&
+          repr->getKind() == TypeReprKind::ImplicitlyUnwrappedOptional) {
+        return createTypeVariableAndDisjunctionForIUOCoercion(
+            toType, CS.getConstraintLocator(expr));
+      }
 
       return toType;
     }
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -5058,63 +5058,6 @@ bool ConstraintSystem::repairFailures(
     if (!anchor)
       return false;
 
-    if (auto *coercion = getAsExpr<CoerceExpr>(anchor)) {
-      // Coercion from T.Type to T.Protocol.
-      if (hasConversionOrRestriction(
-              ConversionRestrictionKind::MetatypeToExistentialMetatype))
-        return false;
-
-      if (hasConversionOrRestriction(ConversionRestrictionKind::Superclass))
-        return false;
-
-      // Let's check whether the sub-expression is an optional type which
-      // is possible to unwrap (either by force or `??`) to satisfy the cast,
-      // otherwise we'd have to fallback to force downcast.
-      if (repairViaOptionalUnwrap(*this, lhs, rhs, matchKind,
-                                  conversionsOrFixes,
-                                  getConstraintLocator(coercion->getSubExpr())))
-        return true;
-
-      // If the result type of the coercion has an value to optional conversion
-      // we can instead suggest the conditional downcast as it is safer in
-      // situations like conditional binding.
-      auto useConditionalCast =
-          llvm::any_of(ConstraintRestrictions, [&](const auto &restriction) {
-            Type type1, type2;
-            std::tie(type1, type2) = restriction.first;
-            auto restrictionKind = restriction.second;
-
-            if (restrictionKind != ConversionRestrictionKind::ValueToOptional)
-              return false;
-
-            return rhs->isEqual(type1);
-          });
-
-      // Repair a coercion ('as') with a runtime checked cast ('as!' or 'as?').
-      if (auto *coerceToCheckCastFix =
-              CoerceToCheckedCast::attempt(*this, lhs, rhs, useConditionalCast,
-                                           getConstraintLocator(locator))) {
-        conversionsOrFixes.push_back(coerceToCheckCastFix);
-        return true;
-      }
-
-      // If it has a deep equality restriction, defer the diagnostic to
-      // GenericMismatch.
-      if (hasConversionOrRestriction(ConversionRestrictionKind::DeepEquality) &&
-          !hasConversionOrRestriction(
-              ConversionRestrictionKind::OptionalToOptional)) {
-        return false;
-      }
-
-      if (hasConversionOrRestriction(ConversionRestrictionKind::Existential))
-        return false;
-
-      auto *fix = ContextualMismatch::create(*this, lhs, rhs,
-                                             getConstraintLocator(locator));
-      conversionsOrFixes.push_back(fix);
-      return true;
-    }
-
     // This could be:
     // - `InOutExpr` used with r-value e.g. `foo(&x)` where `x` is a `let`.
     // - `ForceValueExpr` e.g. `foo.bar! = 42` where `bar` or `foo` are
@@ -6449,6 +6392,64 @@ bool ConstraintSystem::repairFailures(
     break;
   }
 
+  case ConstraintLocator::CoercionOperand: {
+    auto *coercion = castToExpr<CoerceExpr>(anchor);
+
+    // Coercion from T.Type to T.Protocol.
+    if (hasConversionOrRestriction(
+            ConversionRestrictionKind::MetatypeToExistentialMetatype))
+      return false;
+
+    if (hasConversionOrRestriction(ConversionRestrictionKind::Superclass))
+      return false;
+
+    // Let's check whether the sub-expression is an optional type which
+    // is possible to unwrap (either by force or `??`) to satisfy the cast,
+    // otherwise we'd have to fallback to force downcast.
+    if (repairViaOptionalUnwrap(*this, lhs, rhs, matchKind,
+                                conversionsOrFixes,
+                                getConstraintLocator(coercion->getSubExpr())))
+      return true;
+
+    // If the result type of the coercion has an value to optional conversion
+    // we can instead suggest the conditional downcast as it is safer in
+    // situations like conditional binding.
+    auto useConditionalCast =
+        llvm::any_of(ConstraintRestrictions, [&](const auto &restriction) {
+          Type type1, type2;
+          std::tie(type1, type2) = restriction.first;
+          auto restrictionKind = restriction.second;
+
+          if (restrictionKind != ConversionRestrictionKind::ValueToOptional)
+            return false;
+
+          return rhs->isEqual(type1);
+        });
+
+    // Repair a coercion ('as') with a runtime checked cast ('as!' or 'as?').
+    if (auto *coerceToCheckCastFix =
+            CoerceToCheckedCast::attempt(*this, lhs, rhs, useConditionalCast,
+                                         getConstraintLocator(locator))) {
+      conversionsOrFixes.push_back(coerceToCheckCastFix);
+      return true;
+    }
+
+    // If it has a deep equality restriction, defer the diagnostic to
+    // GenericMismatch.
+    if (hasConversionOrRestriction(ConversionRestrictionKind::DeepEquality) &&
+        !hasConversionOrRestriction(
+            ConversionRestrictionKind::OptionalToOptional)) {
+      return false;
+    }
+
+    if (hasConversionOrRestriction(ConversionRestrictionKind::Existential))
+      return false;
+
+    auto *fix = ContextualMismatch::create(*this, lhs, rhs,
+                                           getConstraintLocator(locator));
+    conversionsOrFixes.push_back(fix);
+    return true;
+  }
   default:
     break;
   }
@@ -6963,7 +6964,8 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
                                    ArrayRef<LocatorPathElt> path) {
           // E.g. contextual conversion from coercion/cast
           // to some other type.
-          if (!path.empty())
+          if (!(path.empty() ||
+                path.back().is<LocatorPathElt::CoercionOperand>()))
             return false;
 
           return isExpr<CoerceExpr>(anchor) || isExpr<IsExpr>(anchor) ||
@@ -11328,7 +11330,7 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
 
     SmallVector<LocatorPathElt, 4> elts;
     auto anchor = locator.getLocatorParts(elts);
-    if (!elts.empty())
+    if (elts.empty() || !elts.back().is<LocatorPathElt::CoercionOperand>())
       return false;
 
     auto *coercion = getAsExpr<CoerceExpr>(anchor);
diff --git a/lib/Sema/ConstraintLocator.cpp b/lib/Sema/ConstraintLocator.cpp
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
diff --git a/test/Constraints/overload.swift b/test/Constraints/overload.swift
