Skip to content

[CSRanking] Favour concrete members over protocol requirements in Swift 5 mode #18951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 30, 2018
Merged

[CSRanking] Favour concrete members over protocol requirements in Swift 5 mode #18951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1520dbb
[test] Expand AnyObject ambiguity test to cover methods and subscripts
hamishknight Aug 22, 2018
286f164
[CSRanking] Favour members on concrete types over protocol members
hamishknight Aug 9, 2018
1912d28
[CSRanking] Exclude concrete-over-protocol rule from dynamic lookup r…
hamishknight Aug 14, 2018
37a9490
[Revert Me] Disable language version check temporarily
hamishknight Aug 24, 2018
b72d7e2
Merge remote-tracking branch 'upstream/master' into concrete-is-bette…
hamishknight Aug 29, 2018
bee01ed
Revert "[Revert Me] Disable language version check temporarily"
hamishknight Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 50 additions & 11 deletions lib/Sema/CSRanking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,15 +389,18 @@ static bool paramIsIUO(Decl *decl, int paramNum) {
///
/// "Specialized" is essentially a form of subtyping, defined below.
static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
ValueDecl *decl1, ValueDecl *decl2) {
ValueDecl *decl1, ValueDecl *decl2,
bool isDynamicOverloadComparison = false) {

if (tc.getLangOpts().DebugConstraintSolver) {
auto &log = tc.Context.TypeCheckerDebug->getStream();
log << "Comparing declarations\n";
decl1->print(log);
log << "\nand\n";
decl2->print(log);
log << "\n";
log << "\n(isDynamicOverloadComparison: ";
log << isDynamicOverloadComparison;
log << ")\n";
}

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

if (!tc.specializedOverloadComparisonCache.count({decl1, decl2})) {
auto overloadComparisonKey =
std::make_tuple(decl1, decl2, isDynamicOverloadComparison);
if (!tc.specializedOverloadComparisonCache.count(overloadComparisonKey)) {

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

// A concrete type member is always more specialised than a protocol
// member (bearing in mind that we have already handled the case where
// exactly one member is in a protocol extension). Only apply this rule in
// Swift 5 mode to better maintain source compatibility under Swift 4
// mode.
//
// Don't apply this rule when comparing two overloads found through
// dynamic lookup to ensure we keep cases like this ambiguous:
//
// @objc protocol P {
// var i: String { get }
// }
// class C {
// @objc var i: Int { return 0 }
// }
// func foo(_ x: AnyObject) {
// x.i // ensure ambiguous.
// }
//
if (tc.Context.isSwiftVersionAtLeast(5) && !isDynamicOverloadComparison) {
auto *proto1 = dyn_cast<ProtocolDecl>(outerDC1);
auto *proto2 = dyn_cast<ProtocolDecl>(outerDC2);
if (proto1 != proto2)
Copy link
Contributor

@rudkx rudkx Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran into an issue with this change.

If you compare two declarations in two different protocols, e.g.:

protocol P {
  func f()
}

protocol Q : P {
  func f()
}

it always returns proto2.

For the concrete vs. protocol case it doesn't seem (from a quick look at the code) to be doing the right thing either.

[EDIT: I meant to make one protocol refine the other; my expectation is that we would return Q's f() as being more specialized.]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see – not sure how I missed that! We should really be doing an is-a check rather than comparing pointers, i.e:

        auto inProto1 = isa<ProtocolDecl>(outerDC1);
        auto inProto2 = isa<ProtocolDecl>(outerDC2);
        if (inProto1 != inProto2)
          return inProto2;

I can't immediately think of a test case that trips up the current logic though – for the case you mention of having a protocol refine another, we should be filtering out overridden protocols requirements in name lookup.

Do you have a test case that trips up the current logic? Would be nice to have one for the fix.

For the concrete vs. protocol case it doesn't seem (from a quick look at the code) to be doing the right thing either.

Could you please elaborate on this? I can't immediately think of a concrete vs. protocol case where it does the wrong thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm experimenting with changes that call compareDeclarations on arbitrary overloads in a disjunction to see if they are ordered. I don't have a test case that I know will hit this otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, sorry about that! I've submitted a fix: #19561

return proto2;
}

Type type1 = decl1->getInterfaceType();
Type type2 = decl2->getInterfaceType();

Expand Down Expand Up @@ -697,21 +728,21 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
return false;
};

tc.specializedOverloadComparisonCache[{decl1, decl2}] =
tc.specializedOverloadComparisonCache[overloadComparisonKey] =
compareSpecializations();
} else if (tc.getLangOpts().DebugConstraintSolver) {
auto &log = tc.Context.TypeCheckerDebug->getStream();
log << "Found cached comparison: "
<< tc.specializedOverloadComparisonCache[{decl1, decl2}] << "\n";
log << "Found cached comparison: "
<< tc.specializedOverloadComparisonCache[overloadComparisonKey] << "\n";
}

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

return tc.specializedOverloadComparisonCache[{decl1, decl2}];
return tc.specializedOverloadComparisonCache[overloadComparisonKey];
}

Comparison TypeChecker::compareDeclarations(DeclContext *dc,
Expand Down Expand Up @@ -849,15 +880,23 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
case OverloadChoiceKind::DynamicMemberLookup:
break;
}


// We don't apply some ranking rules to overloads found through dynamic
// lookup in order to keep a few potentially ill-formed cases ambiguous.
bool isDynamicOverloadComparison =
choice1.getKind() == OverloadChoiceKind::DeclViaDynamic &&
choice2.getKind() == OverloadChoiceKind::DeclViaDynamic;

// Determine whether one declaration is more specialized than the other.
bool firstAsSpecializedAs = false;
bool secondAsSpecializedAs = false;
if (isDeclAsSpecializedAs(tc, cs.DC, decl1, decl2)) {
if (isDeclAsSpecializedAs(tc, cs.DC, decl1, decl2,
isDynamicOverloadComparison)) {
score1 += weight;
firstAsSpecializedAs = true;
}
if (isDeclAsSpecializedAs(tc, cs.DC, decl2, decl1)) {
if (isDeclAsSpecializedAs(tc, cs.DC, decl2, decl1,
isDynamicOverloadComparison)) {
score2 += weight;
secondAsSpecializedAs = true;
}
Expand Down
8 changes: 5 additions & 3 deletions lib/Sema/TypeChecker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,9 +555,11 @@ class TypeChecker final : public LazyResolver {
SmallVector<NominalTypeDecl*, 8> DelayedCircularityChecks;

// Caches whether a given declaration is "as specialized" as another.
llvm::DenseMap<std::pair<ValueDecl*, ValueDecl*>, bool>
specializedOverloadComparisonCache;

llvm::DenseMap<std::tuple<ValueDecl *, ValueDecl *,
/*isDynamicOverloadComparison*/ unsigned>,
bool>
specializedOverloadComparisonCache;

/// A list of closures for the most recently type-checked function, which we
/// will need to compute captures for.
std::vector<AnyFunctionRef> ClosuresWithUncomputedCaptures;
Expand Down
50 changes: 42 additions & 8 deletions test/Constraints/dynamic_lookup.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module %S/Inputs/PrivateObjC.swift -o %t
// RUN: %target-typecheck-verify-swift -I %t -verify-ignore-unknown
// RUN: %target-typecheck-verify-swift -swift-version 4 -I %t -verify-ignore-unknown
// RUN: %target-typecheck-verify-swift -swift-version 5 -I %t -verify-ignore-unknown

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

// Test that we correctly diagnose ambiguity for different typed properties available
// Test that we correctly diagnose ambiguity for different typed members available
// through dynamic lookup.
@objc protocol P3 {
var i: UInt { get } // expected-note {{found this candidate}}
var j: Int { get }
var ambiguousProperty: String { get } // expected-note {{found this candidate}}
var unambiguousProperty: Int { get }

func ambiguousMethod() -> String // expected-note 2{{found this candidate}}
func unambiguousMethod() -> Int

func ambiguousMethodParam(_ x: String) // expected-note {{found this candidate}}
func unambiguousMethodParam(_ x: Int)

subscript(ambiguousSubscript _: Int) -> String { get } // expected-note {{found this candidate}}
subscript(unambiguousSubscript _: String) -> Int { get } // expected-note {{found this candidate}}
}

class C1 {
@objc var i: Int { return 0 } // expected-note {{found this candidate}}
@objc var j: Int { return 0 }
@objc var ambiguousProperty: Int { return 0 } // expected-note {{found this candidate}}
@objc var unambiguousProperty: Int { return 0 }

@objc func ambiguousMethod() -> Int { return 0 } // expected-note 2{{found this candidate}}
@objc func unambiguousMethod() -> Int { return 0 }

@objc func ambiguousMethodParam(_ x: Int) {} // expected-note {{found this candidate}}
@objc func unambiguousMethodParam(_ x: Int) {}

@objc subscript(ambiguousSubscript _: Int) -> Int { return 0 } // expected-note {{found this candidate}}
@objc subscript(unambiguousSubscript _: String) -> Int { return 0 } // expected-note {{found this candidate}}
}

_ = (C1() as AnyObject).i // expected-error {{ambiguous use of 'i'}}
_ = (C1() as AnyObject).j // okay
func testAnyObjectAmbiguity(_ x: AnyObject) {
_ = x.ambiguousProperty // expected-error {{ambiguous use of 'ambiguousProperty'}}
_ = x.unambiguousProperty

_ = x.ambiguousMethod() // expected-error {{ambiguous use of 'ambiguousMethod()'}}
_ = x.unambiguousMethod()

_ = x.ambiguousMethod // expected-error {{ambiguous use of 'ambiguousMethod()'}}
_ = x.unambiguousMethod

_ = x.ambiguousMethodParam // expected-error {{ambiguous use of 'ambiguousMethodParam'}}
_ = x.unambiguousMethodParam

_ = x[ambiguousSubscript: 0] // expected-error {{ambiguous use of 'subscript(ambiguousSubscript:)'}}

// FIX-ME(SR-8611): This is currently ambiguous but shouldn't be.
_ = x[unambiguousSubscript: ""] // expected-error {{ambiguous use of 'subscript(unambiguousSubscript:)'}}
}
115 changes: 114 additions & 1 deletion test/Constraints/ranking.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-swift-emit-silgen %s -verify | %FileCheck %s
// RUN: %target-swift-emit-silgen %s -verify -swift-version 5 | %FileCheck %s

protocol P {
var p: P { get set }
Expand Down Expand Up @@ -156,6 +156,119 @@ func testDerived(b: B) {
// CHECK: end sil function '$S7ranking11testDerived1byAA1BC_tF'
}

protocol X {
var foo: Int { get }
var bar: Int { get }
func baz() -> Int
subscript(foo: String) -> Int { get }
}

class Y {
var foo: Int = 0
func baz() -> Int { return foo }
subscript(foo: String) -> Int { return 0 }
}
extension Y {
var bar: Int { return foo }
}

protocol Z : Y {
var foo: Int { get }
var bar: Int { get }
func baz() -> Int
subscript(foo: String) -> Int { get }
}

class GenericClass<T> {
var foo: T
init(_ foo: T) { self.foo = foo }
func baz() -> T { return foo }
}
extension GenericClass {
var bar: T { return foo }
subscript(foo: String) -> Int { return 0 }
}

// Make sure we favour the class implementation over the protocol requirement.

// CHECK-LABEL: sil hidden @$S7ranking32testGenericPropertyProtocolClassyyxAA1YCRbzAA1XRzlF
func testGenericPropertyProtocolClass<T : X & Y>(_ t: T) {
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
}

// CHECK-LABEL: sil hidden @$S7ranking36testExistentialPropertyProtocolClassyyAA1X_AA1YCXcF
func testExistentialPropertyProtocolClass(_ t: X & Y) {
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
}

// CHECK-LABEL: sil hidden @$S7ranking46testGenericPropertySubclassConstrainedProtocolyyxAA1ZRzlF
func testGenericPropertySubclassConstrainedProtocol<T : Z>(_ t: T) {
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
}

// CHECK-LABEL: sil hidden @$S7ranking50testExistentialPropertySubclassConstrainedProtocolyyAA1Z_pF
func testExistentialPropertySubclassConstrainedProtocol(_ t: Z) {
_ = t.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
_ = t.bar // CHECK: function_ref @$S7ranking1YC3barSivg
_ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
_ = t[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
}

// CHECK-LABEL: sil hidden @$S7ranking43testExistentialPropertyProtocolGenericClassyyAA1X_AA0fG0CySiGXcF
func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<Int> & X) {
_ = t.foo // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.foo!getter.1
_ = t.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
_ = t.baz() // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.baz
_ = t[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
}

// CHECK-LABEL: sil hidden @$S7ranking43testExistentialPropertyProtocolGenericClassyyAA1X_AA0fG0CySSGXcF
func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<String> & X) {
_ = t.foo // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.foo!getter.1
_ = t.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
_ = t.baz() // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.baz
_ = t[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
}

extension X where Self : Y {
// CHECK-LABEL: sil hidden @$S7ranking1XPA2A1YCRbzrlE32testGenericPropertyProtocolClassyyxF
func testGenericPropertyProtocolClass(_ x: Self) {
_ = self.foo // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
_ = self.bar // CHECK: function_ref @$S7ranking1YC3barSivg
_ = self.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
_ = self[""] // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
}
}

extension X where Self : GenericClass<Int> {
// CHECK-LABEL: sil hidden @$S7ranking1XPA2A12GenericClassCySiGRbzrlE04testb16PropertyProtocolbC0yyxF
func testGenericPropertyProtocolGenericClass(_ x: Self) {
_ = self.foo // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.foo!getter.1
_ = self.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
_ = self.baz() // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.baz
_ = self[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
}
}

extension X where Self : GenericClass<String> {
// CHECK-LABEL: sil hidden @$S7ranking1XPA2A12GenericClassCySSGRbzrlE04testb16PropertyProtocolbC0yyxF
func testGenericPropertyProtocolGenericClass(_ x: Self) {
_ = self.foo // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.foo!getter.1
_ = self.bar // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
_ = self.baz() // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.baz
_ = self[""] // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
}
}

//--------------------------------------------------------------------
// Pointer conversions
//--------------------------------------------------------------------
Expand Down
21 changes: 11 additions & 10 deletions test/Constraints/rdar39401774.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-typecheck-verify-swift -swift-version 5

class A<T> {
var foo: Int? { return 42 } // expected-note {{found this candidate}}
func baz() -> T { fatalError() } // expected-note {{found this candidate}}
func fiz() -> Int { return 42 } // expected-note {{found this candidate}}
var foo: Int? { return 42 }
func baz() -> T { fatalError() }
func fiz() -> Int { return 42 }
}

protocol P1 {
associatedtype T
var foo: Int? { get } // expected-note {{found this candidate}}
func baz() -> T // expected-note {{found this candidate}}
func fiz() -> Int // expected-note {{found this candidate}}
var foo: Int? { get }
func baz() -> T
func fiz() -> Int
}

protocol P2 : P1 {
Expand All @@ -19,8 +19,9 @@ protocol P2 : P1 {

extension P2 where Self: A<Int> {
var bar: Int? {
guard let foo = foo else { return 0 } // expected-error {{ambiguous use of 'foo'}}
let _ = baz() // expected-error {{ambiguous use of 'baz()'}}
return fiz() // expected-error {{ambiguous use of 'fiz()'}}
guard let foo = foo else { return 0 }
_ = foo
let _ = baz()
return fiz()
}
}