Improve code and tests for exporting of names to C++ after a lookup failure. (#7984)

Address some review comments from the review of PR#7935 on the changes
from PR#7933.

Consolidate `const_cast`s to a single place with an explanatory comment.
Improve test clarity and organization. Rename lookup-specific helper
function to avoid suggesting it is more broadly applicable.

toolchain/check/cpp/generate_ast.cpp

@@ -305,15 +305,19 @@ auto CarbonExternalASTSource::BuildCarbonNamespace() -> void {
 }
 
 auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
-    const clang::DeclContext* decl_context, clang::DeclarationName decl_name,
-    const clang::DeclContext* /*OriginalDC*/) -> bool {
-  auto clang_decl = GetCarbonOwnedDecl(decl_context);
+    const clang::DeclContext* const_decl_context,
+    clang::DeclarationName decl_name, const clang::DeclContext* /*OriginalDC*/)
+    -> bool {
+  // Clang expects us to modify the DeclContext but passes it as `const`.
+  // TODO: Fix clang to pass in a mutable DeclContext.
+  auto* decl_context = const_cast<clang::DeclContext*>(const_decl_context);
+
+  auto clang_decl = GetCarbonOwnedDeclAsLookupContext(decl_context);
   if (!clang_decl) {
     return false;
   }
 
-  auto* primary_context =
-      const_cast<clang::DeclContext*>(decl_context->getPrimaryContext());
+  auto* primary_context = 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()) {
@@ -397,8 +401,8 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
       auto loc = GetCppLocation(
           *context_, SemIR::LocId(result.scope_result.target_inst_id()));
       auto* typedef_decl = clang::TypedefDecl::Create(
-          ast_context, const_cast<clang::DeclContext*>(decl_context), loc, loc,
-          identifier, ast_context.getTrivialTypeSourceInfo(type, loc));
+          ast_context, decl_context, loc, loc, identifier,
+          ast_context.getTrivialTypeSourceInfo(type, loc));
       if (isa<clang::CXXRecordDecl>(decl_context)) {
         typedef_decl->setAccess(
             MapToCppAccess(result.scope_result.access_kind()));

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

@@ -10,7 +10,7 @@
 // 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
+// --- fail_declare_after_poisoning.carbon
 library "[[@TEST_NAME]]";
 
 import Cpp;
@@ -19,27 +19,29 @@ namespace N;
 
 inline Cpp '''
 namespace Fallback {
-  int LaterFn() { return 1; }
-  int LaterNestedFn() { return 2; }
+  int *LaterFn();
+  int *LaterNestedFn();
 }
 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+1]]:9: error: name `LaterFn` used before it was declared [NameUseBeforeDecl]
-int a = LaterFn();
-int b = LaterNestedFn();
+// triggering typo correction) and resolve to `Fallback`. Following the normal
+// Carbon name poisoning rules, these then trigger poisoning errors when those
+// names are defined in Carbon.
+// CHECK:STDERR: fail_declare_after_poisoning.carbon:[[@LINE+1]]:10: error: name `LaterFn` used before it was declared [NameUseBeforeDecl]
+int *a = LaterFn();
+int *b = LaterNestedFn();
 ''';
 
-// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+3]]:4-10: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR: fail_declare_after_poisoning.carbon:[[@LINE+3]]:4-10: note: declared here [NameUseBeforeDeclNote]
 // CHECK:STDERR:
-// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE-5]]:9: error: name `LaterNestedFn` used before it was declared [NameUseBeforeDecl]
-fn LaterFn() -> i32 { return 3; }
-// CHECK:STDERR: fail_negative_lookup_via_using_directive.carbon:[[@LINE+2]]:6-18: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR: fail_declare_after_poisoning.carbon:[[@LINE-5]]:10: error: name `LaterNestedFn` used before it was declared [NameUseBeforeDecl]
+fn LaterFn() -> i32;
+// CHECK:STDERR: fail_declare_after_poisoning.carbon:[[@LINE+2]]:6-18: note: declared here [NameUseBeforeDeclNote]
 // CHECK:STDERR:
-fn N.LaterNestedFn() -> i32 { return 4; }
+fn N.LaterNestedFn() -> i32;
 
 inline Cpp '''
 // Subsequent qualified lookups find the newly declared Carbon functions rather
@@ -48,6 +50,39 @@ int c = Carbon::LaterFn();
 int d = Carbon::N::LaterNestedFn();
 ''';
 
+// --- fail_declare_after_poisoning_with_lookup_failure.carbon
+library "[[@TEST_NAME]]";
+
+import Cpp;
+
+namespace Nested;
+fn Nested.EarlyFn() {}
+
+inline Cpp '''
+void TestLookupFailureInNested() {
+  // TODO: Only produce one diagnostic here. The first one comes from Clang; the
+  // second is a Carbon error for declaring a poisoned name. Normally we would
+  // suppress the first error when we produce the second, but we don't do that
+  // when the first error is produced by Clang.
+  // CHECK:STDERR: fail_declare_after_poisoning_with_lookup_failure.carbon:[[@LINE+3]]:19-31: error: no member named 'LaterInNested' in namespace 'Carbon::Main::Nested' [CppInteropParseError]
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_declare_after_poisoning_with_lookup_failure.carbon:[[@LINE+1]]:19: error: name `LaterInNested` used before it was declared [NameUseBeforeDecl]
+  Carbon::Nested::LaterInNested();
+  Carbon::Nested::EarlyFn();
+}
+''';
+
+// CHECK:STDERR: fail_declare_after_poisoning_with_lookup_failure.carbon:[[@LINE+2]]:11-23: note: declared here [NameUseBeforeDeclNote]
+// CHECK:STDERR:
+fn Nested.LaterInNested() {}
+
+inline Cpp '''
+void TestAfterTypoInNested() {
+  // No additional error here.
+  Carbon::Nested::LaterInNested();
+}
+''';
+
 // --- fail_typo_in_carbon_namespace.carbon
 library "[[@TEST_NAME]]";
 
@@ -63,67 +98,34 @@ inline Cpp '''
 Carbon::ExportedType* p1 = nullptr;
 
 void TestTypoInCarbon() {
-  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+2]]:11-20: error: no member named 'NoSuchDecl' in namespace 'Carbon' [CppInteropParseError]
-  // CHECK:STDERR:
-  Carbon::NoSuchDecl();
   // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+4]]:11: error: no type named 'ExportedTyp' in namespace 'Carbon'; did you mean 'ExportedType'? [CppInteropParseError]
   // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+3]]:11-21: note: replace with `ExportedType` [CppInteropFixItReplacement]
-  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE-15]]:20: note: 'ExportedType' declared here [CppInteropParseNote]
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE-12]]:20: note: 'ExportedType' declared here [CppInteropParseNote]
   // CHECK:STDERR:
   Carbon::ExportedTyp* p_typo = p1;
-  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+3]]:11-23: error: no member named 'LaterDeclared' in namespace 'Carbon' [CppInteropParseError]
-  // CHECK:STDERR:
-  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+1]]:11: error: name `LaterDeclared` used before it was declared [NameUseBeforeDecl]
-  Carbon::LaterDeclared();
+
+  // When correcting a typo, Clang attempts to fully populate the name lookup
+  // table for the context (namespace `Carbon` in this case). Ensure that
+  // subsequent name lookups still work properly.
   Carbon::EarlyFn();
   Carbon::ExportedType* p2 = p1;
+  // CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+2]]:11-21: error: no member named 'NotDeclared' in namespace 'Carbon' [CppInteropParseError]
+  // CHECK:STDERR:
+  Carbon::NotDeclared();
 }
 ''';
 
-// CHECK:STDERR: fail_typo_in_carbon_namespace.carbon:[[@LINE+2]]:4-16: note: declared here [NameUseBeforeDeclNote]
-// 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+3]]:19-31: error: no member named 'LaterInNested' in namespace 'Carbon::Main::Nested' [CppInteropParseError]
-  // CHECK:STDERR:
-  // CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+1]]:19: error: name `LaterInNested` used before it was declared [NameUseBeforeDecl]
-  Carbon::Nested::LaterInNested();
-  Carbon::Nested::EarlyFn();
-}
-''';
-
-// CHECK:STDERR: fail_typo_in_nested_namespace.carbon:[[@LINE+2]]:11-23: note: declared here [NameUseBeforeDeclNote]
-// CHECK:STDERR:
-fn Nested.LaterInNested() {}
-
-inline Cpp '''
-void TestAfterTypoInNested() {
-  Carbon::Nested::LaterInNested();
-}
-''';
-
 // --- fail_typo_in_carbon_class.carbon
 library "[[@TEST_NAME]]";
 
@@ -138,14 +140,23 @@ class MyClass {
 
 inline Cpp '''
 void TestTypoInClass() {
-  Carbon::MyClass::StaticMethod1();
-  // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE+2]]:20-31: error: no member named 'NoSuchMember' in 'Carbon::Main::MyClass' [CppInteropParseError]
+  // TODO: It'd be nice if Clang's typo correction could correct this.
+  // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE+2]]:20-31: error: no member named 'StaticMetho1' in 'Carbon::Main::MyClass' [CppInteropParseError]
   // CHECK:STDERR:
-  Carbon::MyClass::NoSuchMember();
+  Carbon::MyClass::StaticMetho1();
+
+  // Later name lookups into the class should still work.
   Carbon::MyClass::StaticMethod1();
   Carbon::MyClass::StaticMethod2();
   Carbon::MyClass c{1};
   c.Method();
   static_assert(sizeof(Carbon::MyClass) == 4);
+
+  // Typo correction does work once we've exported the name.
+  // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE+4]]:20-31: error: no member named 'StaticMetho1' in 'Carbon::Main::MyClass'; did you mean 'StaticMethod1'? [CppInteropParseError]
+  // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE+3]]:20-31: note: replace with `StaticMethod1` [CppInteropFixItReplacement]
+  // CHECK:STDERR: fail_typo_in_carbon_class.carbon:[[@LINE-22]]:21: note: 'StaticMethod1' declared here [CppInteropParseNote]
+  // CHECK:STDERR:
+  Carbon::MyClass::StaticMetho1();
 }
 ''';

toolchain/sem_ir/read_only_ast_source.cpp

@@ -14,13 +14,13 @@ namespace Carbon::SemIR {
 
 char ReadOnlyASTSource::id;
 
-auto ReadOnlyASTSource::GetCarbonOwnedDecl(
-    const clang::DeclContext* decl_context) const -> std::optional<ClangDecl> {
+auto ReadOnlyASTSource::GetCarbonOwnedDeclAsLookupContext(
+    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.
+    // We can't perform name lookups into a function, and they don't have
+    // `ClangDeclKey`s in the abstract anyway.
     return std::nullopt;
   }
   auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl);
@@ -38,16 +38,19 @@ auto ReadOnlyASTSource::GetCarbonOwnedDecl(
 }
 
 auto ReadOnlyASTSource::completeVisibleDeclsMap(
-    const clang::DeclContext* decl_context) -> void {
-  if (!GetCarbonOwnedDecl(decl_context)) {
+    const clang::DeclContext* const_decl_context) -> void {
+  // Clang expects us to modify the DeclContext but passes it as `const`.
+  // TODO: Fix clang to pass in a mutable DeclContext.
+  auto* decl_context = const_cast<clang::DeclContext*>(const_decl_context);
+
+  if (!GetCarbonOwnedDeclAsLookupContext(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());
+  auto* primary_context = decl_context->getPrimaryContext();
   primary_context->setHasExternalVisibleStorage(true);
 
   if (auto* lookup_map = primary_context->getLookupPtr();

toolchain/sem_ir/read_only_ast_source.h

@@ -35,9 +35,10 @@ class ReadOnlyASTSource : public clang::ExternalSemaSource {
   }
 
  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
+  // If `decl_context` was created by exporting a Carbon declaration to C++, and
+  // can have name lookups performed into it, returns its `ClangDecl`. Otherwise
+  // returns `std::nullopt`.
+  auto GetCarbonOwnedDeclAsLookupContext(clang::DeclContext* decl_context) const
       -> std::optional<ClangDecl>;
 
  private: