Skip to content
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

@_implementationOnly: fix over-eager checking for vars in extensions #24629

Merged
merged 1 commit into from May 16, 2019

Conversation

Projects
None yet
4 participants
@jrose-apple
Copy link
Member

commented May 9, 2019

The logic I had here checked whether an extension used an implementation-only type whenever there was a declaration within that extension that needed checking...but unfortunately that included not just PatternBindingDecls (whose access is filtered at a later step) but things like IfConfigDecls (#if). Change this to only check extension signatures for ValueDecl members of extensions, where we can properly check access beforehand. Additionally, don't bother doing this checking for accessors, since we'll have already done it for the corresponding VarDecl or SubscriptDecl.

rdar://problem/50541589

@jrose-apple jrose-apple requested review from rjmccall and brentdax May 9, 2019

@jrose-apple

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@swift-ci Please test

@@ -1660,21 +1665,27 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
return true;
}

void checkExtensionMemberIfNeeded(const ValueDecl *D) {
auto *extension = dyn_cast<ExtensionDecl>(D->getDeclContext());

This comment has been minimized.

Copy link
@slavapestov

slavapestov May 9, 2019

Member

It's kind of odd that this is called for every member but it only checks and diagnoses the extension itself?

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 9, 2019

Author Member

Maybe the name is wrong? The trouble is we only want to diagnose the extension if one of the members is public.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 9, 2019

Author Member

(Actually, strictly speaking I'd like this to be a diagnostic on the member somehow even though the problem is at the extension level, but that can come when we productize this thing.)

@jrose-apple jrose-apple force-pushed the jrose-apple:varboten branch from b9a5f87 to 8eff48e May 14, 2019

@jrose-apple

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

New implementation per Slava's suggestion.

@swift-ci Please test

@jrose-apple jrose-apple requested review from brentdax and slavapestov and removed request for brentdax May 14, 2019

checkType(ED->getExtendedTypeLoc(), ED,
getDiagnoseCallback(ED), getDiagnoseCallback(ED));
// FIXME: include a note as to why this restriction is being enforced,
// i.e. there are public members.

This comment has been minimized.

Copy link
@brentdax

brentdax May 14, 2019

Collaborator

Are you planning to leave this as a FIXME until this turns into a public-facing feature?

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 14, 2019

Author Member

Yeah. :-/ It's what was there before this PR, and while it's not even that hard it's also not necessarily worth my time at the moment.

@@ -87,18 +87,26 @@ public var testMultipleBindings1: Int? = nil, testMultipleBindings2: BadStruct?
public var testMultipleBindings3: BadStruct? = nil, testMultipleBindings4: Int? = nil // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
extension BadStruct { // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}

This comment has been minimized.

Copy link
@brentdax

brentdax May 16, 2019

Collaborator

Can the message at least say something like "Cannot use struct 'BadStruct' in extension with public members"? Or use a note or something, I don't care—just let the user know somehow that the problem is that the extension has public members, because if you don't say so, they'll have no idea what's wrong.

@_implementationOnly: fix over-eager checking for vars in extensions
The logic I had here checked whether an extension used an
implementation-only type whenever there was a declaration within that
extension that needed checking...but unfortunately that included not
just PatternBindingDecls (whose access is filtered at a later step)
but things like IfConfigDecls (#if). Change this to only check
signatures of extensions with ABI-public members, something that is
tested once when visiting an ExtensionDecl.

Additionally, skip AccessorDecls entirely, since they'll be tested
as part of the corresponding subscript or var. This saves a duplicate
diagnostic.

rdar://problem/50541589

@jrose-apple jrose-apple force-pushed the jrose-apple:varboten branch from 8eff48e to d63c26a May 16, 2019

@jrose-apple

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Diagnostics clarified per Brent's comment.

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Build failed
Swift Test OS X Platform
Git Sha - 8eff48e

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Build failed
Swift Test Linux Platform
Git Sha - 8eff48e

@brentdax
Copy link
Collaborator

left a comment

There are bits I don't love, but I don't think you have to change them.

@@ -86,27 +86,35 @@ public var (testBadTypeTuplePartlyInferred3, testBadTypeTuplePartlyInferred4): (
public var testMultipleBindings1: Int? = nil, testMultipleBindings2: BadStruct? = nil // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
public var testMultipleBindings3: BadStruct? = nil, testMultipleBindings4: Int? = nil // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
extension BadStruct { // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
public func testExtensionOfBadType() {} // FIXME: Should complain here instead of at the extension decl.
extension BadStruct { // expected-error {{cannot use struct 'BadStruct' in an extension with public or '@usableFromInline' members; 'BADLibrary' has been imported as implementation-only}}

This comment has been minimized.

Copy link
@brentdax

brentdax May 16, 2019

Collaborator

Excellent.

}

extension Int: BadProto {} // expected-error {{cannot use protocol 'BadProto' here; 'BADLibrary' has been imported as implementation-only}}
struct TestExtensionConformanceOkay {}
extension TestExtensionConformanceOkay: BadProto {} // okay
public protocol TestConstrainedExtensionProto {}
extension Array: TestConstrainedExtensionProto where Element == BadStruct { // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
extension Array: TestConstrainedExtensionProto where Element == BadStruct { // expected-error {{cannot use struct 'BadStruct' in an extension with conditional conformances; 'BADLibrary' has been imported as implementation-only}}

This comment has been minimized.

Copy link
@brentdax

brentdax May 16, 2019

Collaborator

Would this be allowed if TestConstrainedExtensionProto was internal or less? If so, I wish the message conveyed that...but as I started writing an example, I realized that fully describing it gets pretty long and complicated. I guess this Is fine for now; we can introduce notes pointing to the relevant declarations when we package this.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 16, 2019

Author Member

I think we don't actually allow that right now even though we probably should.

@@ -249,7 +249,7 @@ diagnoseGenericArgumentsExportability(SourceLoc loc,
ASTContext &ctx = M->getASTContext();
ctx.Diags.diagnose(loc, diag::conformance_from_implementation_only_module,
rootConf->getType(),
rootConf->getProtocol()->getFullName(), M->getName());
rootConf->getProtocol()->getFullName(), 0, M->getName());

This comment has been minimized.

Copy link
@brentdax

brentdax May 16, 2019

Collaborator

It'd be better to expose the Reason enum widely enough to use it here, but I can live with this if you can.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple May 16, 2019

Author Member

Yeah, I'm not sure where it would live. Splitting it into two diagnostics might make more sense, but I think this is okay specifically because it's 0 and the first case and not any of the others.

@@ -3678,7 +3678,7 @@ void ConformanceChecker::ensureRequirementsAreSatisfied(
conformanceBeingChecked->getLoc(),
diag::conformance_from_implementation_only_module,
rootConformance->getType(),
rootConformance->getProtocol()->getName(), M->getName());
rootConformance->getProtocol()->getName(), 0, M->getName());

This comment has been minimized.

Copy link
@brentdax

brentdax May 16, 2019

Collaborator

As above.

@jrose-apple jrose-apple merged commit 3b4bb19 into apple:master May 16, 2019

4 checks passed

Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details

@jrose-apple jrose-apple deleted the jrose-apple:varboten branch May 16, 2019

jrose-apple added a commit to jrose-apple/swift that referenced this pull request May 16, 2019

@_implementationOnly: fix over-eager checking for vars in extensions (a…
…pple#24629)

The logic I had here checked whether an extension used an
implementation-only type whenever there was a declaration within that
extension that needed checking...but unfortunately that included not
just PatternBindingDecls (whose access is filtered at a later step)
but things like IfConfigDecls (#if). Change this to only check
signatures of extensions with ABI-public members, something that is
tested once when visiting an ExtensionDecl.

Additionally, skip AccessorDecls entirely, since they'll be tested
as part of the corresponding subscript or var. This saves a duplicate
diagnostic.

rdar://problem/50541589
(cherry picked from commit 3b4bb19)

jrose-apple added a commit that referenced this pull request May 16, 2019

@_implementationOnly: fix over-eager checking for vars in extensions (#…
…24629) (#24843)

The logic I had here checked whether an extension used an
implementation-only type whenever there was a declaration within that
extension that needed checking...but unfortunately that included not
just PatternBindingDecls (whose access is filtered at a later step)
but things like IfConfigDecls (#if). Change this to only check
signatures of extensions with ABI-public members, something that is
tested once when visiting an ExtensionDecl.

Additionally, skip AccessorDecls entirely, since they'll be tested
as part of the corresponding subscript or var. This saves a duplicate
diagnostic.

rdar://problem/50541589
(cherry picked from commit 3b4bb19)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.