Merge pull request #67435 from xedin/rdar-112029630

[CSRanking] Augment overload ranking to account for variadic generics

Author: xedin

include/swift/Sema/ConstraintSystem.h

@@ -6268,11 +6268,16 @@ Type isPlaceholderVar(PatternBindingDecl *PB);
 /// Dump an anchor node for a constraint locator or contextual type.
 void dumpAnchor(ASTNode anchor, SourceManager *SM, raw_ostream &out);
 
+bool isPackExpansionType(Type type);
+
 /// Check whether the type is a tuple consisting of a single unlabeled element
 /// of \c PackExpansionType or a type variable that represents a pack expansion
 /// type.
 bool isSingleUnlabeledPackExpansionTuple(Type type);
 
+bool containsPackExpansionType(ArrayRef<AnyFunctionType::Param> params);
+bool containsPackExpansionType(TupleType *tuple);
+
 /// \returns null if \c type is not a single unlabeled pack expansion tuple.
 Type getPatternTypeOfSingleUnlabeledPackExpansionTuple(Type type);
 

lib/Sema/CSGen.cpp

@@ -592,7 +592,25 @@ namespace {
     // Find the argument type.
     size_t nArgs = expr->getArgs()->size();
     auto fnExpr = expr->getFn();
-    
+
+    auto mustConsiderVariadicGenericOverloads = [&](ValueDecl *overload) {
+      if (overload->getAttrs().hasAttribute<DisfavoredOverloadAttr>())
+        return false;
+
+      auto genericContext = overload->getAsGenericContext();
+      if (!genericContext)
+        return false;
+
+      auto *GPL = genericContext->getGenericParams();
+      if (!GPL)
+        return false;
+
+      return llvm::any_of(GPL->getParams(),
+                          [&](const GenericTypeParamDecl *GP) {
+                            return GP->isParameterPack();
+                          });
+    };
+
     // Check to ensure that we have an OverloadedDeclRef, and that we're not
     // favoring multiple overload constraints. (Otherwise, in this case
     // favoring is useless.
@@ -630,8 +648,9 @@ namespace {
         return nArgs == paramCount.first ||
                nArgs == paramCount.second;
       };
-      
-      favorCallOverloads(expr, CS, isFavoredDecl);
+
+      favorCallOverloads(expr, CS, isFavoredDecl,
+                         mustConsiderVariadicGenericOverloads);
     }
 
     // We only currently perform favoring for unary args.
@@ -655,7 +674,8 @@ namespace {
       // inside an extension context, since any archetypes in the parameter
       // list could match exactly.
       auto mustConsider = [&](ValueDecl *value) -> bool {
-        return isa<ProtocolDecl>(value->getDeclContext());
+        return isa<ProtocolDecl>(value->getDeclContext()) ||
+               mustConsiderVariadicGenericOverloads(value);
       };
 
       favorCallOverloads(expr, CS, isFavoredDecl, mustConsider);

lib/Sema/CSRanking.cpp

@@ -633,75 +633,126 @@ bool CompareDeclSpecializationRequest::evaluate(
     auto params1 = funcTy1->getParams();
     auto params2 = funcTy2->getParams();
 
-    unsigned numParams1 = params1.size();
-    unsigned numParams2 = params2.size();
-    if (numParams1 > numParams2)
-      return completeResult(false);
-
-    // If they both have trailing closures, compare those separately.
-    bool compareTrailingClosureParamsSeparately = false;
-    if (numParams1 > 0 && numParams2 > 0 &&
-        params1.back().getParameterType()->is<AnyFunctionType>() &&
-        params2.back().getParameterType()->is<AnyFunctionType>()) {
-      compareTrailingClosureParamsSeparately = true;
-    }
+    // TODO: We should consider merging these two branches together in
+    //       the future instead of re-implementing `matchCallArguments`.
+    if (containsPackExpansionType(params1) ||
+        containsPackExpansionType(params2)) {
+      ParameterListInfo paramListInfo(params2, decl2, decl2->hasCurriedSelf());
+
+      MatchCallArgumentListener listener;
+      SmallVector<AnyFunctionType::Param> args(params1);
+      auto matching = matchCallArguments(
+          args, params2, paramListInfo, llvm::None,
+          /*allowFixes=*/false, listener, TrailingClosureMatching::Forward);
+
+      if (!matching)
+        return completeResult(false);
+
+      for (unsigned paramIdx = 0,
+                    numParams = matching->parameterBindings.size();
+           paramIdx != numParams; ++paramIdx) {
+        const auto &param = params2[paramIdx];
+        auto paramTy = param.getOldType();
+
+        if (paramListInfo.isVariadicGenericParameter(paramIdx) &&
+            isPackExpansionType(paramTy)) {
+          SmallVector<Type, 2> argTypes;
+          for (auto argIdx : matching->parameterBindings[paramIdx]) {
+            // Don't prefer `T...` over `repeat each T`.
+            if (args[argIdx].isVariadic())
+              return completeResult(false);
+            argTypes.push_back(args[argIdx].getPlainType());
+          }
 
-    auto maybeAddSubtypeConstraint =
-        [&](const AnyFunctionType::Param &param1,
-            const AnyFunctionType::Param &param2) -> bool {
-      // If one parameter is variadic and the other is not...
-      if (param1.isVariadic() != param2.isVariadic()) {
-        // If the first parameter is the variadic one, it's not
-        // more specialized.
-        if (param1.isVariadic())
-          return false;
+          auto *argPack = PackType::get(cs.getASTContext(), argTypes);
+          cs.addConstraint(ConstraintKind::Subtype,
+                           PackExpansionType::get(argPack, argPack), paramTy,
+                           locator);
+          continue;
+        }
+
+        for (auto argIdx : matching->parameterBindings[paramIdx]) {
+          const auto &arg = args[argIdx];
+          // Always prefer non-variadic version when possible.
+          if (arg.isVariadic())
+            return completeResult(false);
 
-        fewerEffectiveParameters = true;
+          cs.addConstraint(ConstraintKind::Subtype, arg.getOldType(),
+                           paramTy, locator);
+        }
+      }
+    } else {
+      unsigned numParams1 = params1.size();
+      unsigned numParams2 = params2.size();
+
+      if (numParams1 > numParams2)
+        return completeResult(false);
+
+      // If they both have trailing closures, compare those separately.
+      bool compareTrailingClosureParamsSeparately = false;
+      if (numParams1 > 0 && numParams2 > 0 &&
+          params1.back().getParameterType()->is<AnyFunctionType>() &&
+          params2.back().getParameterType()->is<AnyFunctionType>()) {
+        compareTrailingClosureParamsSeparately = true;
       }
 
-      Type paramType1 = getAdjustedParamType(param1);
-      Type paramType2 = getAdjustedParamType(param2);
+      auto maybeAddSubtypeConstraint =
+          [&](const AnyFunctionType::Param &param1,
+              const AnyFunctionType::Param &param2) -> bool {
+        // If one parameter is variadic and the other is not...
+        if (param1.isVariadic() != param2.isVariadic()) {
+          // If the first parameter is the variadic one, it's not
+          // more specialized.
+          if (param1.isVariadic())
+            return false;
+
+          fewerEffectiveParameters = true;
+        }
 
-      // Check whether the first parameter is a subtype of the second.
-      cs.addConstraint(ConstraintKind::Subtype, paramType1, paramType2,
-                       locator);
-      return true;
-    };
-
-    auto pairMatcher = [&](unsigned idx1, unsigned idx2) -> bool {
-      // Emulate behavior from when IUO was a type, where IUOs
-      // were considered subtypes of plain optionals, but not
-      // vice-versa.  This wouldn't normally happen, but there are
-      // cases where we can rename imported APIs so that we have a
-      // name collision, and where the parameter type(s) are the
-      // same except for details of the kind of optional declared.
-      auto param1IsIUO = paramIsIUO(decl1, idx1);
-      auto param2IsIUO = paramIsIUO(decl2, idx2);
-      if (param2IsIUO && !param1IsIUO)
-        return false;
-
-      if (!maybeAddSubtypeConstraint(params1[idx1], params2[idx2]))
-        return false;
+        Type paramType1 = getAdjustedParamType(param1);
+        Type paramType2 = getAdjustedParamType(param2);
 
-      return true;
-    };
+        // Check whether the first parameter is a subtype of the second.
+        cs.addConstraint(ConstraintKind::Subtype, paramType1, paramType2,
+                         locator);
+        return true;
+      };
 
-    ParameterListInfo paramInfo(params2, decl2, decl2->hasCurriedSelf());
-    auto params2ForMatching = params2;
-    if (compareTrailingClosureParamsSeparately) {
-      --numParams1;
-      params2ForMatching = params2.drop_back();
-    }
+      auto pairMatcher = [&](unsigned idx1, unsigned idx2) -> bool {
+        // Emulate behavior from when IUO was a type, where IUOs
+        // were considered subtypes of plain optionals, but not
+        // vice-versa.  This wouldn't normally happen, but there are
+        // cases where we can rename imported APIs so that we have a
+        // name collision, and where the parameter type(s) are the
+        // same except for details of the kind of optional declared.
+        auto param1IsIUO = paramIsIUO(decl1, idx1);
+        auto param2IsIUO = paramIsIUO(decl2, idx2);
+        if (param2IsIUO && !param1IsIUO)
+          return false;
+
+        if (!maybeAddSubtypeConstraint(params1[idx1], params2[idx2]))
+          return false;
 
-    InputMatcher IM(params2ForMatching, paramInfo);
-    if (IM.match(numParams1, pairMatcher) != InputMatcher::IM_Succeeded)
-      return completeResult(false);
+        return true;
+      };
+
+      ParameterListInfo paramInfo(params2, decl2, decl2->hasCurriedSelf());
+      auto params2ForMatching = params2;
+      if (compareTrailingClosureParamsSeparately) {
+        --numParams1;
+        params2ForMatching = params2.drop_back();
+      }
+
+      InputMatcher IM(params2ForMatching, paramInfo);
+      if (IM.match(numParams1, pairMatcher) != InputMatcher::IM_Succeeded)
+        return completeResult(false);
 
-    fewerEffectiveParameters |= (IM.getNumSkippedParameters() != 0);
+      fewerEffectiveParameters |= (IM.getNumSkippedParameters() != 0);
 
-    if (compareTrailingClosureParamsSeparately)
-      if (!maybeAddSubtypeConstraint(params1.back(), params2.back()))
-        knownNonSubtype = true;
+      if (compareTrailingClosureParamsSeparately)
+        if (!maybeAddSubtypeConstraint(params1.back(), params2.back()))
+          knownNonSubtype = true;
+    }
   }
 
   if (!knownNonSubtype) {

lib/Sema/CSSimplify.cpp

@@ -117,7 +117,7 @@ static llvm::Optional<unsigned> scoreParamAndArgNameTypo(StringRef paramName,
   return dist;
 }
 
-static bool isPackExpansionType(Type type) {
+bool constraints::isPackExpansionType(Type type) {
   if (type->is<PackExpansionType>())
     return true;
 
@@ -152,13 +152,13 @@ Type constraints::getPatternTypeOfSingleUnlabeledPackExpansionTuple(Type type) {
   return {};
 }
 
-static bool containsPackExpansionType(ArrayRef<AnyFunctionType::Param> params) {
+bool constraints::containsPackExpansionType(ArrayRef<AnyFunctionType::Param> params) {
   return llvm::any_of(params, [&](const auto &param) {
     return isPackExpansionType(param.getPlainType());
   });
 }
 
-static bool containsPackExpansionType(TupleType *tuple) {
+bool constraints::containsPackExpansionType(TupleType *tuple) {
   return llvm::any_of(tuple->getElements(), [&](const auto &elt) {
     return isPackExpansionType(elt.getType());
   });

test/Constraints/pack-expansion-expressions.swift

@@ -680,3 +680,30 @@ do {
     }
   }
 }
+
+// rdar://112029630 - incorrect variadic generic overload ranking
+do {
+  func test1<T>(_: T...) {}
+  // expected-note@-1 {{found this candidate}}
+  func test1<each T>(_: repeat each T) {}
+  // expected-note@-1 {{found this candidate}}
+
+  test1(1, 2, 3) // expected-error {{ambiguous use of 'test1'}}
+  test1(1, "a") // Ok
+
+  func test2<each T>(_: repeat each T) {}
+  // expected-note@-1 {{found this candidate}}
+  func test2<each T>(vals: repeat each T) {}
+  // expected-note@-1 {{found this candidate}}
+
+  test2() // expected-error {{ambiguous use of 'test2'}}
+
+  func test_different_requirements<A: BinaryInteger & StringProtocol>(_ a: A) {
+    func test3<each T: BinaryInteger>(str: String, _: repeat each T) {}
+    // expected-note@-1 {{found this candidate}}
+    func test3<each U: StringProtocol>(str: repeat each U) {}
+    // expected-note@-1 {{found this candidate}}
+
+    test3(str: "", a, a)  // expected-error {{ambiguous use of 'test3'}}
+  }
+}

test/Constraints/variadic_generic_overload_ranking.swift

@@ -0,0 +1,87 @@
+// RUN: %target-swift-emit-silgen %s -verify -swift-version 5 -disable-availability-checking | %FileCheck %s
+
+// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF
+func test_ranking_concrete_over_variadic() {
+  func test() {}
+  func test<T>(_: T) {}
+  func test<each T>(_: repeat each T) {}
+
+  // CHECK: // function_ref test #1 () in test_ranking_concrete_over_variadic()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF0E0L_yyF : $@convention(thin) () -> ()
+  test()
+  // CHECK: // function_ref test #2 <A>(_:) in test_ranking_concrete_over_variadic()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF0E0L0_yyxlF : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  test(1)
+  // CHECK: // function_ref test #3 <each A>(_:) in test_ranking_concrete_over_variadic()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF0E0L1_yyxxQpRvzlF : $@convention(thin) <each τ_0_0> (@pack_guaranteed Pack{repeat each τ_0_0}) -> ()
+  test(1, "")
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_d1_A31_over_concrete_with_conversionsyyF
+func test_ranking_variadic_over_concrete_with_conversions() {
+  func test<T>(_: T, _: Any) {}
+  func test<each T>(_: repeat each T) {}
+
+  // CHECK: // function_ref test #2 <each A>(_:) in test_ranking_variadic_over_concrete_with_conversions()
+  // CHECK-LABEL: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d1_A31_over_concrete_with_conversionsyyF0E0L0_yyxxQpRvzlF : $@convention(thin) <each τ_0_0> (@pack_guaranteed Pack{repeat each τ_0_0}) -> ()
+  test(1, "")
+
+  func test_disfavored<T>(_: T, _: Any) {}
+  @_disfavoredOverload
+  func test_disfavored<each T>(_: repeat each T) {}
+
+  // CHECK: // function_ref test_disfavored #1 <A>(_:_:) in test_ranking_variadic_over_concrete_with_conversions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d1_A31_over_concrete_with_conversionsyyF0E11_disfavoredL_yyx_yptlF : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0, @in_guaranteed Any) -> ()
+  test_disfavored(2, "a")
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_d1_a1_B13_over_regularyyF : $@convention(thin) () -> ()
+func test_ranking_variadic_generic_over_regular() {
+  func test1<T>(_: T...) {}
+  func test1<each T>(_: repeat each T) {}
+
+  // CHECK: // function_ref test1 #2 <each A>(_:) in test_ranking_variadic_generic_over_regular()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d1_a1_B13_over_regularyyF5test1L0_yyxxQpRvzlF : $@convention(thin) <each τ_0_0> (@pack_guaranteed Pack{repeat each τ_0_0}) -> ()
+  test1(1, "a")
+}
+
+protocol P {
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF
+func test_ranking_with_multiple_expansions() {
+  struct Empty : P {}
+  struct Tuple<T> : P {
+    init(_: T) {}
+  }
+
+  struct Builder {
+    static func build() -> Empty { Empty() }
+    static func build<T: P>(_ a: T) -> T { a }
+    static func build<T: P>(_ a: T, _ b: T) -> Tuple<(T, T)> { Tuple((a, b)) }
+    static func build<each T: P>(_ v: repeat each T) -> Tuple<(repeat each T)> { Tuple((repeat each v)) }
+
+    static func otherBuild<T: P, U: P>(a: T, b: U) {}
+    static func otherBuild<each T: P, each U: P>(a: repeat each T, b: repeat each U) {}
+  }
+
+  // CHECK: // function_ref static otherBuild<A, B>(a:b:) in Builder #1 in test_ranking_with_multiple_expansions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V10otherBuild1a1byx_q_tAA1PRzAaHR_r0_lFZ : $@convention(method) <τ_0_0, τ_0_1 where τ_0_0 : P, τ_0_1 : P> (@in_guaranteed τ_0_0, @in_guaranteed τ_0_1, @thin Builder.Type) -> ()
+  Builder.otherBuild(a: Empty(), b: Empty())
+  // CHECK: // function_ref static otherBuild<A, B>(a:b:) in Builder #1 in test_ranking_with_multiple_expansions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V10otherBuild1a1byx_q_tAA1PRzAaHR_r0_lFZ : $@convention(method) <τ_0_0, τ_0_1 where τ_0_0 : P, τ_0_1 : P> (@in_guaranteed τ_0_0, @in_guaranteed τ_0_1, @thin Builder.Type) -> ()
+  Builder.otherBuild(a: Empty(), b: Tuple<Int>(42))
+
+  // CHECK: // function_ref static build() in Builder #1 in test_ranking_with_multiple_expansions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V5buildAaByyF5EmptyL_VyFZ : $@convention(method) (@thin Builder.Type) -> Empty
+  _ = Builder.build()
+  // CHECK: // function_ref static build<A>(_:) in Builder #1 in test_ranking_with_multiple_expansions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V5buildyxxAA1PRzlFZ : $@convention(method) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0, @thin Builder.Type) -> @out τ_0_0
+  _ = Builder.build(Empty())
+  // CHECK: // function_ref static build<A>(_:_:) in Builder #1 in test_ranking_with_multiple_expansions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V5buildyAaByyF5TupleL_Vyx_xtGx_xtAA1PRzlFZ : $@convention(method) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0, @in_guaranteed τ_0_0, @thin Builder.Type) -> Tuple<(τ_0_0, τ_0_0)>
+  _ = Builder.build(Empty(), Empty())
+  // CHECK: // function_ref static build<each A>(_:) in Builder #1 in test_ranking_with_multiple_expansions()
+  // CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V5buildyAaByyF5TupleL_VyxxQp_tGxxQpRvzAA1PRzlFZ : $@convention(method) <each τ_0_0 where repeat each τ_0_0 : P> (@pack_guaranteed Pack{repeat each τ_0_0}, @thin Builder.Type) -> Tuple<(repeat each τ_0_0)>
+  _ = Builder.build(Empty(), Tuple<(Int, String)>((42, "")))
+}