From f502e8d6fff9374d690b43f26ff8e5e1a1e92978 Mon Sep 17 00:00:00 2001 From: Geoff Romer Date: Wed, 12 Feb 2025 15:21:57 -0800 Subject: [PATCH] Avoid speculatively pushing a pattern block in impl handling (#4943) To do this, we restructure the parse tree to make `forall` a leaf node that comes before the parameter list. --------- Co-authored-by: Jon Ross-Perkins --- toolchain/check/handle_impl.cpp | 27 ++++---- toolchain/check/node_stack.h | 2 +- toolchain/parse/handle_impl.cpp | 3 +- toolchain/parse/node_kind.def | 2 +- .../testdata/generics/impl/fail_impl.carbon | 62 +++++++++---------- .../testdata/generics/impl/forall.carbon | 32 +++++----- toolchain/parse/typed_nodes.h | 9 +-- 7 files changed, 67 insertions(+), 70 deletions(-) diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 14c2a4796d12..31571baa82b6 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -38,22 +38,14 @@ auto HandleParseNode(Context& context, Parse::ImplIntroducerId node_id) // This might be a generic impl. StartGenericDecl(context); - - // Push a pattern block for the signature of the `forall` (if any). - // TODO: Instead use a separate parse node kinds for `impl` and `impl forall`, - // and only push a pattern block in `forall` case. - context.pattern_block_stack().Push(); - context.full_pattern_stack().PushFullPattern( - FullPatternStack::Kind::ImplicitParamList); return true; } -auto HandleParseNode(Context& context, Parse::ImplForallId node_id) -> bool { - auto params_id = - context.node_stack().Pop(); - context.node_stack() - .PopAndDiscardSoloNodeId(); - context.node_stack().Push(node_id, params_id); +auto HandleParseNode(Context& context, Parse::ForallId /*node_id*/) -> bool { + // Push a pattern block for the signature of the `forall`. + context.pattern_block_stack().Push(); + context.full_pattern_stack().PushFullPattern( + FullPatternStack::Kind::ImplicitParamList); return true; } @@ -208,9 +200,12 @@ static auto PopImplIntroducerAndParamsAsNameComponent( Context& context, Parse::AnyImplDeclId end_of_decl_node_id) -> NameComponent { auto [implicit_params_loc_id, implicit_param_patterns_id] = - context.node_stack().PopWithNodeIdIf(); + context.node_stack() + .PopWithNodeIdIf(); if (implicit_param_patterns_id) { + context.node_stack() + .PopAndDiscardSoloNodeId(); // Emit the `forall` match. This shouldn't produce any valid `Call` params, // because `impl`s are never actually called at runtime. auto call_params_id = @@ -243,7 +238,9 @@ static auto PopImplIntroducerAndParamsAsNameComponent( .param_patterns_id = SemIR::InstBlockId::None, .call_params_id = SemIR::InstBlockId::None, .return_slot_pattern_id = SemIR::InstId::None, - .pattern_block_id = context.pattern_block_stack().Pop(), + .pattern_block_id = implicit_param_patterns_id + ? context.pattern_block_stack().Pop() + : SemIR::InstBlockId::None, }; } diff --git a/toolchain/check/node_stack.h b/toolchain/check/node_stack.h index e1926cd36f46..6e8655fe254d 100644 --- a/toolchain/check/node_stack.h +++ b/toolchain/check/node_stack.h @@ -409,7 +409,6 @@ class NodeStack { return Id::KindFor(); case Parse::NodeKind::IfCondition: case Parse::NodeKind::IfExprIf: - case Parse::NodeKind::ImplForall: case Parse::NodeKind::ImplicitParamList: case Parse::NodeKind::TuplePattern: case Parse::NodeKind::WhileCondition: @@ -467,6 +466,7 @@ class NodeStack { case Parse::NodeKind::ExportIntroducer: case Parse::NodeKind::FileEnd: case Parse::NodeKind::FileStart: + case Parse::NodeKind::Forall: case Parse::NodeKind::ForHeader: case Parse::NodeKind::ForHeaderStart: case Parse::NodeKind::ForIn: diff --git a/toolchain/parse/handle_impl.cpp b/toolchain/parse/handle_impl.cpp index b5d4fcbbb721..bbf3b3ad0a00 100644 --- a/toolchain/parse/handle_impl.cpp +++ b/toolchain/parse/handle_impl.cpp @@ -27,7 +27,7 @@ auto HandleImplAfterIntroducer(Context& context) -> void { if (context.PositionIs(Lex::TokenKind::Forall)) { // forall [] ... context.PushState(State::ImplAfterForall); - context.ConsumeAndDiscard(); + context.AddLeafNode(NodeKind::Forall, context.Consume()); if (context.PositionIs(Lex::TokenKind::OpenSquareBracket)) { context.PushState(State::PatternListAsImplicit); } else { @@ -53,7 +53,6 @@ auto HandleImplAfterForall(Context& context) -> void { if (state.has_error) { context.ReturnErrorOnState(); } - context.AddNode(NodeKind::ImplForall, state.token, state.has_error); // One of: // as ... // as ... diff --git a/toolchain/parse/node_kind.def b/toolchain/parse/node_kind.def index 7096a5371736..31690f2a30f6 100644 --- a/toolchain/parse/node_kind.def +++ b/toolchain/parse/node_kind.def @@ -340,7 +340,7 @@ CARBON_PARSE_NODE_KIND(ImplIntroducer) CARBON_PARSE_NODE_KIND(ImplDefinitionStart) CARBON_PARSE_NODE_KIND(ImplDefinition) CARBON_PARSE_NODE_KIND(ImplDecl) -CARBON_PARSE_NODE_KIND(ImplForall) +CARBON_PARSE_NODE_KIND(Forall) CARBON_PARSE_NODE_KIND(TypeImplAs) CARBON_PARSE_NODE_KIND(DefaultSelfImplAs) diff --git a/toolchain/parse/testdata/generics/impl/fail_impl.carbon b/toolchain/parse/testdata/generics/impl/fail_impl.carbon index c9b145d41b2e..7e07fb5315ca 100644 --- a/toolchain/parse/testdata/generics/impl/fail_impl.carbon +++ b/toolchain/parse/testdata/generics/impl/fail_impl.carbon @@ -123,57 +123,57 @@ impl // CHECK:STDOUT: {kind: 'InvalidParse', text: 'return', has_error: yes}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 3}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'InvalidParse', text: 'f32', has_error: yes}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', has_error: yes, subtree_size: 2}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'InvalidParse', text: 'f32', has_error: yes}, // CHECK:STDOUT: {kind: 'FloatTypeLiteral', text: 'f32'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 5}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 2}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 2}, // CHECK:STDOUT: {kind: 'UnsignedIntTypeLiteral', text: 'u32'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 6}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'invalid'}, -// CHECK:STDOUT: {kind: 'InvalidParse', text: ']', has_error: yes}, -// CHECK:STDOUT: {kind: 'LetBindingPattern', text: 'invalid', has_error: yes, subtree_size: 3}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', has_error: yes, subtree_size: 5}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 6}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'invalid'}, +// CHECK:STDOUT: {kind: 'InvalidParse', text: ']', has_error: yes}, +// CHECK:STDOUT: {kind: 'LetBindingPattern', text: 'invalid', has_error: yes, subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', has_error: yes, subtree_size: 5}, // CHECK:STDOUT: {kind: 'IntTypeLiteral', text: 'i8'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 9}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'InvalidParse', text: 'f16', has_error: yes}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', has_error: yes, subtree_size: 2}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'InvalidParse', text: 'f16', has_error: yes}, // CHECK:STDOUT: {kind: 'FloatTypeLiteral', text: 'f16'}, // CHECK:STDOUT: {kind: 'TypeImplAs', text: 'as', subtree_size: 2}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'Quux'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 7}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, -// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, -// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 6}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, +// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, +// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, // CHECK:STDOUT: {kind: 'StringTypeLiteral', text: 'String'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 9}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, -// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, -// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 6}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, +// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, +// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'T'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 9}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, -// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, -// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 6}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, +// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, +// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'T'}, // CHECK:STDOUT: {kind: 'TypeImplAs', text: 'as', subtree_size: 2}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'Interface'}, diff --git a/toolchain/parse/testdata/generics/impl/forall.carbon b/toolchain/parse/testdata/generics/impl/forall.carbon index 21da7a1ec9b7..22997582f06c 100644 --- a/toolchain/parse/testdata/generics/impl/forall.carbon +++ b/toolchain/parse/testdata/generics/impl/forall.carbon @@ -17,27 +17,27 @@ impl forall [T:! type, U:! Interface] U as Interface(T) { // CHECK:STDOUT: parse_tree: [ // CHECK:STDOUT: {kind: 'FileStart', text: ''}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, -// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, -// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 6}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, +// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, +// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'T'}, // CHECK:STDOUT: {kind: 'TypeImplAs', text: 'as', subtree_size: 2}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'Interface'}, // CHECK:STDOUT: {kind: 'ImplDecl', text: ';', subtree_size: 11}, // CHECK:STDOUT: {kind: 'ImplIntroducer', text: 'impl'}, -// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, -// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, -// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'PatternListComma', text: ','}, -// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'U'}, -// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'Interface'}, -// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 9}, -// CHECK:STDOUT: {kind: 'ImplForall', text: 'forall', subtree_size: 10}, +// CHECK:STDOUT: {kind: 'Forall', text: 'forall'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'T'}, +// CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, +// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'PatternListComma', text: ','}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'U'}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'Interface'}, +// CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 9}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'U'}, // CHECK:STDOUT: {kind: 'TypeImplAs', text: 'as', subtree_size: 2}, // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'Interface'}, diff --git a/toolchain/parse/typed_nodes.h b/toolchain/parse/typed_nodes.h index a948fc9aaba2..89e03139c504 100644 --- a/toolchain/parse/typed_nodes.h +++ b/toolchain/parse/typed_nodes.h @@ -1341,11 +1341,12 @@ struct InterfaceDefinition { // `impl` using ImplIntroducer = LeafNode; +// `forall` +using Forall = LeafNode; + // `forall [...]` struct ImplForall { - static constexpr auto Kind = NodeKind::ImplForall.Define({.child_count = 1}); - - Lex::ForallTokenIndex token; + ForallId forall; ImplicitParamListId params; }; @@ -1371,7 +1372,7 @@ struct ImplSignature { ImplIntroducerId introducer; llvm::SmallVector modifiers; - std::optional forall; + std::optional forall; AnyImplAsId as; AnyExprId interface; TokenKind token;