Fix non-public imported declaration diagnostic bug

When DiagnosticEngine needs to diagnose something about an imported declaration, it uses ASTPrinter to print the declaration’s interface into a source buffer and then diagnoses it there. However, this code only printed public declarations, so it failed to account for features like `@testable import` which allow less-than-public declarations to be imported. Errors involving these declarations would therefore be diagnosed at <unknown>:0.

This commit changes DiagnosticEngine to determine the access level of the declaration it needs to print and, if it is below `Public`, instead prints a separate interface whose minimum access level is low enough to include the desired declaration.

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