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

[hamishknight]

(This is the Swift 5 mode portion of #18927)

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.

[rudkx]

@swift-ci Please smoke test

[rudkx]

@swift-ci Please test source compatibility

[hamishknight]

@rudkx Source compat failed to clone the repo – could you try again?

[rudkx]

@swift-ci Please test source compatibility

[hamishknight]

Just resolved a merge conflict. Given the source compat suite passed, should I revert 37a9490?

[rudkx]

@hamishknight Yes, sounds good.

[rudkx]

@swift-ci Please smoke test

[hamishknight]

Linux failure looks unrelated – same failure as https://ci.swift.org/job/swift-PR-Linux-smoke-test/9516/

[rudkx]

@swift-ci Please smoke test Linux platform

[rudkx]

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.]

[hamishknight]

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.

[rudkx]

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.

[hamishknight]

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