From 463ba0e1db3f86b5423e5ba494259ea8d57946c0 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Thu, 11 Dec 2025 17:22:43 -0500 Subject: [PATCH] Clean up helpers for GetOrAddImpl (Refactor Impl construction 5/7 and 6/7) (#6469) Make the `AssignImplIdInWitness` into a `static` helper function since it's only used inside `GetOrAddImpl`. Restructure the diagnostic for unused generic bindings to move more logic into the helper, and out of `GetOrAddImpl` so that it has more clear steps. This is part of #6420 which is being split up into a chain of smaller PRs. It is based on #6468. --- toolchain/check/impl.cpp | 141 +++++++++++++++++++-------------------- toolchain/check/impl.h | 4 -- 2 files changed, 69 insertions(+), 76 deletions(-) diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index 3c3e5839d76e..379d55a8309b 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -262,22 +262,6 @@ auto FillImplWitnessWithErrors(Context& context, SemIR::Impl& impl) -> void { impl.witness_id = SemIR::ErrorInst::InstId; } -auto AssignImplIdInWitness(Context& context, SemIR::ImplId impl_id, - SemIR::InstId witness_id) -> void { - if (witness_id == SemIR::ErrorInst::InstId) { - return; - } - auto witness = context.insts().GetAs(witness_id); - auto witness_table = - context.insts().GetAs(witness.witness_table_id); - witness_table.impl_id = impl_id; - // Note: The `ImplWitnessTable` instruction is `Unique`, so while this marks - // the instruction as being a dependent instruction of a generic impl, it will - // not be substituted into the eval block. - ReplaceInstBeforeConstantUse(context, witness.witness_table_id, - witness_table); -} - auto IsImplEffectivelyFinal(Context& context, const SemIR::Impl& impl) -> bool { return impl.is_final || (context.constant_values().Get(impl.self_id).is_concrete() && @@ -379,6 +363,65 @@ static auto IsValidImplRedecl(Context& context, SemIR::Impl& new_impl, return true; } +// Sets the `ImplId` in the `ImplWitnessTable`. +static auto AssignImplIdInWitness(Context& context, SemIR::ImplId impl_id, + SemIR::InstId witness_id) -> void { + if (witness_id == SemIR::ErrorInst::InstId) { + return; + } + auto witness = context.insts().GetAs(witness_id); + auto witness_table = + context.insts().GetAs(witness.witness_table_id); + witness_table.impl_id = impl_id; + // Note: The `ImplWitnessTable` instruction is `Unique`, so while this marks + // the instruction as being a dependent instruction of a generic impl, it will + // not be substituted into the eval block. + ReplaceInstBeforeConstantUse(context, witness.witness_table_id, + witness_table); +} + +// Looks for any unused generic bindings. If one is found, it is diagnosed and +// false is returned. +static auto VerifyAllGenericBindingsUsed(Context& context, SemIR::LocId loc_id, + SemIR::LocId implicit_params_loc_id, + SemIR::Impl& impl) -> bool { + if (impl.witness_id == SemIR::ErrorInst::InstId) { + return true; + } + if (!impl.generic_id.has_value()) { + return true; + } + + if (impl.implicit_param_patterns_id.has_value()) { + for (auto inst_id : + context.inst_blocks().Get(impl.implicit_param_patterns_id)) { + if (inst_id == SemIR::ErrorInst::InstId) { + // An error was already diagnosed for a generic binding. + return true; + } + } + } + + auto deduced_specific_id = DeduceImplArguments( + context, loc_id, impl, context.constant_values().Get(impl.self_id), + impl.interface.specific_id); + if (deduced_specific_id.has_value()) { + // Deduction succeeded, all bindings were used. + return true; + } + + CARBON_DIAGNOSTIC(ImplUnusedBinding, Error, + "`impl` with unused generic binding"); + // TODO: This location may be incorrect, the binding may be inherited + // from an outer declaration. It would be nice to get the particular + // binding that was undeducible back from DeduceImplArguments here and + // use that. + auto diag_loc_id = + implicit_params_loc_id.has_value() ? implicit_params_loc_id : loc_id; + context.emitter().Emit(diag_loc_id, ImplUnusedBinding); + return false; +} + // Apply an `extend impl` declaration by extending the parent scope with the // `impl`. If there's an error it is diagnosed and false is returned. static auto ApplyExtendImplAs(Context& context, SemIR::LocId loc_id, @@ -443,38 +486,6 @@ static auto ApplyExtendImplAs(Context& context, SemIR::LocId loc_id, return true; } -// Diagnoses when an impl has an unused binding. -static auto DiagnoseUnusedGenericBinding(Context& context, SemIR::LocId loc_id, - SemIR::LocId implicit_params_loc_id, - SemIR::ImplId impl_id) -> void { - auto& impl = context.impls().Get(impl_id); - if (!impl.generic_id.has_value() || - impl.witness_id == SemIR::ErrorInst::InstId) { - return; - } - - auto deduced_specific_id = DeduceImplArguments( - context, loc_id, impl, context.constant_values().Get(impl.self_id), - impl.interface.specific_id); - if (deduced_specific_id.has_value()) { - // Deduction succeeded, all bindings were used. - return; - } - - CARBON_DIAGNOSTIC(ImplUnusedBinding, Error, - "`impl` with unused generic binding"); - // TODO: This location may be incorrect, the binding may be inherited - // from an outer declaration. It would be nice to get the particular - // binding that was undeducible back from DeduceImplArguments here and - // use that. - auto diag_loc_id = - implicit_params_loc_id.has_value() ? implicit_params_loc_id : loc_id; - context.emitter().Emit(diag_loc_id, ImplUnusedBinding); - // Don't try to match the impl at all, save us work and possible future - // diagnostics. - FillImplWitnessWithErrors(context, context.impls().Get(impl_id)); -} - auto GetOrAddImpl(Context& context, SemIR::LocId loc_id, SemIR::LocId implicit_params_loc_id, SemIR::Impl impl, bool is_definition, Parse::NodeId extend_node) @@ -535,37 +546,23 @@ auto GetOrAddImpl(Context& context, SemIR::LocId loc_id, // in order to make and see any changes to the `Impl`. impl_id = context.impls().Add(impl); lookup_bucket_ref.push_back(impl_id); - AssignImplIdInWitness(context, impl_id, impl.witness_id); - auto& stored_impl_info = context.impls().Get(impl_id); + auto& stored_impl = context.impls().Get(impl_id); - // Looking to see if there are any generic bindings on the `impl` - // declaration that are not deducible. If so, and the `impl` does not - // actually use all its generic bindings, and will never be matched. This - // should be diagnossed to the user. - bool has_error_in_implicit_pattern = false; - if (impl.implicit_param_patterns_id.has_value()) { - for (auto inst_id : - context.inst_blocks().Get(impl.implicit_param_patterns_id)) { - if (inst_id == SemIR::ErrorInst::InstId) { - has_error_in_implicit_pattern = true; - break; - } - } + // Look to see if there are any generic bindings on the `impl` declaration + // that are not deducible. If so, and the `impl` does not actually use all its + // generic bindings, and will never be matched. This should be diagnosed to + // the user. + if (!VerifyAllGenericBindingsUsed(context, loc_id, implicit_params_loc_id, + stored_impl)) { + FillImplWitnessWithErrors(context, stored_impl); } - if (!has_error_in_implicit_pattern) { - DiagnoseUnusedGenericBinding(context, loc_id, implicit_params_loc_id, - impl_id); - } - - // For an `extend impl` declaration, mark the impl as extending this `impl`. if (extend_node.has_value()) { - if (!ApplyExtendImplAs(context, loc_id, stored_impl_info, extend_node, + if (!ApplyExtendImplAs(context, loc_id, stored_impl, extend_node, implicit_params_loc_id)) { - // Don't allow the invalid impl to be used. - FillImplWitnessWithErrors(context, stored_impl_info); + FillImplWitnessWithErrors(context, stored_impl); } } diff --git a/toolchain/check/impl.h b/toolchain/check/impl.h index 6cbb50538d96..2dac624b1586 100644 --- a/toolchain/check/impl.h +++ b/toolchain/check/impl.h @@ -27,10 +27,6 @@ auto FinishImplWitness(Context& context, SemIR::ImplId impl_id) -> void; // sets the witness id in the `Impl` to an error. auto FillImplWitnessWithErrors(Context& context, SemIR::Impl& impl) -> void; -// Sets the `ImplId` in the `ImplWitnessTable`. -auto AssignImplIdInWitness(Context& context, SemIR::ImplId impl_id, - SemIR::InstId witness_id) -> void; - // Returns whether the impl is either `final` explicitly, or implicitly due to // being concrete. auto IsImplEffectivelyFinal(Context& context, const SemIR::Impl& impl) -> bool;