[CSRanking] Favour concrete members over protocol requirements #18927

hamishknight · 2018-08-23T18:10:44Z

This PR comes in two pieces:

1. A narrow Swift 4.1 compatibility hack that favours properties in concrete types over protocol requirements

This was unintentionally regressed by #15412, and is tracked by SR-7425. This is added as a ranking tie-breaker in order to ensure we don’t introduce any new ambiguities.

2. A new Swift 5 mode ranking rule that favours arbitrary concrete type members over protocol requirements

We already have a similar rule that prefers concrete members over protocol extension members – IMO this new rule is a natural extension of it.

This allows the following code to become unambiguous in Swift 5 mode:

protocol X {
  func baz() -> Int
}

class Y {
  func baz() -> Int { return 0 }
}

func foo(_ x: X & Y) {
  // currently ambiguous, will be made unambigous in Swift 5 mode (favouring Y's `baz()`)
  x.baz()
}

Note that this new rule isn’t applied to overloads found through dynamic lookup in order to ensure we keep cases such as SR-7299 ambiguous.

Note also that this new rule is potentially source breaking (hence why it requires Swift 5 mode) – for example, the following contrived case is currently unambiguous, but will be made ambiguous by the change:

protocol P {
  static func foo() -> Int
}

class C {
  func foo() -> Int { return 0 }
}

extension P where Self : C {
  static func bar() {
    let m = foo // currently unambiguous, will be made ambiguous in Swift 5 mode
  }
}

Arguably this change is unifying the behaviour with that of protocol extensions, where a similar example is already ambiguous:

protocol P {}
extension P {
  static func foo() -> Int { return 0 }
}

class C {
  func foo() -> Int { return 0 }
}

extension P where Self : C {
  static func bar() {
    let m = foo // error: Ambiguous use of 'foo()'
  }
}

And currently, adding static func foo() as a protocol requirement to the above suddenly resolves the ambiguity – with this Swift 5 mode change, the ambiguity is kept in both cases, which seems more consistent.

I’m happy to split this PR up if the Swift 5 mode change requires more discussion.

Resolves SR-7425.

…concrete types over protocols Changes in shadowing behaviour by swiftlang#15412 caused a property on a concrete type to no longer shadow a protocol property member, which created unintentional ambiguities in 4.2. This commit ensures we at least keep these cases unambiguous in Swift 5 under Swift 4 compatibility mode. This is intentionally narrow in order to best preserve source compatibility under Swift 4 mode by ensuring we don't introduce any new ambiguities. Resolves SR-7425, SR-7940 & SR-8343.

This commit adds a new rule to `isDeclAsSpecializedAs` in order to favour a member on a concrete type over a protocol member. This rule is effectively an extension of the existing rule that prefers concrete type members over protocol extension members.

…anking This rule caused us to lose ambiguities in places where we really want ambiguity for `AnyObject` lookup, so only apply it when not comparing such overloads. This whole situation is a bit of a hack – really we shouldn't be applying any type-based or context-based overload ranking rules to overloads found through `AnyObject` lookup, but unfortunately we don't have syntax to precisely disambiguate overloads. This commit can be reverted if/when we ever remove `AnyObject` lookup.

hamishknight · 2018-08-23T18:20:58Z

cc. @slavapestov – I believe this can supersede #18206.

slavapestov · 2018-08-23T18:24:10Z

Excellent!

slavapestov · 2018-08-23T18:24:20Z

I'll defer to @rudkx and @xedin for the review though.

slavapestov · 2018-08-23T18:24:25Z

@swift-ci Please test

slavapestov · 2018-08-23T18:24:30Z

@swift-ci Please test source compatibility

swift-ci · 2018-08-23T18:45:02Z

Build failed
Swift Test Linux Platform
Git Sha - 2e6ba53

Apparently, we cannot use an initializer list to construct a std::tuple there. Use `make_tuple` instead, and also simplify a condition to use `isDynamicOverloadComparison` while I'm here.

hamishknight · 2018-08-23T22:46:18Z

Just fixed the Linux failure. The source compatibility suite succeeded, though looking through the projects I realise that we wouldn't have exercised -swift-version 5 at all (assuming I've understood how the suite is run). Is there a way to do so?

rudkx · 2018-08-23T22:53:33Z

@hamishknight No. What I would suggest is removing the version check for the sake of running the compat suite to check that there aren't any unexpected breaks (and see what actually will break for people moving to Swift 5).

hamishknight · 2018-08-23T23:02:27Z

@rudkx Thanks, I've gone ahead and done that – could you kick off the compat suite again?

rudkx · 2018-08-24T00:15:20Z

@swift-ci Please test source compatibility

rudkx · 2018-08-24T01:26:40Z

Can you split the changes up into separate PRs we get some feedback from the source compatibility suite?

I'll take a closer look at each once that's done. I am a little concerned that at first glance some of the changes remind me of #17995

hamishknight · 2018-08-24T15:05:56Z

Sure thing – I've split it up into #18952 & #18951. Shall I go ahead and close this PR?

xedin · 2018-08-24T18:58:02Z

lib/Sema/CSRanking.cpp

+      //      x.i // ensure ambiguous.
+      //    }
+      //
+      if (true && !isDynamicOverloadComparison) {


It should probably be simplified to if (!isDynamicOverloadComparison)

@xedin I removed the version checks temporarily in 685ec32 to allow the compat suite to exercise the Swift 5 path. true should be tc.Context.isSwiftVersionAtLeast(5), and below, false should be !tc.Context.isSwiftVersionAtLeast(5).

Ah, gotcha!

xedin · 2018-08-24T18:59:46Z

lib/Sema/CSRanking.cpp

+    // compatibility under Swift 4 mode by ensuring we don't introduce any new
+    // ambiguities. This will become a more general "is more specialised" rule
+    // in Swift 5 mode.
+    if (false && !isDynamicOverloadComparison &&


Does this mean that this code shouldn't exist?

xedin · 2018-08-24T18:59:55Z

lib/Sema/CSRanking.cpp

+  // All other things being equal, apply the Swift 4 compatibility hack for
+  // preferring var members in concrete types over a protocol requirement
+  // (see the comment above for the rationale of this hack).
+  if (false && score1 == score2) {


xedin · 2018-08-24T19:04:36Z

@rudkx As this branch stands today it preserves behavior when it comes to protocol vs. protocol extension declarations, and would always pick declaration on protocol...

rudkx · 2018-08-25T00:07:50Z

@hamishknight Yes, we can close this out in favor of the two new PRs. Thanks!

hamishknight added 4 commits August 23, 2018 15:52

[test] Expand AnyObject ambiguity test to cover methods and subscripts

9cb3e36

Fix Linux failure for concrete-over-protocol

0dabb11

Apparently, we cannot use an initializer list to construct a std::tuple there. Use `make_tuple` instead, and also simplify a condition to use `isDynamicOverloadComparison` while I'm here.

[Revert Me] Disable Swift 5 version check for concrete-over-protocol

685ec32

This was referenced Aug 24, 2018

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

Merged

[CSRanking] Swift 4.1 compatibility hack for favouring properties on concrete types over protocols #18952

Merged

xedin reviewed Aug 24, 2018

View reviewed changes

hamishknight closed this Aug 25, 2018

hamishknight deleted the concrete-is-better branch August 30, 2018 09:22

hamishknight mentioned this pull request Aug 23, 2018

[SR-7425] ambiguity error when class constraining protocol extension fulfills protocol requirement #49968

Closed

[CSRanking] Favour concrete members over protocol requirements #18927

[CSRanking] Favour concrete members over protocol requirements #18927

Uh oh!

Conversation

hamishknight commented Aug 23, 2018

1. A narrow Swift 4.1 compatibility hack that favours properties in concrete types over protocol requirements

2. A new Swift 5 mode ranking rule that favours arbitrary concrete type members over protocol requirements

Uh oh!

hamishknight commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Aug 23, 2018

Uh oh!

slavapestov commented Aug 23, 2018

Uh oh!

slavapestov commented Aug 23, 2018

Uh oh!

slavapestov commented Aug 23, 2018

Uh oh!

swift-ci commented Aug 23, 2018

Uh oh!

hamishknight commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rudkx commented Aug 23, 2018

Uh oh!

hamishknight commented Aug 23, 2018

Uh oh!

rudkx commented Aug 24, 2018

Uh oh!

rudkx commented Aug 24, 2018

Uh oh!

hamishknight commented Aug 24, 2018

Uh oh!

xedin Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

hamishknight Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

xedin Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

xedin Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

xedin commented Aug 24, 2018

Uh oh!

rudkx commented Aug 25, 2018

Uh oh!

Uh oh!

hamishknight commented Aug 23, 2018 •

edited

Loading

hamishknight commented Aug 23, 2018 •

edited

Loading

hamishknight Aug 24, 2018 •

edited

Loading