From f2d99f31f03f6248d44dc910a6a9edb6cc2970b4 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 24 Jun 2026 11:36:59 -0700 Subject: [PATCH] Fix contention between Clang and Carbon over external name lookup. (#7411) The function to set the visible declarations with a given name overwrites any existing declarations imported from an AST file, so we need to avoid calling that for declaration contexts whose names are managed by Clang to avoid clobbering names imported from modules. Assisted-by: Gemini via Antigravity --- toolchain/check/cpp/generate_ast.cpp | 20 +++++-- .../cpp/modules/mixed_with_export.carbon | 53 +++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 toolchain/check/testdata/interop/cpp/modules/mixed_with_export.carbon diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index e1571eaf40c6..577f77fb604f 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/toolchain/check/cpp/generate_ast.cpp @@ -523,12 +523,22 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName( // Find the Carbon declaration corresponding to this Clang declaration. 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. + return false; + } auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl); auto decl_id = context_->clang_decls().LookupId(key); - CARBON_CHECK( - decl_id.has_value(), - "The DeclContext should already be associated with a Carbon InstId."); - auto decl_context_inst_id = context_->clang_decls().Get(decl_id).inst_id; + 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; + } llvm::SmallVector lookup_scopes; @@ -536,7 +546,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(decl_context_inst_id), + context_->constant_values().Get(clang_decl.inst_id), SemIR::ConstantId::None, /*extended_scope=*/false, &lookup_scopes)) { return false; } diff --git a/toolchain/check/testdata/interop/cpp/modules/mixed_with_export.carbon b/toolchain/check/testdata/interop/cpp/modules/mixed_with_export.carbon new file mode 100644 index 000000000000..507adf3e1f75 --- /dev/null +++ b/toolchain/check/testdata/interop/cpp/modules/mixed_with_export.carbon @@ -0,0 +1,53 @@ +// 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/full.carbon +// EXTRA-ARGS: --clang-arg=-fmodules --clang-arg=-fmodules-cache-path=%t.cache +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/modules/mixed_with_export.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/modules/mixed_with_export.carbon + +// --- module.modulemap + +module A { + header "a.h" + export * +} + +// --- a.h + +struct A { void F(); }; + +struct C { void H(); }; + +namespace N { void I(); } + +// --- b.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "a.h"; + +class B { + extend base: Cpp.A; + fn G(self); +} + +inline Cpp ''' +Carbon::B b; +A *a = &b; + +void call(C *c) { + a->F(); + + b.G(); + + c->H(); + + N::I(); +} +''';