Favor private imports during name lookup

Private imports are intended to make the file performing the import more or less source-compatible with the file being imported from, so that code from the original file can be modified by relatively simple syntactic transformations. However, their name shadowing behavior was very different from the original file. In the original file, other declarations in the same module would have shadowed declarations imported from any other module; in a file using a @_private import, they would all be considered imports, and thus all would be preferred equally. This could cause ambiguity in a file using a @_private import that was not present in the original file.

This commit changes that behavior by favoring @_private imports over other imports, so that if a name is visible through both a private and a non-private import, the one visible through the private import will shadow the other. This shadowing takes a higher priority than a scoped import, but a lower priority than the check for whether one of the modules is only visible through the other.

Fixes rdar://68312053.

Author: beccadax

lib/AST/NameLookup.cpp

@@ -286,11 +286,27 @@ static void recordShadowedDeclsAfterTypeMatch(
       return false;
     };
 
+    auto isPrivateImport = [&](ModuleDecl *module) {
+      auto file = dc->getParentSourceFile();
+      if (!file) return false;
+      for (const auto &import : file->getImports()) {
+        if (import.importOptions.contains(
+                SourceFile::ImportFlags::PrivateImport)
+            && import.module.importedModule == module
+            && import.module.accessPath.matches(name))
+          return true;
+      }
+      return false;
+    };
+
+    bool firstPrivate = isPrivateImport(firstModule);
+
     for (unsigned secondIdx : range(firstIdx + 1, decls.size())) {
       // Determine whether one module takes precedence over another.
       auto secondDecl = decls[secondIdx];
       auto secondModule = secondDecl->getModuleContext();
       bool secondTopLevel = secondDecl->getDeclContext()->isModuleScopeContext();
+      bool secondPrivate = isPrivateImport(secondModule);
 
       // For member types, we skip most of the below rules. Instead, we allow
       // member types defined in a subclass to shadow member types defined in
@@ -348,6 +364,18 @@ static void recordShadowedDeclsAfterTypeMatch(
           continue;
         }
 
+        // If neither module shadows the other, but one was imported with
+        // '@_private import' in dc, we want to favor that module. This makes
+        // name lookup in this file behave more like name lookup in the file we
+        // imported from, avoiding headaches for source-transforming tools.
+        if (!firstPrivate && secondPrivate) {
+          shadowed.insert(firstDecl);
+          break;
+        } else if (firstPrivate && !secondPrivate) {
+          shadowed.insert(secondDecl);
+          continue;
+        }
+
         // We might be in a situation where neither module shadows the
         // other, but one declaration is visible via a scoped import.
         bool firstScoped = isScopedImport(firstPaths);

test/NameLookup/Inputs/private_import/Ghost.swift

@@ -0,0 +1,2 @@
+@available(*, deprecated, message: "got Ghost version")
+public enum Coast { case airCurrent }     // Also defined in Most

test/NameLookup/Inputs/private_import/Host.swift

@@ -0,0 +1,7 @@
+import Most
+
+struct Post {}
+struct Boast {}                   // Deprecated in Most
+
+@available(*, deprecated, message: "got Host version")
+struct Roast {}                   // Different definition in Toast

test/NameLookup/Inputs/private_import/Most.swift

@@ -0,0 +1,5 @@
+@available(*, deprecated, message: "got Boast version")
+public struct Boast { }                   // Not deprecated in Host
+
+@available(*, deprecated, message: "got Boast version")
+public enum Coast { case downhill }       // Also defined in Ghost

test/NameLookup/Inputs/private_import/Toast.swift

@@ -0,0 +1,2 @@
+@available(*, deprecated, message: "got Toast version")
+public struct Roast {}         // Different definition in Host

test/NameLookup/private_import.swift

@@ -0,0 +1,86 @@
+// '@_private import' is intended to be used to compile "thunks" which contain
+// modified versions of declarations. For this to work properly, name lookup
+// needs to favor declarations in the private import over other imports. This
+// emulates the increased visibility of same-module declarations in the original
+// file, preserving source compatibility as much as possible.
+//
+// In this test:
+//
+// * Module Host contains this file and Host.swift. Note that this file compiles
+//   successfully with COMPILING_THUNK undefined.
+// * This file is also built as a thunk with COMPILING_THUNK defined to "edit"
+//   it.
+// * We also build three other modules called Most, Ghost, and Toast. Each of
+//   these is imported by both Host and the thunk.
+//
+// There are various same-name types scattered throughout these modules. If
+// name lookup selects the right ones for each of the test cases, the compiler
+// will generate the expected diagnostics and the test will pass. If it selects
+// incorrect types, deprecation warnings will be emitted and the test will fail.
+
+// We'll put our modules here.
+// RUN: %empty-directory(%t)
+
+// Build some libraries to work with.
+// RUN: %target-swift-frontend -emit-module -parse-as-library -module-name Most -emit-module-path %t/Most.swiftmodule -primary-file %S/Inputs/private_import/Most.swift
+// RUN: %target-swift-frontend -emit-module -parse-as-library -module-name Ghost -emit-module-path %t/Ghost.swiftmodule -primary-file %S/Inputs/private_import/Ghost.swift
+// RUN: %target-swift-frontend -emit-module -enable-private-imports -parse-as-library -module-name Toast -emit-module-path %t/Toast.swiftmodule -primary-file %S/Inputs/private_import/Toast.swift
+
+// Build the host module that the thunk is pretending to be part of.
+// RUN: %target-swift-frontend -emit-module -module-name Host -I %t -emit-module-path %t/Host.swiftmodule -enable-private-imports %s %S/Inputs/private_import/Host.swift
+
+// Build a thunk for the host module.
+// RUN: %target-typecheck-verify-swift -parse-as-library -I %t -DCOMPILING_THUNK
+
+#if COMPILING_THUNK
+@_private(sourceFile: "private_import.swift") import Host
+#endif
+
+import Most
+import Ghost
+
+@_private(sourceFile: "Toast.swift") import Toast
+
+//
+// Types with varying definitions
+//
+
+struct Hunk {}                  // Present in both Host and thunk
+
+#if COMPILING_THUNK
+
+struct Thunk {}                 // Only present in thunk
+struct Bunk {}                  // Not deprecated in thunk
+
+#else
+
+struct Shrunk {}                // Only present in Host
+@available(*, deprecated, message: "got Host version")
+struct Bunk {}                  // Only deprecated in Host
+
+#endif
+
+//
+// Test cases
+//
+
+#if COMPILING_THUNK
+func thunkCanUseTypesOnlyInThunk(_: Thunk) {}
+#endif
+
+func thunkCanRedeclareTypes(_: Hunk) {}
+func thunkCanUseTypesOnlyInHost(_: Shrunk, _: Post) {}
+func thunkTypesShadowHostTypes(_: Bunk) {}
+
+func hostTypesShadowMostTypes(_: Boast) {}
+// no-error@-1; previously, this would give "'Boast' is ambiguous for type lookup in this context"
+
+#if COMPILING_THUNK
+func ambiguousWithTwoNonPrivateImports(_: Coast) {}
+// expected-error@-1{{'Coast' is ambiguous for type lookup in this context}}
+#endif
+
+// The intended clients of '@_private import' should not actually use it twice
+// in a single file, so this behavior is more accidental than anything.
+func ambiguousWithTwoPrivateImports(_: Roast) {}
+// expected-error@-1{{'Roast' is ambiguous for type lookup in this context}}