Skip to content

Commit 713a434

Browse files
hborlahborla
authored
Merge pull request #67456 from xedin/rdar-112029630-5.9
[5.9][CSRanking] Augment overload ranking to account for variadic generics
2 parents f3cb88a + 71d5eaa commit 713a434

File tree

6 files changed

+256
-66
lines changed

6 files changed

+256
-66
lines changed

‎include/swift/Sema/ConstraintSystem.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6169,8 +6169,13 @@ Type isPlaceholderVar(PatternBindingDecl *PB);
61696169
/// Dump an anchor node for a constraint locator or contextual type.
61706170
void dumpAnchor(ASTNode anchor, SourceManager *SM, raw_ostream &out);
61716171

6172+
bool isPackExpansionType(Type type);
6173+
61726174
bool isSingleUnlabeledPackExpansionTuple(Type type);
61736175

6176+
bool containsPackExpansionType(ArrayRef<AnyFunctionType::Param> params);
6177+
bool containsPackExpansionType(TupleType *tuple);
6178+
61746179
/// \returns null if \c type is not a single unlabeled pack expansion tuple.
61756180
Type getPatternTypeOfSingleUnlabeledPackExpansionTuple(Type type);
61766181

‎lib/Sema/CSGen.cpp

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,25 @@ namespace {
592592
// Find the argument type.
593593
size_t nArgs = expr->getArgs()->size();
594594
auto fnExpr = expr->getFn();
595-
595+
596+
auto mustConsiderVariadicGenericOverloads = [&](ValueDecl *overload) {
597+
if (overload->getAttrs().hasAttribute<DisfavoredOverloadAttr>())
598+
return false;
599+
600+
auto genericContext = overload->getAsGenericContext();
601+
if (!genericContext)
602+
return false;
603+
604+
auto *GPL = genericContext->getGenericParams();
605+
if (!GPL)
606+
return false;
607+
608+
return llvm::any_of(GPL->getParams(),
609+
[&](const GenericTypeParamDecl *GP) {
610+
return GP->isParameterPack();
611+
});
612+
};
613+
596614
// Check to ensure that we have an OverloadedDeclRef, and that we're not
597615
// favoring multiple overload constraints. (Otherwise, in this case
598616
// favoring is useless.
@@ -630,8 +648,9 @@ namespace {
630648
return nArgs == paramCount.first ||
631649
nArgs == paramCount.second;
632650
};
633-
634-
favorCallOverloads(expr, CS, isFavoredDecl);
651+
652+
favorCallOverloads(expr, CS, isFavoredDecl,
653+
mustConsiderVariadicGenericOverloads);
635654
}
636655

637656
// We only currently perform favoring for unary args.
@@ -655,7 +674,8 @@ namespace {
655674
// inside an extension context, since any archetypes in the parameter
656675
// list could match exactly.
657676
auto mustConsider = [&](ValueDecl *value) -> bool {
658-
return isa<ProtocolDecl>(value->getDeclContext());
677+
return isa<ProtocolDecl>(value->getDeclContext()) ||
678+
mustConsiderVariadicGenericOverloads(value);
659679
};
660680

661681
favorCallOverloads(expr, CS, isFavoredDecl, mustConsider);

‎lib/Sema/CSRanking.cpp

Lines changed: 110 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -567,75 +567,126 @@ bool CompareDeclSpecializationRequest::evaluate(
567567
auto params1 = funcTy1->getParams();
568568
auto params2 = funcTy2->getParams();
569569

570-
unsigned numParams1 = params1.size();
571-
unsigned numParams2 = params2.size();
572-
if (numParams1 > numParams2)
573-
return completeResult(false);
574-
575-
// If they both have trailing closures, compare those separately.
576-
bool compareTrailingClosureParamsSeparately = false;
577-
if (numParams1 > 0 && numParams2 > 0 &&
578-
params1.back().getParameterType()->is<AnyFunctionType>() &&
579-
params2.back().getParameterType()->is<AnyFunctionType>()) {
580-
compareTrailingClosureParamsSeparately = true;
581-
}
570+
// TODO: We should consider merging these two branches together in
571+
// the future instead of re-implementing `matchCallArguments`.
572+
if (containsPackExpansionType(params1) ||
573+
containsPackExpansionType(params2)) {
574+
ParameterListInfo paramListInfo(params2, decl2, decl2->hasCurriedSelf());
575+
576+
MatchCallArgumentListener listener;
577+
SmallVector<AnyFunctionType::Param> args(params1);
578+
auto matching = matchCallArguments(
579+
args, params2, paramListInfo, llvm::None,
580+
/*allowFixes=*/false, listener, TrailingClosureMatching::Forward);
581+
582+
if (!matching)
583+
return completeResult(false);
584+
585+
for (unsigned paramIdx = 0,
586+
numParams = matching->parameterBindings.size();
587+
paramIdx != numParams; ++paramIdx) {
588+
const auto &param = params2[paramIdx];
589+
auto paramTy = param.getOldType();
590+
591+
if (paramListInfo.isVariadicGenericParameter(paramIdx) &&
592+
isPackExpansionType(paramTy)) {
593+
SmallVector<Type, 2> argTypes;
594+
for (auto argIdx : matching->parameterBindings[paramIdx]) {
595+
// Don't prefer `T...` over `repeat each T`.
596+
if (args[argIdx].isVariadic())
597+
return completeResult(false);
598+
argTypes.push_back(args[argIdx].getPlainType());
599+
}
582600

583-
auto maybeAddSubtypeConstraint =
584-
[&](const AnyFunctionType::Param &param1,
585-
const AnyFunctionType::Param &param2) -> bool {
586-
// If one parameter is variadic and the other is not...
587-
if (param1.isVariadic() != param2.isVariadic()) {
588-
// If the first parameter is the variadic one, it's not
589-
// more specialized.
590-
if (param1.isVariadic())
591-
return false;
601+
auto *argPack = PackType::get(cs.getASTContext(), argTypes);
602+
cs.addConstraint(ConstraintKind::Subtype,
603+
PackExpansionType::get(argPack, argPack), paramTy,
604+
locator);
605+
continue;
606+
}
607+
608+
for (auto argIdx : matching->parameterBindings[paramIdx]) {
609+
const auto &arg = args[argIdx];
610+
// Always prefer non-variadic version when possible.
611+
if (arg.isVariadic())
612+
return completeResult(false);
592613

593-
fewerEffectiveParameters = true;
614+
cs.addConstraint(ConstraintKind::Subtype, arg.getOldType(),
615+
paramTy, locator);
616+
}
617+
}
618+
} else {
619+
unsigned numParams1 = params1.size();
620+
unsigned numParams2 = params2.size();
621+
622+
if (numParams1 > numParams2)
623+
return completeResult(false);
624+
625+
// If they both have trailing closures, compare those separately.
626+
bool compareTrailingClosureParamsSeparately = false;
627+
if (numParams1 > 0 && numParams2 > 0 &&
628+
params1.back().getParameterType()->is<AnyFunctionType>() &&
629+
params2.back().getParameterType()->is<AnyFunctionType>()) {
630+
compareTrailingClosureParamsSeparately = true;
594631
}
595632

596-
Type paramType1 = getAdjustedParamType(param1);
597-
Type paramType2 = getAdjustedParamType(param2);
633+
auto maybeAddSubtypeConstraint =
634+
[&](const AnyFunctionType::Param &param1,
635+
const AnyFunctionType::Param &param2) -> bool {
636+
// If one parameter is variadic and the other is not...
637+
if (param1.isVariadic() != param2.isVariadic()) {
638+
// If the first parameter is the variadic one, it's not
639+
// more specialized.
640+
if (param1.isVariadic())
641+
return false;
642+
643+
fewerEffectiveParameters = true;
644+
}
598645

599-
// Check whether the first parameter is a subtype of the second.
600-
cs.addConstraint(ConstraintKind::Subtype, paramType1, paramType2,
601-
locator);
602-
return true;
603-
};
604-
605-
auto pairMatcher = [&](unsigned idx1, unsigned idx2) -> bool {
606-
// Emulate behavior from when IUO was a type, where IUOs
607-
// were considered subtypes of plain optionals, but not
608-
// vice-versa. This wouldn't normally happen, but there are
609-
// cases where we can rename imported APIs so that we have a
610-
// name collision, and where the parameter type(s) are the
611-
// same except for details of the kind of optional declared.
612-
auto param1IsIUO = paramIsIUO(decl1, idx1);
613-
auto param2IsIUO = paramIsIUO(decl2, idx2);
614-
if (param2IsIUO && !param1IsIUO)
615-
return false;
616-
617-
if (!maybeAddSubtypeConstraint(params1[idx1], params2[idx2]))
618-
return false;
646+
Type paramType1 = getAdjustedParamType(param1);
647+
Type paramType2 = getAdjustedParamType(param2);
619648

620-
return true;
621-
};
649+
// Check whether the first parameter is a subtype of the second.
650+
cs.addConstraint(ConstraintKind::Subtype, paramType1, paramType2,
651+
locator);
652+
return true;
653+
};
622654

623-
ParameterListInfo paramInfo(params2, decl2, decl2->hasCurriedSelf());
624-
auto params2ForMatching = params2;
625-
if (compareTrailingClosureParamsSeparately) {
626-
--numParams1;
627-
params2ForMatching = params2.drop_back();
628-
}
655+
auto pairMatcher = [&](unsigned idx1, unsigned idx2) -> bool {
656+
// Emulate behavior from when IUO was a type, where IUOs
657+
// were considered subtypes of plain optionals, but not
658+
// vice-versa. This wouldn't normally happen, but there are
659+
// cases where we can rename imported APIs so that we have a
660+
// name collision, and where the parameter type(s) are the
661+
// same except for details of the kind of optional declared.
662+
auto param1IsIUO = paramIsIUO(decl1, idx1);
663+
auto param2IsIUO = paramIsIUO(decl2, idx2);
664+
if (param2IsIUO && !param1IsIUO)
665+
return false;
666+
667+
if (!maybeAddSubtypeConstraint(params1[idx1], params2[idx2]))
668+
return false;
629669

630-
InputMatcher IM(params2ForMatching, paramInfo);
631-
if (IM.match(numParams1, pairMatcher) != InputMatcher::IM_Succeeded)
632-
return completeResult(false);
670+
return true;
671+
};
672+
673+
ParameterListInfo paramInfo(params2, decl2, decl2->hasCurriedSelf());
674+
auto params2ForMatching = params2;
675+
if (compareTrailingClosureParamsSeparately) {
676+
--numParams1;
677+
params2ForMatching = params2.drop_back();
678+
}
679+
680+
InputMatcher IM(params2ForMatching, paramInfo);
681+
if (IM.match(numParams1, pairMatcher) != InputMatcher::IM_Succeeded)
682+
return completeResult(false);
633683

634-
fewerEffectiveParameters |= (IM.getNumSkippedParameters() != 0);
684+
fewerEffectiveParameters |= (IM.getNumSkippedParameters() != 0);
635685

636-
if (compareTrailingClosureParamsSeparately)
637-
if (!maybeAddSubtypeConstraint(params1.back(), params2.back()))
638-
knownNonSubtype = true;
686+
if (compareTrailingClosureParamsSeparately)
687+
if (!maybeAddSubtypeConstraint(params1.back(), params2.back()))
688+
knownNonSubtype = true;
689+
}
639690
}
640691

641692
if (!knownNonSubtype) {

‎lib/Sema/CSSimplify.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ static llvm::Optional<unsigned> scoreParamAndArgNameTypo(StringRef paramName,
117117
return dist;
118118
}
119119

120-
static bool isPackExpansionType(Type type) {
120+
bool constraints::isPackExpansionType(Type type) {
121121
if (type->is<PackExpansionType>())
122122
return true;
123123

@@ -143,13 +143,13 @@ Type constraints::getPatternTypeOfSingleUnlabeledPackExpansionTuple(Type type) {
143143
return expansion->getPatternType();
144144
}
145145

146-
static bool containsPackExpansionType(ArrayRef<AnyFunctionType::Param> params) {
146+
bool constraints::containsPackExpansionType(ArrayRef<AnyFunctionType::Param> params) {
147147
return llvm::any_of(params, [&](const auto &param) {
148148
return isPackExpansionType(param.getPlainType());
149149
});
150150
}
151151

152-
static bool containsPackExpansionType(TupleType *tuple) {
152+
bool constraints::containsPackExpansionType(TupleType *tuple) {
153153
return llvm::any_of(tuple->getElements(), [&](const auto &elt) {
154154
return isPackExpansionType(elt.getType());
155155
});

‎test/Constraints/pack-expansion-expressions.swift

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,3 +634,30 @@ do {
634634
}
635635
}
636636
}
637+
638+
// rdar://112029630 - incorrect variadic generic overload ranking
639+
do {
640+
func test1<T>(_: T...) {}
641+
// expected-note@-1 {{found this candidate}}
642+
func test1<each T>(_: repeat each T) {}
643+
// expected-note@-1 {{found this candidate}}
644+
645+
test1(1, 2, 3) // expected-error {{ambiguous use of 'test1'}}
646+
test1(1, "a") // Ok
647+
648+
func test2<each T>(_: repeat each T) {}
649+
// expected-note@-1 {{found this candidate}}
650+
func test2<each T>(vals: repeat each T) {}
651+
// expected-note@-1 {{found this candidate}}
652+
653+
test2() // expected-error {{ambiguous use of 'test2'}}
654+
655+
func test_different_requirements<A: BinaryInteger & StringProtocol>(_ a: A) {
656+
func test3<each T: BinaryInteger>(str: String, _: repeat each T) {}
657+
// expected-note@-1 {{found this candidate}}
658+
func test3<each U: StringProtocol>(str: repeat each U) {}
659+
// expected-note@-1 {{found this candidate}}
660+
661+
test3(str: "", a, a) // expected-error {{ambiguous use of 'test3'}}
662+
}
663+
}

‎test/Constraints/variadic_generic_overload_ranking.swift

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// RUN: %target-swift-emit-silgen %s -verify -swift-version 5 -disable-availability-checking | %FileCheck %s
2+
3+
// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF
4+
func test_ranking_concrete_over_variadic() {
5+
func test() {}
6+
func test<T>(_: T) {}
7+
func test<each T>(_: repeat each T) {}
8+
9+
// CHECK: // function_ref test #1 () in test_ranking_concrete_over_variadic()
10+
// CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF0E0L_yyF : $@convention(thin) () -> ()
11+
test()
12+
// CHECK: // function_ref test #2 <A>(_:) in test_ranking_concrete_over_variadic()
13+
// CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_d15_concrete_over_A0yyF0E0L0_yyxlF : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
14+
test(1)
15+
// CHECK: // function_ref test #3 <each A>(_:) in test_ranking_concrete_over_variadic()
16+
// 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}) -> ()
17+
test(1, "")
18+
}
19+
20+
// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_d1_A31_over_concrete_with_conversionsyyF
21+
func test_ranking_variadic_over_concrete_with_conversions() {
22+
func test<T>(_: T, _: Any) {}
23+
func test<each T>(_: repeat each T) {}
24+
25+
// CHECK: // function_ref test #2 <each A>(_:) in test_ranking_variadic_over_concrete_with_conversions()
26+
// 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}) -> ()
27+
test(1, "")
28+
29+
func test_disfavored<T>(_: T, _: Any) {}
30+
@_disfavoredOverload
31+
func test_disfavored<each T>(_: repeat each T) {}
32+
33+
// CHECK: // function_ref test_disfavored #1 <A>(_:_:) in test_ranking_variadic_over_concrete_with_conversions()
34+
// 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) -> ()
35+
test_disfavored(2, "a")
36+
}
37+
38+
// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_d1_a1_B13_over_regularyyF : $@convention(thin) () -> ()
39+
func test_ranking_variadic_generic_over_regular() {
40+
func test1<T>(_: T...) {}
41+
func test1<each T>(_: repeat each T) {}
42+
43+
// CHECK: // function_ref test1 #2 <each A>(_:) in test_ranking_variadic_generic_over_regular()
44+
// 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}) -> ()
45+
test1(1, "a")
46+
}
47+
48+
protocol P {
49+
}
50+
51+
// CHECK-LABEL: sil hidden [ossa] @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF
52+
func test_ranking_with_multiple_expansions() {
53+
struct Empty : P {}
54+
struct Tuple<T> : P {
55+
init(_: T) {}
56+
}
57+
58+
struct Builder {
59+
static func build() -> Empty { Empty() }
60+
static func build<T: P>(_ a: T) -> T { a }
61+
static func build<T: P>(_ a: T, _ b: T) -> Tuple<(T, T)> { Tuple((a, b)) }
62+
static func build<each T: P>(_ v: repeat each T) -> Tuple<(repeat each T)> { Tuple((repeat each v)) }
63+
64+
static func otherBuild<T: P, U: P>(a: T, b: U) {}
65+
static func otherBuild<each T: P, each U: P>(a: repeat each T, b: repeat each U) {}
66+
}
67+
68+
// CHECK: // function_ref static otherBuild<A, B>(a:b:) in Builder #1 in test_ranking_with_multiple_expansions()
69+
// 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) -> ()
70+
Builder.otherBuild(a: Empty(), b: Empty())
71+
// CHECK: // function_ref static otherBuild<A, B>(a:b:) in Builder #1 in test_ranking_with_multiple_expansions()
72+
// 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) -> ()
73+
Builder.otherBuild(a: Empty(), b: Tuple<Int>(42))
74+
75+
// CHECK: // function_ref static build() in Builder #1 in test_ranking_with_multiple_expansions()
76+
// CHECK-NEXT: {{.*}} = function_ref @$s33variadic_generic_overload_ranking05test_D25_with_multiple_expansionsyyF7BuilderL_V5buildAaByyF5EmptyL_VyFZ : $@convention(method) (@thin Builder.Type) -> Empty
77+
_ = Builder.build()
78+
// CHECK: // function_ref static build<A>(_:) in Builder #1 in test_ranking_with_multiple_expansions()
79+
// 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
80+
_ = Builder.build(Empty())
81+
// CHECK: // function_ref static build<A>(_:_:) in Builder #1 in test_ranking_with_multiple_expansions()
82+
// 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)>
83+
_ = Builder.build(Empty(), Empty())
84+
// CHECK: // function_ref static build<each A>(_:) in Builder #1 in test_ranking_with_multiple_expansions()
85+
// 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)>
86+
_ = Builder.build(Empty(), Tuple<(Int, String)>((42, "")))
87+
}

0 commit comments

Comments
 (0)