Skip to content

Commit ea00508

Browse files
rudkxrudkx
authored
Merge pull request #18951 from hamishknight/concrete-is-better-swift5
[CSRanking] Favour concrete members over protocol requirements in Swift 5 mode
2 parents 0a15fb6 + bee01ed commit ea00508

File tree

5 files changed

+222
-33
lines changed

5 files changed

+222
-33
lines changed

‎lib/Sema/CSRanking.cpp

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -389,15 +389,18 @@ static bool paramIsIUO(Decl *decl, int paramNum) {
389389
///
390390
/// "Specialized" is essentially a form of subtyping, defined below.
391391
static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
392-
ValueDecl *decl1, ValueDecl *decl2) {
392+
ValueDecl *decl1, ValueDecl *decl2,
393+
bool isDynamicOverloadComparison = false) {
393394

394395
if (tc.getLangOpts().DebugConstraintSolver) {
395396
auto &log = tc.Context.TypeCheckerDebug->getStream();
396397
log << "Comparing declarations\n";
397398
decl1->print(log);
398399
log << "\nand\n";
399400
decl2->print(log);
400-
log << "\n";
401+
log << "\n(isDynamicOverloadComparison: ";
402+
log << isDynamicOverloadComparison;
403+
log << ")\n";
401404
}
402405

403406
auto *innerDC1 = decl1->getInnermostDeclContext();
@@ -406,7 +409,9 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
406409
auto *outerDC1 = decl1->getDeclContext();
407410
auto *outerDC2 = decl2->getDeclContext();
408411

409-
if (!tc.specializedOverloadComparisonCache.count({decl1, decl2})) {
412+
auto overloadComparisonKey =
413+
std::make_tuple(decl1, decl2, isDynamicOverloadComparison);
414+
if (!tc.specializedOverloadComparisonCache.count(overloadComparisonKey)) {
410415

411416
auto compareSpecializations = [&] () -> bool {
412417
// If the kinds are different, there's nothing we can do.
@@ -446,6 +451,32 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
446451
return inProtocolExtension2;
447452
}
448453

454+
// A concrete type member is always more specialised than a protocol
455+
// member (bearing in mind that we have already handled the case where
456+
// exactly one member is in a protocol extension). Only apply this rule in
457+
// Swift 5 mode to better maintain source compatibility under Swift 4
458+
// mode.
459+
//
460+
// Don't apply this rule when comparing two overloads found through
461+
// dynamic lookup to ensure we keep cases like this ambiguous:
462+
//
463+
// @objc protocol P {
464+
// var i: String { get }
465+
// }
466+
// class C {
467+
// @objc var i: Int { return 0 }
468+
// }
469+
// func foo(_ x: AnyObject) {
470+
// x.i // ensure ambiguous.
471+
// }
472+
//
473+
if (tc.Context.isSwiftVersionAtLeast(5) && !isDynamicOverloadComparison) {
474+
auto *proto1 = dyn_cast<ProtocolDecl>(outerDC1);
475+
auto *proto2 = dyn_cast<ProtocolDecl>(outerDC2);
476+
if (proto1 != proto2)
477+
return proto2;
478+
}
479+
449480
Type type1 = decl1->getInterfaceType();
450481
Type type2 = decl2->getInterfaceType();
451482

@@ -697,21 +728,21 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
697728
return false;
698729
};
699730

700-
tc.specializedOverloadComparisonCache[{decl1, decl2}] =
731+
tc.specializedOverloadComparisonCache[overloadComparisonKey] =
701732
compareSpecializations();
702733
} else if (tc.getLangOpts().DebugConstraintSolver) {
703734
auto &log = tc.Context.TypeCheckerDebug->getStream();
704-
log << "Found cached comparison: "
705-
<< tc.specializedOverloadComparisonCache[{decl1, decl2}] << "\n";
735+
log << "Found cached comparison: "
736+
<< tc.specializedOverloadComparisonCache[overloadComparisonKey] << "\n";
706737
}
707738

708739
if (tc.getLangOpts().DebugConstraintSolver) {
709740
auto &log = tc.Context.TypeCheckerDebug->getStream();
710-
auto result = tc.specializedOverloadComparisonCache[{decl1, decl2}];
741+
auto result = tc.specializedOverloadComparisonCache[overloadComparisonKey];
711742
log << "comparison result: " << (result ? "better" : "not better") << "\n";
712743
}
713744

714-
return tc.specializedOverloadComparisonCache[{decl1, decl2}];
745+
return tc.specializedOverloadComparisonCache[overloadComparisonKey];
715746
}
716747

717748
Comparison TypeChecker::compareDeclarations(DeclContext *dc,
@@ -862,15 +893,23 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
862893
case OverloadChoiceKind::DynamicMemberLookup:
863894
break;
864895
}
865-
896+
897+
// We don't apply some ranking rules to overloads found through dynamic
898+
// lookup in order to keep a few potentially ill-formed cases ambiguous.
899+
bool isDynamicOverloadComparison =
900+
choice1.getKind() == OverloadChoiceKind::DeclViaDynamic &&
901+
choice2.getKind() == OverloadChoiceKind::DeclViaDynamic;
902+
866903
// Determine whether one declaration is more specialized than the other.
867904
bool firstAsSpecializedAs = false;
868905
bool secondAsSpecializedAs = false;
869-
if (isDeclAsSpecializedAs(tc, cs.DC, decl1, decl2)) {
906+
if (isDeclAsSpecializedAs(tc, cs.DC, decl1, decl2,
907+
isDynamicOverloadComparison)) {
870908
score1 += weight;
871909
firstAsSpecializedAs = true;
872910
}
873-
if (isDeclAsSpecializedAs(tc, cs.DC, decl2, decl1)) {
911+
if (isDeclAsSpecializedAs(tc, cs.DC, decl2, decl1,
912+
isDynamicOverloadComparison)) {
874913
score2 += weight;
875914
secondAsSpecializedAs = true;
876915
}

‎lib/Sema/TypeChecker.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -555,9 +555,11 @@ class TypeChecker final : public LazyResolver {
555555
SmallVector<NominalTypeDecl*, 8> DelayedCircularityChecks;
556556

557557
// Caches whether a given declaration is "as specialized" as another.
558-
llvm::DenseMap<std::pair<ValueDecl*, ValueDecl*>, bool>
559-
specializedOverloadComparisonCache;
560-
558+
llvm::DenseMap<std::tuple<ValueDecl *, ValueDecl *,
559+
/*isDynamicOverloadComparison*/ unsigned>,
560+
bool>
561+
specializedOverloadComparisonCache;
562+
561563
/// A list of closures for the most recently type-checked function, which we
562564
/// will need to compute captures for.
563565
std::vector<AnyFunctionRef> ClosuresWithUncomputedCaptures;

‎test/Constraints/dynamic_lookup.swift

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// RUN: %empty-directory(%t)
22
// RUN: %target-swift-frontend -emit-module %S/Inputs/PrivateObjC.swift -o %t
3-
// RUN: %target-typecheck-verify-swift -I %t -verify-ignore-unknown
3+
// RUN: %target-typecheck-verify-swift -swift-version 4 -I %t -verify-ignore-unknown
4+
// RUN: %target-typecheck-verify-swift -swift-version 5 -I %t -verify-ignore-unknown
45

56
// REQUIRES: objc_interop
67
import Foundation
@@ -341,18 +342,51 @@ func dynamicInitCrash(ao: AnyObject.Type) {
341342
// expected-error@-1 {{incorrect argument label in call (have 'blahblah:', expected 'toMemory:')}}
342343
}
343344

344-
// Test that we correctly diagnose ambiguity for different typed properties available
345+
// Test that we correctly diagnose ambiguity for different typed members available
345346
// through dynamic lookup.
346347
@objc protocol P3 {
347-
var i: UInt { get } // expected-note {{found this candidate}}
348-
var j: Int { get }
348+
var ambiguousProperty: String { get } // expected-note {{found this candidate}}
349+
var unambiguousProperty: Int { get }
350+
351+
func ambiguousMethod() -> String // expected-note 2{{found this candidate}}
352+
func unambiguousMethod() -> Int
353+
354+
func ambiguousMethodParam(_ x: String) // expected-note {{found this candidate}}
355+
func unambiguousMethodParam(_ x: Int)
356+
357+
subscript(ambiguousSubscript _: Int) -> String { get } // expected-note {{found this candidate}}
358+
subscript(unambiguousSubscript _: String) -> Int { get } // expected-note {{found this candidate}}
349359
}
350360

351361
class C1 {
352-
@objc var i: Int { return 0 } // expected-note {{found this candidate}}
353-
@objc var j: Int { return 0 }
362+
@objc var ambiguousProperty: Int { return 0 } // expected-note {{found this candidate}}
363+
@objc var unambiguousProperty: Int { return 0 }
364+
365+
@objc func ambiguousMethod() -> Int { return 0 } // expected-note 2{{found this candidate}}
366+
@objc func unambiguousMethod() -> Int { return 0 }
367+
368+
@objc func ambiguousMethodParam(_ x: Int) {} // expected-note {{found this candidate}}
369+
@objc func unambiguousMethodParam(_ x: Int) {}
370+
371+
@objc subscript(ambiguousSubscript _: Int) -> Int { return 0 } // expected-note {{found this candidate}}
372+
@objc subscript(unambiguousSubscript _: String) -> Int { return 0 } // expected-note {{found this candidate}}
354373
}
355374

356-
_ = (C1() as AnyObject).i // expected-error {{ambiguous use of 'i'}}
357-
_ = (C1() as AnyObject).j // okay
375+
func testAnyObjectAmbiguity(_ x: AnyObject) {
376+
_ = x.ambiguousProperty // expected-error {{ambiguous use of 'ambiguousProperty'}}
377+
_ = x.unambiguousProperty
358378

379+
_ = x.ambiguousMethod() // expected-error {{ambiguous use of 'ambiguousMethod()'}}
380+
_ = x.unambiguousMethod()
381+
382+
_ = x.ambiguousMethod // expected-error {{ambiguous use of 'ambiguousMethod()'}}
383+
_ = x.unambiguousMethod
384+
385+
_ = x.ambiguousMethodParam // expected-error {{ambiguous use of 'ambiguousMethodParam'}}
386+
_ = x.unambiguousMethodParam
387+
388+
_ = x[ambiguousSubscript: 0] // expected-error {{ambiguous use of 'subscript(ambiguousSubscript:)'}}
389+
390+
// FIX-ME(SR-8611): This is currently ambiguous but shouldn't be.
391+
_ = x[unambiguousSubscript: ""] // expected-error {{ambiguous use of 'subscript(unambiguousSubscript:)'}}
392+
}

‎test/Constraints/ranking.swift

Lines changed: 114 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-swift-emit-silgen %s -verify | %FileCheck %s
1+
// RUN: %target-swift-emit-silgen %s -verify -swift-version 5 | %FileCheck %s
22

33
protocol P {
44
var p: P { get set }
@@ -156,6 +156,119 @@ func testDerived(b: B) {
156156
// CHECK: end sil function '$S7ranking11testDerived1byAA1BC_tF'
157157
}
158158

159+
protocol X {
160+
var foo: Int { get }
161+
var bar: Int { get }
162+
func baz() -> Int
163+
subscript(foo: String) -> Int { get }
164+
}
165+
166+
class Y {
167+
var foo: Int = 0
168+
func baz() -> Int { return foo }
169+
subscript(foo: String) -> Int { return 0 }
170+
}
171+
extension Y {
172+
var bar: Int { return foo }
173+
}
174+
175+
protocol Z : Y {
176+
var foo: Int { get }
177+
var bar: Int { get }
178+
func baz() -> Int
179+
subscript(foo: String) -> Int { get }
180+
}
181+
182+
class GenericClass<T> {
183+
var foo: T
184+
init(_ foo: T) { self.foo = foo }
185+
func baz() -> T { return foo }
186+
}
187+
extension GenericClass {
188+
var bar: T { return foo }
189+
subscript(foo: String) -> Int { return 0 }
190+
}
191+
192+
// Make sure we favour the class implementation over the protocol requirement.
193+
194+
// CHECK-LABEL: sil hidden @$S7ranking32testGenericPropertyProtocolClassyyxAA1YCRbzAA1XRzlF
195+
func testGenericPropertyProtocolClass<T : X & Y>(_ t: T) {
196+
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
197+
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
198+
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
199+
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
200+
}
201+
202+
// CHECK-LABEL: sil hidden @$S7ranking36testExistentialPropertyProtocolClassyyAA1X_AA1YCXcF
203+
func testExistentialPropertyProtocolClass(_ t: X & Y) {
204+
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
205+
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
206+
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
207+
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
208+
}
209+
210+
// CHECK-LABEL: sil hidden @$S7ranking46testGenericPropertySubclassConstrainedProtocolyyxAA1ZRzlF
211+
func testGenericPropertySubclassConstrainedProtocol<T : Z>(_ t: T) {
212+
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
213+
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
214+
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
215+
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
216+
}
217+
218+
// CHECK-LABEL: sil hidden @$S7ranking50testExistentialPropertySubclassConstrainedProtocolyyAA1Z_pF
219+
func testExistentialPropertySubclassConstrainedProtocol(_ t: Z) {
220+
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
221+
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
222+
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
223+
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
224+
}
225+
226+
// CHECK-LABEL: sil hidden @$S7ranking43testExistentialPropertyProtocolGenericClassyyAA1X_AA0fG0CySiGXcF
227+
func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<Int> & X) {
228+
_ = t.foo // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.foo!getter.1
229+
_ = t.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
230+
_ = t.baz() // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.baz
231+
_ = t[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
232+
}
233+
234+
// CHECK-LABEL: sil hidden @$S7ranking43testExistentialPropertyProtocolGenericClassyyAA1X_AA0fG0CySSGXcF
235+
func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<String> & X) {
236+
_ = t.foo // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.foo!getter.1
237+
_ = t.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
238+
_ = t.baz() // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.baz
239+
_ = t[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
240+
}
241+
242+
extension X where Self : Y {
243+
// CHECK-LABEL: sil hidden @$S7ranking1XPA2A1YCRbzrlE32testGenericPropertyProtocolClassyyxF
244+
func testGenericPropertyProtocolClass(_ x: Self) {
245+
_ = self.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
246+
_ = self.bar // CHECK: function_ref @$S7ranking1YC3barSivg
247+
_ = self.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
248+
_ = self[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
249+
}
250+
}
251+
252+
extension X where Self : GenericClass<Int> {
253+
// CHECK-LABEL: sil hidden @$S7ranking1XPA2A12GenericClassCySiGRbzrlE04testb16PropertyProtocolbC0yyxF
254+
func testGenericPropertyProtocolGenericClass(_ x: Self) {
255+
_ = self.foo // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.foo!getter.1
256+
_ = self.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
257+
_ = self.baz() // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.baz
258+
_ = self[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
259+
}
260+
}
261+
262+
extension X where Self : GenericClass<String> {
263+
// CHECK-LABEL: sil hidden @$S7ranking1XPA2A12GenericClassCySSGRbzrlE04testb16PropertyProtocolbC0yyxF
264+
func testGenericPropertyProtocolGenericClass(_ x: Self) {
265+
_ = self.foo // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.foo!getter.1
266+
_ = self.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
267+
_ = self.baz() // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.baz
268+
_ = self[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
269+
}
270+
}
271+
159272
//--------------------------------------------------------------------
160273
// Pointer conversions
161274
//--------------------------------------------------------------------

‎test/Constraints/rdar39401774.swift

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
1-
// RUN: %target-typecheck-verify-swift
1+
// RUN: %target-typecheck-verify-swift -swift-version 5
22

33
class A<T> {
4-
var foo: Int? { return 42 } // expected-note {{found this candidate}}
5-
func baz() -> T { fatalError() } // expected-note {{found this candidate}}
6-
func fiz() -> Int { return 42 } // expected-note {{found this candidate}}
4+
var foo: Int? { return 42 }
5+
func baz() -> T { fatalError() }
6+
func fiz() -> Int { return 42 }
77
}
88

99
protocol P1 {
1010
associatedtype T
11-
var foo: Int? { get } // expected-note {{found this candidate}}
12-
func baz() -> T // expected-note {{found this candidate}}
13-
func fiz() -> Int // expected-note {{found this candidate}}
11+
var foo: Int? { get }
12+
func baz() -> T
13+
func fiz() -> Int
1414
}
1515

1616
protocol P2 : P1 {
@@ -19,8 +19,9 @@ protocol P2 : P1 {
1919

2020
extension P2 where Self: A<Int> {
2121
var bar: Int? {
22-
guard let foo = foo else { return 0 } // expected-error {{ambiguous use of 'foo'}}
23-
let _ = baz() // expected-error {{ambiguous use of 'baz()'}}
24-
return fiz() // expected-error {{ambiguous use of 'fiz()'}}
22+
guard let foo = foo else { return 0 }
23+
_ = foo
24+
let _ = baz()
25+
return fiz()
2526
}
2627
}

0 commit comments

Comments
 (0)