From f9c21d769e56b2b294f43c29c41efe070d5c10a3 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 16 Oct 2026 16:33:29 +0000 Subject: [PATCH] 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 | 18 ++- .../cpp/namespace/lookup_after_failure.carbon | 123 ++++++++++-------- toolchain/sem_ir/read_only_ast_source.cpp | 19 +-- toolchain/sem_ir/read_only_ast_source.h | 7 +- 4 files changed, 93 insertions(+), 74 deletions(-) diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index 5ae7d2a70932..42bf7b9db45d 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/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(const_decl_context); + + auto clang_decl = GetCarbonOwnedDeclAsLookupContext(decl_context); if (!clang_decl) { return false; } - auto* primary_context = - const_cast(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(decl_context), loc, loc, - identifier, ast_context.getTrivialTypeSourceInfo(type, loc)); + ast_context, decl_context, loc, loc, identifier, + ast_context.getTrivialTypeSourceInfo(type, loc)); if (isa(decl_context)) { typedef_decl->setAccess( MapToCppAccess(result.scope_result.access_kind())); diff --git a/toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon b/toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon index dc5b5d69eb08..06104b7db270 100644 --- a/toolchain/check/testdata/interop/cpp/namespace/lookup_after_failure.carbon +++ b/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(); } '''; diff --git a/toolchain/sem_ir/read_only_ast_source.cpp b/toolchain/sem_ir/read_only_ast_source.cpp index 1467f4170f58..d3b42691860a 100644 --- a/toolchain/sem_ir/read_only_ast_source.cpp +++ b/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 { +auto ReadOnlyASTSource::GetCarbonOwnedDeclAsLookupContext( + clang::DeclContext* decl_context) const -> std::optional { auto* decl = cast( const_cast(decl_context->getPrimaryContext())); if (isa(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(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(decl_context->getPrimaryContext()); + auto* primary_context = decl_context->getPrimaryContext(); primary_context->setHasExternalVisibleStorage(true); if (auto* lookup_map = primary_context->getLookupPtr(); diff --git a/toolchain/sem_ir/read_only_ast_source.h b/toolchain/sem_ir/read_only_ast_source.h index f770edc909a2..5fbfb94ae040 100644 --- a/toolchain/sem_ir/read_only_ast_source.h +++ b/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; private: