Fix exporting of names to C++ after a lookup failure. (#7933)

If Clang encounters a lookup failure, it caches that result, creating
problems with `-share-cpp-ast` where a lookup failure in one file
effectively poisons the same name when looked up in a different file.

Also, if Clang triggers loading a complete name table (which happens,
for example, during typo correction, but Clang could do that any time it
wants to), it clears out the "has external visible results" flag, which
means that introducing new names in Carbon and then attempting to use
them from C++ after that point does not work.

Fix both of these problems by re-enabling the "has external visible
results" flag on callbacks that indicate Clang just cleared it, and by
not caching failed name lookups into Carbon scopes in Clang's lookup
table. (They still get cached in *our* lookup table.)

Assisted-by: Gemini via Antigravity

toolchain/check/cpp/generate_ast.cpp

@@ -9,6 +9,7 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/AST/Mangle.h"
 #include "clang/Basic/DiagnosticParse.h"
 #include "clang/Basic/FileManager.h"
@@ -271,11 +272,11 @@ auto CarbonExternalASTSource::BuildCarbonNamespace() -> void {
         clang::SourceLocation(), identifier, /*PrevDecl=*/nullptr,
         /*Nested=*/false);
     decl_context->addDecl(carbon_cpp_namespace);
-
-    // We provide custom lookup results within this namespace.
-    carbon_cpp_namespace->setHasExternalVisibleStorage();
   }
 
+  // We provide custom lookup results within this namespace.
+  carbon_cpp_namespace->getPrimaryContext()->setHasExternalVisibleStorage();
+
   // Register this file's package scope as corresponding to the `Carbon`
   // namespace in C++.
   // TODO: For mangling purposes, include the package as a sub-namespace.
@@ -290,24 +291,26 @@ auto CarbonExternalASTSource::BuildCarbonNamespace() -> void {
 auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
     const clang::DeclContext* decl_context, clang::DeclarationName decl_name,
     const clang::DeclContext* /*OriginalDC*/) -> bool {
-  // Find the Carbon declaration corresponding to this Clang declaration.
-  auto* decl = cast<clang::Decl>(
-      const_cast<clang::DeclContext*>(decl_context->getPrimaryContext()));
-  if (isa<clang::FunctionDecl>(decl)) {
-    // Functions don't meaningfully have visible decls, but bail out early since
-    // we can't form a `ClangDeclKey` for a function in the abstract.
+  auto clang_decl = GetCarbonOwnedDecl(decl_context);
+  if (!clang_decl) {
     return false;
   }
-  auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl);
-  auto decl_id = context_->clang_decls().LookupId(key);
-  if (!decl_id.has_value()) {
-    return false;
-  }
-  auto clang_decl = context_->clang_decls().Get(decl_id);
-  if (clang_decl.is_imported) {
-    // This is imported from C++, presumably from a Clang AST file, so it's not
-    // our responsibility to provide its name lookup results.
-    return false;
+
+  auto* primary_context =
+      const_cast<clang::DeclContext*>(decl_context->getPrimaryContext());
+  if (auto* lookup_map = primary_context->getLookupPtr()) {
+    if (auto it = lookup_map->find(decl_name);
+        it != lookup_map->end() && !it->second.isNull()) {
+      // We already exported declarations for this name, and may have been
+      // asked again after `completeVisibleDeclsMap` re-enabled external
+      // visible storage. Mark external lookup complete without re-exporting.
+      //
+      // Note that this only sets that there are no external decls from an AST
+      // file, despite its name. External visible decls already added by other
+      // AST sources, such as this one, are retained.
+      SetNoExternalVisibleDeclsForName(primary_context, decl_name);
+      return true;
+    }
   }
 
   llvm::SmallVector<Check::LookupScope> lookup_scopes;
@@ -316,7 +319,7 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
   // here - completeness should've been checked by clang before this point.
   if (!AppendLookupScopesForConstant(
           *context_, SemIR::LocId::None,
-          context_->constant_values().Get(clang_decl.inst_id),
+          context_->constant_values().Get(clang_decl->inst_id),
           SemIR::ConstantId::None, /*extended_scope=*/false, &lookup_scopes)) {
     return false;
   }
@@ -335,6 +338,7 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
       break;
     }
     default:
+      SetNoExternalVisibleDeclsForName(primary_context, decl_name);
       return false;
   }
 
@@ -342,18 +346,26 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
 
   // `required=false` so Carbon doesn't diagnose a failure, let Clang diagnose
   // it or even SFINAE.
-  LookupResult result =
-      LookupQualifiedName(*context_, SemIR::LocId::None, name_id, lookup_scopes,
-                          /*required=*/false);
+  LookupResult result = LookupQualifiedName(*context_, GetCurrentCppLocId(),
+                                            name_id, lookup_scopes,
+                                            /*required=*/false);
   if (!result.scope_result.is_found()) {
+    // Avoid caching a negative lookup, as a declaration with this name may be
+    // added in Carbon later.
+    if (auto* lookup_map = primary_context->getLookupPtr()) {
+      if (auto it = lookup_map->find(decl_name);
+          it != lookup_map->end() && it->second.isNull()) {
+        lookup_map->erase(it);
+      }
+    }
     return false;
   }
 
   // Map the found Carbon entity to a Clang NamedDecl.
   CARBON_KIND_SWITCH(MapInstIdToClangDeclOrType(result)) {
-    case CARBON_KIND(clang::NamedDecl* clang_decl): {
-      if (clang_decl) {
-        SetExternalVisibleDeclsForName(decl_context, decl_name, {clang_decl});
+    case CARBON_KIND(clang::NamedDecl* mapped_decl): {
+      if (mapped_decl) {
+        SetExternalVisibleDeclsForName(decl_context, decl_name, {mapped_decl});
         return true;
       } else {
         SetNoExternalVisibleDeclsForName(decl_context, decl_name);

toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon

@@ -0,0 +1,179 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon
+
+// --- fail_negative_lookup_via_using_directive.carbon
+library "[[@TEST_NAME]]";
+
+import Cpp;
+
+namespace N;
+
+inline Cpp '''
+namespace Fallback {
+  int LaterFn() { return 1; }
+  int LaterNestedFn() { return 2; }
+}
+using namespace Fallback;
+using namespace Carbon;
+using namespace Carbon::N;
+
+// Unqualified lookups search `Carbon` and `Carbon::N` (failing there without
+// triggering typo correction) and resolve to `Fallback`.
+// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+3]]:9: error: name `LaterFn` used before it was declared [NameUseBeforeDecl]
+// CHECK:STDERR: int a = LaterFn();
+// CHECK:STDERR:         ^
+int a = LaterFn();
+int b = LaterNestedFn();
+''';
+
+// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+7]]:4: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR: fn LaterFn() -> i32 { return 3; }
+// CHECK:STDERR:    ^~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE-7]]:9: error: name `LaterNestedFn` used before it was declared [NameUseBeforeDecl]
+// CHECK:STDERR: int b = LaterNestedFn();
+// CHECK:STDERR:         ^
+fn LaterFn() -> i32 { return 3; }
+// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+4]]:6: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR: fn N.LaterNestedFn() -> i32 { return 4; }
+// CHECK:STDERR:      ^~~~~~~~~~~~~
+// CHECK:STDERR:
+fn N.LaterNestedFn() -> i32 { return 4; }
+
+inline Cpp '''
+// Subsequent qualified lookups find the newly declared Carbon functions rather
+// than seeing cached negative lookup results.
+int c = Carbon::LaterFn();
+int d = Carbon::N::LaterNestedFn();
+''';
+
+// --- fail_typo_in_carbon_namespace.carbon
+library "[[@TEST_NAME]]";
+
+import Cpp;
+
+class ExportedType {
+  var x: i32;
+}
+
+fn EarlyFn() {}
+
+inline Cpp '''
+Carbon::ExportedType* p1 = nullptr;
+
+void TestTypoInCarbon() {
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+4]]:11: error: no member named 'NoSuchDecl' in namespace 'Carbon' [CppInteropParseError]
+  // CHECK:STDERR:    19 |   Carbon::NoSuchDecl();
+  // CHECK:STDERR:       |           ^~~~~~~~~~
+  // CHECK:STDERR:
+  Carbon::NoSuchDecl();
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+8]]:11: error: no type named 'ExportedTyp' in namespace 'Carbon'; did you mean 'ExportedType'? [CppInteropParseError]
+  // CHECK:STDERR:    28 |   Carbon::ExportedTyp* p_typo = p1;
+  // CHECK:STDERR:       |   ~~~~~~~~^~~~~~~~~~~
+  // CHECK:STDERR:       |           ExportedType
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE-19]]:20: note: 'ExportedType' declared here [CppInteropParseNote]
+  // CHECK:STDERR:     5 | class ExportedType {
+  // CHECK:STDERR:       |                    ^
+  // CHECK:STDERR:
+  Carbon::ExportedTyp* p_typo = p1;
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+7]]:11: error: no member named 'LaterDeclared' in namespace 'Carbon' [CppInteropParseError]
+  // CHECK:STDERR:    36 |   Carbon::LaterDeclared();
+  // CHECK:STDERR:       |           ^~~~~~~~~~~~~
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+3]]:11: error: name `LaterDeclared` used before it was declared [NameUseBeforeDecl]
+  // CHECK:STDERR:   Carbon::LaterDeclared();
+  // CHECK:STDERR:           ^
+  Carbon::LaterDeclared();
+  Carbon::EarlyFn();
+  Carbon::ExportedType* p2 = p1;
+}
+''';
+
+// CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+4]]:4: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR: fn LaterDeclared() -> i32 { return 42; }
+// CHECK:STDERR:    ^~~~~~~~~~~~~
+// CHECK:STDERR:
+fn LaterDeclared() -> i32 { return 42; }
+
+class LaterType {
+  var y: i32;
+}
+
+inline Cpp '''
+void TestAfterTypoInCarbon() {
+  int v = Carbon::LaterDeclared();
+  static_assert(sizeof(Carbon::ExportedType) == 4);
+  static_assert(sizeof(Carbon::LaterType) == 4);
+}
+''';
+
+// --- fail_typo_in_nested_namespace.carbon
+library "[[@TEST_NAME]]";
+
+import Cpp;
+
+namespace Nested;
+fn Nested.EarlyFn() {}
+
+inline Cpp '''
+void TestTypoInNested() {
+  // CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+7]]:19: error: no member named 'LaterInNested' in namespace 'Carbon::Nested' [CppInteropParseError]
+  // CHECK:STDERR:    17 |   Carbon::Nested::LaterInNested();
+  // CHECK:STDERR:       |                   ^~~~~~~~~~~~~
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+3]]:19: error: name `LaterInNested` used before it was declared [NameUseBeforeDecl]
+  // CHECK:STDERR:   Carbon::Nested::LaterInNested();
+  // CHECK:STDERR:                   ^
+  Carbon::Nested::LaterInNested();
+  Carbon::Nested::EarlyFn();
+}
+''';
+
+// CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+4]]:11: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR: fn Nested.LaterInNested() {}
+// CHECK:STDERR:           ^~~~~~~~~~~~~
+// CHECK:STDERR:
+fn Nested.LaterInNested() {}
+
+inline Cpp '''
+void TestAfterTypoInNested() {
+  Carbon::Nested::LaterInNested();
+}
+''';
+
+// --- fail_typo_in_carbon_class.carbon
+library "[[@TEST_NAME]]";
+
+import Cpp;
+
+class MyClass {
+  var a: i32;
+  fn StaticMethod1();
+  fn StaticMethod2();
+  fn Method(self);
+}
+
+inline Cpp '''
+void TestTypoInClass() {
+  Carbon::MyClass::StaticMethod1();
+  // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE+4]]:20: error: no member named 'NoSuchMember' in 'Carbon::MyClass' [CppInteropParseError]
+  // CHECK:STDERR:    19 |   Carbon::MyClass::NoSuchMember();
+  // CHECK:STDERR:       |                    ^~~~~~~~~~~~
+  // CHECK:STDERR:
+  Carbon::MyClass::NoSuchMember();
+  Carbon::MyClass::StaticMethod1();
+  Carbon::MyClass::StaticMethod2();
+  Carbon::MyClass c{1};
+  c.Method();
+  static_assert(sizeof(Carbon::MyClass) == 4);
+}
+''';

toolchain/sem_ir/BUILD

@@ -470,6 +470,7 @@ cc_library(
     hdrs = ["read_only_ast_source.h"],
     deps = [
         "//toolchain/sem_ir:file",
+        "@llvm-project//clang:ast",
         "@llvm-project//clang:sema",
     ],
 )

toolchain/sem_ir/class.cpp

@@ -73,9 +73,9 @@ auto Class::GetStructTypeFields(const File& sem_ir,
 
 auto GetAsCarbonOwnedClass(const File& sem_ir, const clang::TagDecl* tag_decl)
     -> std::optional<std::pair<SemIR::TypeId, SemIR::ClassType>> {
-  // Quickly check whether we could possibly own this class.
-  // TODO: Once we multiplex with the ASTReader, handle
-  // ASTReader::completeVisibleDeclsMap setting this to `false`.
+  // Quickly check whether we could possibly own this class. Carbon-owned
+  // classes always have external visible storage (preserved across
+  // `ASTReader::completeVisibleDeclsMap` by `ReadOnlyASTSource`).
   if (!tag_decl->hasExternalVisibleStorage()) {
     return std::nullopt;
   }

toolchain/sem_ir/read_only_ast_source.cpp

@@ -4,10 +4,71 @@
 
 #include "toolchain/sem_ir/read_only_ast_source.h"
 
+#include <optional>
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclContextInternals.h"
+
 namespace Carbon::SemIR {
 
 char ReadOnlyASTSource::id;
 
+auto ReadOnlyASTSource::GetCarbonOwnedDecl(
+    const clang::DeclContext* decl_context) const -> std::optional<ClangDecl> {
+  auto* decl = cast<clang::Decl>(
+      const_cast<clang::DeclContext*>(decl_context->getPrimaryContext()));
+  if (isa<clang::FunctionDecl>(decl)) {
+    // Functions don't meaningfully have visible decls, but bail out early since
+    // we can't form a `ClangDeclKey` for a function in the abstract.
+    return std::nullopt;
+  }
+  auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl);
+  auto decl_id = sem_ir_.clang_decls().LookupId(key);
+  if (!decl_id.has_value()) {
+    return std::nullopt;
+  }
+  auto clang_decl = sem_ir_.clang_decls().Get(decl_id);
+  if (clang_decl.is_imported) {
+    // This is imported from C++, presumably from a Clang AST file, so it's not
+    // our responsibility to provide its name lookup results.
+    return std::nullopt;
+  }
+  return clang_decl;
+}
+
+auto ReadOnlyASTSource::completeVisibleDeclsMap(
+    const clang::DeclContext* decl_context) -> void {
+  if (!GetCarbonOwnedDecl(decl_context)) {
+    return;
+  }
+
+  // `ASTReader::completeVisibleDeclsMap` unconditionally clears external
+  // visible storage, even for DeclContexts it does not own. Restore it for
+  // Carbon-owned DeclContexts so future lookups continue to query us.
+  auto* primary_context =
+      const_cast<clang::DeclContext*>(decl_context->getPrimaryContext());
+  primary_context->setHasExternalVisibleStorage(true);
+
+  if (auto* lookup_map = primary_context->getLookupPtr();
+      lookup_map && !lookup_map->empty()) {
+    // `setHasExternalVisibleStorage(true)` sets
+    // `NeedToReconcileExternalVisibleStorage`, which would otherwise mark all
+    // already-resolved entries as needing another external lookup. Trigger
+    // reconciliation now and clear `hasExternalDecls` on all existing entries
+    // so we don't re-export or re-diagnose them.
+    //
+    // Note that this only clears out external decls from an AST file, despite
+    // its name. External visible decls already added by other AST sources, such
+    // as this one, are retained.
+    SetNoExternalVisibleDeclsForName(primary_context,
+                                     lookup_map->begin()->first);
+    for (auto& [_, decls] : *lookup_map) {
+      decls.removeExternalDecls();
+    }
+  }
+}
+
 // Get the field offset for each field in a class.
 //
 // Returns true on success, false if any error occurs.

toolchain/sem_ir/read_only_ast_source.h

@@ -5,6 +5,8 @@
 #ifndef CARBON_TOOLCHAIN_SEM_IR_READ_ONLY_AST_SOURCE_H_
 #define CARBON_TOOLCHAIN_SEM_IR_READ_ONLY_AST_SOURCE_H_
 
+#include <optional>
+
 #include "clang/Sema/ExternalSemaSource.h"
 #include "toolchain/sem_ir/file.h"
 
@@ -14,6 +16,9 @@ class ReadOnlyASTSource : public clang::ExternalSemaSource {
  public:
   explicit ReadOnlyASTSource(const File& sem_ir) : sem_ir_(sem_ir) {}
 
+  auto completeVisibleDeclsMap(const clang::DeclContext* decl_context)
+      -> void override;
+
   auto layoutRecordType(
       const clang::RecordDecl* record_decl, uint64_t& size, uint64_t& alignment,
       llvm::DenseMap<const clang::FieldDecl*, uint64_t>& field_offsets,
@@ -29,6 +34,12 @@ class ReadOnlyASTSource : public clang::ExternalSemaSource {
     return s->isA(&id);
   }
 
+ protected:
+  // If `decl_context` was created by exporting a Carbon declaration to C++,
+  // returns its `ClangDecl`. Otherwise returns `std::nullopt`.
+  auto GetCarbonOwnedDecl(const clang::DeclContext* decl_context) const
+      -> std::optional<ClangDecl>;
+
  private:
   // For LLVM RTTI.
   static char id;