From 9d0aaa740b61ac374583122241fbebb5ba721c19 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Tue, 8 Jul 2025 15:12:32 +0200 Subject: [PATCH] When adding an imported C++ name, make sure that its `clang::Decl` is mapped if import failed (#5769) When mapping parameter types, we assume that if the `clang::Decl` isn't mapped, the name wasn't added, so this fixes a bug that triggers a crash otherwise. Part of #5533. --- toolchain/check/import_cpp.cpp | 12 ++++ .../interop/cpp/function/class.carbon | 72 +++++++++++++++++++ .../interop/cpp/function/struct.carbon | 72 +++++++++++++++++++ .../interop/cpp/function/union.carbon | 65 +++++++++++++++++ 4 files changed, 221 insertions(+) diff --git a/toolchain/check/import_cpp.cpp b/toolchain/check/import_cpp.cpp index b07322e7f701..6ced5552543f 100644 --- a/toolchain/check/import_cpp.cpp +++ b/toolchain/check/import_cpp.cpp @@ -453,6 +453,12 @@ static auto BuildClassDefinition(Context& context, return {class_id, class_inst_id}; } +// Mark the given `Decl` as failed in `clang_decls`. +static auto MarkFailedDecl(Context& context, clang::Decl* clang_decl) { + context.sem_ir().clang_decls().Add( + {.decl = clang_decl, .inst_id = SemIR::ErrorInst::InstId}); +} + // Imports a record declaration from Clang to Carbon. If successful, returns // the new Carbon class declaration `InstId`. // TODO: Change `clang_decl` to `const &` when lookup is using `clang::DeclID` @@ -466,11 +472,13 @@ static auto ImportCXXRecordDecl(Context& context, SemIR::LocId loc_id, if (!clang_def) { context.TODO(loc_id, "Unsupported: Record declarations without a definition"); + MarkFailedDecl(context, clang_decl); return SemIR::ErrorInst::InstId; } if (clang_def->isDynamicClass()) { context.TODO(loc_id, "Unsupported: Dynamic Class"); + MarkFailedDecl(context, clang_decl); return SemIR::ErrorInst::InstId; } @@ -717,14 +725,17 @@ static auto ImportFunctionDecl(Context& context, SemIR::LocId loc_id, -> SemIR::InstId { if (clang_decl->isVariadic()) { context.TODO(loc_id, "Unsupported: Variadic function"); + MarkFailedDecl(context, clang_decl); return SemIR::ErrorInst::InstId; } if (!clang_decl->isGlobal()) { context.TODO(loc_id, "Unsupported: Non-global function"); + MarkFailedDecl(context, clang_decl); return SemIR::ErrorInst::InstId; } if (clang_decl->getTemplatedKind() != clang::FunctionDecl::TK_NonTemplate) { context.TODO(loc_id, "Unsupported: Template function"); + MarkFailedDecl(context, clang_decl); return SemIR::ErrorInst::InstId; } @@ -740,6 +751,7 @@ static auto ImportFunctionDecl(Context& context, SemIR::LocId loc_id, context.scope_stack().Pop(); if (!function_params_insts.has_value()) { + MarkFailedDecl(context, clang_decl); return SemIR::ErrorInst::InstId; } diff --git a/toolchain/check/testdata/interop/cpp/function/class.carbon b/toolchain/check/testdata/interop/cpp/function/class.carbon index fe40bf8d1945..b06f0be3db76 100644 --- a/toolchain/check/testdata/interop/cpp/function/class.carbon +++ b/toolchain/check/testdata/interop/cpp/function/class.carbon @@ -44,6 +44,78 @@ fn F() { Cpp.foo({}); } +// --- fail_todo_import_decl_value_param_type_previously_imported.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "decl_value_param_type.h"; + +fn F() { + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+11]]:10: error: semantics TODO: `Unsupported: Record declarations without a definition` [SemanticsTodo] + // CHECK:STDERR: let c: Cpp.C; + // CHECK:STDERR: ^~~~~ + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+8]]:10: note: in `Cpp` name lookup for `C` [InCppNameLookup] + // CHECK:STDERR: let c: Cpp.C; + // CHECK:STDERR: ^~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+4]]:15: error: expected `=`; `let` declaration must have an initializer [ExpectedInitializerAfterLet] + // CHECK:STDERR: let c: Cpp.C; + // CHECK:STDERR: ^ + // CHECK:STDERR: + let c: Cpp.C; + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: class C` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo(c); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo(c); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: + Cpp.foo(c); +} + +// ============================================================================ +// Forward-declared class as parameter type imported twice +// ============================================================================ + +// --- double_decl_value_param_type.h + +class C; + +auto foo1(C) -> void; +auto foo2(C) -> void; + +// --- fail_todo_import_double_decl_value_param_type.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "double_decl_value_param_type.h"; + +fn F() { + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+14]]:3: error: semantics TODO: `Unsupported: Record declarations without a definition` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+11]]:3: note: in `Cpp` name lookup for `foo1` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: class C` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo1` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + Cpp.foo1({}); + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: class C` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo2({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo2` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo2({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + Cpp.foo2({}); +} + // ============================================================================ // Defined class without data members as parameter type // ============================================================================ diff --git a/toolchain/check/testdata/interop/cpp/function/struct.carbon b/toolchain/check/testdata/interop/cpp/function/struct.carbon index d6887bd83f6d..0af2ca5b79f4 100644 --- a/toolchain/check/testdata/interop/cpp/function/struct.carbon +++ b/toolchain/check/testdata/interop/cpp/function/struct.carbon @@ -44,6 +44,78 @@ fn F() { Cpp.foo({}); } +// --- fail_todo_import_decl_value_param_type_previously_imported.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "decl_value_param_type.h"; + +fn F() { + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+11]]:10: error: semantics TODO: `Unsupported: Record declarations without a definition` [SemanticsTodo] + // CHECK:STDERR: let s: Cpp.S; + // CHECK:STDERR: ^~~~~ + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+8]]:10: note: in `Cpp` name lookup for `S` [InCppNameLookup] + // CHECK:STDERR: let s: Cpp.S; + // CHECK:STDERR: ^~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+4]]:15: error: expected `=`; `let` declaration must have an initializer [ExpectedInitializerAfterLet] + // CHECK:STDERR: let s: Cpp.S; + // CHECK:STDERR: ^ + // CHECK:STDERR: + let s: Cpp.S; + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: struct S` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo(s); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo(s); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: + Cpp.foo(s); +} + +// ============================================================================ +// Forward-declared struct as parameter type imported twice +// ============================================================================ + +// --- double_decl_value_param_type.h + +struct S; + +auto foo1(S) -> void; +auto foo2(S) -> void; + +// --- fail_todo_import_double_decl_value_param_type.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "double_decl_value_param_type.h"; + +fn F() { + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+14]]:3: error: semantics TODO: `Unsupported: Record declarations without a definition` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+11]]:3: note: in `Cpp` name lookup for `foo1` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: struct S` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo1` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + Cpp.foo1({}); + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: struct S` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo2({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo2` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo2({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + Cpp.foo2({}); +} + // ============================================================================ // Defined struct without data members as parameter type // ============================================================================ diff --git a/toolchain/check/testdata/interop/cpp/function/union.carbon b/toolchain/check/testdata/interop/cpp/function/union.carbon index 76fd7468a13b..6cf657cf3789 100644 --- a/toolchain/check/testdata/interop/cpp/function/union.carbon +++ b/toolchain/check/testdata/interop/cpp/function/union.carbon @@ -40,6 +40,71 @@ fn F() { //@dump-sem-ir-end } +// --- fail_todo_import_decl_value_param_type_previously_imported.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "decl_value_param_type.h"; + +fn F() { + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+11]]:10: error: semantics TODO: `Unsupported: Record declarations without a definition` [SemanticsTodo] + // CHECK:STDERR: let u: Cpp.U; + // CHECK:STDERR: ^~~~~ + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+8]]:10: note: in `Cpp` name lookup for `U` [InCppNameLookup] + // CHECK:STDERR: let u: Cpp.U; + // CHECK:STDERR: ^~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+4]]:15: error: expected `=`; `let` declaration must have an initializer [ExpectedInitializerAfterLet] + // CHECK:STDERR: let u: Cpp.U; + // CHECK:STDERR: ^ + // CHECK:STDERR: + let u: Cpp.U; + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: union U` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo(u); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: fail_todo_import_decl_value_param_type_previously_imported.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo(u); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: + Cpp.foo(u); +} + +// ============================================================================ +// Forward-declared union as parameter type imported twice +// ============================================================================ + +// --- double_decl_value_param_type.h + +union U; + +auto foo1(U) -> void; +auto foo2(U) -> void; + +// --- fail_todo_import_double_decl_value_param_type.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "double_decl_value_param_type.h"; + +fn F() { + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: union U` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo1` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo1({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + Cpp.foo1({}); + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: parameter type: union U` [SemanticsTodo] + // CHECK:STDERR: Cpp.foo2({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: fail_todo_import_double_decl_value_param_type.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `foo2` [InCppNameLookup] + // CHECK:STDERR: Cpp.foo2({}); + // CHECK:STDERR: ^~~~~~~~ + // CHECK:STDERR: + Cpp.foo2({}); +} + // ============================================================================ // Defined union without data members as parameter type // ============================================================================