[Sema] Improve diagnostics for access level of protocol witness in extension.

[dingobye]

If the access level of a protocol witness does not satisfies a requirement, the compiler always suggests marking it as the required level. This is not suitable when the witness is in an extension whose specified access level is less than the required level, since the fixit fights with other warnings in this case. This patch identifies such case and produces improved diagnostics.

Consider the following example, if we follow the original notes to mark foo and bar as 'internal', we would get further warnings redundant 'internal' modifier and conflict with extension's default access, respectively. The diagnostics improved by this patch makes more sense. It uses removal (instead of replacement) fix-it for foo, and suggests moving bar to another extension.

protocol Proto {
  func foo()
  func bar()
}

public struct S : Proto {}

internal extension S {
  private func foo() {} // note: mark the instance method as 'internal' to satisfy the requirement
  ^^^^^^^^
- internel
}

private extension S {
  private func bar() {}
- note: mark the instance method as 'internal' to satisfy the requirement
+ note: move the instance method to another extension where it can be declared 'internal' to satisfy the requirement
}

Resolves SR-9793.

[xwu]

In your examples:

• the first diagnostic is correct as-is and is more succinct than the proposed change
• as to the second diagnostic, moving may not be possible if the method requires other members private to the extension--it would be more helpful to suggest marking the existing extension and the method as internal (that is, simply change the diagnostic to say "mark the extension and instance method as 'internal'")

[dingobye]

@xwu Thanks for your inputs!

If we follow the first one to update the code (i.e., replacing 'private' by 'internal'), we would get another warning as shown below.

internal extension S {
  internal func foo() {} // warning: 'internal' modifier is redundant for instance method declared in an internal extension
}

So it makes sense to improve the diagnostics. If the proposed message is not concise, I have it rephrased as remove 'private' modifier of the instance method to satisfy the requirement. How does it sound?

[dingobye]

For the second one, I don't quite get your idea. I believe after the method is moved to another extension in the same file, it still can use those members declared 'private' in the original extension.

For your advised change (i.e., marking extension as 'internal'), it may be a problem when other members in extension adopt the default access level. For the following example, after marking extension as 'internal', baz() would be unexpectedly changed from 'fileprivate' to 'internal'.

private extension S {
  private func bar() {}
  func baz() { print("I am fileprivate") }
}

[dingobye]

@jrose-apple Hi Jordan, would you take a look at this, please?

[jrose-apple]

I think I'd split the difference on @xwu's first point by using the existing text but the removal fix-it. For the second point I agree with @dingobye; with SE-0169 this should always be possible.

[jrose-apple]

The implementation looks pretty good to me!

[dingobye]

Thanks for the reply, Jordan! I updated the code as suggested.

[jrose-apple]

This looks good to me! @xwu, are you convinced?

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 5e4420a6a88267b6ebe3fd8051497a097412954f

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 5e4420a6a88267b6ebe3fd8051497a097412954f

[xwu]

👍

[xwu]

@swift-ci Please test OS X platform