From 2784f33221046bed553bcdf3c5f829515115aaed Mon Sep 17 00:00:00 2001 From: Nicholas Bishop Date: Thu, 13 Aug 2026 17:30:20 +0000 Subject: [PATCH] Fix clang_decls InstId for generic class export (#7633) The exported class was being inserted with a type inst ID as the key (and looked up that way elsewhere), but when checking if the generic class was already exported, the `first_decl_id` was being used. Make it consistent, and opt for `first_decl_id` everywhere since it provides a better location for diagnostics. --- toolchain/check/cpp/export.cpp | 19 +++++++++---------- toolchain/check/cpp/export.h | 2 +- toolchain/check/cpp/generate_ast.cpp | 2 +- .../cpp/function/export/constructor.carbon | 16 +++++++++++----- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/toolchain/check/cpp/export.cpp b/toolchain/check/cpp/export.cpp index d5a38dcda75d..e854a4fb3dbf 100644 --- a/toolchain/check/cpp/export.cpp +++ b/toolchain/check/cpp/export.cpp @@ -299,18 +299,18 @@ static auto MakeSpecificForTemplateArgs( return MakeSpecific(context, loc_id, generic_id, specific_arg_ids); } -auto ExportGenericClassToCpp(Context& context, SemIR::InstId inst_id, +auto ExportGenericClassToCpp(Context& context, SemIR::GenericClassType generic_class_type) -> clang::ClassTemplateDecl* { // Use existing export if possible. const auto& class_info = context.classes().Get(generic_class_type.class_id); - if (const auto* clang_decl = - context.clang_decls().Lookup(class_info.first_decl_id())) { + auto decl_id = class_info.first_decl_id(); + if (const auto* clang_decl = context.clang_decls().Lookup(decl_id)) { return cast(clang_decl->decl()); } // Map the parent scope into the C++ AST. - SemIR::LocId loc_id(inst_id); + SemIR::LocId loc_id(decl_id); auto* decl_context = ExportNameScopeToCpp(context, loc_id, class_info.parent_scope_id); if (!decl_context) { @@ -333,7 +333,7 @@ auto ExportGenericClassToCpp(Context& context, SemIR::InstId inst_id, auto key = SemIR::ClangDeclKey::ForNonFunctionDecl( cast(class_template_decl)); - context.clang_decls().Add({.key = key, .inst_id = inst_id}); + context.clang_decls().Add({.key = key, .inst_id = decl_id}); return class_template_decl; } @@ -348,7 +348,7 @@ static auto GetClassTypeInstId(Context& context, SemIR::ClassId class_id, auto ExportClassSpecializationToCpp( Context& context, clang::ClassTemplateDecl* class_template_decl, llvm::ArrayRef template_args) -> bool { - // Map from the `clang::ClassTemplateDecl` to the Carbon `GenericClassType`. + // Map from the `clang::ClassTemplateDecl` to the Carbon `ClassDecl`. auto clang_decl_id = context.clang_decls().LookupId(SemIR::ClangDeclKey(class_template_decl)); if (clang_decl_id == SemIR::ClangDeclId::None) { @@ -358,10 +358,9 @@ auto ExportClassSpecializationToCpp( if (clang_decl.is_imported) { return false; } - auto generic_class_type = - context.insts().GetAs(clang_decl.inst_id); + auto class_decl = context.insts().GetAs(clang_decl.inst_id); - const auto& class_info = context.classes().Get(generic_class_type.class_id); + const auto& class_info = context.classes().Get(class_decl.class_id); SemIR::LocId loc_id(class_info.first_decl_id()); auto specific_id = MakeSpecificForTemplateArgs( @@ -387,7 +386,7 @@ auto ExportClassSpecializationToCpp( // Create and store the `ClangDeclId`. auto class_type_inst_id = - GetClassTypeInstId(context, generic_class_type.class_id, specific_id); + GetClassTypeInstId(context, class_decl.class_id, specific_id); auto key = SemIR::ClangDeclKey::ForNonFunctionDecl( class_template_specialization_decl); context.clang_decls().Add({.key = key, .inst_id = class_type_inst_id}); diff --git a/toolchain/check/cpp/export.h b/toolchain/check/cpp/export.h index 876e90e357ba..a36261701d23 100644 --- a/toolchain/check/cpp/export.h +++ b/toolchain/check/cpp/export.h @@ -42,7 +42,7 @@ auto ExportClassToCpp(Context& context, SemIR::ClassType class_type) // C++ class template. Otherwise, creates a new C++ class template and // returns it. Returns nullptr if the class could not be exported and an // error was diagnosed. -auto ExportGenericClassToCpp(Context& context, SemIR::InstId inst_id, +auto ExportGenericClassToCpp(Context& context, SemIR::GenericClassType generic_class_type) -> clang::ClassTemplateDecl*; diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index fe7a57f54174..8d5b810a534e 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/toolchain/check/cpp/generate_ast.cpp @@ -238,7 +238,7 @@ auto CarbonExternalASTSource::MapInstIdToClangDeclOrType(LookupResult lookup) } else if (auto generic_class = context_->insts().TryGetAs( type_inst_id)) { - return ExportGenericClassToCpp(*context_, type_inst_id, *generic_class); + return ExportGenericClassToCpp(*context_, *generic_class); } return nullptr; diff --git a/toolchain/check/testdata/interop/cpp/function/export/constructor.carbon b/toolchain/check/testdata/interop/cpp/function/export/constructor.carbon index 34437c3b7eb7..9063b9b9aba0 100644 --- a/toolchain/check/testdata/interop/cpp/function/export/constructor.carbon +++ b/toolchain/check/testdata/interop/cpp/function/export/constructor.carbon @@ -160,12 +160,18 @@ class GenericClass(T: type) { inline Cpp ''' void f() { - // CHECK:STDERR: fail_todo_call_generic_class_constructor.carbon:[[@LINE+7]]:29: error: no matching constructor for initialization of 'Carbon::GenericClass' [CppInteropParseError] - // CHECK:STDERR: 24 | Carbon::GenericClass c(0); + // CHECK:STDERR: fail_todo_call_generic_class_constructor.carbon:[[@LINE+13]]:29: error: no matching constructor for initialization of 'Carbon::GenericClass' [CppInteropParseError] + // CHECK:STDERR: 30 | Carbon::GenericClass c(0); // CHECK:STDERR: | ^ ~ - // CHECK:STDERR: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const GenericClass' for 1st argument [CppInteropParseNote] - // CHECK:STDERR: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'GenericClass' for 1st argument [CppInteropParseNote] - // CHECK:STDERR: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided [CppInteropParseNote] + // CHECK:STDERR: fail_todo_call_generic_class_constructor.carbon:[[@LINE-15]]:29: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const GenericClass' for 1st argument [CppInteropParseNote] + // CHECK:STDERR: 5 | class GenericClass(T: type) { + // CHECK:STDERR: | ^ + // CHECK:STDERR: fail_todo_call_generic_class_constructor.carbon:[[@LINE-18]]:29: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'GenericClass' for 1st argument [CppInteropParseNote] + // CHECK:STDERR: 5 | class GenericClass(T: type) { + // CHECK:STDERR: | ^ + // CHECK:STDERR: fail_todo_call_generic_class_constructor.carbon:[[@LINE-21]]:29: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided [CppInteropParseNote] + // CHECK:STDERR: 5 | class GenericClass(T: type) { + // CHECK:STDERR: | ^ // CHECK:STDERR: Carbon::GenericClass c(0); }