Don't fix access of an 'open' override in a 'public' extension #22188
Conversation
|
@swift-ci Please test source compatibility |
| open override func method() {} | ||
| } | ||
| public extension OverrideInExtensionSub { | ||
| open override func accessWarning() {} // expected-warning {{'open' modifier conflicts with extension's default access of 'public'}} |
There was a problem hiding this comment.
@brentdax How does this sound? It's still vague about how it's going to be treated for now, but at least it sounds like a problem.
|
[waiting for the previous tests to finish before starting more tests, since the only thing that changed was the diagnostic message] |
lib/Sema/TypeCheckAttr.cpp
Outdated
| // (which can happen with Objective-C extensions). That can lead to a | ||
| // diagnostic fight between this and "declaration must be as accessible | ||
| // as the declaration it overrides". | ||
| if (!D->getAttrs().hasAttribute<OverrideAttr>()) { |
There was a problem hiding this comment.
Apart from override, there might be other cases where fixit is inappropriate.
Here is an example I can figure out so Far:
public protocol Proto {
func foo()
}
public struct S : Proto {
}
internal extension S {
public func foo() {} // fixit suggests removing 'public', but it fights against 'type_witness_not_accessible_proto' which suggests adding 'public' to foo() :-/
}
There was a problem hiding this comment.
Hrm. Maybe it'd be better to just remove the fix-it altogether. It's hard to tell whether something's trying to satisfy a requirement. What do you all think?
There was a problem hiding this comment.
Agree! It is always better to be quiet than misleading.
There was a problem hiding this comment.
We will still have the fighting cases for now in the error cases, but, well…that's not a regression.
There was a problem hiding this comment.
Well, that is an orthogonal issue. 'type_witness_not_accessible_proto' keeps warning even though foo() is already declared public.
Before: declaring a public instance method in a private extension
After: 'public' modifier conflicts with extension's default access of
'private'
This is reasonable to diagnose with a warning, but dropping the 'open' down to 'public' isn't the right fix, because now it's not a valid override. The declaration has to get moved to another extension instead, or the extension has to not set a default access level. This turned out to be a source compat issue because the same logic that emits the fix-it also updates the access of the member, which then resulted in "must be as accessible as the declaration it overrides" in the /same/ build. It's not immediately clear what caused this; probably something's just being validated in a different order than it was before. The change makes sense either way. Stepping back, it's weird that a warning would change how the compiler saw the code, and while we could check for 'override' explicitly, we can't know if the member might be satisfying a protocol requirement. Better to just not guess at the right answer here. rdar://problem/47557376&28493971
Warnings should not result in early exits because we might end up missing an error!
jrose-apple
force-pushed
the
open-your-heart
branch
from
e9b1f03 to
15056ed
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
Don't fix access of an 'open' override in a 'public' extension rdar://problem/47557376&28493971 (cherry picked from commit de93ba0)
This is reasonable to diagnose with a warning, but dropping the 'open' down to 'public' isn't the right fix, because now it's not a valid override. The declaration has to get moved to another extension instead, or the extension has to not set a default access level.
This turned out to be a source compat issue because the same logic that emits the fix-it also updates the access of the member, which then resulted in "must be as accessible as the declaration it overrides" in the same build. It's not immediately clear what caused this; probably something's just being validated in a different order than it was before. The change makes sense either way.
rdar://problem/47557376
rdar://problem/28493971