Merge pull request swiftlang#31744 from xymus/dont-leak-ioi-in-spi

[Sema] Forbid leaking implementation-only imported types in SPI decls

Author: xymus

lib/Sema/TypeCheckAccess.cpp

@@ -42,15 +42,6 @@ enum class DowngradeToWarning: bool {
   Yes
 };
 
-/// A uniquely-typed boolean to reduce the chances of accidentally inverting
-/// a check.
-///
-/// \see checkTypeAccessImpl
-enum class FromSPI: bool {
-  No,
-  Yes
-};
-
 /// Calls \p callback for each type in each requirement provided by
 /// \p source.
 static void forAllRequirementTypes(
@@ -87,7 +78,7 @@ class AccessControlCheckerBase {
 
   void checkTypeAccessImpl(
       Type type, TypeRepr *typeRepr, AccessScope contextAccessScope,
-      const DeclContext *useDC, bool mayBeInferred, FromSPI fromSPI,
+      const DeclContext *useDC, bool mayBeInferred,
       llvm::function_ref<CheckTypeAccessCallback> diagnose);
 
   void checkTypeAccess(
@@ -109,7 +100,7 @@ class AccessControlCheckerBase {
       llvm::function_ref<CheckTypeAccessCallback> diagnose) {
     forAllRequirementTypes(std::move(source), [&](Type type, TypeRepr *typeRepr) {
       checkTypeAccessImpl(type, typeRepr, accessScope, useDC,
-                          /*mayBeInferred*/false, FromSPI::No, diagnose);
+                          /*mayBeInferred*/false, diagnose);
     });
   }
 
@@ -196,12 +187,9 @@ class TypeAccessScopeDiagnoser : private ASTWalker {
 /// using `Array` to mean `Array<Element>` in an extension of Array.) If
 /// \p typeRepr is known to be absent, it's okay to pass \c false for
 /// \p mayBeInferred.
-///
-/// If searching from an SPI context, pass \c FromSPI::YES for \p fromSPI.
-/// In this mode, all types must be public and diagnostic messages are adapted.
 void AccessControlCheckerBase::checkTypeAccessImpl(
     Type type, TypeRepr *typeRepr, AccessScope contextAccessScope,
-    const DeclContext *useDC, bool mayBeInferred, FromSPI fromSPI,
+    const DeclContext *useDC, bool mayBeInferred,
     llvm::function_ref<CheckTypeAccessCallback> diagnose) {
 
   auto &Context = useDC->getASTContext();
@@ -310,9 +298,8 @@ void AccessControlCheckerBase::checkTypeAccess(
     context->getFormalAccessScope(
       context->getDeclContext(), checkUsableFromInline);
 
-  auto fromSPI = static_cast<FromSPI>(context->isSPI());
   checkTypeAccessImpl(type, typeRepr, contextAccessScope, DC, mayBeInferred,
-                      fromSPI, diagnose);
+                      diagnose);
 }
 
 /// Highlights the given TypeRepr, and adds a note pointing to the type's
@@ -372,10 +359,6 @@ void AccessControlCheckerBase::checkGenericParamAccess(
   };
 
   auto *DC = ownerDecl->getDeclContext();
-  auto fromSPI = FromSPI::No;
-  if (auto ownerValueDecl = dyn_cast<ValueDecl>(ownerDecl)) {
-    fromSPI = static_cast<FromSPI>(ownerValueDecl->isSPI());
-  }
 
   for (auto param : *params) {
     if (param->getInherited().empty())
@@ -384,7 +367,7 @@ void AccessControlCheckerBase::checkGenericParamAccess(
     checkTypeAccessImpl(param->getInherited().front().getType(),
                         param->getInherited().front().getTypeRepr(),
                         accessScope, DC, /*mayBeInferred*/false,
-                        fromSPI, callback);
+                        callback);
   }
   callbackACEK = ACEK::Requirement;
 
@@ -1483,11 +1466,14 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
   }
 };
 
+// Diagnose public APIs exposing types that are either imported as
+// implementation-only or declared as SPI.
 class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
   class Diagnoser;
 
   void checkTypeImpl(
       Type type, const TypeRepr *typeRepr, const SourceFile &SF,
+      const Decl *context,
       const Diagnoser &diagnoser) {
     // Don't bother checking errors.
     if (type && type->hasError())
@@ -1496,18 +1482,19 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     bool foundAnyIssues = false;
 
     // Check the TypeRepr first (if present), because that will give us a
-    // better diagonstic.
+    // better diagnostic.
     if (typeRepr) {
       const_cast<TypeRepr *>(typeRepr)->walk(TypeReprIdentFinder(
           [&](const ComponentIdentTypeRepr *component) {
-        ModuleDecl *M = component->getBoundDecl()->getModuleContext();
-        if (!SF.isImportedImplementationOnly(M) &&
-            !SF.isImportedAsSPI(component->getBoundDecl()))
-          return true;
-
-        diagnoser.diagnoseType(component->getBoundDecl(), component,
-                               SF.isImportedImplementationOnly(M));
-        foundAnyIssues = true;
+        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);
+          foundAnyIssues = true;
+        }
+
         // We still continue even in the diagnostic case to report multiple
         // violations.
         return true;
@@ -1525,19 +1512,19 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
 
     class ProblematicTypeFinder : public TypeDeclFinder {
       const SourceFile &SF;
+      const Decl *context;
       const Diagnoser &diagnoser;
     public:
-      ProblematicTypeFinder(const SourceFile &SF, const Diagnoser &diagnoser)
-        : SF(SF), diagnoser(diagnoser) {}
+      ProblematicTypeFinder(const SourceFile &SF, const Decl *context, const Diagnoser &diagnoser)
+        : SF(SF), context(context), diagnoser(diagnoser) {}
 
       void visitTypeDecl(const TypeDecl *typeDecl) {
         ModuleDecl *M = typeDecl->getModuleContext();
-        if (!SF.isImportedImplementationOnly(M) &&
-            !SF.isImportedAsSPI(typeDecl))
-          return;
-
-        diagnoser.diagnoseType(typeDecl, /*typeRepr*/nullptr,
-                               SF.isImportedImplementationOnly(M));
+        bool isImplementationOnly = SF.isImportedImplementationOnly(M);
+        if (isImplementationOnly ||
+            (SF.isImportedAsSPI(typeDecl) && !context->isSPI()))
+          diagnoser.diagnoseType(typeDecl, /*typeRepr*/nullptr,
+                                 isImplementationOnly);
       }
 
       void visitSubstitutionMap(SubstitutionMap subs) {
@@ -1597,15 +1584,15 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
       }
     };
 
-    type.walk(ProblematicTypeFinder(SF, diagnoser));
+    type.walk(ProblematicTypeFinder(SF, context, diagnoser));
   }
 
   void checkType(
       Type type, const TypeRepr *typeRepr, const Decl *context,
       const Diagnoser &diagnoser) {
     auto *SF = context->getDeclContext()->getParentSourceFile();
     assert(SF && "checking a non-source declaration?");
-    return checkTypeImpl(type, typeRepr, *SF, diagnoser);
+    return checkTypeImpl(type, typeRepr, *SF, context, diagnoser);
   }
 
   void checkType(
@@ -1702,7 +1689,7 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     AccessScope accessScope =
         VD->getFormalAccessScope(nullptr,
                                  /*treatUsableFromInlineAsPublic*/true);
-    if (accessScope.isPublic() && !accessScope.isSPI())
+    if (accessScope.isPublic())
       return false;
 
     // Is this a stored property in a non-resilient struct or class?

test/SPI/implementation_only_spi_import_exposability.swift

@@ -0,0 +1,46 @@
+/// @_implementationOnly imported decls (SPI or not) should not be exposed in SPI.
+
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -DLIB %s -module-name Lib -emit-module-path %t/Lib.swiftmodule
+// RUN: %target-typecheck-verify-swift -DCLIENT -I %t
+
+#if LIB
+
+@_spi(A) public func spiFunc() {}
+
+@_spi(A) public struct SPIStruct {
+  public init() {}
+}
+
+@_spi(A) public protocol SPIProtocol {}
+
+public func ioiFunc() {}
+
+public struct IOIStruct {
+  public init() {}
+}
+
+public protocol IOIProtocol {}
+
+#elseif CLIENT
+
+@_spi(A) @_implementationOnly import Lib
+
+@_spi(B) public func leakSPIStruct(_ a: SPIStruct) -> SPIStruct { fatalError() } // expected-error 2 {{cannot use struct 'SPIStruct' here; 'Lib' has been imported as implementation-only}}
+@_spi(B) public func leakIOIStruct(_ a: IOIStruct) -> IOIStruct { fatalError() } // expected-error 2 {{cannot use struct 'IOIStruct' here; 'Lib' has been imported as implementation-only}}
+
+public struct PublicStruct : IOIProtocol, SPIProtocol { // expected-error {{cannot use protocol 'IOIProtocol' here; 'Lib' has been imported as implementation-only}}
+// expected-error @-1 {{cannot use protocol 'SPIProtocol' here; 'Lib' has been imported as implementation-only}}
+  public var spiStruct = SPIStruct() // expected-error {{cannot use struct 'SPIStruct' here; 'Lib' has been imported as implementation-only}}
+  public var ioiStruct = IOIStruct() // expected-error {{cannot use struct 'IOIStruct' here; 'Lib' has been imported as implementation-only}}
+
+  @inlinable
+  public func publicInlinable() {
+    spiFunc() // expected-error {{global function 'spiFunc()' is '@_spi' and cannot be referenced from an '@inlinable' function}}
+    ioiFunc() // expected-error {{global function 'ioiFunc()' cannot be used in an '@inlinable' function because 'Lib' was imported implementation-only}}
+    let s = SPIStruct() // expected-error {{struct 'SPIStruct' is '@_spi' and cannot be referenced from an '@inlinable' function}}
+    let i = IOIStruct() // expected-error {{struct 'IOIStruct' cannot be used in an '@inlinable' function because 'Lib' was imported implementation-only}}
+  }
+}
+
+#endif