[Sema] Report conflicting attributes on imports

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -964,8 +964,9 @@ 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 %1 and %2",
+      (Identifier, StringRef, StringRef))
 
 WARNING(module_not_compiled_with_library_evolution,none,
         "module %0 was not compiled with library evolution support; "

lib/Sema/ImportResolution.cpp

@@ -128,7 +128,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);
@@ -598,7 +602,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.
@@ -633,16 +637,55 @@ 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;
+
+  DeclAttrKind attrToRemove = conflicts[0] == ImportFlags::ImplementationOnly?
+                                      DAK_Exported : DAK_ImplementationOnly;
+
+  auto flagName = [](ImportFlags flag) {
+    switch (flag) {
+      case ImportFlags::ImplementationOnly:
+        return "implementation-only";
+      case ImportFlags::SPIOnly:
+        return "SPI only";
+      case ImportFlags::Exported:
+        return "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,
+                                 flagName(conflicts[0]),
+                                 flagName(conflicts[1]));
 
-  diagnoseInvalidAttr(DAK_ImplementationOnly, ctx.Diags,
-                      diag::import_implementation_cannot_be_exported);
+  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) {

test/Sema/conflicting-import-restrictions.swift

@@ -0,0 +1,37 @@
+/// Test conflicting imports modifiers from the same line.
+
+// 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_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.swift -I %t -verify \
+// RUN:   -experimental-spi-only-imports -verify
+
+//--- Lib.swift
+// Empty source file for import.
+
+//--- SPIOnly_Exported.swift
+@_spiOnly @_exported import Lib // expected-error {{module 'Lib' cannot be both exported and SPI only}}
+
+//--- Exported_SPIOnly.swift
+@_exported @_spiOnly import Lib // expected-error {{module 'Lib' cannot be both exported and SPI only}}
+
+//--- SPIOnly_IOI.swift
+@_spiOnly @_implementationOnly import Lib // expected-error {{module 'Lib' cannot be both implementation-only and SPI only}}
+
+//--- Exported_IOI.swift
+@_exported @_implementationOnly import Lib // expected-error {{module 'Lib' cannot be both exported and implementation-only}}
+
+//--- SPIOnly_IOI_Exported.swift
+@_spiOnly @_implementationOnly @_exported import Lib // expected-error {{module 'Lib' cannot be both exported and implementation-only}}