Merge pull request swiftlang#33864 from brentdax/imported-by-the-east-aldenard-trading-company

Fix non-public imported declaration diagnostic bug + Favor private imports during name lookup

Author: beccadax

include/swift/AST/PrintOptions.h

@@ -499,7 +499,7 @@ struct PrintOptions {
   }
 
   /// Retrieve the set of options suitable for diagnostics printing.
-  static PrintOptions printForDiagnostics() {
+  static PrintOptions printForDiagnostics(AccessLevel accessFilter) {
     PrintOptions result = printVerbose();
     result.PrintAccess = true;
     result.Indent = 4;
@@ -512,7 +512,7 @@ struct PrintOptions {
     result.ExcludeAttrList.push_back(DAK_Optimize);
     result.ExcludeAttrList.push_back(DAK_Rethrows);
     result.PrintOverrideKeyword = false;
-    result.AccessFilter = AccessLevel::Public;
+    result.AccessFilter = accessFilter;
     result.PrintIfConfig = false;
     result.ShouldQualifyNestedDeclarations =
         QualifyNestedDeclarations::TypesOnly;
@@ -522,7 +522,7 @@ struct PrintOptions {
 
   /// Retrieve the set of options suitable for interface generation.
   static PrintOptions printInterface() {
-    PrintOptions result = printForDiagnostics();
+    PrintOptions result = printForDiagnostics(AccessLevel::Public);
     result.SkipUnavailable = true;
     result.SkipImplicit = true;
     result.SkipSwiftPrivateClangDecls = true;

lib/AST/DiagnosticEngine.cpp

@@ -846,6 +846,20 @@ void DiagnosticEngine::emitTentativeDiagnostics() {
   TentativeDiagnostics.clear();
 }
 
+/// Returns the access level of the least accessible PrettyPrintedDeclarations
+/// buffer that \p decl should appear in.
+///
+/// This is always \c Public unless \p decl is a \c ValueDecl and its
+/// access level is below \c Public. (That can happen with @testable and
+/// @_private imports.)
+static AccessLevel getBufferAccessLevel(const Decl *decl) {
+  AccessLevel level = AccessLevel::Public;
+  if (auto *VD = dyn_cast<ValueDecl>(decl))
+    level = VD->getFormalAccessScope().accessLevelForDiagnostics();
+  if (level > AccessLevel::Public) level = AccessLevel::Public;
+  return level;
+}
+
 Optional<DiagnosticInfo>
 DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {
   auto behavior = state.determineBehavior(diagnostic.getID());
@@ -867,21 +881,31 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {
       if (ppLoc.isInvalid()) {
         class TrackingPrinter : public StreamPrinter {
           SmallVectorImpl<std::pair<const Decl *, uint64_t>> &Entries;
+          AccessLevel bufferAccessLevel;
 
         public:
           TrackingPrinter(
               SmallVectorImpl<std::pair<const Decl *, uint64_t>> &Entries,
-              raw_ostream &OS) :
-            StreamPrinter(OS), Entries(Entries) {}
+              raw_ostream &OS, AccessLevel bufferAccessLevel) :
+            StreamPrinter(OS), Entries(Entries),
+            bufferAccessLevel(bufferAccessLevel) {}
 
           void printDeclLoc(const Decl *D) override {
-            Entries.push_back({ D, OS.tell() });
+            if (getBufferAccessLevel(D) == bufferAccessLevel)
+              Entries.push_back({ D, OS.tell() });
           }
         };
         SmallVector<std::pair<const Decl *, uint64_t>, 8> entries;
         llvm::SmallString<128> buffer;
         llvm::SmallString<128> bufferName;
         {
+          // The access level of the buffer we want to print. Declarations below
+          // this access level will be omitted from the buffer; declarations
+          // above it will be printed, but (except for Open declarations in the
+          // Public buffer) will not be recorded in PrettyPrintedDeclarations as
+          // the "true" SourceLoc for the declaration.
+          AccessLevel bufferAccessLevel = getBufferAccessLevel(decl);
+
           // Figure out which declaration to print. It's the top-most
           // declaration (not a module).
           const Decl *ppDecl = decl;
@@ -942,10 +966,21 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {
             bufferName += ext->getExtendedType().getString();
           }
 
+          // If we're using a lowered access level, give the buffer a distinct
+          // name.
+          if (bufferAccessLevel != AccessLevel::Public) {
+            assert(bufferAccessLevel < AccessLevel::Public
+                   && "Above-public access levels should use public buffer");
+            bufferName += " (";
+            bufferName += getAccessLevelSpelling(bufferAccessLevel);
+            bufferName += ")";
+          }
+
           // Pretty-print the declaration we've picked.
           llvm::raw_svector_ostream out(buffer);
-          TrackingPrinter printer(entries, out);
-          ppDecl->print(printer, PrintOptions::printForDiagnostics());
+          TrackingPrinter printer(entries, out, bufferAccessLevel);
+          ppDecl->print(printer,
+                        PrintOptions::printForDiagnostics(bufferAccessLevel));
         }
 
         // Build a buffer with the pretty-printed declaration.

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);

lib/Sema/TypeCheckProtocol.cpp

@@ -2974,9 +2974,9 @@ printRequirementStub(ValueDecl *Requirement, DeclContext *Adopter,
       }
     }
 
-    PrintOptions Options = PrintOptions::printForDiagnostics();
+    PrintOptions Options =
+        PrintOptions::printForDiagnostics(AccessLevel::Private);
     Options.PrintDocumentationComments = false;
-    Options.AccessFilter = AccessLevel::Private;
     Options.PrintAccess = false;
     Options.SkipAttributes = true;
     Options.FunctionDefinitions = true;

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}}

test/diagnostics/Inputs/TestablePrintASTLocationsModule.swift

@@ -0,0 +1 @@
+func ambiguous() {}

test/diagnostics/testable-printast-locations.swift

@@ -0,0 +1,13 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -module-name ModuleA -emit-module-path %t/ModuleA.swiftmodule -primary-file %S/Inputs/TestablePrintASTLocationsModule.swift -enable-testing
+// RUN: %target-swift-frontend -emit-module -module-name ModuleB -emit-module-path %t/ModuleB.swiftmodule -primary-file %S/Inputs/TestablePrintASTLocationsModule.swift -enable-testing
+// RUN: not %target-swift-frontend -typecheck -I %t %s 2>&1 | %FileCheck %s
+
+@testable import ModuleA
+@testable import ModuleB
+
+ambiguous()
+
+// CHECK: testable-printast-locations.swift:[[@LINE-2]]:1: error: ambiguous use of 'ambiguous()'
+// CHECK: ModuleA.ambiguous (internal):1:15: note: found this candidate
+// CHECK: ModuleB.ambiguous (internal):1:15: note: found this candidate