From 1ce26589641054981a61fe3cf711f9f5ceeb6a97 Mon Sep 17 00:00:00 2001 From: Josh L Date: Wed, 8 Jan 2025 02:05:59 +0000 Subject: [PATCH] Save `WhereOperand` node instead of performing a scan --- toolchain/check/decl_name_stack.cpp | 4 ++ toolchain/check/decl_name_stack.h | 8 +++ toolchain/check/handle_impl.cpp | 56 ++++++------------- toolchain/check/handle_where.cpp | 10 +++- .../impl/no_prelude/impl_where_redecl.carbon | 4 ++ 5 files changed, 42 insertions(+), 40 deletions(-) diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index ca0407c84fa71..18b0219a5bf69 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/toolchain/check/decl_name_stack.cpp @@ -81,6 +81,10 @@ auto DeclNameStack::FinishImplName() -> NameContext { return result; } +auto DeclNameStack::SetImplWhereOperandNodeId(Parse::NodeId node_id) -> void { + decl_name_stack_.back().where_operand_node_id = node_id; +} + auto DeclNameStack::PopScope() -> void { CARBON_CHECK(decl_name_stack_.back().state == NameContext::State::Finished, "Missing call to FinishName before PopScope"); diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h index d985e90fde478..f5496e839850e 100644 --- a/toolchain/check/decl_name_stack.h +++ b/toolchain/check/decl_name_stack.h @@ -151,6 +151,11 @@ class DeclNameStack { // The ID of an unresolved identifier. SemIR::NameId unresolved_name_id = SemIR::NameId::Invalid; }; + + // The `WhereOperand` node_id for the constraint in an impl decl, if any. + // FIXME: Can this be in a union with some other field that isn't used for + // impl decls? + Parse::NodeId where_operand_node_id = Parse::NodeId::Invalid; }; // Information about a declaration name that has been temporarily removed from @@ -200,6 +205,9 @@ class DeclNameStack { // logic such as building parameter scopes, so are a special case. auto FinishImplName() -> NameContext; + // Set the position of the `WhereOperand` node in an `impl...as...where...`. + auto SetImplWhereOperandNodeId(Parse::NodeId node_id) -> void; + // Pops the declaration name from the declaration name stack, and pops all // scopes that were entered as part of handling the declaration name. These // are the scopes corresponding to name qualifiers in the name, for example diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 30367ceaa7e09..05975fd0ef926 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -191,8 +191,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node, // Pops the parameters of an `impl`, forming a `NameComponent` with no // associated name that describes them. static auto PopImplIntroducerAndParamsAsNameComponent( - Context& context, Parse::AnyImplDeclId end_of_decl_node_id) - -> NameComponent { + Context& context, Parse::NodeId end_of_decl_node_id) -> NameComponent { auto [implicit_params_loc_id, implicit_param_patterns_id] = context.node_stack().PopWithNodeIdIf(); @@ -213,39 +212,15 @@ static auto PopImplIntroducerAndParamsAsNameComponent( Parse::NodeId first_param_node_id = context.node_stack().PopForSoloNodeId(); - // Subtracting 1 since we don't want to include the final `{` or `;` of the - // declaration when performing syntactic match. - auto end_node_kind = context.parse_tree().node_kind(end_of_decl_node_id); - CARBON_CHECK(end_node_kind == Parse::NodeKind::ImplDefinitionStart || - end_node_kind == Parse::NodeKind::ImplDecl); + // Subtracting 1 since we don't want to include the final `{`, `;`, or `where` + // of the declaration when performing syntactic match. + auto node_kind = context.parse_tree().node_kind(end_of_decl_node_id); + CARBON_CHECK(node_kind == Parse::NodeKind::ImplDefinitionStart || + node_kind == Parse::NodeKind::ImplDecl || + node_kind == Parse::NodeKind::WhereOperand); Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id); --last_param_iter; - // Following proposal #3763, exclude a final `where` clause, if present. See: - // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations - - // Caches the NodeKind for the current value of *last_param_iter so - if (context.parse_tree().node_kind(*last_param_iter) == - Parse::NodeKind::WhereExpr) { - int where_operands_to_skip = 1; - --last_param_iter; - CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) < - last_param_iter); - do { - auto node_kind = context.parse_tree().node_kind(*last_param_iter); - if (node_kind == Parse::NodeKind::WhereExpr) { - // If we have a nested `where`, we need to see another `WhereOperand` - // before we find the one that matches our original `WhereExpr` node. - ++where_operands_to_skip; - } else if (node_kind == Parse::NodeKind::WhereOperand) { - --where_operands_to_skip; - } - --last_param_iter; - CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) < - last_param_iter); - } while (where_operands_to_skip > 0); - } - return { .name_loc_id = Parse::NodeId::Invalid, .name_id = SemIR::NameId::Invalid, @@ -313,8 +288,18 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id, auto [self_type_node, self_inst_id] = context.node_stack().PopWithNodeId(); auto self_type_id = context.GetTypeIdForTypeInst(self_inst_id); + + // Finish processing the name, which should be empty, but might have + // parameters. + auto name_context = context.decl_name_stack().FinishImplName(); + CARBON_CHECK(name_context.state == DeclNameStack::NameContext::State::Empty); + // Pop the `impl` introducer and any `forall` parameters as a "name". - auto name = PopImplIntroducerAndParamsAsNameComponent(context, node_id); + // Stop at the `where` node, if present, to exclude it from syntactic match. + auto name = PopImplIntroducerAndParamsAsNameComponent( + context, name_context.where_operand_node_id.is_valid() + ? name_context.where_operand_node_id + : node_id); auto decl_block_id = context.inst_block_stack().Pop(); // Convert the constraint expression to a type. @@ -334,11 +319,6 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id, context.decl_introducer_state_stack().Pop(); LimitModifiersOnDecl(context, introducer, KeywordModifierSet::ImplDecl); - // Finish processing the name, which should be empty, but might have - // parameters. - auto name_context = context.decl_name_stack().FinishImplName(); - CARBON_CHECK(name_context.state == DeclNameStack::NameContext::State::Empty); - // TODO: Check for an orphan `impl`. // Add the impl declaration. diff --git a/toolchain/check/handle_where.cpp b/toolchain/check/handle_where.cpp index 6de36ed159021..0ad0d67b25bdc 100644 --- a/toolchain/check/handle_where.cpp +++ b/toolchain/check/handle_where.cpp @@ -122,9 +122,15 @@ auto HandleParseNode(Context& context, Parse::WhereExprId node_id) -> bool { // Remove `PeriodSelf` from name lookup, undoing the `Push` done for the // `WhereOperand`. context.scope_stack().Pop(); - SemIR::InstId period_self_id = - context.node_stack().Pop(); + auto [operand_node_id, period_self_id] = + context.node_stack().PopWithNodeId(); SemIR::InstBlockId requirements_id = context.args_type_info_stack().Pop(); + if (context.node_stack().PeekIs()) { + // In the `impl ... as ... where ...` case, save the `WhereOperand` node in + // the impl declaration so that part can be trimmed off for syntactic decl + // matching. + context.decl_name_stack().SetImplWhereOperandNodeId(operand_node_id); + } context.AddInstAndPush( node_id, {.type_id = SemIR::TypeType::SingletonTypeId, .period_self_id = period_self_id, diff --git a/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon b/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon index c59c393f64714..05c81c7c68f7b 100644 --- a/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon +++ b/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon @@ -32,6 +32,10 @@ interface K {} // `impl` matching only ignores a root-level `where` clause. +// CHECK:STDERR: parens_other_nesting.carbon:[[@LINE+4]]:1: error: impl declared but not defined [MissingImplDefinition] +// CHECK:STDERR: impl {} as (K where .Self impls type) where .Self impls type; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// CHECK:STDERR: impl {} as (K where .Self impls type) where .Self impls type; impl {} as (K where .Self impls type) {}