Merge pull request #35904 from beccadax/go-back-to-the-shadow

Warn about module name shadowing in interfaces

Author: beccadax

include/swift/AST/DiagnosticsFrontend.def

@@ -353,6 +353,13 @@ 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 may cause failures when "
+        "importing %3 or its clients in some configurations; please rename "
+        "either the %0 %1 or the module %2, or see "
+        "https://bugs.swift.org/browse/SR-14195 for workarounds",
+        (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"
@@ -36,24 +37,9 @@
 
 using namespace swift;
 
-version::Version swift::InterfaceFormatVersion({1, 0});
+// MARK: Module interface header comments
 
-/// Diagnose any scoped imports in \p imports, i.e. those with a non-empty
-/// access path. These are not yet supported by module interfaces, since the
-/// information about the declaration kind is not preserved through the binary
-/// serialization that happens as an intermediate step in non-whole-module
-/// builds.
-///
-/// These come from declarations like `import class FooKit.MainFooController`.
-static void diagnoseScopedImports(DiagnosticEngine &diags,
-                                  ArrayRef<ImportedModule> imports){
-  for (const ImportedModule &importPair : imports) {
-    if (importPair.accessPath.empty())
-      continue;
-    diags.diagnose(importPair.accessPath.front().Loc,
-                   diag::module_interface_scoped_import_unsupported);
-  }
-}
+version::Version swift::InterfaceFormatVersion({1, 0});
 
 /// Prints to \p out a comment containing a format version number, tool version
 /// string as well as any relevant command-line flags in \p Opts used to
@@ -93,6 +79,106 @@ 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
+/// access path. These are not yet supported by module interfaces, since the
+/// information about the declaration kind is not preserved through the binary
+/// serialization that happens as an intermediate step in non-whole-module
+/// builds.
+///
+/// These come from declarations like `import class FooKit.MainFooController`.
+static void diagnoseScopedImports(DiagnosticEngine &diags,
+                                  ArrayRef<ImportedModule> imports){
+  for (const ImportedModule &importPair : imports) {
+    if (importPair.accessPath.empty())
+      continue;
+    diags.diagnose(importPair.accessPath.front().Loc,
+                   diag::module_interface_scoped_import_unsupported);
+  }
+}
+
 /// Prints the imported modules in \p M to \p out in the form of \c import
 /// source declarations.
 static void printImports(raw_ostream &out,
@@ -171,9 +257,13 @@ static void printImports(raw_ostream &out,
     }
 
     out << "\n";
+
+    diagnoseIfModuleImportsShadowingDecl(Opts, importedModule, M);
   }
 }
 
+// MARK: Dummy protocol conformances
+
 // FIXME: Copied from ASTPrinter.cpp...
 static bool isPublicOrUsableFromInline(const ValueDecl *VD) {
   AccessScope scope =
@@ -553,6 +643,8 @@ const StringLiteral InheritedProtocolCollector::DummyProtocolName =
     "_ConstraintThatIsNotPartOfTheAPIOfThisLibrary";
 } // end anonymous namespace
 
+// MARK: Interface emission
+
 bool swift::emitSwiftInterface(raw_ostream &out,
                                ModuleInterfaceOptions const &Opts,
                                ModuleDecl *M) {
@@ -580,6 +672,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 may cause failures when importing 'module_shadowing' or its clients in some configurations; please rename either the class 'ShadowyHorror.module_shadowing' or the module 'module_shadowing', or see https://bugs.swift.org/browse/SR-14195 for workarounds{{$}}
+
+@_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 may cause failures when importing 'module_shadowing' or its clients in some configurations; please rename either the struct 'module_shadowing.ShadowyHorror' or the module 'ShadowyHorror', or see https://bugs.swift.org/browse/SR-14195 for workarounds{{$}}
+  // SELF: module_shadowing.swift:[[@LINE-2]]:15: warning: public struct 'ShadowyHorror.ShadowyHorror' shadows module 'ShadowyHorror', which may cause failures when importing 'ShadowyHorror' or its clients in some configurations; please rename either the struct 'ShadowyHorror.ShadowyHorror' or the module 'ShadowyHorror', or see https://bugs.swift.org/browse/SR-14195 for workarounds{{$}}
+
+  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 {}