@_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 · May 16, 2019 · 6bc1ecd · 6bc1ecd
1 parent c2ecb87
commit 6bc1ecd
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 32 deletions.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2427,12 +2427,17 @@ NOTE(construct_raw_representable_from_unwrapped_value,none,
      "construct %0 from unwrapped %1 value", (Type, Type))
 
 ERROR(decl_from_implementation_only_module,none,
-      "cannot use %0 %1 here; %2 has been imported as implementation-only",
-      (DescriptiveDeclKind, DeclName, Identifier))
+      "cannot use %0 %1 %select{here|"
+      "in an extension with public or '@usableFromInline' members|"
+      "in an extension with conditional conformances}2; %3 has been imported "
+      "as implementation-only",
+      (DescriptiveDeclKind, DeclName, unsigned, Identifier))
 ERROR(conformance_from_implementation_only_module,none,
-      "cannot use conformance of %0 to %1 here; %2 has been imported as "
-      "implementation-only",
-      (Type, DeclName, Identifier))
+      "cannot use conformance of %0 to %1 %select{here|"
+      "in an extension with public or '@usableFromInline' members|"
+      "in an extension with conditional conformances}2; %3 has been imported "
+      "as implementation-only",
+      (Type, DeclName, unsigned, Identifier))
 ERROR(assoc_conformance_from_implementation_only_module,none,
       "cannot use conformance of %0 to %1 in associated type %3 (inferred as "
       "%4); %2 has been imported as implementation-only",

diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/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());
     hadAnyIssues = true;
   }
   return hadAnyIssues;

diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp
@@ -568,7 +568,7 @@ class AccessControlChecker : public AccessControlCheckerBase,
       highlightOffendingType(TC, diag, complainRepr);
     });
   }
-                               
+
   void visitOpaqueTypeDecl(OpaqueTypeDecl *OTD) {
     // TODO(opaque): The constraint class/protocols on the opaque interface, as
     // well as the naming decl for the opaque type, need to be accessible.
@@ -1559,18 +1559,30 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     });
   }
 
+  // This enum must be kept in sync with
+  // diag::decl_from_implementation_only_module and
+  // diag::conformance_from_implementation_only_module.
+  enum class Reason : unsigned {
+    General,
+    ExtensionWithPublicMembers,
+    ExtensionWithConditionalConformances
+  };
+
   class DiagnoseGenerically {
     TypeChecker &TC;
     const Decl *D;
+    Reason reason;
   public:
-    DiagnoseGenerically(TypeChecker &TC, const Decl *D) : TC(TC), D(D) {}
+    DiagnoseGenerically(TypeChecker &TC, const Decl *D, Reason reason)
+        : TC(TC), D(D), reason(reason) {}
 
     void operator()(const TypeDecl *offendingType,
                     const TypeRepr *complainRepr) {
       ModuleDecl *M = offendingType->getModuleContext();
       auto diag = TC.diagnose(D, diag::decl_from_implementation_only_module,
                               offendingType->getDescriptiveKind(),
-                              offendingType->getFullName(), M->getName());
+                              offendingType->getFullName(),
+                              static_cast<unsigned>(reason), M->getName());
       highlightOffendingType(TC, diag, complainRepr);
     }
 
@@ -1579,7 +1591,7 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
       TC.diagnose(D, diag::conformance_from_implementation_only_module,
                   offendingConformance->getType(),
                   offendingConformance->getProtocol()->getFullName(),
-                  M->getName());
+                  static_cast<unsigned>(reason), M->getName());
     }
   };
 
@@ -1592,14 +1604,20 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
                           CheckExportabilityConformanceCallback>::value,
       "DiagnoseGenerically has wrong call signature for conformance diags");
 
-  DiagnoseGenerically getDiagnoseCallback(const Decl *D) {
-    return DiagnoseGenerically(TC, D);
+  DiagnoseGenerically getDiagnoseCallback(const Decl *D,
+                                          Reason reason = Reason::General) {
+    return DiagnoseGenerically(TC, D, reason);
   }
 
 public:
   explicit ExportabilityChecker(TypeChecker &TC) : TC(TC) {}
 
   static bool shouldSkipChecking(const ValueDecl *VD) {
+    // Accessors are handled as part of their Var or Subscript, and we don't
+    // want to redo extension signature checking for them.
+    if (isa<AccessorDecl>(VD))
+      return true;
+
     // Is this part of the module's API or ABI?
     AccessScope accessScope =
         VD->getFormalAccessScope(nullptr,
@@ -1633,12 +1651,6 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
         return;
 
     DeclVisitor<ExportabilityChecker>::visit(D);
-
-    if (auto *extension = dyn_cast<ExtensionDecl>(D->getDeclContext())) {
-      checkType(extension->getExtendedTypeLoc(), extension,
-                getDiagnoseCallback(extension), getDiagnoseCallback(extension));
-      checkConstrainedExtensionRequirements(extension);
-    }
   }
 
   // Force all kinds to be handled at a lower level.
@@ -1678,12 +1690,12 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
   void checkNamedPattern(const NamedPattern *NP,
                          const llvm::DenseSet<const VarDecl *> &seenVars) {
     const VarDecl *theVar = NP->getDecl();
-    if (shouldSkipChecking(theVar))
-      return;
     // Only check individual variables if we didn't check an enclosing
     // TypedPattern.
     if (seenVars.count(theVar) || theVar->isInvalid())
       return;
+    if (shouldSkipChecking(theVar))
+      return;
 
     checkType(theVar->getInterfaceType(), /*typeRepr*/nullptr, theVar,
               getDiagnoseCallback(theVar), getDiagnoseCallback(theVar));
@@ -1809,12 +1821,13 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
                 getDiagnoseCallback(EED));
   }
 
-  void checkConstrainedExtensionRequirements(ExtensionDecl *ED) {
+  void checkConstrainedExtensionRequirements(ExtensionDecl *ED,
+                                             Reason reason) {
     if (!ED->getTrailingWhereClause())
       return;
     forAllRequirementTypes(ED, [&](Type type, TypeRepr *typeRepr) {
-      checkType(type, typeRepr, ED, getDiagnoseCallback(ED),
-                getDiagnoseCallback(ED));
+      checkType(type, typeRepr, ED, getDiagnoseCallback(ED, reason),
+                getDiagnoseCallback(ED, reason));
     });
   }
 
@@ -1830,8 +1843,26 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
                 getDiagnoseCallback(ED));
     });
 
-    if (!ED->getInherited().empty())
-      checkConstrainedExtensionRequirements(ED);
+    bool hasPublicMembers = llvm::any_of(ED->getMembers(),
+                                         [](const Decl *member) -> bool {
+      auto *valueMember = dyn_cast<ValueDecl>(member);
+      if (!valueMember)
+        return false;
+      return !shouldSkipChecking(valueMember);
+    });
+
+    if (hasPublicMembers) {
+      checkType(ED->getExtendedTypeLoc(), ED,
+                getDiagnoseCallback(ED, Reason::ExtensionWithPublicMembers),
+                getDiagnoseCallback(ED, Reason::ExtensionWithPublicMembers));
+    }
+
+    if (hasPublicMembers || !ED->getInherited().empty()) {
+      Reason reason =
+          hasPublicMembers ? Reason::ExtensionWithPublicMembers
+                           : Reason::ExtensionWithConditionalConformances;
+      checkConstrainedExtensionRequirements(ED, reason);
+    }
   }
 
   void checkPrecedenceGroup(const PrecedenceGroupDecl *PGD,
@@ -1844,6 +1875,7 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
 
     auto diag = TC.diagnose(diagLoc, diag::decl_from_implementation_only_module,
                             PGD->getDescriptiveKind(), PGD->getName(),
+                            static_cast<unsigned>(Reason::General),
                             M->getName());
     if (refRange.isValid())
       diag.highlight(refRange);

diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -3684,7 +3684,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());
       } else {
         ctx.Diags.diagnose(
             conformanceBeingChecked->getLoc(),

diff --git a/test/Sema/implementation-only-import-in-decls.swift b/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}}
+  public func testExtensionOfBadType() {}
+  public var testExtensionVarBad: Int { 0 }
+  public subscript(bad _: Int) -> Int { 0 }
 }
 extension BadStruct {
   func testExtensionOfOkayType() {}
+  var testExtensionVarOkay: Int { 0 }
+  subscript(okay _: Int) -> Int { 0 }
 }
 
-extension Array where Element == BadStruct { // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
-  public func testExtensionWithBadRequirement() {} // FIXME: Should complain here instead of at the extension decl.
+extension Array where Element == BadStruct { // expected-error {{cannot use struct 'BadStruct' in an extension with public or '@usableFromInline' members; 'BADLibrary' has been imported as implementation-only}}
+  public func testExtensionWithBadRequirement() {}
+  public var testExtensionVarBad: Int { 0 }
+  public subscript(bad _: Int) -> Int { 0 }
 }
 
 extension Array where Element == BadStruct {
   func testExtensionWithOkayRequirement() {} // okay
+  var testExtensionVarOkay: Int { 0 } // okay
+  subscript(okay _: Int) -> Int { 0 } // okay
 }
 
 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}}
 }
 
 
@@ -165,7 +173,7 @@ public class SubclassOfNormalClass: NormalClass {}
 public func testInheritedConformance(_: NormalProtoAssocHolder<SubclassOfNormalClass>) {} // expected-error {{cannot use conformance of 'NormalClass' to 'NormalProto' here; 'BADLibrary' has been imported as implementation-only}}
 public func testSpecializedConformance(_: NormalProtoAssocHolder<GenericStruct<Int>>) {} // expected-error {{cannot use conformance of 'GenericStruct<T>' to 'NormalProto' here; 'BADLibrary' has been imported as implementation-only}}
 
-extension Array where Element == NormalProtoAssocHolder<NormalStruct> { // expected-error {{cannot use conformance of 'NormalStruct' to 'NormalProto' here; 'BADLibrary' has been imported as implementation-only}}
+extension Array where Element == NormalProtoAssocHolder<NormalStruct> { // expected-error {{cannot use conformance of 'NormalStruct' to 'NormalProto' in an extension with public or '@usableFromInline' members; 'BADLibrary' has been imported as implementation-only}}
   public func testConstrainedExtensionUsingBadConformance() {}
 }