Warn about module name shadowing in interfaces

There is a known issue with module interfaces where a type with the same name as a module will disrupt references to types in that module. Fully fixing it will require a new language feature (SR-898) which is not yet available. In the meantime, module interfaces support a workaround flag (“-Xfrontend -module-interface-preserve-types-as-written”) which prints an alternate form that usually works. However, you have to know to add this flag, and it’s not obvious because nothing breaks until a compiler tries to consume the affected module interface (or sometimes even one of its clients).

This commit emits a warning during module interface emission whenever the module interface either imports a type with the same name as the module being built, or declares a type with the same name as a visible module. This lets the user know that the type may cause problems and they might need to implement a workaround.

Author: beccadax

include/swift/AST/DiagnosticsFrontend.def

@@ -350,6 +350,15 @@ ERROR(error_opening_explicit_module_file,none,
       "failed to open explicit Swift module: %0", (StringRef))
 ERROR(error_extracting_flags_from_module_interface,none,
       "error extracting flags from module interface", ())
+WARNING(warning_module_shadowing_may_break_module_interface,none,
+        "public %0 %1 shadows module %2, which is permitted by Swift but may "
+        "stop other compiler versions from importing module %3 or its clients; "
+        "please work around this language bug (SR-898) by renaming either the "
+        "%0 %1 or the module %2, or by passing the '-Xfrontend "
+        "-module-interface-preserve-types-as-written' flags to the Swift "
+        "compiler",
+        (DescriptiveDeclKind, FullyQualified<Type>,
+        /*shadowedModule=*/ModuleDecl *, /*interfaceModule*/ModuleDecl *))
 REMARK(rebuilding_module_from_interface,none,
        "rebuilding module '%0' from interface '%1'", (StringRef, StringRef))
 NOTE(sdk_version_pbm_version,none,

lib/AST/Module.cpp

@@ -2051,6 +2051,8 @@ bool SourceFile::isImportedImplementationOnly(const ModuleDecl *module) const {
 }
 
 bool ModuleDecl::isImportedImplementationOnly(const ModuleDecl *module) const {
+  if (module == this) return false;
+
   auto &imports = getASTContext().getImportCache();
 
   // Look through non-implementation-only imports to see if module is imported

lib/Frontend/ModuleInterfaceSupport.cpp

@@ -18,6 +18,7 @@
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/FileSystem.h"
 #include "swift/AST/Module.h"
+#include "swift/AST/ModuleNameLookup.h"
 #include "swift/AST/ProtocolConformance.h"
 #include "swift/Basic/STLExtras.h"
 #include "swift/Frontend/Frontend.h"
@@ -78,6 +79,87 @@ llvm::Regex swift::getSwiftInterfaceCompilerVersionRegex() {
                      ": (.+)$", llvm::Regex::Newline);
 }
 
+// MARK: Module name shadowing warnings (SR-898)
+//
+// When swiftc emits a module interface, it qualifies most types with their
+// module name. This usually makes the interface less ambiguous, but if a type
+// exists with the same name as a module, then references to that module will
+// incorrectly look inside the type instead. This breakage is not obvious until
+// someone tries to load the module interface, and may sometimes only occur in
+// clients' module interfaces.
+//
+// Truly fixing this will require a new module-qualification syntax which
+// completely ignores shadowing. In lieu of that, we detect and warn about three
+// common examples which are relatively actionable:
+//
+// 1. An `import` statement written into the module interface will
+//    (transitively) import a type with the module interface's name.
+//
+// 2. The module interface declares a type with the same name as the module the
+//    interface is for.
+//
+// 3. The module interface declares a type with the same name as a module it has
+//     (transitively) imported without `@_implementationOnly`.
+//
+// We do not check for shadowing between imported module names and imported
+// declarations; this is both much rarer and much more difficult to solve.
+// We silence these warnings if you use the temporary workaround flag,
+// '-module-interface-preserve-types-as-written'.
+
+/// Emit a warning explaining that \p shadowingDecl will interfere with
+/// references to types in \p shadowedModule in the module interfaces of
+/// \p brokenModule and its clients.
+static void
+diagnoseDeclShadowsModule(ModuleInterfaceOptions const &Opts,
+                          TypeDecl *shadowingDecl, ModuleDecl *shadowedModule,
+                          ModuleDecl *brokenModule) {
+  if (Opts.PreserveTypesAsWritten || shadowingDecl == shadowedModule)
+    return;
+
+  shadowingDecl->diagnose(
+      diag::warning_module_shadowing_may_break_module_interface,
+      shadowingDecl->getDescriptiveKind(),
+      FullyQualified<Type>(shadowingDecl->getDeclaredInterfaceType()),
+      shadowedModule, brokenModule);
+}
+
+/// Check whether importing \p importedModule will bring in any declarations
+/// that will shadow \p importingModule, and diagnose them if so.
+static void
+diagnoseIfModuleImportsShadowingDecl(ModuleInterfaceOptions const &Opts,
+                                     ModuleDecl *importedModule,
+                                     ModuleDecl *importingModule) {
+  using namespace namelookup;
+
+  SmallVector<ValueDecl *, 4> decls;
+  lookupInModule(importedModule, importingModule->getName(), decls,
+                 NLKind::UnqualifiedLookup, ResolutionKind::TypesOnly,
+                 importedModule,
+                 NL_UnqualifiedDefault | NL_IncludeUsableFromInline);
+  for (auto decl : decls)
+    diagnoseDeclShadowsModule(Opts, cast<TypeDecl>(decl), importingModule,
+                              importingModule);
+}
+
+/// Check whether \p D will shadow any modules imported by \p M, and diagnose
+/// them if so.
+static void diagnoseIfDeclShadowsKnownModule(ModuleInterfaceOptions const &Opts,
+                                             Decl *D, ModuleDecl *M) {
+  ASTContext &ctx = M->getASTContext();
+
+  // We only care about types (and modules, which are a subclass of TypeDecl);
+  // when the grammar expects a type name, it ignores non-types during lookup.
+  TypeDecl *TD = dyn_cast<TypeDecl>(D);
+  if (!TD)
+    return;
+
+  ModuleDecl *shadowedModule = ctx.getLoadedModule(TD->getName());
+  if (!shadowedModule || M->isImportedImplementationOnly(shadowedModule))
+    return;
+
+  diagnoseDeclShadowsModule(Opts, TD, shadowedModule, M);
+}
+
 // MARK: Import statements
 
 /// Diagnose any scoped imports in \p imports, i.e. those with a non-empty
@@ -175,6 +257,8 @@ static void printImports(raw_ostream &out,
     }
 
     out << "\n";
+
+    diagnoseIfModuleImportsShadowingDecl(Opts, importedModule, M);
   }
 }
 
@@ -596,6 +680,8 @@ bool swift::emitSwiftInterface(raw_ostream &out,
 
     D->print(out, printOptions);
     out << "\n";
+
+    diagnoseIfDeclShadowsKnownModule(Opts, const_cast<Decl *>(D), M);
   }
 
   // Print dummy extensions for any protocols that were indirectly conformed to.

test/ModuleInterface/Inputs/ShadowyHorror.swift

@@ -0,0 +1 @@
+public class module_shadowing {}

test/ModuleInterface/Inputs/TestModule.swift

@@ -1,3 +1,5 @@
 public struct TestStruct {
   public init() {}
 }
+
+public enum module_shadowing {}

test/ModuleInterface/module_shadowing.swift

@@ -0,0 +1,42 @@
+// RUN: %empty-directory(%t/mcp)
+
+// Build modules imported by this file.
+// RUN: %empty-directory(%t/lib)
+// RUN: %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/mcp -emit-module-interface-path %t/lib/ShadowyHorror.swiftinterface %S/Inputs/ShadowyHorror.swift -enable-library-evolution -module-name ShadowyHorror -swift-version 5
+// RUN: %target-swift-frontend -typecheck -parse-stdlib -emit-module-interface-path %t/lib/TestModule.swiftinterface %S/Inputs/TestModule.swift -enable-library-evolution -module-name TestModule -swift-version 5
+
+// Build this file as a module and check that it emits the expected warnings.
+// RUN: %empty-directory(%t/subtest-1)
+// RUN: %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/mcp -emit-module-interface-path %t/subtest-1/module_shadowing.swiftinterface %s -enable-library-evolution -module-name module_shadowing -I %t/lib -swift-version 5 2>&1 | %FileCheck --check-prefix OTHER --implicit-check-not TestModule %s
+
+// Make sure that preserve-types-as-written disables this warning.
+// RUN: %empty-directory(%t/subtest-2)
+// RUN: %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/mcp -emit-module-interface-path %t/subtest-2/module_shadowing.swiftinterface %s -enable-library-evolution -module-name module_shadowing -I %t/lib -module-interface-preserve-types-as-written -swift-version 5 -verify
+
+// Build this module in a different configuration where it will shadow itself.
+// RUN: %empty-directory(%t/subtest-3)
+// RUN: %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/mcp -emit-module-interface-path %t/subtest-3/ShadowyHorror.swiftinterface %s -enable-library-evolution -module-name ShadowyHorror -DSELF_SHADOW -swift-version 5 2>&1 | %FileCheck --check-prefix SELF --implicit-check-not TestModule %s
+
+// Verify that the module-shadowing bugs we're trying to address haven't been fixed. (SR-898)
+// RUN: not %target-swift-frontend -typecheck-module-from-interface %t/subtest-1/module_shadowing.swiftinterface -module-cache-path %t/mcp -I %t/lib -module-name module_shadowing 2>&1 | %FileCheck --check-prefix VERIFICATION %s
+// RUN: %target-swift-frontend -typecheck-module-from-interface %t/subtest-2/module_shadowing.swiftinterface -module-cache-path %t/mcp -I %t/lib -module-name module_shadowing
+// RUN: not %target-swift-frontend -typecheck-module-from-interface %t/subtest-3/ShadowyHorror.swiftinterface -module-cache-path %t/mcp -I %t/lib -module-name ShadowyHorror 2>&1 | %FileCheck --check-prefix VERIFICATION %s
+
+#if !SELF_SHADOW
+import ShadowyHorror
+// OTHER-DAG: ShadowyHorror.module_shadowing:{{[0-9]+:[0-9]+}}: warning: public class 'ShadowyHorror.module_shadowing' shadows module 'module_shadowing', which is permitted by Swift but may stop other compiler versions from importing module 'module_shadowing' or its clients; please work around this language bug (SR-898) by renaming either the class 'ShadowyHorror.module_shadowing' or the module 'module_shadowing', or by passing the '-Xfrontend -module-interface-preserve-types-as-written' flags to the Swift compiler{{$}}
+
+@_implementationOnly import TestModule
+#endif
+
+public struct ShadowyHorror {
+  // OTHER-DAG: module_shadowing.swift:[[@LINE-1]]:15: warning: public struct 'module_shadowing.ShadowyHorror' shadows module 'ShadowyHorror', which is permitted by Swift but may stop other compiler versions from importing module 'module_shadowing' or its clients; please work around this language bug (SR-898) by renaming either the struct 'module_shadowing.ShadowyHorror' or the module 'ShadowyHorror', or by passing the '-Xfrontend -module-interface-preserve-types-as-written' flags to the Swift compiler{{$}}
+  // SELF: module_shadowing.swift:[[@LINE-2]]:15: warning: public struct 'ShadowyHorror.ShadowyHorror' shadows module 'ShadowyHorror', which is permitted by Swift but may stop other compiler versions from importing module 'ShadowyHorror' or its clients; please work around this language bug (SR-898) by renaming either the struct 'ShadowyHorror.ShadowyHorror' or the module 'ShadowyHorror', or by passing the '-Xfrontend -module-interface-preserve-types-as-written' flags to the Swift compiler{{$}}
+
+  init() {}
+  public var property: ShadowyHorror { ShadowyHorror() }
+  // VERIFICATION: error: 'ShadowyHorror' is not a member type of {{class|struct}} 'ShadowyHorror.{{ShadowyHorror|module_shadowing}}'
+  // VERIFICATION: error: failed to verify module interface of '{{ShadowyHorror|module_shadowing}}' due to the errors above;
+}
+
+public struct TestModule {}