-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Diagnose public protocol requirements witnessed by internal protocol extensions #85846
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1762,11 +1762,14 @@ static bool checkWitnessAccess(DeclContext *dc, | |
|
|
||
| auto actualScopeToCheck = requiredAccessScope.first; | ||
|
|
||
| // without StrictTextualInterfaceChecking feature forConformance is set true | ||
| bool forConformance = not dc->getASTContext().LangOpts.hasFeature( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| Feature::StrictSemaForTextualInterface); | ||
| // Setting the 'forConformance' flag means that we admit witnesses in | ||
| // protocol extensions that we can see, but are not necessarily as | ||
| // visible as the conforming type and protocol. | ||
| if (!witness->isAccessibleFrom(actualScopeToCheck.getDeclContext(), | ||
| /*forConformance=*/true)) { | ||
| forConformance)) { | ||
| // Special case: if we have `@testable import` of the witness's module, | ||
| // allow the witness to match if it would have matched for just this file. | ||
| // That is, if '@testable' allows us to see the witness here, it should | ||
|
|
@@ -4449,11 +4452,14 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) { | |
| bool isSetter = check.isForSetterAccess(); | ||
|
|
||
| auto &diags = DC->getASTContext().Diags; | ||
| diags.diagnose(getLocForDiagnosingWitness(conformance, witness), | ||
| diagKind, getProtocolRequirementKind(requirement), | ||
| witness, isSetter, requiredAccess, | ||
| protoAccessScope.accessLevelForDiagnostics(), | ||
| proto); | ||
| diags | ||
| .diagnose(getLocForDiagnosingWitness(conformance, witness), | ||
| diagKind, getProtocolRequirementKind(requirement), | ||
| witness, isSetter, requiredAccess, | ||
| protoAccessScope.accessLevelForDiagnostics(), proto) | ||
| .warnUntilFutureSwiftVersionIf( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this change, all access control errors from conformance checking now become warnings, we don't want that |
||
| DC->getASTContext().LangOpts.hasFeature( | ||
| Feature::StrictSemaForTextualInterface)); | ||
|
|
||
| auto *decl = dyn_cast<AbstractFunctionDecl>(witness); | ||
| if (decl && decl->isSynthesized()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // RUN: %empty-directory(%t) | ||
| // RUN: %target-typecheck-verify-swift -enable-library-evolution -enable-experimental-feature StrictSemaForTextualInterface -language-mode 6 -verify-additional-prefix swift6- | ||
| // RUN: %target-typecheck-verify-swift -enable-library-evolution -enable-experimental-feature StrictSemaForTextualInterface -language-mode 7 -verify-additional-prefix swift7- | ||
|
|
||
| public protocol P { | ||
| func foo() -> Int | ||
| } | ||
| internal protocol Q: P { | ||
| func fooImpl() -> Int32 | ||
| } | ||
| extension Q { | ||
| public func foo() -> Int { // expected-note {{mark the instance method as 'public' to satisfy the requirement}} | ||
| return Int(self.fooImpl()) | ||
| } | ||
| } | ||
| public struct S: Q { | ||
| // expected-swift7-error@-1 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'P'}} | ||
| // expected-swift6-warning@-2 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'P'; this will be an error in a future Swift language mode}} | ||
| internal func fooImpl() -> Int32 { | ||
| return 42 | ||
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too many blank lines |
||
| public struct Foo { | ||
| public init(value: Int) {} | ||
| } | ||
| public protocol PublicProtocol { | ||
| init?(integer: Int) | ||
| } | ||
| protocol InternalProtocol: PublicProtocol {} | ||
| extension InternalProtocol { | ||
| public init(integer: Int) {} // expected-note {{mark the initializer as 'public' to satisfy the requirement}} | ||
| } | ||
| extension Foo: PublicProtocol, InternalProtocol {} | ||
| // expected-swift7-error@-1 {{initializer 'init(integer:)' must be declared public because it matches a requirement in public protocol 'PublicProtocol'}} | ||
| // expected-swift6-warning@-2 {{initializer 'init(integer:)' must be declared public because it matches a requirement in public protocol 'PublicProtocol'; this will be an error in a future Swift language mode}} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unnecessary in my opinion