Merge pull request #31761 from xymus/spi-checks-5.3

[5.3] Check SPI usage of implementation-only types, in frozen types and on protocol requirements

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -1705,6 +1705,13 @@ ERROR(spi_attribute_on_non_public,none,
       "cannot be declared '@_spi' because only public and open "
       "declarations can be '@_spi'",
       (AccessLevel, DescriptiveDeclKind))
+ERROR(spi_attribute_on_protocol_requirement,none,
+      "protocol requirement %0 cannot be declared '@_spi' without "
+      "a default implementation in a protocol extension",
+      (DeclName))
+ERROR(spi_attribute_on_frozen_stored_properties,none,
+      "stored property %0 cannot be declared '@_spi' in a '@frozen' struct",
+      (DeclName))
 
 // Opaque return types
 ERROR(opaque_type_invalid_constraint,none,

lib/Sema/TypeCheckAccess.cpp

@@ -1483,11 +1483,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())
@@ -1500,14 +1503,15 @@ 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();
+        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 +1529,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 +1601,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 +1706,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?

lib/Sema/TypeCheckAttr.cpp

@@ -862,13 +862,60 @@ void AttributeChecker::visitSetterAccessAttr(
 
 void AttributeChecker::visitSPIAccessControlAttr(SPIAccessControlAttr *attr) {
   if (auto VD = dyn_cast<ValueDecl>(D)) {
+    // VD must be public or open to use an @_spi attribute.
     auto declAccess = VD->getFormalAccess();
     if (declAccess < AccessLevel::Public) {
       diagnoseAndRemoveAttr(attr,
                             diag::spi_attribute_on_non_public,
                             declAccess,
                             D->getDescriptiveKind());
     }
+
+    // If VD is a public protocol requirement it can be SPI only if there's
+    // a default implementation.
+    if (auto protocol = dyn_cast<ProtocolDecl>(D->getDeclContext())) {
+      auto implementations = TypeChecker::lookupMember(
+                                             D->getDeclContext(),
+                                             protocol->getDeclaredType(),
+                                             VD->createNameRef(),
+                                             NameLookupFlags::ProtocolMembers);
+      bool hasDefaultImplementation = llvm::any_of(implementations,
+        [&](const LookupResultEntry &entry) {
+          auto entryDecl = entry.getValueDecl();
+          auto DC = entryDecl->getDeclContext();
+          auto extension = dyn_cast<ExtensionDecl>(DC);
+
+          // The implementation must be defined in the same module in
+          // an unconstrained extension.
+          if (!extension ||
+              extension->getParentModule() != protocol->getParentModule() ||
+              extension->isConstrainedExtension())
+            return false;
+
+          // For computed properties and subscripts, check that the default
+          // implementation defines `set` if the protocol declares it.
+          if (auto protoStorage = dyn_cast<AbstractStorageDecl>(VD))
+            if (auto entryStorage = dyn_cast<AbstractStorageDecl>(entryDecl))
+              if (protoStorage->getAccessor(AccessorKind::Set) &&
+                  !entryStorage->getAccessor(AccessorKind::Set))
+                return false;
+
+          return true;
+        });
+
+      if (!hasDefaultImplementation)
+        diagnoseAndRemoveAttr(attr,
+                              diag::spi_attribute_on_protocol_requirement,
+                              VD->getName());
+    }
+
+    // Forbid stored properties marked SPI in frozen types.
+    if (auto property = dyn_cast<AbstractStorageDecl>(VD))
+      if (auto DC = dyn_cast<NominalTypeDecl>(D->getDeclContext()))
+        if (property->hasStorage() && !DC->isFormallyResilient())
+          diagnoseAndRemoveAttr(attr,
+                                diag::spi_attribute_on_frozen_stored_properties,
+                                VD->getName());
   }
 }
 

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

test/SPI/local_spi_decls.swift

@@ -31,7 +31,9 @@ func inlinable() -> SPIClass { // expected-error {{class 'SPIClass' is '@_spi' a
 @_spi(S) public struct SPIStruct {} // expected-note 2 {{struct 'SPIStruct' is not '@usableFromInline' or public}}
 
 @frozen public struct FrozenStruct {
-  @_spi(S) public var asdf = SPIStruct() // expected-error {{struct 'SPIStruct' is '@_spi' and cannot be referenced from a property initializer in a '@frozen' type}}
+  @_spi(S) public var spiInFrozen = SPIStruct() // expected-error {{struct 'SPIStruct' is '@_spi' and cannot be referenced from a property initializer in a '@frozen' type}}
+  // expected-error @-1 {{stored property 'spiInFrozen' cannot be declared '@_spi' in a '@frozen' struct}}
+
   var asdf = SPIStruct() // expected-error {{struct 'SPIStruct' is '@_spi' and cannot be referenced from a property initializer in a '@frozen' type}}
 }
 

test/SPI/protocol_requirement.swift

@@ -0,0 +1,71 @@
+// Test limitations on SPI protocol requirements.
+
+// RUN: %target-typecheck-verify-swift
+
+// Reject SPI protocol requirements without a default implementation.
+public protocol PublicProtoRejected {
+  @_spi(Private) // expected-error{{protocol requirement 'reqWithoutDefault()' cannot be declared '@_spi' without a default implementation in a protocol extension}}
+  func reqWithoutDefault()
+
+  @_spi(Private) // expected-error{{protocol requirement 'property' cannot be declared '@_spi' without a default implementation in a protocol extension}}
+  var property: Int { get set }
+
+  @_spi(Private) // expected-error{{protocol requirement 'propertyWithoutSetter' cannot be declared '@_spi' without a default implementation in a protocol extension}}
+  var propertyWithoutSetter: Int { get set }
+
+  @_spi(Private) // expected-error{{protocol requirement 'subscript(_:)' cannot be declared '@_spi' without a default implementation in a protocol extension}}
+  subscript(index: Int) -> Int { get set }
+
+  @_spi(Private) // expected-error{{protocol requirement 'init()' cannot be declared '@_spi' without a default implementation in a protocol extension}}
+  init()
+
+  @_spi(Private) // expected-error{{'@_spi' attribute cannot be applied to this declaration}}
+  associatedtype T
+}
+
+extension PublicProtoRejected {
+  @_spi(Private)
+  public var propertyWithoutSetter: Int { get { return 42 } }
+}
+
+extension PublicProtoRejected where Self : Equatable {
+  @_spi(Private)
+  public func reqWithoutDefault() {
+    // constrainted implementation
+  }
+}
+
+// Accept SPI protocol requirements with an implementation.
+public protocol PublicProto {
+  @_spi(Private)
+  func reqWithDefaultImplementation()
+
+  @_spi(Private)
+  var property: Int { get set }
+
+  @_spi(Private)
+  subscript(index: Int) -> Int { get set }
+
+  @_spi(Private)
+  init()
+}
+
+extension PublicProto {
+  @_spi(Private)
+  public func reqWithDefaultImplementation() { }
+
+  @_spi(Private)
+  public var property: Int {
+    get { return 42 }
+    set { }
+  }
+
+  @_spi(Private)
+  public subscript(index: Int) -> Int {
+    get { return 42 }
+    set { }
+  }
+
+  @_spi(Private)
+  public init() { }
+}

test/SPI/resilience.swift

@@ -0,0 +1,29 @@
+// Test limits of SPI members in frozen types.
+
+// RUN: %target-typecheck-verify-swift
+
+@frozen
+public struct FrozenStruct {
+  public var okProperty: Int
+
+  @_spi(S)
+  public var okComputedProperty: Int { get { return 42 } }
+
+  @_spi(S) // expected-error {{stored property 'spiProperty' cannot be declared '@_spi' in a '@frozen' struct}}
+  public var spiProperty: Int
+
+  @_spi(S) // expected-error {{stored property 'spiPropertySet' cannot be declared '@_spi' in a '@frozen' struct}}
+  public var spiPropertySet = 4
+
+  @_spi(S) // expected-error {{stored property 'spiPropertyDoubleErrors' cannot be declared '@_spi' in a '@frozen' struct}}
+  // expected-error @-1 {{internal property cannot be declared '@_spi' because only public and open declarations can be '@_spi'}}
+  var spiPropertyDoubleErrors: Int
+}
+
+public struct UnfrozenStruct {
+  @_spi(S)
+  public var spiProperty: Int
+
+  @_spi(S)
+  public var spiPropertySet = 4
+}