From 5abd214d9d11f895ff5dbb9df446064be6577248 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Tue, 30 Sep 2025 00:45:22 +0200 Subject: [PATCH] Set the location for the candidate set when looking up C++ operators (#6138) This adds location information and prevents crashes in some cases of template instantiation in operator lookup. Removed `InCppOperatorLookup` note as it is no longer necessary. Part of #5995. --- toolchain/check/cpp/operators.cpp | 16 +- .../interop/cpp/function/operators.carbon | 187 +++++++++++++++--- toolchain/diagnostics/diagnostic_kind.def | 1 - 3 files changed, 165 insertions(+), 39 deletions(-) diff --git a/toolchain/check/cpp/operators.cpp b/toolchain/check/cpp/operators.cpp index dda8d2d4d4b6..a3771ad901e6 100644 --- a/toolchain/check/cpp/operators.cpp +++ b/toolchain/check/cpp/operators.cpp @@ -7,6 +7,7 @@ #include "clang/Sema/Overload.h" #include "clang/Sema/Sema.h" #include "toolchain/check/cpp/import.h" +#include "toolchain/check/cpp/location.h" #include "toolchain/check/cpp/type_mapping.h" #include "toolchain/check/inst.h" #include "toolchain/check/type.h" @@ -157,12 +158,11 @@ static auto GetClangOperatorKind(Context& context, SemIR::LocId loc_id, auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op, llvm::ArrayRef arg_ids) -> SemIR::InstId { - Diagnostics::AnnotationScope annotate_diagnostics( - &context.emitter(), [&](auto& builder) { - CARBON_DIAGNOSTIC(InCppOperatorLookup, Note, - "in `Cpp` operator `{0}` lookup", std::string); - builder.Note(loc_id, InCppOperatorLookup, op.interface_name.str()); - }); + // Register an annotation scope to flush any Clang diagnostics when we return. + // This is important to ensure that Clang diagnostics are properly interleaved + // with Carbon diagnostics. + Diagnostics::AnnotationScope annotate_diagnostics(&context.emitter(), + [](auto& /*builder*/) {}); auto op_kind = GetClangOperatorKind(context, loc_id, op.interface_name, op.op_name); @@ -191,9 +191,9 @@ auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op, } clang::UnresolvedSet<4> functions; - // TODO: Add location accordingly. clang::OverloadCandidateSet candidate_set( - clang::SourceLocation(), clang::OverloadCandidateSet::CSK_Operator); + GetCppLocation(context, loc_id), + clang::OverloadCandidateSet::CSK_Operator); // This works for both unary and binary operators. context.clang_sema().LookupOverloadedBinOp(candidate_set, *op_kind, functions, *arg_exprs); diff --git a/toolchain/check/testdata/interop/cpp/function/operators.carbon b/toolchain/check/testdata/interop/cpp/function/operators.carbon index 2d1b177894cb..d6fe4efc86b9 100644 --- a/toolchain/check/testdata/interop/cpp/function/operators.carbon +++ b/toolchain/check/testdata/interop/cpp/function/operators.carbon @@ -462,10 +462,7 @@ fn F() { var c1: Cpp.C = Cpp.C.C(); // Bitwise. - // CHECK:STDERR: fail_todo_import_unsupported_binary_operators.carbon:[[@LINE+11]]:3: error: semantics TODO: `Unsupported operator interface `LeftShiftAssignWith`` [SemanticsTodo] - // CHECK:STDERR: c1 <<= 1; - // CHECK:STDERR: ^~~~~~~~ - // CHECK:STDERR: fail_todo_import_unsupported_binary_operators.carbon:[[@LINE+8]]:3: note: in `Cpp` operator `LeftShiftAssignWith` lookup [InCppOperatorLookup] + // CHECK:STDERR: fail_todo_import_unsupported_binary_operators.carbon:[[@LINE+8]]:3: error: semantics TODO: `Unsupported operator interface `LeftShiftAssignWith`` [SemanticsTodo] // CHECK:STDERR: c1 <<= 1; // CHECK:STDERR: ^~~~~~~~ // CHECK:STDERR: @@ -474,10 +471,7 @@ fn F() { // CHECK:STDERR: ^~~~~~~~ // CHECK:STDERR: c1 <<= 1; - // CHECK:STDERR: fail_todo_import_unsupported_binary_operators.carbon:[[@LINE+11]]:3: error: semantics TODO: `Unsupported operator interface `RightShiftAssignWith`` [SemanticsTodo] - // CHECK:STDERR: c1 >>= 2; - // CHECK:STDERR: ^~~~~~~~ - // CHECK:STDERR: fail_todo_import_unsupported_binary_operators.carbon:[[@LINE+8]]:3: note: in `Cpp` operator `RightShiftAssignWith` lookup [InCppOperatorLookup] + // CHECK:STDERR: fail_todo_import_unsupported_binary_operators.carbon:[[@LINE+8]]:3: error: semantics TODO: `Unsupported operator interface `RightShiftAssignWith`` [SemanticsTodo] // CHECK:STDERR: c1 >>= 2; // CHECK:STDERR: ^~~~~~~~ // CHECK:STDERR: @@ -770,16 +764,13 @@ library "[[@TEST_NAME]]"; import Cpp library "incomplete.h"; fn F() { - // CHECK:STDERR: fail_import_incomplete_unary.carbon:[[@LINE+11]]:27: error: looking up a C++ operator with incomplete operand type `Cpp.Incomplete` [IncompleteOperandTypeInCppOperatorLookup] + // CHECK:STDERR: fail_import_incomplete_unary.carbon:[[@LINE+8]]:27: error: looking up a C++ operator with incomplete operand type `Cpp.Incomplete` [IncompleteOperandTypeInCppOperatorLookup] // CHECK:STDERR: let result_unary: i32 = -*Cpp.CreateIncomplete(); // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: fail_import_incomplete_unary.carbon:[[@LINE-6]]:10: in file included here [InCppInclude] // CHECK:STDERR: ./incomplete.h:2:7: note: class was forward declared here [ClassForwardDeclaredHere] // CHECK:STDERR: class Incomplete; // CHECK:STDERR: ^ - // CHECK:STDERR: fail_import_incomplete_unary.carbon:[[@LINE+4]]:27: note: in `Cpp` operator `Negate` lookup [InCppOperatorLookup] - // CHECK:STDERR: let result_unary: i32 = -*Cpp.CreateIncomplete(); - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: let result_unary: i32 = -*Cpp.CreateIncomplete(); } @@ -792,16 +783,13 @@ import Cpp library "incomplete.h"; fn F() { var complete: Cpp.Complete = Cpp.Complete.Complete(); - // CHECK:STDERR: fail_import_incomplete_binary.carbon:[[@LINE+11]]:28: error: looking up a C++ operator with incomplete operand type `Cpp.Incomplete` [IncompleteOperandTypeInCppOperatorLookup] + // CHECK:STDERR: fail_import_incomplete_binary.carbon:[[@LINE+8]]:28: error: looking up a C++ operator with incomplete operand type `Cpp.Incomplete` [IncompleteOperandTypeInCppOperatorLookup] // CHECK:STDERR: let result_binary: i32 = complete + *Cpp.CreateIncomplete(); // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: fail_import_incomplete_binary.carbon:[[@LINE-7]]:10: in file included here [InCppInclude] // CHECK:STDERR: ./incomplete.h:2:7: note: class was forward declared here [ClassForwardDeclaredHere] // CHECK:STDERR: class Incomplete; // CHECK:STDERR: ^ - // CHECK:STDERR: fail_import_incomplete_binary.carbon:[[@LINE+4]]:28: note: in `Cpp` operator `AddWith` lookup [InCppOperatorLookup] - // CHECK:STDERR: let result_binary: i32 = complete + *Cpp.CreateIncomplete(); - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: let result_binary: i32 = complete + *Cpp.CreateIncomplete(); } @@ -825,15 +813,12 @@ fn CreateIncomplete() -> Incomplete*; fn F() { var complete: Cpp.Complete = Cpp.Complete.Complete(); - // CHECK:STDERR: fail_incomplete_operand_carbon_type.carbon:[[@LINE+10]]:21: error: looking up a C++ operator with incomplete operand type `Incomplete` [IncompleteOperandTypeInCppOperatorLookup] + // CHECK:STDERR: fail_incomplete_operand_carbon_type.carbon:[[@LINE+7]]:21: error: looking up a C++ operator with incomplete operand type `Incomplete` [IncompleteOperandTypeInCppOperatorLookup] // CHECK:STDERR: let result: i32 = *CreateIncomplete() + complete; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: fail_incomplete_operand_carbon_type.carbon:[[@LINE-8]]:1: note: class was forward declared here [ClassForwardDeclaredHere] // CHECK:STDERR: class Incomplete; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_incomplete_operand_carbon_type.carbon:[[@LINE+4]]:21: note: in `Cpp` operator `AddWith` lookup [InCppOperatorLookup] - // CHECK:STDERR: let result: i32 = *CreateIncomplete() + complete; - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: let result: i32 = *CreateIncomplete() + complete; } @@ -858,13 +843,10 @@ library "[[@TEST_NAME]]"; import Cpp library "unsupported_in_instantiation.h"; fn F() { - // CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+10]]:21: error: semantics TODO: `class with virtual bases` [SemanticsTodo] + // CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+7]]:21: error: semantics TODO: `class with virtual bases` [SemanticsTodo] // CHECK:STDERR: let result: i32 = -Cpp.unsupported; // CHECK:STDERR: ^~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+7]]:21: note: while completing C++ type `Cpp.Unsupported` [InCppTypeCompletion] - // CHECK:STDERR: let result: i32 = -Cpp.unsupported; - // CHECK:STDERR: ^~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+4]]:21: note: in `Cpp` operator `Negate` lookup [InCppOperatorLookup] + // CHECK:STDERR: fail_import_unsupported_in_instantiation_unary.carbon:[[@LINE+4]]:21: note: while completing C++ type `Cpp.Unsupported` [InCppTypeCompletion] // CHECK:STDERR: let result: i32 = -Cpp.unsupported; // CHECK:STDERR: ^~~~~~~~~~~~~~~~ // CHECK:STDERR: @@ -879,19 +861,70 @@ import Cpp library "unsupported_in_instantiation.h"; fn F() { var supported: Cpp.Supported = Cpp.Supported.Supported(); - // CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+10]]:21: error: semantics TODO: `class with virtual bases` [SemanticsTodo] + // CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+7]]:21: error: semantics TODO: `class with virtual bases` [SemanticsTodo] // CHECK:STDERR: let result: i32 = supported + Cpp.unsupported; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+7]]:21: note: while completing C++ type `Cpp.Unsupported` [InCppTypeCompletion] - // CHECK:STDERR: let result: i32 = supported + Cpp.unsupported; - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+4]]:21: note: in `Cpp` operator `AddWith` lookup [InCppOperatorLookup] + // CHECK:STDERR: fail_import_unsupported_in_instantiation_binary.carbon:[[@LINE+4]]:21: note: while completing C++ type `Cpp.Unsupported` [InCppTypeCompletion] // CHECK:STDERR: let result: i32 = supported + Cpp.unsupported; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: let result: i32 = supported + Cpp.unsupported; } +// ============================================================================ +// Indirect template instantiation +// ============================================================================ + +// --- indirect_template_instantiation.carbon + +library "[[@TEST_NAME]]"; + +import Cpp inline ''' +template struct B {}; +template struct A {}; + +using X = B>; +int operator+(X, X); +'''; + +fn F(x: Cpp.X) -> i32 { + //@dump-sem-ir-begin + return x + x; + //@dump-sem-ir-end +} + +// ============================================================================ +// Indirect template instantiation error +// ============================================================================ + +// --- fail_indirect_template_instantiation_error.carbon + +library "[[@TEST_NAME]]"; + +import Cpp inline ''' +template struct B {}; +template struct A { + // CHECK:STDERR: fail_indirect_template_instantiation_error.carbon:[[@LINE+3]]:19: error: type 'int' cannot be used prior to '::' because it has no members [CppInteropParseError] + // CHECK:STDERR: 10 | friend typename T::error operator+(B, B); + // CHECK:STDERR: | ^ + friend typename T::error operator+(B, B); +}; + +using X = B>; +'''; + +fn F(x: Cpp.X) -> i32 { + // CHECK:STDERR: fail_indirect_template_instantiation_error.carbon:[[@LINE+8]]:12: note: in instantiation of template class 'A' requested here [CppInteropParseNote] + // CHECK:STDERR: 25 | return x + x; + // CHECK:STDERR: | ^ + // CHECK:STDERR: + // CHECK:STDERR: fail_indirect_template_instantiation_error.carbon:[[@LINE+4]]:12: error: no matching function for call to '' [CppInteropParseError] + // CHECK:STDERR: 25 | return x + x; + // CHECK:STDERR: | ^ + // CHECK:STDERR: + return x + x; +} + // ============================================================================ // Operator overloading // ============================================================================ @@ -912,7 +945,9 @@ import Cpp library "overloading.h"; fn F() { let c1: Cpp.C = Cpp.C.C(); let c2: Cpp.C = Cpp.C.C(); + //@dump-sem-ir-begin let c3: Cpp.C = c1 + c2; + //@dump-sem-ir-end } // CHECK:STDOUT: --- import_unary_operators.carbon @@ -2883,3 +2918,95 @@ fn F() { // CHECK:STDOUT: // CHECK:STDOUT: } // CHECK:STDOUT: +// CHECK:STDOUT: --- indirect_template_instantiation.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %B: type = class_type @B [concrete] +// CHECK:STDOUT: %int_32: Core.IntLiteral = int_value 32 [concrete] +// CHECK:STDOUT: %i32: type = class_type @Int, @Int(%int_32) [concrete] +// CHECK:STDOUT: %ptr.a04: type = ptr_type %B [concrete] +// CHECK:STDOUT: %operator+__carbon_thunk.type: type = fn_type @operator+__carbon_thunk [concrete] +// CHECK:STDOUT: %operator+__carbon_thunk: %operator+__carbon_thunk.type = struct_value () [concrete] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: imports { +// CHECK:STDOUT: %operator+__carbon_thunk.decl: %operator+__carbon_thunk.type = fn_decl @operator+__carbon_thunk [concrete = constants.%operator+__carbon_thunk] { +// CHECK:STDOUT: +// CHECK:STDOUT: } { +// CHECK:STDOUT: +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F(%x.param: %B) -> %i32 { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %x.ref.loc14_10: %B = name_ref x, %x +// CHECK:STDOUT: %x.ref.loc14_14: %B = name_ref x, %x +// CHECK:STDOUT: %.loc14_10: ref %B = value_as_ref %x.ref.loc14_10 +// CHECK:STDOUT: %addr.loc14_12.1: %ptr.a04 = addr_of %.loc14_10 +// CHECK:STDOUT: %.loc14_14: ref %B = value_as_ref %x.ref.loc14_14 +// CHECK:STDOUT: %addr.loc14_12.2: %ptr.a04 = addr_of %.loc14_14 +// CHECK:STDOUT: %operator+__carbon_thunk.call: init %i32 = call imports.%operator+__carbon_thunk.decl(%addr.loc14_12.1, %addr.loc14_12.2) +// CHECK:STDOUT: return %operator+__carbon_thunk.call to %return +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: --- import_overloading.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [concrete] +// CHECK:STDOUT: %C: type = class_type @C [concrete] +// CHECK:STDOUT: %pattern_type.217: type = pattern_type %C [concrete] +// CHECK:STDOUT: %ptr.d9e: type = ptr_type %C [concrete] +// CHECK:STDOUT: %operator+__carbon_thunk.type: type = fn_type @operator+__carbon_thunk [concrete] +// CHECK:STDOUT: %operator+__carbon_thunk: %operator+__carbon_thunk.type = struct_value () [concrete] +// CHECK:STDOUT: %type_where: type = facet_type > [concrete] +// CHECK:STDOUT: %facet_value: %type_where = facet_value %C, () [concrete] +// CHECK:STDOUT: %AggregateT.as_type.as.Destroy.impl.Op.type.fc1: type = fn_type @AggregateT.as_type.as.Destroy.impl.Op, @AggregateT.as_type.as.Destroy.impl(%facet_value) [concrete] +// CHECK:STDOUT: %AggregateT.as_type.as.Destroy.impl.Op.6b9: %AggregateT.as_type.as.Destroy.impl.Op.type.fc1 = struct_value () [concrete] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: imports { +// CHECK:STDOUT: %Cpp: = namespace file.%Cpp.import_cpp, [concrete] { +// CHECK:STDOUT: .C = %C.decl +// CHECK:STDOUT: import Cpp//... +// CHECK:STDOUT: } +// CHECK:STDOUT: %C.decl: type = class_decl @C [concrete = constants.%C] {} {} +// CHECK:STDOUT: %operator+__carbon_thunk.decl: %operator+__carbon_thunk.type = fn_decl @operator+__carbon_thunk [concrete = constants.%operator+__carbon_thunk] { +// CHECK:STDOUT: +// CHECK:STDOUT: } { +// CHECK:STDOUT: +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: +// CHECK:STDOUT: name_binding_decl { +// CHECK:STDOUT: %c3.patt: %pattern_type.217 = binding_pattern c3 [concrete] +// CHECK:STDOUT: } +// CHECK:STDOUT: %c1.ref: %C = name_ref c1, %c1 +// CHECK:STDOUT: %c2.ref: %C = name_ref c2, %c2 +// CHECK:STDOUT: %.loc10_22.1: ref %C = temporary_storage +// CHECK:STDOUT: %.loc10_19: ref %C = value_as_ref %c1.ref +// CHECK:STDOUT: %addr.loc10_22.1: %ptr.d9e = addr_of %.loc10_19 +// CHECK:STDOUT: %.loc10_24: ref %C = value_as_ref %c2.ref +// CHECK:STDOUT: %addr.loc10_22.2: %ptr.d9e = addr_of %.loc10_24 +// CHECK:STDOUT: %addr.loc10_22.3: %ptr.d9e = addr_of %.loc10_22.1 +// CHECK:STDOUT: %operator+__carbon_thunk.call: init %empty_tuple.type = call imports.%operator+__carbon_thunk.decl(%addr.loc10_22.1, %addr.loc10_22.2, %addr.loc10_22.3) +// CHECK:STDOUT: %.loc10_22.2: init %C = in_place_init %operator+__carbon_thunk.call, %.loc10_22.1 +// CHECK:STDOUT: %.loc10_14: type = splice_block %C.ref.loc10 [concrete = constants.%C] { +// CHECK:STDOUT: %Cpp.ref.loc10: = name_ref Cpp, imports.%Cpp [concrete = imports.%Cpp] +// CHECK:STDOUT: %C.ref.loc10: type = name_ref C, imports.%C.decl [concrete = constants.%C] +// CHECK:STDOUT: } +// CHECK:STDOUT: %.loc10_22.3: ref %C = temporary %.loc10_22.1, %.loc10_22.2 +// CHECK:STDOUT: %.loc10_22.4: %C = bind_value %.loc10_22.3 +// CHECK:STDOUT: %c3: %C = bind_name c3, %.loc10_22.4 +// CHECK:STDOUT: %facet_value.loc10: %type_where = facet_value constants.%C, () [concrete = constants.%facet_value] +// CHECK:STDOUT: %.loc10_22.5: %type_where = converted constants.%C, %facet_value.loc10 [concrete = constants.%facet_value] +// CHECK:STDOUT: %AggregateT.as_type.as.Destroy.impl.Op.bound.loc10: = bound_method %.loc10_22.3, constants.%AggregateT.as_type.as.Destroy.impl.Op.6b9 +// CHECK:STDOUT: +// CHECK:STDOUT: %bound_method.loc10: = bound_method %.loc10_22.3, %AggregateT.as_type.as.Destroy.impl.Op.specific_fn.1 +// CHECK:STDOUT: %addr.loc10_22.4: %ptr.d9e = addr_of %.loc10_22.3 +// CHECK:STDOUT: %AggregateT.as_type.as.Destroy.impl.Op.call.loc10: init %empty_tuple.type = call %bound_method.loc10(%addr.loc10_22.4) +// CHECK:STDOUT: +// CHECK:STDOUT: } +// CHECK:STDOUT: diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index ef27e1bf1f40..656f1c3281b3 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -360,7 +360,6 @@ CARBON_DIAGNOSTIC_KIND(QualifiedDeclInUndefinedInterfaceScope) // Name lookup. CARBON_DIAGNOSTIC_KIND(InCppNameLookup) -CARBON_DIAGNOSTIC_KIND(InCppOperatorLookup) CARBON_DIAGNOSTIC_KIND(InNameLookup) CARBON_DIAGNOSTIC_KIND(IncompleteOperandTypeInCppOperatorLookup) CARBON_DIAGNOSTIC_KIND(NameAmbiguousDueToExtend)