From b07b8a122a86cce6b6cf092f6b88a10e38622bba Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Tue, 9 Dec 2025 17:02:50 -0500 Subject: [PATCH] Make StartImplDecl into a more explicit GetOrAddImpl (Refactor Impl construction 2/7) (#6466) The GetOrAddImpl operation looks for an existing `Impl` with a matching declaration and returns its ImplId, or finished the construction of a new `Impl`, adds it to the store and returns a fresh `ImplId`. This makes the case of reusing an existing `Impl` into a short early-out, demonstrating more clearly that we are reusing existing work, and avoiding duplicate work such as checking for diagnostics that would have already been checked in the previous (matching) declaration. The `ExtendImpl` helper is renamed to be more explicit about its behaviour, as `ApplyExtendImplAs`, and it constructs the data it needs from the `Impl` and the `extend_node_id`, eliminating the need for a `ExtendImplDecl` struct. This is part of #6420 which is being split up into a chain of smaller PRs. It is based on #6465. --- toolchain/check/handle_impl.cpp | 44 +++++----- toolchain/check/impl.cpp | 142 ++++++++++++++++---------------- toolchain/check/impl.h | 26 ++---- 3 files changed, 104 insertions(+), 108 deletions(-) diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 99b6c76dd951..11d443faae8a 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -228,28 +228,34 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id, SemIR::ImplDecl{.impl_id = SemIR::ImplId::None, .decl_block_id = decl_block_id}); - SemIR::Impl impl_info = {name_context.MakeEntityWithParamsBase( - name, impl_decl_id, - /*is_extern=*/false, SemIR::LibraryNameId::None), - {.self_id = self_type_inst_id, - .constraint_id = constraint_type_inst_id, - // This requires that the facet type is identified. - .interface = CheckConstraintIsInterface( - context, impl_decl_id, constraint_type_inst_id), - .is_final = is_final}}; + // This requires that the facet type is identified. It returns None if an + // error was diagnosed. + auto specific_interface = CheckConstraintIsInterface(context, impl_decl_id, + constraint_type_inst_id); - std::optional extend_impl; - if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) { - extend_impl = ExtendImplDecl{ - .self_type_node_id = self_type_node, - .constraint_type_id = constraint_type_id, - .extend_node_id = introducer.modifier_node_id(ModifierOrder::Extend), - }; + auto impl_id = SemIR::ImplId::None; + { + SemIR::Impl impl_info = { + name_context.MakeEntityWithParamsBase(name, impl_decl_id, + /*is_extern=*/false, + SemIR::LibraryNameId::None), + {.self_id = self_type_inst_id, + .constraint_id = constraint_type_inst_id, + .interface = specific_interface, + .is_final = is_final}}; + auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend); + impl_id = GetOrAddImpl(context, node_id, name.implicit_params_loc_id, + impl_info, is_definition, extend_node); } - return StartImplDecl(context, SemIR::LocId(node_id), - name.implicit_params_loc_id, impl_info, is_definition, - extend_impl); + // `GetOrAddImpl` either filled in the `impl_info` and returned a fresh + // ImplId, or if we're redeclaring a previous impl, returned an existing + // ImplId. Write that ImplId into the ImplDecl instruction and finish it. + auto impl_decl = context.insts().GetAs(impl_decl_id); + impl_decl.impl_id = impl_id; + ReplaceInstBeforeConstantUse(context, impl_decl_id, impl_decl); + + return {impl_id, impl_decl_id}; } auto HandleParseNode(Context& context, Parse::ImplDeclId node_id) -> bool { diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index 4ca196d4f1d7..901063cfed47 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -399,13 +399,13 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id) return context.insts().TryGetAs(scope.inst_id()); } -// Process an `extend impl` declaration by extending the impl scope with the -// `impl`'s scope. -static auto ExtendImpl(Context& context, Parse::NodeId extend_node, - SemIR::LocId loc_id, SemIR::ImplId impl_id, - SemIR::LocId implicit_params_loc_id, - SemIR::TypeInstId constraint_type_inst_id, - SemIR::TypeId constraint_type_id) -> bool { +// 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, Parse::NodeId extend_node, + SemIR::LocId loc_id, SemIR::ImplId impl_id, + SemIR::LocId implicit_params_loc_id, + SemIR::TypeInstId constraint_type_inst_id) + -> bool { auto parent_scope_id = context.decl_name_stack().PeekParentScopeId(); if (!parent_scope_id.has_value()) { DiagnoseExtendImplOutsideClass(context, loc_id); @@ -432,6 +432,8 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node, if (impl.witness_id == SemIR::ErrorInst::InstId) { parent_scope.set_has_error(); } else { + auto constraint_type_id = + context.types().GetTypeIdForTypeInstId(constraint_type_inst_id); bool is_complete = RequireCompleteType( context, constraint_type_id, SemIR::LocId(constraint_type_inst_id), [&] { @@ -484,11 +486,10 @@ static auto DiagnoseUnusedGenericBinding(Context& context, SemIR::LocId loc_id, FillImplWitnessWithErrors(context, context.impls().Get(impl_id)); } -auto StartImplDecl(Context& context, SemIR::LocId loc_id, - SemIR::LocId implicit_params_loc_id, SemIR::Impl impl, - bool is_definition, - std::optional extend_impl) - -> std::pair { +auto GetOrAddImpl(Context& context, SemIR::LocId loc_id, + SemIR::LocId implicit_params_loc_id, SemIR::Impl impl, + bool is_definition, Parse::NodeId extend_node) + -> SemIR::ImplId { auto impl_id = SemIR::ImplId::None; // Add the impl declaration. @@ -500,72 +501,70 @@ auto StartImplDecl(Context& context, SemIR::LocId loc_id, if (IsValidImplRedecl(context, impl, prev_impl_id)) { impl_id = prev_impl_id; } else { - // IsValidImplRedecl() has issued a diagnostic, avoid generating more - // diagnostics for this declaration. + // IsValidImplRedecl() has issued a diagnostic, take care to avoid + // generating more diagnostics for this declaration. impl.witness_id = SemIR::ErrorInst::InstId; } break; } } - // Create a new impl if this isn't a valid redeclaration. - if (!impl_id.has_value()) { - impl.generic_id = BuildGeneric(context, impl.latest_decl_id()); - if (impl.witness_id != SemIR::ErrorInst::InstId) { - if (impl.interface.interface_id.has_value()) { - impl.witness_id = - ImplWitnessForDeclaration(context, impl, is_definition); - } else { - impl.witness_id = SemIR::ErrorInst::InstId; - // TODO: We might also want to mark that the name scope for the impl has - // an error -- at least once we start making name lookups within the - // impl also look into the facet (eg, so you can name associated - // constants from within the impl). - } - } - FinishGenericDecl(context, SemIR::LocId(impl.latest_decl_id()), - impl.generic_id); - // From here on, use the `Impl` from the `ImplStore` instead of `impl` - // 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); - - // 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; - } - } - } - - if (!has_error_in_implicit_pattern) { - DiagnoseUnusedGenericBinding(context, loc_id, implicit_params_loc_id, - impl_id); - } - } else { - auto& stored_impl = context.impls().Get(impl_id); - FinishGenericRedecl(context, stored_impl.generic_id); + if (impl_id.has_value()) { + // This is a redeclaration of another impl, now held in `impl_id`. + auto& prev_impl = context.impls().Get(impl_id); + FinishGenericRedecl(context, prev_impl.generic_id); + return impl_id; } - // Write the impl ID into the ImplDecl. - auto impl_decl = - context.insts().GetAs(impl.first_owning_decl_id); - CARBON_CHECK(!impl_decl.impl_id.has_value()); - impl_decl.impl_id = impl_id; - ReplaceInstBeforeConstantUse(context, impl.first_owning_decl_id, impl_decl); + // This is a new declaration (possibly with an attached definition). Create a + // new `impl_id`, filling the missing generic and witness in the provided + // `Impl`. + + impl.generic_id = BuildGeneric(context, impl.latest_decl_id()); + if (impl.witness_id != SemIR::ErrorInst::InstId) { + if (impl.interface.interface_id.has_value()) { + impl.witness_id = ImplWitnessForDeclaration(context, impl, is_definition); + } else { + impl.witness_id = SemIR::ErrorInst::InstId; + // TODO: We might also want to mark that the name scope for the impl has + // an error -- at least once we start making name lookups within the + // impl also look into the facet (eg, so you can name associated + // constants from within the impl). + } + } + FinishGenericDecl(context, SemIR::LocId(impl.latest_decl_id()), + impl.generic_id); + // From here on, use the `Impl` from the `ImplStore` instead of `impl` + // 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); + + // 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; + } + } + } + + 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_impl) { - auto& stored_impl_info = context.impls().Get(impl_decl.impl_id); + if (extend_node.has_value()) { auto self_type_id = context.types().GetTypeIdForTypeInstId(stored_impl_info.self_id); if (self_type_id != SemIR::ErrorInst::TypeId) { @@ -578,9 +577,8 @@ auto StartImplDecl(Context& context, SemIR::LocId loc_id, .specific_id = context.generics().GetSelfSpecific( stored_impl_info.generic_id)}); } - if (!ExtendImpl(context, extend_impl->extend_node_id, loc_id, - impl_decl.impl_id, implicit_params_loc_id, constraint_id, - extend_impl->constraint_type_id)) { + if (!ApplyExtendImplAs(context, extend_node, loc_id, impl_id, + implicit_params_loc_id, constraint_id)) { // Don't allow the invalid impl to be used. FillImplWitnessWithErrors(context, stored_impl_info); } @@ -598,7 +596,7 @@ auto StartImplDecl(Context& context, SemIR::LocId loc_id, } } - return {impl_id, impl.latest_decl_id()}; + return impl_id; } } // namespace Carbon::Check diff --git a/toolchain/check/impl.h b/toolchain/check/impl.h index e3e7a9f06c33..6cbb50538d96 100644 --- a/toolchain/check/impl.h +++ b/toolchain/check/impl.h @@ -52,24 +52,16 @@ auto CheckConstraintIsInterface(Context& context, SemIR::InstId impl_decl_id, SemIR::TypeInstId constraint_id) -> SemIR::SpecificInterface; -// For `StartImplDecl`, additional details for an `extend impl` declaration. -struct ExtendImplDecl { - Parse::NodeId self_type_node_id; - SemIR::TypeId constraint_type_id; - Parse::NodeId extend_node_id; -}; - -// Starts an impl declaration. The caller is responsible for ensuring a generic -// declaration has been started. This returns the produced `ImplId` and -// `ImplDecl`'s `InstId`. +// Finds an existing `Impl` if the `impl` is a redeclaration. Otherwise, +// finishes construction of the `impl`, adds it to the ImplStore, and returns +// the new `ImplId`. This ensures all redeclarations share the same `ImplId`. // -// The `impl` should be constructed with a placeholder `ImplDecl` which this -// will add the `ImplId` to. -auto StartImplDecl(Context& context, SemIR::LocId loc_id, - SemIR::LocId implicit_params_loc_id, SemIR::Impl impl, - bool is_definition, - std::optional extend_impl) - -> std::pair; +// If the impl is modified with `extend` then the parent's scope is extended +// with it. +auto GetOrAddImpl(Context& context, SemIR::LocId loc_id, + SemIR::LocId implicit_params_loc_id, SemIR::Impl impl, + bool is_definition, Parse::NodeId extend_node) + -> SemIR::ImplId; } // namespace Carbon::Check