From f5a1579d4d69e7adc0cd80fda340eee0c95ef7f8 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 23 Jan 2026 15:07:15 -0800 Subject: [PATCH] Refactor LookupCopyImpl and LookupDestroyImpl to share logic. (#6649) Assisted-by: Google Antigravity with Gemini 3 Flash --- toolchain/check/cpp/impl_lookup.cpp | 127 ++++++++------------ toolchain/check/cpp/operators.cpp | 10 +- toolchain/check/cpp/overload_resolution.cpp | 11 +- toolchain/sem_ir/inst.h | 18 +-- 4 files changed, 70 insertions(+), 96 deletions(-) diff --git a/toolchain/check/cpp/impl_lookup.cpp b/toolchain/check/cpp/impl_lookup.cpp index 87d51b3f21b0..b5a0a9cdb436 100644 --- a/toolchain/check/cpp/impl_lookup.cpp +++ b/toolchain/check/cpp/impl_lookup.cpp @@ -49,70 +49,31 @@ static auto TypeAsClassDecl(Context& context, context.clang_decls().Get(decl_id).key.decl); } -static auto BuildSingleFunctionWitness( - Context& context, SemIR::LocId loc_id, clang::FunctionDecl* cpp_fn, - clang::DeclAccessPair found_decl, int num_params, - SemIR::ConstantId query_self_const_id, - SemIR::SpecificInterfaceId query_specific_interface_id) -> SemIR::InstId { - auto fn_id = context.clang_sema().DiagnoseUseOfOverloadedDecl( - cpp_fn, GetCppLocation(context, loc_id)) - ? SemIR::ErrorInst::InstId - : ImportCppFunctionDecl(context, loc_id, cpp_fn, num_params); - if (auto fn_decl = - context.insts().TryGetAsWithId(fn_id)) { - CheckCppOverloadAccess(context, loc_id, found_decl, fn_decl->inst_id); - } else { - CARBON_CHECK(fn_id == SemIR::ErrorInst::InstId); - return SemIR::ErrorInst::InstId; +namespace { +// See `GetDeclForCoreInterface`. +struct DeclInfo { + clang::NamedDecl* decl; + int num_params; +}; +} // namespace + +// Retrieves a `core_interface`'s corresponding `NamedDecl`, also with the +// expected number of parameters. May return a null decl. +auto GetDeclForCoreInterface(clang::Sema& clang_sema, + CoreInterface core_interface, + clang::CXXRecordDecl* class_decl) -> DeclInfo { + // TODO: Handle other interfaces. + + switch (core_interface) { + case CoreInterface::Copy: + return {.decl = clang_sema.LookupCopyingConstructor( + class_decl, clang::Qualifiers::Const), + .num_params = 1}; + case CoreInterface::Destroy: + return {.decl = clang_sema.LookupDestructor(class_decl), .num_params = 0}; + case CoreInterface::Unknown: + CARBON_FATAL("shouldn't be called with `Unknown`"); } - return BuildCustomWitness(context, loc_id, query_self_const_id, - query_specific_interface_id, {fn_id}); -} - -static auto LookupCopyImpl( - Context& context, SemIR::LocId loc_id, - SemIR::ConstantId query_self_const_id, - SemIR::SpecificInterfaceId query_specific_interface_id) -> SemIR::InstId { - auto* class_decl = TypeAsClassDecl(context, query_self_const_id); - if (!class_decl) { - // TODO: Should we also provide a `Copy` implementation for enumerations? - return SemIR::InstId::None; - } - - auto* ctor = context.clang_sema().LookupCopyingConstructor( - class_decl, clang::Qualifiers::Const); - if (!ctor) { - // TODO: If the impl lookup failure is an error, we should produce a - // diagnostic explaining why the class is not copyable. - return SemIR::InstId::None; - } - - return BuildSingleFunctionWitness( - context, loc_id, ctor, - clang::DeclAccessPair::make(ctor, ctor->getAccess()), /*num_params=*/1, - query_self_const_id, query_specific_interface_id); -} - -static auto LookupDestroyImpl( - Context& context, SemIR::LocId loc_id, - SemIR::ConstantId query_self_const_id, - SemIR::SpecificInterfaceId query_specific_interface_id) -> SemIR::InstId { - auto* class_decl = TypeAsClassDecl(context, query_self_const_id); - if (!class_decl) { - return SemIR::InstId::None; - } - - auto* dtor = context.clang_sema().LookupDestructor(class_decl); - if (!dtor) { - // TODO: If the impl lookup failure is an error, we should produce a - // diagnostic explaining why the class is not destructible. - return SemIR::InstId::None; - } - - return BuildSingleFunctionWitness( - context, loc_id, dtor, - clang::DeclAccessPair::make(dtor, dtor->getAccess()), /*num_params=*/0, - query_self_const_id, query_specific_interface_id); } auto LookupCppImpl(Context& context, SemIR::LocId loc_id, @@ -121,25 +82,41 @@ auto LookupCppImpl(Context& context, SemIR::LocId loc_id, SemIR::SpecificInterfaceId query_specific_interface_id, const TypeStructure* best_impl_type_structure, SemIR::LocId best_impl_loc_id) -> SemIR::InstId { - // TODO: Handle other interfaces. - switch (core_interface) { - case CoreInterface::Copy: - return LookupCopyImpl(context, loc_id, query_self_const_id, - query_specific_interface_id); - case CoreInterface::Destroy: - return LookupDestroyImpl(context, loc_id, query_self_const_id, - query_specific_interface_id); - - case CoreInterface::Unknown: - CARBON_FATAL("shouldn't be called with `Unknown`"); + auto* class_decl = TypeAsClassDecl(context, query_self_const_id); + if (!class_decl) { + return SemIR::InstId::None; } + auto decl_info = + GetDeclForCoreInterface(context.clang_sema(), core_interface, class_decl); + if (!decl_info.decl) { + // TODO: If the impl lookup failure is an error, we should produce a + // diagnostic explaining why the class is not copyable/destructible. + return SemIR::InstId::None; + } + auto* cpp_fn = cast(decl_info.decl); + + if (context.clang_sema().DiagnoseUseOfOverloadedDecl( + cpp_fn, GetCppLocation(context, loc_id))) { + return SemIR::ErrorInst::InstId; + } + + auto fn_id = + ImportCppFunctionDecl(context, loc_id, cpp_fn, decl_info.num_params); + if (fn_id == SemIR::ErrorInst::InstId) { + return SemIR::ErrorInst::InstId; + } + CheckCppOverloadAccess( + context, loc_id, clang::DeclAccessPair::make(cpp_fn, cpp_fn->getAccess()), + context.insts().GetAsKnownInstId(fn_id)); + // TODO: Infer a C++ type structure and check whether it's less strict than // the best Carbon type structure. static_cast(best_impl_type_structure); static_cast(best_impl_loc_id); - return SemIR::InstId::None; + return BuildCustomWitness(context, loc_id, query_self_const_id, + query_specific_interface_id, {fn_id}); } } // namespace Carbon::Check diff --git a/toolchain/check/cpp/operators.cpp b/toolchain/check/cpp/operators.cpp index 64a528884294..6ee07fa8e3c1 100644 --- a/toolchain/check/cpp/operators.cpp +++ b/toolchain/check/cpp/operators.cpp @@ -262,12 +262,10 @@ auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op, // If this is an operator method, the first arg will be used as self. arg_ids.size() - (isa(best_viable_fn->Function) ? 1 : 0)); - if (auto fn_decl = - context.insts().TryGetAsWithId(result_id)) { - CheckCppOverloadAccess(context, loc_id, best_viable_fn->FoundDecl, - fn_decl->inst_id); - } else { - CARBON_CHECK(result_id == SemIR::ErrorInst::InstId); + if (result_id != SemIR::ErrorInst::InstId) { + CheckCppOverloadAccess( + context, loc_id, best_viable_fn->FoundDecl, + context.insts().GetAsKnownInstId(result_id)); } return result_id; } diff --git a/toolchain/check/cpp/overload_resolution.cpp b/toolchain/check/cpp/overload_resolution.cpp index 5ef6bab5627f..034b283c045b 100644 --- a/toolchain/check/cpp/overload_resolution.cpp +++ b/toolchain/check/cpp/overload_resolution.cpp @@ -164,12 +164,11 @@ auto PerformCppOverloadResolution(Context& context, SemIR::LocId loc_id, CARBON_CHECK(!best_viable_fn->RewriteKind); SemIR::InstId result_id = ImportCppFunctionDecl( context, loc_id, best_viable_fn->Function, arg_exprs.size()); - if (auto fn_decl = - context.insts().TryGetAsWithId(result_id)) { - CheckCppOverloadAccess(context, loc_id, best_viable_fn->FoundDecl, - fn_decl->inst_id, overload_set.parent_scope_id); - } else { - CARBON_CHECK(result_id == SemIR::ErrorInst::InstId); + if (result_id != SemIR::ErrorInst::InstId) { + CheckCppOverloadAccess( + context, loc_id, best_viable_fn->FoundDecl, + context.insts().GetAsKnownInstId(result_id), + overload_set.parent_scope_id); } return result_id; } diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index 899d8cab1a0e..f09308e446ed 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -546,21 +546,21 @@ class InstStore { return TryGetAs(inst_id); } + // Returns the `KnownInstId` form of `inst_id`. Requires a matching + // instruction type. + template + auto GetAsKnownInstId(InstId inst_id) const -> KnownInstId { + CARBON_CHECK(Is(inst_id), "Casting inst {0} to wrong kind {1}", + Get(inst_id), Internal::InstLikeTypeInfo::DebugName()); + return KnownInstId::UnsafeMake(inst_id); + } + template struct GetAsWithIdResult { KnownInstId inst_id; InstT inst; }; - // Returns the requested instruction, which is known to have the specified - // type, along with the original `InstId`, encoding the work of checking its - // type in a `KnownInstId`. - template - auto GetAsWithId(InstId inst_id) const -> GetAsWithIdResult { - auto inst = GetAs(inst_id); - return {.inst_id = KnownInstId::UnsafeMake(inst_id), .inst = inst}; - } - // Returns the requested instruction, if it is of that type, along with the // original `InstId`, encoding the work of checking its type in a // `KnownInstId`.