[Sema] Report public use of local SPIs by the exportability checker

xymus · xymus · commit 63d9acda3ea2 · 2020-06-29T11:18:04.000-07:00
This change improves slightly the quality of the diagnostics and detects
the use of local SPI types on public property with a default value.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2692,7 +2692,8 @@ ERROR(decl_from_hidden_module,none,
       "in an extension with public or '@usableFromInline' members|"
       "in an extension with conditional conformances}2; "
       "%select{%3 has been imported as implementation-only|"
-      "it is an SPI imported from %3}4",
+      "it is an SPI imported from %3|"
+      "it is SPI}4",
       (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
 ERROR(conformance_from_implementation_only_module,none,
       "cannot use conformance of %0 to %1 %select{here|as property wrapper here|"
@@ -4766,7 +4767,8 @@ WARNING(resilience_decl_unavailable_warn,
 ERROR(inlinable_decl_ref_from_hidden_module,
       none, "%0 %1 cannot be used in " FRAGILE_FUNC_KIND "2 "
       "because %select{%3 was imported implementation-only|"
-      "it is an SPI imported from %3}4",
+      "it is an SPI imported from %3|"
+      "it is SPI}4",
       (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
 
 #undef FRAGILE_FUNC_KIND
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -3550,17 +3550,9 @@ static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
     return useSF && useSF->hasTestableOrPrivateImport(access, sourceModule);
   }
   case AccessLevel::Public:
-  case AccessLevel::Open: {
-    if (useDC && VD->isSPI()) {
-      auto useModuleScopeContext = useDC->getModuleScopeContext();
-      if (useModuleScopeContext == sourceDC->getModuleScopeContext()) return true;
-
-      auto *useSF = dyn_cast<SourceFile>(useModuleScopeContext);
-      return !useSF || useSF->isImportedAsSPI(VD);
-    }
+  case AccessLevel::Open:
     return true;
   }
-  }
   llvm_unreachable("bad access level");
 }
 
diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp
@@ -235,13 +235,7 @@ void AccessControlCheckerBase::checkTypeAccessImpl(
         contextAccessScope.isChildOf(*typeReprAccessScope)) {
       // Only if both the Type and the TypeRepr follow the access rules can
       // we exit; otherwise we have to emit a diagnostic.
-
-      if (typeReprAccessScope->isPublic() != contextAccessScope.isPublic() ||
-          !typeReprAccessScope->isSPI() ||
-          contextAccessScope.isSPI()) {
-        // And we exit only if there is no SPI violation.
-        return;
-      }
+      return;
     }
     problematicAccessScope = *typeReprAccessScope;
 
@@ -349,8 +343,7 @@ void AccessControlCheckerBase::checkGenericParamAccess(
         (thisDowngrade == DowngradeToWarning::No &&
          downgradeToWarning == DowngradeToWarning::Yes) ||
         (!complainRepr &&
-         typeAccessScope.hasEqualDeclContextWith(minAccessScope)) ||
-         typeAccessScope.isSPI()) {
+         typeAccessScope.hasEqualDeclContextWith(minAccessScope))) {
       minAccessScope = typeAccessScope;
       complainRepr = thisComplainRepr;
       accessControlErrorKind = callbackACEK;
@@ -375,7 +368,7 @@ void AccessControlCheckerBase::checkGenericParamAccess(
                            const_cast<GenericContext *>(ownerCtx)),
                          accessScope, DC, callback);
 
-  if (minAccessScope.isPublic() && !minAccessScope.isSPI())
+  if (minAccessScope.isPublic())
     return;
 
   // FIXME: Promote these to an error in the next -swift-version break.
@@ -958,7 +951,7 @@ class AccessControlChecker : public AccessControlCheckerBase,
       });
     }
 
-    if (!minAccessScope.isPublic() || minAccessScope.isSPI()) {
+    if (!minAccessScope.isPublic()) {
       auto minAccess = minAccessScope.accessLevelForDiagnostics();
       auto functionKind = isa<ConstructorDecl>(fn)
         ? FK_Initializer
@@ -970,24 +963,13 @@ class AccessControlChecker : public AccessControlCheckerBase,
         ? fn->getFormalAccess()
         : minAccessScope.requiredAccessForDiagnostics();
 
-      // Report as an SPI problem if the type is at least as public as the decl.
-      AccessScope contextAccessScope =
-        fn->getFormalAccessScope(fn->getDeclContext(), checkUsableFromInline);
-
-      if (contextAccessScope.isSPI()) {
-        auto diag = fn->diagnose(diag::function_type_spi, functionKind,
-                                 problemIsResult, minAccess,
-                                 minAccess >= AccessLevel::Public);
-        highlightOffendingType(diag, complainRepr);
-      } else {
-        auto diagID = diag::function_type_access;
-        if (downgradeToWarning == DowngradeToWarning::Yes)
-          diagID = diag::function_type_access_warn;
-        auto diag = fn->diagnose(diagID, isExplicit, fnAccess,
-                                 isa<FileUnit>(fn->getDeclContext()), minAccess,
-                                 functionKind, problemIsResult);
-        highlightOffendingType(diag, complainRepr);
-      }
+      auto diagID = diag::function_type_access;
+      if (downgradeToWarning == DowngradeToWarning::Yes)
+        diagID = diag::function_type_access_warn;
+      auto diag = fn->diagnose(diagID, isExplicit, fnAccess,
+                               isa<FileUnit>(fn->getDeclContext()), minAccess,
+                               functionKind, problemIsResult);
+      highlightOffendingType(diag, complainRepr);
     }
   }
 
@@ -1471,6 +1453,31 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
 class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
   class Diagnoser;
 
+  // Problematic origin of an exported type.
+  //
+  // This enum must be kept in sync with
+  // diag::decl_from_hidden_module and
+  // diag::conformance_from_implementation_only_module.
+  enum class DisallowedOriginKind : uint8_t {
+    ImplementationOnly,
+    SPIImported,
+    SPILocal,
+    None
+  };
+
+  // If there's an exportability problem with \p typeDecl, get its origin kind.
+  static DisallowedOriginKind getDisallowedOriginKind(
+      const TypeDecl *typeDecl, const SourceFile &SF, const Decl *context) {
+    ModuleDecl *M = typeDecl->getModuleContext();
+    if (SF.isImportedImplementationOnly(M))
+      return DisallowedOriginKind::ImplementationOnly;
+    else if (typeDecl->isSPI() && !context->isSPI())
+      return context->getModuleContext() == M ?
+        DisallowedOriginKind::SPILocal :
+        DisallowedOriginKind::SPIImported;
+    return DisallowedOriginKind::None;
+  };
+
   void checkTypeImpl(
       Type type, const TypeRepr *typeRepr, const SourceFile &SF,
       const Decl *context,
@@ -1487,11 +1494,9 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
       const_cast<TypeRepr *>(typeRepr)->walk(TypeReprIdentFinder(
           [&](const ComponentIdentTypeRepr *component) {
         TypeDecl *typeDecl = component->getBoundDecl();
-        ModuleDecl *M = typeDecl->getModuleContext();
-        bool isImplementationOnly = SF.isImportedImplementationOnly(M);
-        if (isImplementationOnly ||
-            (SF.isImportedAsSPI(typeDecl) && !context->isSPI())) {
-          diagnoser.diagnoseType(typeDecl, component, isImplementationOnly);
+        auto originKind = getDisallowedOriginKind(typeDecl, SF, context);
+        if (originKind != DisallowedOriginKind::None) {
+          diagnoser.diagnoseType(typeDecl, component, originKind);
           foundAnyIssues = true;
         }
 
@@ -1519,12 +1524,9 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
         : SF(SF), context(context), diagnoser(diagnoser) {}
 
       void visitTypeDecl(const TypeDecl *typeDecl) {
-        ModuleDecl *M = typeDecl->getModuleContext();
-        bool isImplementationOnly = SF.isImportedImplementationOnly(M);
-        if (isImplementationOnly ||
-            (SF.isImportedAsSPI(typeDecl) && !context->isSPI()))
-          diagnoser.diagnoseType(typeDecl, /*typeRepr*/nullptr,
-                                 isImplementationOnly);
+        auto originKind = getDisallowedOriginKind(typeDecl, SF, context);
+        if (originKind != DisallowedOriginKind::None)
+          diagnoser.diagnoseType(typeDecl, /*typeRepr*/nullptr, originKind);
       }
 
       void visitSubstitutionMap(SubstitutionMap subs) {
@@ -1621,7 +1623,7 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     });
   }
 
-  // These enums must be kept in sync with
+  // This enum must be kept in sync with
   // diag::decl_from_hidden_module and
   // diag::conformance_from_implementation_only_module.
   enum class Reason : unsigned {
@@ -1630,10 +1632,6 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     ExtensionWithPublicMembers,
     ExtensionWithConditionalConformances
   };
-  enum class HiddenImportKind : uint8_t {
-    ImplementationOnly,
-    SPI
-  };
 
   class Diagnoser {
     const Decl *D;
@@ -1643,16 +1641,13 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
 
     void diagnoseType(const TypeDecl *offendingType,
                       const TypeRepr *complainRepr,
-                      bool isImplementationOnly) const {
+                      DisallowedOriginKind originKind) const {
       ModuleDecl *M = offendingType->getModuleContext();
-      HiddenImportKind importKind = isImplementationOnly?
-                                HiddenImportKind::ImplementationOnly:
-                                HiddenImportKind::SPI;
       auto diag = D->diagnose(diag::decl_from_hidden_module,
                               offendingType->getDescriptiveKind(),
                               offendingType->getName(),
                               static_cast<unsigned>(reason), M->getName(),
-                              static_cast<unsigned>(importKind));
+                              static_cast<unsigned>(originKind));
       highlightOffendingType(diag, complainRepr);
     }
 
@@ -1981,7 +1976,7 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
         DE.diagnose(diagLoc, diag::decl_from_hidden_module,
                     PGD->getDescriptiveKind(), PGD->getName(),
                     static_cast<unsigned>(Reason::General), M->getName(),
-                    static_cast<unsigned>(HiddenImportKind::ImplementationOnly)
+                    static_cast<unsigned>(DisallowedOriginKind::ImplementationOnly)
                     );
     if (refRange.isValid())
       diag.highlight(refRange);
diff --git a/test/SPI/local_spi_decls.swift b/test/SPI/local_spi_decls.swift
@@ -10,17 +10,17 @@
 @_spi() public func emptyParensSPI() {} // expected-error {{expected an SPI identifier as subject of the '@_spi' attribute}}
 @_spi(set) public func keywordSPI() {} // expected-error {{expected an SPI identifier as subject of the '@_spi' attribute}}
 
-@_spi(S) public class SPIClass {} // expected-note 2 {{type declared here}}
+@_spi(S) public class SPIClass {} // expected-note 3 {{type declared here}}
   // expected-note @-1 2 {{class 'SPIClass' is not '@usableFromInline' or public}}
 class InternalClass {} // expected-note 2 {{type declared here}}
 private class PrivateClass {} // expected-note 2 {{type declared here}}
 
 @_spi(S) public protocol SPIProtocol {} // expected-note {{type declared here}}
 
 @_spi(S) public func useOfSPITypeOk(_ p0: SPIProtocol, p1: SPIClass) -> SPIClass { fatalError() } // OK
-public func useOfSPITypeInvalid() -> SPIClass { fatalError() } // expected-error {{function cannot be declared public because its result uses an '@_spi' type}}
-@_spi(S) public func spiUseOfInternalType() -> InternalClass { fatalError() } // expected-error{{function cannot be declared '@_spi' because its result uses an internal type}}
-@_spi(S) public func spiUseOfPrivateType(_ a: PrivateClass)  { fatalError() } // expected-error{{function cannot be declared '@_spi' because its parameter uses a private type}}
+public func useOfSPITypeInvalid() -> SPIClass { fatalError() } // expected-error {{cannot use class 'SPIClass' here; it is SPI}}
+@_spi(S) public func spiUseOfInternalType() -> InternalClass { fatalError() } // expected-error{{function cannot be declared public because its result uses an internal type}}
+@_spi(S) public func spiUseOfPrivateType(_ a: PrivateClass)  { fatalError() } // expected-error{{function cannot be declared public because its parameter uses a private type}}
 
 @inlinable
 func inlinable() -> SPIClass { // expected-error {{class 'SPIClass' is '@_spi' and cannot be referenced from an '@inlinable' function}}
@@ -41,12 +41,17 @@ private protocol PrivateProtocol {} // expected-note {{type declared here}}
 
 @_spi(S) public class BadSubclass : InternalClass {} // expected-error{{class cannot be declared public because its superclass is internal}}
 @_spi(S) public class OkSPISubclass : SPIClass {} // OK
-public class BadPublicClass : SPIClass {} // expected-error {{class cannot be declared public because its superclass is '@_spi'}}
+public class BadPublicClass : SPIClass {} // expected-error {{cannot use class 'SPIClass' here; it is SPI}}
 @_spi(S) public class BadSPIClass : PrivateClass {} // expected-error {{class cannot be declared public because its superclass is private}}
 
 @_spi(s) public func genFunc<T: PrivateProtocol>(_ t: T) {} // expected-error {{global function cannot be declared public because its generic parameter uses a private type}}
-public func genFuncBad<T: SPIProtocol>(_ t: T) {} // expected-error {{global function cannot be declared public because its generic parameter uses an '@_spi' type}}
+public func genFuncBad<T: SPIProtocol>(_ t: T) {} // expected-error {{cannot use protocol 'SPIProtocol' here; it is SPI}}
 @_spi(S) public func genFuncSPI<T: SPIProtocol>(_ t: T) {} // OK
 
 @_spi(S) private func privateCantBeSPI() {} // expected-error{{private global function cannot be declared '@_spi' because only public and open declarations can be '@_spi'}} {{1-10=}}
 @_spi(S) func internalCantBeSPI() {} // expected-error{{internal global function cannot be declared '@_spi' because only public and open declarations can be '@_spi'}} {{1-10=}}
+
+public struct PublicStructWithProperties {
+  public var a: SPIClass // expected-error {{cannot use class 'SPIClass' here; it is SPI}}
+  public var b = SPIClass() // expected-error {{cannot use class 'SPIClass' here; it is SPI}}
+}
diff --git a/test/SPI/spi_client.swift b/test/SPI/spi_client.swift
@@ -50,9 +50,7 @@ print(ps.spiVar)
 otherApiFunc() // expected-error {{cannot find 'otherApiFunc' in scope}}
 
 public func publicUseOfSPI(param: SPIClass) -> SPIClass {} // expected-error 2{{cannot use class 'SPIClass' here; it is an SPI imported from 'SPIHelper'}}
-// expected-error@-1 {{function cannot be declared public because its parameter uses an '@_spi' type}}
 public func publicUseOfSPI2() -> [SPIClass] {} // expected-error {{cannot use class 'SPIClass' here; it is an SPI imported from 'SPIHelper'}}
-// expected-error@-1 {{function cannot be declared public because its result uses an '@_spi' type}}
 
 @inlinable
 func inlinable() -> SPIClass { // expected-error {{class 'SPIClass' is '@_spi' and cannot be referenced from an '@inlinable' function}}

Original file line number	Diff line number	Diff line change
`@@ -3550,17 +3550,9 @@ static bool checkAccess(const DeclContext useDC, const ValueDecl VD,`
`3550`	`3550`	`return useSF && useSF->hasTestableOrPrivateImport(access, sourceModule);`
`3551`	`3551`	`}`
`3552`	`3552`	`case AccessLevel::Public:`
`3553`		`- case AccessLevel::Open: {`
`3554`		`- if (useDC && VD->isSPI()) {`
`3555`		`- auto useModuleScopeContext = useDC->getModuleScopeContext();`
`3556`		`- if (useModuleScopeContext == sourceDC->getModuleScopeContext()) return true;`
`3557`		`-`
`3558`		`- auto *useSF = dyn_cast<SourceFile>(useModuleScopeContext);`
`3559`		`- return !useSF \|\| useSF->isImportedAsSPI(VD);`
`3560`		`- }`
	`3553`	`+ case AccessLevel::Open:`
`3561`	`3554`	`return true;`
`3562`	`3555`	`}`
`3563`		`- }`
`3564`	`3556`	`llvm_unreachable("bad access level");`
`3565`	`3557`	`}`
`3566`	`3558`