Merge pull request #61058 from xymus/spi-only-consistent

xymus · web-flow · commit 2cb6d7244443 · 2022-09-13T07:49:22.000-07:00
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -973,8 +973,11 @@ ERROR(module_not_testable,Fatal,
 ERROR(module_not_compiled_for_private_import,none,
       "module %0 was not compiled for private import", (Identifier))
 
-ERROR(import_implementation_cannot_be_exported,none,
-      "module %0 cannot be both exported and implementation-only", (Identifier))
+ERROR(import_restriction_conflict,none,
+      "module %0 cannot be both "
+      "%select{implementation-only|SPI only|exported}1 and "
+      "%select{implementation-only|SPI only|exported}2 ",
+      (Identifier, unsigned, unsigned))
 
 WARNING(module_not_compiled_with_library_evolution,none,
         "module %0 was not compiled with library evolution support; "
@@ -3003,6 +3006,13 @@ WARNING(warn_implementation_only_conflict,none,
         (Identifier))
 NOTE(implementation_only_conflict_here,none,
      "imported as implementation-only here", ())
+
+ERROR(spi_only_import_conflict,none,
+      "%0 inconsistently imported for SPI only",
+      (Identifier))
+NOTE(spi_only_import_conflict_here,none,
+     "imported for SPI only here", ())
+
 ERROR(error_public_import_of_private_module,none,
         "private module %0 is imported publicly from the public module %1",
         (Identifier, Identifier))
diff --git a/include/swift/AST/TypeCheckRequests.h b/include/swift/AST/TypeCheckRequests.h
@@ -3289,6 +3289,25 @@ class CheckInconsistentImplementationOnlyImportsRequest
   bool isCached() const { return true; }
 };
 
+/// Check that if one import in a file uses \c @_spiOnly, all imports from the
+/// same file are consistently using \c @_spiOnly.
+class CheckInconsistentSPIOnlyImportsRequest
+    : public SimpleRequest<CheckInconsistentSPIOnlyImportsRequest,
+                           evaluator::SideEffect(SourceFile *),
+                           RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  evaluator::SideEffect evaluate(Evaluator &evaluator, SourceFile *mod) const;
+
+public:
+  // Cached.
+  bool isCached() const { return true; }
+};
+
 /// Checks to see if any of the imports in a module use \c @_weakLinked
 /// in one file and not in another.
 ///
diff --git a/include/swift/AST/TypeCheckerTypeIDZone.def b/include/swift/AST/TypeCheckerTypeIDZone.def
@@ -33,6 +33,8 @@ SWIFT_REQUEST(TypeChecker, CallerSideDefaultArgExprRequest,
               Expr *(DefaultArgumentExpr *), SeparatelyCached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckInconsistentImplementationOnlyImportsRequest,
               evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, CheckInconsistentSPIOnlyImportsRequest,
+              evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckInconsistentWeakLinkedImportsRequest,
               evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckRedeclarationRequest,
diff --git a/lib/Sema/ImportResolution.cpp b/lib/Sema/ImportResolution.cpp
@@ -129,7 +129,11 @@ struct UnboundImport {
 
 private:
   void validatePrivate(ModuleDecl *topLevelModule);
-  void validateImplementationOnly(ASTContext &ctx);
+
+  /// Check that no import has more than one of the following modifiers:
+  /// @_exported, @_implementationOnly, and @_spiOnly.
+  void validateRestrictedImport(ASTContext &ctx);
+
   void validateTestable(ModuleDecl *topLevelModule);
   void validateResilience(NullablePtr<ModuleDecl> topLevelModule,
                           SourceFile &SF);
@@ -601,7 +605,7 @@ bool UnboundImport::checkModuleLoaded(ModuleDecl *M, SourceFile &SF) {
 
 void UnboundImport::validateOptions(NullablePtr<ModuleDecl> topLevelModule,
                                     SourceFile &SF) {
-  validateImplementationOnly(SF.getASTContext());
+  validateRestrictedImport(SF.getASTContext());
 
   if (auto *top = topLevelModule.getPtrOrNull()) {
     // FIXME: Having these two calls in this if condition seems dubious.
@@ -636,16 +640,62 @@ void UnboundImport::validatePrivate(ModuleDecl *topLevelModule) {
   import.sourceFileArg = StringRef();
 }
 
-void UnboundImport::validateImplementationOnly(ASTContext &ctx) {
-  if (!import.options.contains(ImportFlags::ImplementationOnly) ||
-      !import.options.contains(ImportFlags::Exported))
+void UnboundImport::validateRestrictedImport(ASTContext &ctx) {
+  static llvm::SmallVector<ImportFlags, 2> flags = {ImportFlags::Exported,
+                                                    ImportFlags::ImplementationOnly,
+                                                    ImportFlags::SPIOnly};
+  llvm::SmallVector<ImportFlags, 2> conflicts;
+
+  for (auto flag : flags) {
+    if (import.options.contains(flag))
+      conflicts.push_back(flag);
+  }
+
+  // Quit if there's no conflicting attributes.
+  if (conflicts.size() < 2)
     return;
 
-  // Remove one flag to maintain the invariant.
-  import.options -= ImportFlags::ImplementationOnly;
+  // Remove all but one flag to maintain the invariant.
+  for (auto iter = conflicts.begin(); iter != std::prev(conflicts.end()); iter ++)
+    import.options -= *iter;
 
-  diagnoseInvalidAttr(DAK_ImplementationOnly, ctx.Diags,
-                      diag::import_implementation_cannot_be_exported);
+  DeclAttrKind attrToRemove = conflicts[0] == ImportFlags::ImplementationOnly?
+                                      DAK_Exported : DAK_ImplementationOnly;
+
+  // More dense enum with some cases of ImportFlags,
+  // used by import_restriction_conflict.
+  enum class ImportFlagForDiag : uint8_t {
+    ImplementationOnly,
+    SPIOnly,
+    Exported
+  };
+  auto flagToDiag = [](ImportFlags flag) {
+    switch (flag) {
+      case ImportFlags::ImplementationOnly:
+        return ImportFlagForDiag::ImplementationOnly;
+      case ImportFlags::SPIOnly:
+        return ImportFlagForDiag::SPIOnly;
+      case ImportFlags::Exported:
+        return ImportFlagForDiag::Exported;
+      default:
+        llvm_unreachable("Unexpected ImportFlag");
+    }
+  };
+
+  // Report the conflict, only the first two conflicts should be enough.
+  auto diag = ctx.Diags.diagnose(import.module.getModulePath().front().Loc,
+                                 diag::import_restriction_conflict,
+                                 import.module.getModulePath().front().Item,
+                                 (uint8_t)flagToDiag(conflicts[0]),
+                                 (uint8_t)flagToDiag(conflicts[1]));
+
+  auto *ID = getImportDecl().getPtrOrNull();
+  if (!ID) return;
+  auto *attr = ID->getAttrs().getAttribute(attrToRemove);
+  if (!attr) return;
+
+  diag.fixItRemove(attr->getRangeWithAt());
+  attr->setInvalid();
 }
 
 void UnboundImport::validateTestable(ModuleDecl *topLevelModule) {
@@ -709,54 +759,66 @@ static bool moduleHasAnyImportsMatchingFlag(ModuleDecl *mod, ImportFlags flag) {
   return false;
 }
 
-/// Finds all import declarations for a single module that inconsistently match
+/// Finds all import declarations for a single file that inconsistently match
 /// \c predicate and passes each pair of inconsistent imports to \c diagnose.
 template <typename Pred, typename Diag>
-static void findInconsistentImports(ModuleDecl *mod, Pred predicate,
-                                    Diag diagnose) {
-  llvm::DenseMap<ModuleDecl *, const ImportDecl *> matchingImports;
-  llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> otherImports;
+static void findInconsistentImportsAcrossFile(
+    const SourceFile *SF, Pred predicate, Diag diagnose,
+    llvm::DenseMap<ModuleDecl *, const ImportDecl *> &matchingImports,
+    llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> &otherImports) {
+
+  for (auto *topLevelDecl : SF->getTopLevelDecls()) {
+    auto *nextImport = dyn_cast<ImportDecl>(topLevelDecl);
+    if (!nextImport)
+      continue;
 
-  for (const FileUnit *file : mod->getFiles()) {
-    auto *SF = dyn_cast<SourceFile>(file);
-    if (!SF)
+    ModuleDecl *module = nextImport->getModule();
+    if (!module)
       continue;
 
-    for (auto *topLevelDecl : SF->getTopLevelDecls()) {
-      auto *nextImport = dyn_cast<ImportDecl>(topLevelDecl);
-      if (!nextImport)
+    if (predicate(nextImport)) {
+      // We found a matching import.
+      bool isNew = matchingImports.insert({module, nextImport}).second;
+      if (!isNew)
         continue;
 
-      ModuleDecl *module = nextImport->getModule();
-      if (!module)
-        continue;
+      auto seenOtherImportPosition = otherImports.find(module);
+      if (seenOtherImportPosition != otherImports.end()) {
+        for (auto *seenOtherImport : seenOtherImportPosition->getSecond())
+          diagnose(seenOtherImport, nextImport);
 
-      if (predicate(nextImport)) {
-        // We found a matching import.
-        bool isNew = matchingImports.insert({module, nextImport}).second;
-        if (!isNew)
-          continue;
+        // We're done with these; keep the map small if possible.
+        otherImports.erase(seenOtherImportPosition);
+      }
+      continue;
+    }
 
-        auto seenOtherImportPosition = otherImports.find(module);
-        if (seenOtherImportPosition != otherImports.end()) {
-          for (auto *seenOtherImport : seenOtherImportPosition->getSecond())
-            diagnose(seenOtherImport, nextImport);
+    // We saw a non-matching import. Is that in conflict with what we've seen?
+    if (auto *seenMatchingImport = matchingImports.lookup(module)) {
+      diagnose(nextImport, seenMatchingImport);
+      continue;
+    }
 
-          // We're done with these; keep the map small if possible.
-          otherImports.erase(seenOtherImportPosition);
-        }
-        continue;
-      }
+    // Otherwise, record it for later.
+    otherImports[module].push_back(nextImport);
+  }
+}
 
-      // We saw a non-matching import. Is that in conflict with what we've seen?
-      if (auto *seenMatchingImport = matchingImports.lookup(module)) {
-        diagnose(nextImport, seenMatchingImport);
-        continue;
-      }
+/// Finds all import declarations for a single module that inconsistently match
+/// \c predicate and passes each pair of inconsistent imports to \c diagnose.
+template <typename Pred, typename Diag>
+static void findInconsistentImportsAcrossModule(ModuleDecl *mod, Pred predicate,
+                                    Diag diagnose) {
+  llvm::DenseMap<ModuleDecl *, const ImportDecl *> matchingImports;
+  llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> otherImports;
 
-      // Otherwise, record it for later.
-      otherImports[module].push_back(nextImport);
-    }
+  for (const FileUnit *file : mod->getFiles()) {
+    auto *SF = dyn_cast<SourceFile>(file);
+    if (!SF)
+      continue;
+
+    findInconsistentImportsAcrossFile(SF, predicate, diagnose,
+                                      matchingImports, otherImports);
   }
 }
 
@@ -790,7 +852,34 @@ CheckInconsistentImplementationOnlyImportsRequest::evaluate(
     return decl->getAttrs().hasAttribute<ImplementationOnlyAttr>();
   };
 
-  findInconsistentImports(mod, predicate, diagnose);
+  findInconsistentImportsAcrossModule(mod, predicate, diagnose);
+  return {};
+}
+
+evaluator::SideEffect
+CheckInconsistentSPIOnlyImportsRequest::evaluate(
+    Evaluator &evaluator, SourceFile *SF) const {
+
+  auto mod = SF->getParentModule();
+  auto diagnose = [mod](const ImportDecl *normalImport,
+                        const ImportDecl *spiOnlyImport) {
+    auto &diags = mod->getDiags();
+    {
+      diags.diagnose(normalImport, diag::spi_only_import_conflict,
+                     normalImport->getModule()->getName());
+    }
+    diags.diagnose(spiOnlyImport,
+                   diag::spi_only_import_conflict_here);
+  };
+
+  auto predicate = [](ImportDecl *decl) {
+    return decl->getAttrs().hasAttribute<SPIOnlyAttr>();
+  };
+
+  llvm::DenseMap<ModuleDecl *, const ImportDecl *> matchingImports;
+  llvm::DenseMap<ModuleDecl *, std::vector<const ImportDecl *>> otherImports;
+  findInconsistentImportsAcrossFile(SF, predicate, diagnose,
+                                    matchingImports, otherImports);
   return {};
 }
 
@@ -815,7 +904,7 @@ CheckInconsistentWeakLinkedImportsRequest::evaluate(Evaluator &evaluator,
     return decl->getAttrs().hasAttribute<WeakLinkedAttr>();
   };
 
-  findInconsistentImports(mod, predicate, diagnose);
+  findInconsistentImportsAcrossModule(mod, predicate, diagnose);
   return {};
 }
 
diff --git a/lib/Sema/TypeChecker.cpp b/lib/Sema/TypeChecker.cpp
@@ -335,6 +335,11 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
       CheckInconsistentImplementationOnlyImportsRequest{SF->getParentModule()},
       {});
 
+  evaluateOrDefault(
+      Ctx.evaluator,
+      CheckInconsistentSPIOnlyImportsRequest{SF},
+      {});
+
   evaluateOrDefault(
       Ctx.evaluator,
       CheckInconsistentWeakLinkedImportsRequest{SF->getParentModule()}, {});
diff --git a/test/SPI/spi-only-import-conflicting.swift b/test/SPI/spi-only-import-conflicting.swift
@@ -0,0 +1,79 @@
+/// Test duplicate and conflicting imports from the same file.
+
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+/// Generate dependencies.
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift \
+// RUN:   -module-name Lib -emit-module-path %t/Lib.swiftmodule \
+// RUN:   -swift-version 5 -enable-library-evolution
+
+/// Build clients.
+// RUN: %target-swift-frontend -typecheck %t/SPIOnly_Default.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck %t/Default_SPIOnly.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck %t/SPIOnly_Exported.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck %t/Exported_SPIOnly.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck %t/SPIOnly_IOI.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck %t/SPIOnly_IOI_Exported_Default.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck -primary-file %t/SPIOnly_Default_FileA.swift \
+// RUN:   %t/SPIOnly_Default_FileB.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+// RUN: %target-swift-frontend -typecheck -primary-file %t/IOI_Default_FileA.swift \
+// RUN:   %t/IOI_Default_FileB.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+
+//--- Lib.swift
+// Empty source file for import.
+
+//--- SPIOnly_Default.swift
+@_spiOnly import Lib // expected-note {{imported for SPI only here}}
+import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+
+//--- Default_SPIOnly.swift
+import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+@_spiOnly import Lib // expected-note {{imported for SPI only here}}
+
+//--- SPIOnly_Exported.swift
+@_spiOnly import Lib // expected-note {{imported for SPI only here}}
+@_exported import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+
+//--- Exported_SPIOnly.swift
+@_exported import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+@_spiOnly import Lib // expected-note {{imported for SPI only here}}
+
+//--- SPIOnly_IOI.swift
+@_spiOnly import Lib // expected-note {{imported for SPI only here}}
+// expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
+@_implementationOnly import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+// expected-note @-1 {{imported as implementation-only here}}
+
+/// Many confliciting imports lead to many diagnostics.
+//--- SPIOnly_IOI_Exported_Default.swift
+@_spiOnly import Lib // expected-note 3 {{imported for SPI only here}}
+// expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
+@_implementationOnly import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+// expected-note @-1 3 {{imported as implementation-only here}}
+@_exported import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+// expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
+import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
+// expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
+
+/// Different SPI only imports in different files of the same module are accepted.
+//--- SPIOnly_Default_FileA.swift
+@_spiOnly import Lib
+
+//--- SPIOnly_Default_FileB.swift
+import Lib
+
+/// Different IOI in different files of the same module are still rejected.
+//--- IOI_Default_FileA.swift
+@_implementationOnly import Lib // expected-note {{imported as implementation-only here}}
+
+//--- IOI_Default_FileB.swift
+import Lib // expected-warning {{'Lib' inconsistently imported as implementation-only}}
diff --git a/test/Sema/conflicting-import-restrictions.swift b/test/Sema/conflicting-import-restrictions.swift
