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

jrose-apple · 2019-05-09T00:48:51Z

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 · 2019-05-09T00:49:02Z

@swift-ci Please test

slavapestov · 2019-05-09T01:53:50Z

lib/Sema/TypeCheckAccess.cpp

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

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


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

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

(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 · 2019-05-14T03:18:09Z

New implementation per Slava's suggestion.

@swift-ci Please test

beccadax · 2019-05-14T04:37:39Z

lib/Sema/TypeCheckAccess.cpp

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


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

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.

beccadax · 2019-05-16T00:57:02Z

test/Sema/implementation-only-import-in-decls.swift

@@ -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}}


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.

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 · 2019-05-16T04:40:18Z

Diagnostics clarified per Brent's comment.

@swift-ci Please test

swift-ci · 2019-05-16T04:40:47Z

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

swift-ci · 2019-05-16T04:42:35Z

Build failed
Swift Test Linux Platform
Git Sha - 8eff48e26e5aceec976c88de690560fdc7d7b5a9

beccadax

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

beccadax · 2019-05-16T18:22:03Z

test/Sema/implementation-only-import-in-decls.swift

@@ -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}}


beccadax · 2019-05-16T18:24:15Z

test/Sema/implementation-only-import-in-decls.swift

 }

 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}}


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.

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

beccadax · 2019-05-16T18:30:17Z

lib/Sema/ResilienceDiagnostics.cpp

@@ -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());


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

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.

beccadax · 2019-05-16T18:30:40Z

lib/Sema/TypeCheckProtocol.cpp

@@ -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());


…wiftlang#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)

…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)

jrose-apple requested review from rjmccall and beccadax May 9, 2019 00:48

slavapestov reviewed May 9, 2019

View reviewed changes

beccadax approved these changes May 13, 2019

View reviewed changes

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

jrose-apple requested review from beccadax and slavapestov and removed request for beccadax May 14, 2019 03:18

beccadax reviewed May 14, 2019

View reviewed changes

beccadax reviewed May 16, 2019

View reviewed changes

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

beccadax approved these changes May 16, 2019

View reviewed changes

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

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

jrose-apple mentioned this pull request May 16, 2019

[5.1] @_implementationOnly: fix over-eager checking for vars in extensions #24843

Merged

		@@ -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}}

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

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

Uh oh!

Conversation

jrose-apple commented May 9, 2019

Uh oh!

jrose-apple commented May 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 16, 2019

Uh oh!

swift-ci commented May 16, 2019

Uh oh!

swift-ci commented May 16, 2019

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!