Merge pull request #33213 from xymus/spi-ioi-check-workaround-5.3

[5.3][Sema] Warn on exporting implementation-only imported types in SPI

Author: tkremenek

include/swift/AST/DiagnosticsSema.def

@@ -2684,6 +2684,12 @@ ERROR(decl_from_hidden_module,none,
       "%select{%3 has been imported as implementation-only|"
       "it is an SPI imported from %3}4",
       (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
+WARNING(decl_from_hidden_module_warn,none,
+      "cannot use %0 %1 %select{in SPI|as property wrapper in SPI|"
+      "in an extension with public or '@usableFromInline' members|"
+      "in an extension with conditional conformances}2; "
+      "%select{%3 has been imported as implementation-only}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|"
       "in an extension with public or '@usableFromInline' members|"

lib/Sema/TypeCheckAccess.cpp

@@ -1486,8 +1486,42 @@ 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,
+    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,
+      DowngradeToWarning &downgradeToWarning) {
+    downgradeToWarning = DowngradeToWarning::No;
+    ModuleDecl *M = typeDecl->getModuleContext();
+    if (SF.isImportedImplementationOnly(M)) {
+      // Temporarily downgrade implementation-only exportability in SPI to
+      // a warning.
+      if (context->isSPI())
+        downgradeToWarning = DowngradeToWarning::Yes;
+
+      // Implementation-only imported, cannot be reexported.
+      return DisallowedOriginKind::ImplementationOnly;
+    } else if (SF.isImportedAsSPI(typeDecl) && !context->isSPI()) {
+      // SPI can only be exported in SPI.
+      return DisallowedOriginKind::SPIImported;
+    }
+
+    return DisallowedOriginKind::None;
+  };
+
   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())
@@ -1500,14 +1534,14 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     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();
+        auto downgradeToWarning = DowngradeToWarning::No;
+        auto originKind = getDisallowedOriginKind(typeDecl, SF, context, downgradeToWarning);
+        if (originKind != DisallowedOriginKind::None) {
+          diagnoser.diagnoseType(typeDecl, component, originKind, downgradeToWarning);
+          foundAnyIssues = true;
+        }
+
         // We still continue even in the diagnostic case to report multiple
         // violations.
         return true;
@@ -1525,19 +1559,17 @@ 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));
+        auto downgradeToWarning = DowngradeToWarning::No;
+        auto originKind = getDisallowedOriginKind(typeDecl, SF, context, downgradeToWarning);
+        if (originKind != DisallowedOriginKind::None)
+          diagnoser.diagnoseType(typeDecl, /*typeRepr*/nullptr, originKind, downgradeToWarning);
       }
 
       void visitSubstitutionMap(SubstitutionMap subs) {
@@ -1597,15 +1629,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(
@@ -1634,7 +1666,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 {
@@ -1643,10 +1675,6 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
     ExtensionWithPublicMembers,
     ExtensionWithConditionalConformances
   };
-  enum class HiddenImportKind : uint8_t {
-    ImplementationOnly,
-    SPI
-  };
 
   class Diagnoser {
     const Decl *D;
@@ -1656,16 +1684,17 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
 
     void diagnoseType(const TypeDecl *offendingType,
                       const TypeRepr *complainRepr,
-                      bool isImplementationOnly) const {
+                      DisallowedOriginKind originKind,
+                      DowngradeToWarning downgradeToWarning) const {
       ModuleDecl *M = offendingType->getModuleContext();
-      HiddenImportKind importKind = isImplementationOnly?
-                                HiddenImportKind::ImplementationOnly:
-                                HiddenImportKind::SPI;
-      auto diag = D->diagnose(diag::decl_from_hidden_module,
+      auto errorOrWarning = downgradeToWarning == DowngradeToWarning::Yes?
+                            diag::decl_from_hidden_module_warn:
+                            diag::decl_from_hidden_module;
+      auto diag = D->diagnose(errorOrWarning,
                               offendingType->getDescriptiveKind(),
                               offendingType->getName(),
                               static_cast<unsigned>(reason), M->getName(),
-                              static_cast<unsigned>(importKind));
+                              static_cast<unsigned>(originKind));
       highlightOffendingType(diag, complainRepr);
     }
 
@@ -1702,7 +1731,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?
@@ -1994,7 +2023,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);

test/SPI/implementation_only_spi_import_exposability.swift

@@ -0,0 +1,51 @@
+/// @_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-warning 2 {{cannot use struct 'SPIStruct' in SPI; 'Lib' has been imported as implementation-only}}
+@_spi(B) public func leakIOIStruct(_ a: IOIStruct) -> IOIStruct { fatalError() } // expected-warning 2 {{cannot use struct 'IOIStruct' in SPI; '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}}
+  }
+}
+
+@_spi(B)
+public struct LocalSPIStruct : IOIProtocol, SPIProtocol { // expected-warning {{cannot use protocol 'IOIProtocol' in SPI; 'Lib' has been imported as implementation-only}}
+// expected-warning @-1 {{cannot use protocol 'SPIProtocol' in SPI; 'Lib' has been imported as implementation-only}}
+}
+
+#endif