Add a warning about redundant cross-import declarations

These are mostly harmless, except that they make the two module names synonymous in qualified lookup. A hard error seems too aggressive for something that could easily be caused by uncoordinated changes to two modules, so warn instead.

Author: beccadax

include/swift/AST/DiagnosticsCommon.def

@@ -174,6 +174,11 @@ ERROR(cannot_load_swiftoverlay_file, none,
 ERROR(cannot_list_swiftcrossimport_dir, none,
       "cannot list cross-import overlays for %0: %1 (declared in '%2')",
       (Identifier, StringRef, StringRef))
+WARNING(cross_imported_by_both_modules, none,
+        "modules %0 and %1 both declare module %2 as a cross-import overlay, "
+        "which may cause paradoxical behavior when looking up names in them; "
+        "please report this bug to the maintainers of these modules",
+        (Identifier, Identifier, Identifier))
 
 #ifndef DIAG_NO_UNDEF
 # if defined(DIAG)

lib/Sema/NameBinding.cpp

@@ -201,7 +201,8 @@ namespace {
     /// locations from \p I.
     void findCrossImports(UnboundImport &I,
                           const ImportedModuleDesc &declaringImport,
-                          const ImportedModuleDesc &bystandingImport);
+                          const ImportedModuleDesc &bystandingImport,
+                          SmallVectorImpl<Identifier> &overlayNames);
 
     /// Load a module referenced by an import statement.
     ///
@@ -815,8 +816,21 @@ void NameBinder::crossImport(ModuleDecl *M, UnboundImport &I) {
       if (!canCrossImport(oldImport))
         continue;
 
-      findCrossImports(I, newImport, oldImport);
-      findCrossImports(I, oldImport, newImport);
+      SmallVector<Identifier, 2> newImportOverlays;
+      findCrossImports(I, newImport, oldImport, newImportOverlays);
+
+      SmallVector<Identifier, 2> oldImportOverlays;
+      findCrossImports(I, oldImport, newImport, oldImportOverlays);
+
+      // If both sides of the cross-import declare some of the same overlays,
+      // this will cause some strange name lookup behavior; let's warn about it.
+      for (auto name : newImportOverlays) {
+        if (llvm::is_contained(oldImportOverlays, name)) {
+          ctx.Diags.diagnose(I.importLoc, diag::cross_imported_by_both_modules,
+                             newImport.module.second->getName(),
+                             oldImport.module.second->getName(), name);
+        }
+      }
 
       // If findCrossImports() ever changed the visibleModules list, we'd see
       // memory smashers here.
@@ -830,7 +844,8 @@ void NameBinder::crossImport(ModuleDecl *M, UnboundImport &I) {
 
 void NameBinder::findCrossImports(UnboundImport &I,
                                   const ImportedModuleDesc &declaringImport,
-                                  const ImportedModuleDesc &bystandingImport) {
+                                  const ImportedModuleDesc &bystandingImport,
+                                  SmallVectorImpl<Identifier> &names) {
   assert(&declaringImport != &bystandingImport);
 
   LLVM_DEBUG(
@@ -839,7 +854,6 @@ void NameBinder::findCrossImports(UnboundImport &I,
                    << bystandingImport.module.second->getName() << "'\n");
 
   // Find modules we need to import.
-  SmallVector<Identifier, 2> names;
   declaringImport.module.second->findDeclaredCrossImportOverlays(
       bystandingImport.module.second->getName(), names, I.importLoc);
 

test/CrossImport/Inputs/lib-templates/lib/swift/HorribleGoose.swiftcrossimport/RedundantGoose.swiftoverlay

@@ -0,0 +1,4 @@
+---
+version: 1
+modules:
+- name: _OverlayLibrary

test/CrossImport/Inputs/lib-templates/lib/swift/RedundantGoose.swiftcrossimport/HorribleGoose.swiftoverlay

@@ -0,0 +1,4 @@
+---
+version: 1
+modules:
+- name: _OverlayLibrary

test/CrossImport/Inputs/lib-templates/lib/swift/RedundantGoose.swiftinterface

@@ -0,0 +1,5 @@
+// swift-interface-format-version: 1.0
+// swift-module-flags: -swift-version 5 -enable-library-evolution -module-name RedundantGoose
+
+import Swift
+

test/CrossImport/horrible.swift

@@ -36,6 +36,9 @@ import ListlessGoose
 import AnonymousGoose
 // expected-error@-1 {{cannot load cross-import overlay for 'HorribleGoose' and 'AnonymousGoose': missing required key 'name'}}
 
+import RedundantGoose
+// expected-warning@-1 {{modules 'RedundantGoose' and 'HorribleGoose' both declare module '_OverlayLibrary' as a cross-import overlay, which may cause paradoxical behavior when looking up names in them; please report this bug to the maintainers of these modules}}
+
 // CircularGoose overlays itself with EndlessGoose, which overlays itself with
 // CircularGoose, which should then break the cycle because we don't load the
 // same overlay on an underlying module twice. If the test hangs, we've broken