Merge pull request swiftlang#33686 from xymus/check-spi-conformances

xymus · web-flow · commit ca721677e844 · 2020-09-03T14:13:41.000-07:00
[Sema] Check SPI conformances exportability
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2739,13 +2739,17 @@ WARNING(decl_from_hidden_module_warn,none,
 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|"
-      "in an extension with conditional conformances}2; %3 has been imported "
-      "as implementation-only",
-      (Type, Identifier, unsigned, Identifier))
+      "in an extension with conditional conformances}2; "
+      "%select{%3 has been imported as implementation-only|"
+      "the conformance is declared as SPI in %3|"
+      "the conformance is declared as SPI}4",
+      (Type, Identifier, unsigned, Identifier, unsigned))
 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",
-      (Type, Identifier, Identifier, Type, Type))
+      "%4); %select{%2 has been imported as implementation-only|"
+      "the conformance is declared as SPI in %2|"
+      "the conformance is declared as SPI}5",
+      (Type, Identifier, Identifier, Type, Type, unsigned))
 ERROR(unexportable_clang_function_type,none,
       "cannot export the underlying C type of the function type %0; "
       "it may use anonymous types or types defined outside of a module",
diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp
@@ -16,6 +16,7 @@
 
 #include "TypeChecker.h"
 #include "TypeCheckAvailability.h"
+#include "TypeCheckAccess.h"
 #include "swift/AST/Attr.h"
 #include "swift/AST/Decl.h"
 #include "swift/AST/DeclContext.h"
@@ -171,7 +172,8 @@ static bool diagnoseDeclExportability(SourceLoc loc, const ValueDecl *D,
 static bool
 diagnoseGenericArgumentsExportability(SourceLoc loc,
                                       SubstitutionMap subs,
-                                      const SourceFile &userSF) {
+                                      const SourceFile &userSF,
+                                      const DeclContext *userDC) {
   bool hadAnyIssues = false;
   for (ProtocolConformanceRef conformance : subs.getConformances()) {
     if (!conformance.isConcrete())
@@ -180,18 +182,23 @@ diagnoseGenericArgumentsExportability(SourceLoc loc,
 
     SubstitutionMap subConformanceSubs =
         concreteConf->getSubstitutions(userSF.getParentModule());
-    diagnoseGenericArgumentsExportability(loc, subConformanceSubs, userSF);
+    diagnoseGenericArgumentsExportability(loc, subConformanceSubs, userSF, userDC);
 
     const RootProtocolConformance *rootConf =
         concreteConf->getRootConformance();
     ModuleDecl *M = rootConf->getDeclContext()->getParentModule();
-    if (!userSF.isImportedImplementationOnly(M))
+
+    auto originKind = getDisallowedOriginKind(
+        rootConf->getDeclContext()->getAsDecl(),
+        userSF, userDC->getInnermostDeclarationDeclContext());
+    if (originKind == DisallowedOriginKind::None)
       continue;
 
     ASTContext &ctx = M->getASTContext();
     ctx.Diags.diagnose(loc, diag::conformance_from_implementation_only_module,
                        rootConf->getType(),
-                       rootConf->getProtocol()->getName(), 0, M->getName());
+                       rootConf->getProtocol()->getName(), 0, M->getName(),
+                       static_cast<unsigned>(originKind));
     hadAnyIssues = true;
   }
   return hadAnyIssues;
@@ -209,10 +216,10 @@ void TypeChecker::diagnoseGenericTypeExportability(SourceLoc Loc, Type T,
   if (auto *BGT = dyn_cast<BoundGenericType>(T.getPointer())) {
     ModuleDecl *useModule = SF->getParentModule();
     auto subs = T->getContextSubstitutionMap(useModule, BGT->getDecl());
-    (void)diagnoseGenericArgumentsExportability(Loc, subs, *SF);
+    (void)diagnoseGenericArgumentsExportability(Loc, subs, *SF, DC);
   } else if (auto *TAT = dyn_cast<TypeAliasType>(T.getPointer())) {
     auto subs = TAT->getSubstitutionMap();
-    (void)diagnoseGenericArgumentsExportability(Loc, subs, *SF);
+    (void)diagnoseGenericArgumentsExportability(Loc, subs, *SF, DC);
   }
 }
 
@@ -226,19 +233,11 @@ TypeChecker::diagnoseDeclRefExportability(SourceLoc loc,
   if (!userSF)
     return false;
 
-  // If the source file doesn't have any implementation-only imports,
-  // we can fast-path this.  In the current language design, we never
-  // need to consider the possibility of implementation-only imports
-  // from other source files in the module (or indirectly in other modules).
-  // TODO: maybe check whether D is from a bridging header?
-  if (!userSF->hasImplementationOnlyImports())
-    return false;
-
   const ValueDecl *D = declRef.getDecl();
   if (diagnoseDeclExportability(loc, D, *userSF, fragileKind))
     return true;
   if (diagnoseGenericArgumentsExportability(loc, declRef.getSubstitutions(),
-                                            *userSF)) {
+                                            *userSF, DC)) {
     return true;
   }
   return false;
diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp
@@ -1454,47 +1454,40 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
   }
 };
 
+/// Returns the kind of origin, implementation-only import or SPI declaration,
+/// that restricts exporting \p decl from the given file and context.
+///
+/// Local variant to swift::getDisallowedOriginKind for downgrade to warnings.
+DisallowedOriginKind
+getDisallowedOriginKind(const Decl *decl,
+                        const SourceFile &userSF,
+                        const Decl *userContext,
+                        DowngradeToWarning &downgradeToWarning) {
+  downgradeToWarning = DowngradeToWarning::No;
+  ModuleDecl *M = decl->getModuleContext();
+  if (userSF.isImportedImplementationOnly(M)) {
+    // Temporarily downgrade implementation-only exportability in SPI to
+    // a warning.
+    if (userContext->isSPI())
+      downgradeToWarning = DowngradeToWarning::Yes;
+
+    // Implementation-only imported, cannot be reexported.
+    return DisallowedOriginKind::ImplementationOnly;
+  } else if (decl->isSPI() && !userContext->isSPI()) {
+    // SPI can only be exported in SPI.
+    return userContext->getModuleContext() == M ?
+      DisallowedOriginKind::SPILocal :
+      DisallowedOriginKind::SPIImported;
+  }
+
+  return DisallowedOriginKind::None;
+};
+
 // Diagnose public APIs exposing types that are either imported as
 // implementation-only or declared as SPI.
 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,
-      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 (typeDecl->isSPI() && !context->isSPI()) {
-      // SPI can only be exported in SPI.
-      return context->getModuleContext() == M ?
-        DisallowedOriginKind::SPILocal :
-        DisallowedOriginKind::SPIImported;
-    }
-
-    return DisallowedOriginKind::None;
-  };
-
   void checkTypeImpl(
       Type type, const TypeRepr *typeRepr, const SourceFile &SF,
       const Decl *context,
@@ -1560,10 +1553,12 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
 
           const RootProtocolConformance *rootConf =
               concreteConf->getRootConformance();
-          ModuleDecl *M = rootConf->getDeclContext()->getParentModule();
-          if (!SF.isImportedImplementationOnly(M))
+          auto originKind = getDisallowedOriginKind(
+              rootConf->getDeclContext()->getAsDecl(),
+              SF, context);
+          if (originKind == DisallowedOriginKind::None)
             continue;
-          diagnoser.diagnoseConformance(rootConf);
+          diagnoser.diagnoseConformance(rootConf, originKind);
         }
       }
 
@@ -1674,12 +1669,14 @@ class ExportabilityChecker : public DeclVisitor<ExportabilityChecker> {
       highlightOffendingType(diag, complainRepr);
     }
 
-    void diagnoseConformance(const ProtocolConformance *offendingConformance) const {
+    void diagnoseConformance(const ProtocolConformance *offendingConformance,
+                             DisallowedOriginKind originKind) const {
       ModuleDecl *M = offendingConformance->getDeclContext()->getParentModule();
       D->diagnose(diag::conformance_from_implementation_only_module,
                   offendingConformance->getType(),
                   offendingConformance->getProtocol()->getName(),
-                  static_cast<unsigned>(reason), M->getName());
+                  static_cast<unsigned>(reason), M->getName(),
+                  static_cast<unsigned>(originKind));
     }
 
     void diagnoseClangFunctionType(Type fnType, const clang::Type *type) const {
@@ -2067,6 +2064,13 @@ static void checkExtensionGenericParamAccess(const ExtensionDecl *ED) {
       ED, ED, desiredAccessScope, userSpecifiedAccess);
 }
 
+DisallowedOriginKind swift::getDisallowedOriginKind(const Decl *decl,
+                                                    const SourceFile &userSF,
+                                                    const Decl *declContext) {
+  auto downgradeToWarning = DowngradeToWarning::No;
+  return getDisallowedOriginKind(decl, userSF, declContext, downgradeToWarning);
+}
+
 void swift::checkAccessControl(Decl *D) {
   if (isa<ValueDecl>(D) || isa<PatternBindingDecl>(D)) {
     AccessControlChecker().visit(D);
diff --git a/lib/Sema/TypeCheckAccess.h b/lib/Sema/TypeCheckAccess.h
@@ -17,9 +17,12 @@
 #ifndef TYPECHECKACCESS_H
 #define TYPECHECKACCESS_H
 
+#include <cstdint>
+
 namespace swift {
 
 class Decl;
+class SourceFile;
 
 /// Performs access-related checks for \p D.
 ///
@@ -28,6 +31,24 @@ class Decl;
 /// itself. Related checks may also be performed.
 void checkAccessControl(Decl *D);
 
+// 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
+};
+
+/// Returns the kind of origin, implementation-only import or SPI declaration,
+/// that restricts exporting \p decl from the given file and context.
+DisallowedOriginKind getDisallowedOriginKind(const Decl *decl,
+                                             const SourceFile &userSF,
+                                             const Decl *userContext);
+
 } // end namespace swift
 
 #endif
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -19,6 +19,7 @@
 #include "DerivedConformances.h"
 #include "MiscDiagnostics.h"
 #include "TypeAccessScopeChecker.h"
+#include "TypeCheckAccess.h"
 #include "TypeCheckAvailability.h"
 #include "TypeCheckObjC.h"
 #include "swift/AST/ASTContext.h"
@@ -4039,7 +4040,8 @@ ResolveWitnessResult ConformanceChecker::resolveTypeWitnessViaLookup(
 static void checkExportability(Type depTy, Type replacementTy,
                                const ProtocolConformance *conformance,
                                NormalProtocolConformance *conformanceBeingChecked,
-                               SourceFile *SF) {
+                               DeclContext *DC) {
+  SourceFile *SF = DC->getParentSourceFile();
   if (!SF)
     return;
 
@@ -4049,13 +4051,17 @@ static void checkExportability(Type depTy, Type replacementTy,
     if (!subConformance.isConcrete())
       continue;
     checkExportability(depTy, replacementTy, subConformance.getConcrete(),
-                       conformanceBeingChecked, SF);
+                       conformanceBeingChecked, DC);
   }
 
   const RootProtocolConformance *rootConformance =
       conformance->getRootConformance();
   ModuleDecl *M = rootConformance->getDeclContext()->getParentModule();
-  if (!SF->isImportedImplementationOnly(M))
+
+  auto originKind = getDisallowedOriginKind(
+      rootConformance->getDeclContext()->getAsDecl(),
+      *SF, DC->getAsDecl());
+  if (originKind == DisallowedOriginKind::None)
     return;
 
   ASTContext &ctx = SF->getASTContext();
@@ -4066,14 +4072,16 @@ static void checkExportability(Type depTy, Type replacementTy,
         conformanceBeingChecked->getLoc(),
         diag::conformance_from_implementation_only_module,
         rootConformance->getType(),
-        rootConformance->getProtocol()->getName(), 0, M->getName());
+        rootConformance->getProtocol()->getName(), 0, M->getName(),
+        static_cast<unsigned>(originKind));
   } else {
     ctx.Diags.diagnose(
         conformanceBeingChecked->getLoc(),
         diag::assoc_conformance_from_implementation_only_module,
         rootConformance->getType(),
         rootConformance->getProtocol()->getName(), M->getName(),
-        depTy, replacementTy->getCanonicalType());
+        depTy, replacementTy->getCanonicalType(),
+        static_cast<unsigned>(originKind));
   }
 }
 
@@ -4122,11 +4130,7 @@ void ConformanceChecker::ensureRequirementsAreSatisfied() {
 
   // Now check that our associated conformances are at least as visible as
   // the conformance itself.
-  //
-  // FIXME: Do we need to check SPI here too?
   if (getRequiredAccessScope().isPublic() || isUsableFromInlineRequired()) {
-    auto *fileForCheckingExportability = DC->getParentSourceFile();
-
     for (auto req : proto->getRequirementSignature()) {
       if (req.getKind() == RequirementKind::Conformance) {
         auto depTy = req.getFirstType();
@@ -4136,7 +4140,7 @@ void ConformanceChecker::ensureRequirementsAreSatisfied() {
           auto *concrete = conformance.getConcrete();
           auto replacementTy = DC->mapTypeIntoContext(concrete->getType());
           checkExportability(depTy, replacementTy, concrete,
-                             Conformance, fileForCheckingExportability);
+                             Conformance, DC);
         }
       }
     }
diff --git a/test/SPI/protocol_requirement.swift b/test/SPI/protocol_requirement.swift
@@ -96,3 +96,20 @@ public protocol SPIProtocol {
   @_spi(Private)
   static var staticProperty: Int { get }
 }
+
+public protocol OtherProto {}
+public protocol Proto {
+  associatedtype A : Sequence where A.Element : OtherProto
+}
+
+public struct BadStruct {}
+@_spi(Horse) extension BadStruct : OtherProto {}
+public struct BadConforms : Proto { // expected-error {{cannot use conformance of 'BadStruct' to 'OtherProto' in associated type 'Self.A.Element' (inferred as 'BadStruct'); the conformance is declared as SPI}}
+  public typealias A = [BadStruct]
+}
+
+public struct OKStruct {}
+@_spi(Horse) extension OKStruct : OtherProto {}
+@_spi(Horse) public struct OKConforms : Proto {
+  public typealias A = [OKStruct]
+}
diff --git a/test/Sema/spi-in-decls.swift b/test/Sema/spi-in-decls.swift
diff --git a/test/Sema/spi-inlinable-conformances.swift b/test/Sema/spi-inlinable-conformances.swift
