Merge pull request #26207 from owenv/new-vararg-conversion-diag

[Diagnostics] Improve diagnostic when attempting to pass an Array to a variadic argument

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -384,6 +384,15 @@ NOTE(candidate_has_invalid_argument_at_position,none,
      "candidate expects value of type %0 at position #%1",
      (Type, unsigned))
 
+ERROR(cannot_convert_array_to_variadic,none,
+      "cannot pass array of type %0 as variadic arguments of type %1",
+      (Type,Type))
+NOTE(candidate_would_match_array_to_variadic,none,
+     "candidate would match if array elements were passed as"
+     " variadic arguments of type %0", (Type))
+NOTE(suggest_pass_elements_directly,none,
+     "remove brackets to pass array elements directly", ())
+
 ERROR(cannot_convert_argument_value_generic,none,
       "cannot convert value of type %0 (%1) to expected argument type %2 (%3)",
       (Type, StringRef, Type, StringRef))

lib/Sema/CSApply.cpp

@@ -5384,10 +5384,11 @@ Expr *ExprRewriter::coerceCallArguments(
   auto params = funcType->getParams();
 
   // Local function to produce a locator to refer to the given parameter.
-  auto getArgLocator = [&](unsigned argIdx, unsigned paramIdx)
-                         -> ConstraintLocatorBuilder {
+  auto getArgLocator =
+      [&](unsigned argIdx, unsigned paramIdx,
+          ParameterTypeFlags flags) -> ConstraintLocatorBuilder {
     return locator.withPathElement(
-             LocatorPathElt::ApplyArgToParam(argIdx, paramIdx));
+        LocatorPathElt::ApplyArgToParam(argIdx, paramIdx, flags));
   };
 
   bool matchCanFail =
@@ -5499,8 +5500,9 @@ Expr *ExprRewriter::coerceCallArguments(
         }
 
         // Convert the argument.
-        auto convertedArg = coerceToType(arg, param.getPlainType(),
-                                         getArgLocator(argIdx, paramIdx));
+        auto convertedArg = coerceToType(
+            arg, param.getPlainType(),
+            getArgLocator(argIdx, paramIdx, param.getParameterFlags()));
         if (!convertedArg)
           return nullptr;
 
@@ -5620,8 +5622,9 @@ Expr *ExprRewriter::coerceCallArguments(
       convertedArg = cs.TC.buildAutoClosureExpr(dc, arg, closureType);
       cs.cacheExprTypes(convertedArg);
     } else {
-      convertedArg =
-          coerceToType(arg, paramType, getArgLocator(argIdx, paramIdx));
+      convertedArg = coerceToType(
+          arg, paramType,
+          getArgLocator(argIdx, paramIdx, param.getParameterFlags()));
     }
 
     if (!convertedArg)

lib/Sema/CSDiag.cpp

@@ -3461,11 +3461,16 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
     //   case (let (_, _, _)) + 1: break
     // }
     if (callExpr->isImplicit() && overloadName == "~=") {
-      auto *locator =
-          CS.getConstraintLocator(callExpr,
-                                  {ConstraintLocator::ApplyArgument,
-                                   LocatorPathElt::ApplyArgToParam(0, 0)},
-                                  /*summaryFlags=*/0);
+      auto flags = ParameterTypeFlags();
+      if (calleeInfo.candidates.size() == 1)
+        if (auto fnType = calleeInfo.candidates[0].getFunctionType())
+          flags = fnType->getParams()[0].getParameterFlags();
+
+      auto *locator = CS.getConstraintLocator(
+          callExpr,
+          {ConstraintLocator::ApplyArgument,
+           LocatorPathElt::ApplyArgToParam(0, 0, flags)},
+          /*summaryFlags=*/0);
 
       ArgumentMismatchFailure failure(expr, CS, lhsType, rhsType, locator);
       return failure.diagnosePatternMatchingMismatch();

lib/Sema/CSDiagnostics.cpp

@@ -4678,9 +4678,11 @@ bool ArgumentMismatchFailure::diagnoseUseOfReferenceEqualityOperator() const {
   // one would cover both arguments.
   if (getAnchor() == rhs && rhsType->is<FunctionType>()) {
     auto &cs = getConstraintSystem();
-    if (cs.hasFixFor(cs.getConstraintLocator(
-            binaryOp, {ConstraintLocator::ApplyArgument,
-                       LocatorPathElt::ApplyArgToParam(0, 0)})))
+    auto info = getFunctionArgApplyInfo(locator);
+    if (info && cs.hasFixFor(cs.getConstraintLocator(
+                    binaryOp, {ConstraintLocator::ApplyArgument,
+                               LocatorPathElt::ApplyArgToParam(
+                                   0, 0, info->getParameterFlagsAtIndex(0))})))
       return true;
   }
 
@@ -4830,3 +4832,44 @@ bool ArgumentMismatchFailure::diagnoseArchetypeMismatch() const {
 
   return true;
 }
+
+void ExpandArrayIntoVarargsFailure::tryDropArrayBracketsFixIt(
+    Expr *anchor) const {
+  // If this is an array literal, offer to remove the brackets and pass the
+  // elements directly as variadic arguments.
+  if (auto *arrayExpr = dyn_cast<ArrayExpr>(anchor)) {
+    auto diag = emitDiagnostic(arrayExpr->getLoc(),
+                               diag::suggest_pass_elements_directly);
+    diag.fixItRemove(arrayExpr->getLBracketLoc())
+        .fixItRemove(arrayExpr->getRBracketLoc());
+    // Handle the case where the array literal has a trailing comma.
+    if (arrayExpr->getNumCommas() == arrayExpr->getNumElements())
+      diag.fixItRemove(arrayExpr->getCommaLocs().back());
+  }
+}
+
+bool ExpandArrayIntoVarargsFailure::diagnoseAsError() {
+  if (auto anchor = getAnchor()) {
+    emitDiagnostic(anchor->getLoc(), diag::cannot_convert_array_to_variadic,
+                   getFromType(), getToType());
+    tryDropArrayBracketsFixIt(anchor);
+    // TODO: Array splat fix-it once that's supported.
+    return true;
+  }
+  return false;
+}
+
+bool ExpandArrayIntoVarargsFailure::diagnoseAsNote() {
+  auto overload = getChoiceFor(getLocator());
+  auto anchor = getAnchor();
+  if (!overload || !anchor)
+    return false;
+
+  if (auto chosenDecl = overload->choice.getDeclOrNull()) {
+    emitDiagnostic(chosenDecl, diag::candidate_would_match_array_to_variadic,
+                   getToType());
+    tryDropArrayBracketsFixIt(anchor);
+    return true;
+  }
+  return false;
+}

lib/Sema/CSDiagnostics.h

@@ -1586,8 +1586,25 @@ class InvalidTupleSplatWithSingleParameterFailure final
                                               Type paramTy,
                                               ConstraintLocator *locator)
       : FailureDiagnostic(root, cs, locator), ParamType(paramTy) {}
+  bool diagnoseAsError() override;
+};
+
+/// Diagnose situation when an array is passed instead of varargs.
+///
+/// ```swift
+/// func foo(_ x: Int...) {}
+/// foo([1,2,3]]) // foo expects varags like foo(1,2,3) instead.
+/// ```
+class ExpandArrayIntoVarargsFailure final : public ContextualFailure {
+public:
+  ExpandArrayIntoVarargsFailure(Expr *root, ConstraintSystem &cs, Type lhs,
+                                Type rhs, ConstraintLocator *locator)
+      : ContextualFailure(root, cs, lhs, rhs, locator) {}
 
   bool diagnoseAsError() override;
+  bool diagnoseAsNote() override;
+
+  void tryDropArrayBracketsFixIt(Expr *anchor) const;
 };
 
 /// Diagnose a situation there is a mismatch between argument and parameter
@@ -1719,6 +1736,10 @@ class FunctionArgApplyInfo {
   ParameterTypeFlags getParameterFlags() const {
     return FnType->getParams()[ParamIdx].getParameterFlags();
   }
+
+  ParameterTypeFlags getParameterFlagsAtIndex(unsigned idx) const {
+    return FnType->getParams()[idx].getParameterFlags();
+  }
 };
 
 } // end namespace constraints

lib/Sema/CSFix.cpp

@@ -815,6 +815,36 @@ static bool isValueOfRawRepresentable(ConstraintSystem &cs,
   return false;
 }
 
+ExpandArrayIntoVarargs *
+ExpandArrayIntoVarargs::attempt(ConstraintSystem &cs, Type argType,
+                                Type paramType,
+                                ConstraintLocatorBuilder locator) {
+  auto constraintLocator = cs.getConstraintLocator(locator);
+  auto elementType = cs.isArrayType(argType);
+  if (elementType &&
+      constraintLocator->getLastElementAs<LocatorPathElt::ApplyArgToParam>()
+          ->getParameterFlags()
+          .isVariadic()) {
+    auto options = ConstraintSystem::TypeMatchOptions(
+        ConstraintSystem::TypeMatchFlags::TMF_ApplyingFix |
+        ConstraintSystem::TypeMatchFlags::TMF_GenerateConstraints);
+    auto result =
+        cs.matchTypes(*elementType, paramType,
+                      ConstraintKind::ArgumentConversion, options, locator);
+    if (result.isSuccess())
+      return new (cs.getAllocator())
+          ExpandArrayIntoVarargs(cs, argType, paramType, constraintLocator);
+  }
+
+  return nullptr;
+}
+
+bool ExpandArrayIntoVarargs::diagnose(Expr *root, bool asNote) const {
+  ExpandArrayIntoVarargsFailure failure(
+      root, getConstraintSystem(), getFromType(), getToType(), getLocator());
+  return failure.diagnose(asNote);
+}
+
 ExplicitlyConstructRawRepresentable *
 ExplicitlyConstructRawRepresentable::attempt(ConstraintSystem &cs, Type argType,
                                              Type paramType,

lib/Sema/CSFix.h

@@ -204,6 +204,9 @@ enum class FixKind : uint8_t {
   /// Use raw value type associated with raw representative accessible
   /// using `.rawValue` member.
   UseValueTypeOfRawRepresentative,
+  /// If an array was passed to a variadic argument, give a specific diagnostic
+  /// and offer to drop the brackets if it's a literal.
+  ExpandArrayIntoVarargs,
 };
 
 class ConstraintFix {
@@ -1359,6 +1362,25 @@ class AllowArgumentMismatch : public ContextualMismatch {
                                        ConstraintLocator *locator);
 };
 
+class ExpandArrayIntoVarargs final : public AllowArgumentMismatch {
+
+  ExpandArrayIntoVarargs(ConstraintSystem &cs, Type argType, Type paramType,
+                         ConstraintLocator *locator)
+      : AllowArgumentMismatch(cs, FixKind::ExpandArrayIntoVarargs, argType,
+                              paramType, locator) {}
+
+public:
+  std::string getName() const override {
+    return "cannot pass Array elements as variadic arguments";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static ExpandArrayIntoVarargs *attempt(ConstraintSystem &cs, Type argType,
+                                         Type paramType,
+                                         ConstraintLocatorBuilder locator);
+};
+
 class ExplicitlyConstructRawRepresentable final : public AllowArgumentMismatch {
   ExplicitlyConstructRawRepresentable(ConstraintSystem &cs, Type argType,
                                       Type paramType,

lib/Sema/CSSimplify.cpp

@@ -913,8 +913,8 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
 
     // Compare each of the bound arguments for this parameter.
     for (auto argIdx : parameterBindings[paramIdx]) {
-      auto loc = locator.withPathElement(
-          LocatorPathElt::ApplyArgToParam(argIdx, paramIdx));
+      auto loc = locator.withPathElement(LocatorPathElt::ApplyArgToParam(
+          argIdx, paramIdx, param.getParameterFlags()));
       auto argTy = argsWithLabels[argIdx].getOldType();
 
       bool matchingAutoClosureResult = param.isAutoClosure();
@@ -2626,6 +2626,11 @@ bool ConstraintSystem::repairFailures(
         elt.castTo<LocatorPathElt::ApplyArgToParam>().getParamIdx() == 1)
       break;
 
+    if (auto *fix = ExpandArrayIntoVarargs::attempt(*this, lhs, rhs, locator)) {
+      conversionsOrFixes.push_back(fix);
+      break;
+    }
+
     if (auto *fix = ExplicitlyConstructRawRepresentable::attempt(
             *this, lhs, rhs, locator)) {
       conversionsOrFixes.push_back(fix);
@@ -7638,6 +7643,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::SkipUnhandledConstructInFunctionBuilder:
   case FixKind::UsePropertyWrapper:
   case FixKind::UseWrappedValue:
+  case FixKind::ExpandArrayIntoVarargs:
   case FixKind::UseValueTypeOfRawRepresentative:
   case FixKind::ExplicitlyConstructRawRepresentable: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;

lib/Sema/ConstraintLocator.h

@@ -184,8 +184,10 @@ class ConstraintLocator : public llvm::FoldingSetNode {
 
     case TypeParameterRequirement:
     case ConditionalRequirement:
-    case ApplyArgToParam:
       return 2;
+
+    case ApplyArgToParam:
+      return 3;
     }
 
     llvm_unreachable("Unhandled PathElementKind in switch.");
@@ -282,12 +284,12 @@ class ConstraintLocator : public llvm::FoldingSetNode {
     uint64_t storedKind : 3;
 
     /// Encode a path element kind and a value into the storage format.
-    static uint64_t encodeStorage(PathElementKind kind, unsigned value) {
-      return ((uint64_t)value << 8) | kind;
+    static uint64_t encodeStorage(PathElementKind kind, uint64_t value) {
+      return (value << 8) | kind;
     }
 
     /// Decode a storage value into path element kind and value.
-    static std::pair<PathElementKind, unsigned>
+    static std::pair<PathElementKind, uint64_t>
     decodeStorage(uint64_t storage) {
       return { (PathElementKind)((unsigned)storage & 0xFF), storage >> 8 };
     }
@@ -323,6 +325,17 @@ class ConstraintLocator : public llvm::FoldingSetNode {
       assert(value1 == getValue(1) && "value1 truncated");
     }
 
+    PathElement(PathElementKind kind, uint64_t value0, uint64_t value1,
+                uint64_t value2)
+        : storage(encodeStorage(kind, value0 << 32 | value1 << 16 | value2)),
+          storedKind(StoredKindAndValue) {
+      assert(numNumericValuesInPathElement(kind) == 3 &&
+             "Path element kind does not require 3 values");
+      assert(value0 == getValue(0) && "value0 truncated");
+      assert(value1 == getValue(1) && "value1 truncated");
+      assert(value2 == getValue(2) && "value2 truncated");
+    }
+
     /// Store a path element with an associated pointer, accessible using
     /// \c getStoredPointer.
     template <typename T>
@@ -695,11 +708,15 @@ dyn_cast(const LocatorPathElt &) = delete; // Use LocatorPathElt::getAs instead.
 
 class LocatorPathElt::ApplyArgToParam final : public LocatorPathElt {
 public:
-  ApplyArgToParam(unsigned argIdx, unsigned paramIdx)
-      : LocatorPathElt(ConstraintLocator::ApplyArgToParam, argIdx, paramIdx) {}
+  ApplyArgToParam(unsigned argIdx, unsigned paramIdx, ParameterTypeFlags flags)
+      : LocatorPathElt(ConstraintLocator::ApplyArgToParam, argIdx, paramIdx,
+                       flags.toRaw()) {}
 
   unsigned getArgIdx() const { return getValue(0); }
   unsigned getParamIdx() const { return getValue(1); }
+  ParameterTypeFlags getParameterFlags() const {
+    return ParameterTypeFlags::fromRaw(getValue(2));
+  }
 
   static bool classof(const LocatorPathElt *elt) {
     return elt->getKind() == ConstraintLocator::ApplyArgToParam;