From b34e349792928653fb1c7c4ff340b7443045f4c4 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 18 Dec 2025 07:37:53 -0800 Subject: [PATCH] Push GetFacetAsType from impl_lookup to custom_witness (#6520) This delays the conversion of `query_self_const_id` until the actual witness creation, mainly so that users of `BuildCustomWitness` don't need to do extra work to ensure `GetFacetAsType` logic is shared/applied. This is coming up for destroy logic. --- toolchain/check/cpp/impl_lookup.cpp | 34 ++++++++++++++++------------- toolchain/check/cpp/impl_lookup.h | 3 ++- toolchain/check/custom_witness.cpp | 24 +++++++++++++++++++- toolchain/check/custom_witness.h | 2 +- toolchain/check/impl_lookup.cpp | 26 +++------------------- 5 files changed, 48 insertions(+), 41 deletions(-) diff --git a/toolchain/check/cpp/impl_lookup.cpp b/toolchain/check/cpp/impl_lookup.cpp index 3756f9ba7970..5217562bd5b7 100644 --- a/toolchain/check/cpp/impl_lookup.cpp +++ b/toolchain/check/cpp/impl_lookup.cpp @@ -23,9 +23,11 @@ namespace Carbon::Check { // If the given type is a C++ class type, returns the corresponding class // declaration. Otherwise returns nullptr. // TODO: Handle qualified types. -static auto TypeAsClassDecl(Context& context, SemIR::TypeId type_id) +static auto TypeAsClassDecl(Context& context, + SemIR::ConstantId query_self_const_id) -> clang::CXXRecordDecl* { - auto class_type = context.types().TryGetAs(type_id); + auto self_inst_id = context.constant_values().GetInstId(query_self_const_id); + auto class_type = context.insts().TryGetAs(self_inst_id); if (!class_type) { // Not a class. return nullptr; @@ -50,8 +52,8 @@ static auto TypeAsClassDecl(Context& context, SemIR::TypeId type_id) static auto BuildSingleFunctionWitness( Context& context, SemIR::LocId loc_id, clang::FunctionDecl* cpp_fn, clang::DeclAccessPair found_decl, int num_params, - SemIR::TypeId self_type_id, SemIR::SpecificInterface specific_interface) - -> SemIR::InstId { + SemIR::ConstantId query_self_const_id, + SemIR::SpecificInterface specific_interface) -> SemIR::InstId { auto fn_id = context.clang_sema().DiagnoseUseOfOverloadedDecl( cpp_fn, GetCppLocation(context, loc_id)) ? SemIR::ErrorInst::InstId @@ -63,15 +65,15 @@ static auto BuildSingleFunctionWitness( CARBON_CHECK(fn_id == SemIR::ErrorInst::InstId); return SemIR::ErrorInst::InstId; } - return BuildCustomWitness(context, loc_id, self_type_id, specific_interface, - {fn_id}); + return BuildCustomWitness(context, loc_id, query_self_const_id, + specific_interface, {fn_id}); } static auto LookupCopyImpl(Context& context, SemIR::LocId loc_id, - SemIR::TypeId self_type_id, + SemIR::ConstantId query_self_const_id, SemIR::SpecificInterface specific_interface) -> SemIR::InstId { - auto* class_decl = TypeAsClassDecl(context, self_type_id); + 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; @@ -88,14 +90,14 @@ static auto LookupCopyImpl(Context& context, SemIR::LocId loc_id, return BuildSingleFunctionWitness( context, loc_id, ctor, clang::DeclAccessPair::make(ctor, ctor->getAccess()), /*num_params=*/1, - self_type_id, specific_interface); + query_self_const_id, specific_interface); } static auto LookupDestroyImpl(Context& context, SemIR::LocId loc_id, - SemIR::TypeId self_type_id, + SemIR::ConstantId query_self_const_id, SemIR::SpecificInterface specific_interface) -> SemIR::InstId { - auto* class_decl = TypeAsClassDecl(context, self_type_id); + auto* class_decl = TypeAsClassDecl(context, query_self_const_id); if (!class_decl) { return SemIR::InstId::None; } @@ -110,20 +112,22 @@ static auto LookupDestroyImpl(Context& context, SemIR::LocId loc_id, return BuildSingleFunctionWitness( context, loc_id, dtor, clang::DeclAccessPair::make(dtor, dtor->getAccess()), /*num_params=*/0, - self_type_id, specific_interface); + query_self_const_id, specific_interface); } auto LookupCppImpl(Context& context, SemIR::LocId loc_id, - SemIR::TypeId self_type_id, CoreInterface core_interface, + CoreInterface core_interface, + SemIR::ConstantId query_self_const_id, SemIR::SpecificInterface specific_interface, 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, self_type_id, specific_interface); + return LookupCopyImpl(context, loc_id, query_self_const_id, + specific_interface); case CoreInterface::Destroy: - return LookupDestroyImpl(context, loc_id, self_type_id, + return LookupDestroyImpl(context, loc_id, query_self_const_id, specific_interface); case CoreInterface::Unknown: diff --git a/toolchain/check/cpp/impl_lookup.h b/toolchain/check/cpp/impl_lookup.h index bee21a0d8c9d..8c5d36435c8a 100644 --- a/toolchain/check/cpp/impl_lookup.h +++ b/toolchain/check/cpp/impl_lookup.h @@ -35,7 +35,8 @@ namespace Carbon::Check { // type structure, and can be `None` if `best_impl_type_structure` is null. This // parameter is used only for ambiguity diagnostics. auto LookupCppImpl(Context& context, SemIR::LocId loc_id, - SemIR::TypeId self_type_id, CoreInterface core_interface, + CoreInterface core_interface, + SemIR::ConstantId query_self_const_id, SemIR::SpecificInterface specific_interface, const TypeStructure* best_impl_type_structure, SemIR::LocId best_impl_loc_id) -> SemIR::InstId; diff --git a/toolchain/check/custom_witness.cpp b/toolchain/check/custom_witness.cpp index 4039361d3f68..714ba766042a 100644 --- a/toolchain/check/custom_witness.cpp +++ b/toolchain/check/custom_witness.cpp @@ -12,8 +12,28 @@ namespace Carbon::Check { +// Given a value whose type `IsFacetTypeOrError`, returns the corresponding +// type. +static auto GetFacetAsType(Context& context, SemIR::LocId loc_id, + SemIR::ConstantId facet_or_type_const_id) + -> SemIR::TypeId { + auto facet_or_type_id = + context.constant_values().GetInstId(facet_or_type_const_id); + auto type_type_id = context.insts().Get(facet_or_type_id).type_id(); + CARBON_CHECK(context.types().IsFacetTypeOrError(type_type_id)); + + if (context.types().Is(type_type_id)) { + // It's a facet; access its type. + facet_or_type_id = GetOrAddInst( + context, loc_id, + {.type_id = SemIR::TypeType::TypeId, + .facet_value_inst_id = facet_or_type_id}); + } + return context.types().GetTypeIdForTypeInstId(facet_or_type_id); +} + auto BuildCustomWitness(Context& context, SemIR::LocId loc_id, - SemIR::TypeId self_type_id, + SemIR::ConstantId query_self_const_id, SemIR::SpecificInterface specific_interface, llvm::ArrayRef values) -> SemIR::InstId { const auto& interface = @@ -64,6 +84,8 @@ auto BuildCustomWitness(Context& context, SemIR::LocId loc_id, if (struct_value.type_id == SemIR::ErrorInst::TypeId) { return SemIR::ErrorInst::InstId; } + auto self_type_id = + GetFacetAsType(context, loc_id, query_self_const_id); // TODO: If a thunk is needed, this will build a different value each // time it's called, so we won't properly deduplicate repeated // witnesses. diff --git a/toolchain/check/custom_witness.h b/toolchain/check/custom_witness.h index 2b08ba7788c1..28157e02f92b 100644 --- a/toolchain/check/custom_witness.h +++ b/toolchain/check/custom_witness.h @@ -15,7 +15,7 @@ namespace Carbon::Check { // lookup result. Produces a diagnostic and returns `None` if the specified // values aren't suitable for the interface. auto BuildCustomWitness(Context& context, SemIR::LocId loc_id, - SemIR::TypeId self_type_id, + SemIR::ConstantId query_self_const_id, SemIR::SpecificInterface specific_interface, llvm::ArrayRef values) -> SemIR::InstId; diff --git a/toolchain/check/impl_lookup.cpp b/toolchain/check/impl_lookup.cpp index cdb053990e3c..6c6365d00852 100644 --- a/toolchain/check/impl_lookup.cpp +++ b/toolchain/check/impl_lookup.cpp @@ -896,26 +896,6 @@ static auto CollectCandidateImplsForQuery( return candidates; } -// Given a value whose type `IsFacetTypeOrError`, returns the corresponding -// type. -static auto GetFacetAsType(Context& context, SemIR::LocId loc_id, - SemIR::ConstantId facet_or_type_const_id) - -> SemIR::TypeId { - auto facet_or_type_id = - context.constant_values().GetInstId(facet_or_type_const_id); - auto type_type_id = context.insts().Get(facet_or_type_id).type_id(); - CARBON_CHECK(context.types().IsFacetTypeOrError(type_type_id)); - - if (context.types().Is(type_type_id)) { - // It's a facet; access its type. - facet_or_type_id = GetOrAddInst( - context, loc_id, - {.type_id = SemIR::TypeType::TypeId, - .facet_value_inst_id = facet_or_type_id}); - } - return context.types().GetTypeIdForTypeInstId(facet_or_type_id); -} - // Record the query which found a final impl witness. It's illegal to // write a final impl afterward that would match the same query. static auto PoisonImplLookupQuery(Context& context, SemIR::LocId loc_id, @@ -1060,9 +1040,9 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, // Also check for a C++ candidate that is a better match than whatever // `impl` we may have found in Carbon. auto cpp_witness_id = LookupCppImpl( - context, loc_id, GetFacetAsType(context, loc_id, query_self_const_id), - core_interface, query_specific_interface, - lookup_result.impl_type_structure, lookup_result.impl_loc_id); + context, loc_id, core_interface, query_self_const_id, + query_specific_interface, lookup_result.impl_type_structure, + lookup_result.impl_loc_id); if (cpp_witness_id.has_value()) { lookup_result = {.result = EvalImplLookupResult::MakeFinal(cpp_witness_id)};