From 5fa341f16a260e6352e21986fa8b99b82ffcfbcc Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 8 May 2024 15:41:42 -0700 Subject: [PATCH] Unify more invalid declaration recovery. (#3942) When we expect a semicolon, we should be able to provide more standard recovery. Note this affects a test of `base`, but it looks like a partial improvement (the prior line was moving too far). Still, it looks like recovery isn't handling `{}` quite right. I'm not trying to address that here though -- leaving a TODO. --- toolchain/parse/context.cpp | 27 +++++++++++++++++++ toolchain/parse/context.h | 7 +++++ toolchain/parse/handle_adapt.cpp | 17 +++--------- toolchain/parse/handle_alias.cpp | 17 +++--------- toolchain/parse/handle_base.cpp | 17 +++--------- toolchain/parse/handle_decl_definition.cpp | 20 +++----------- toolchain/parse/handle_namespace.cpp | 17 +++--------- .../parse/testdata/class/fail_base.carbon | 26 ++++++++++-------- 8 files changed, 65 insertions(+), 83 deletions(-) diff --git a/toolchain/parse/context.cpp b/toolchain/parse/context.cpp index 6844356016d7..76f1c70a04d3 100644 --- a/toolchain/parse/context.cpp +++ b/toolchain/parse/context.cpp @@ -402,6 +402,33 @@ auto Context::ConsumeListToken(NodeKind comma_kind, Lex::TokenKind close_kind, } } +auto Context::AddNodeExpectingDeclSemi(StateStackEntry state, + NodeKind node_kind, + Lex::TokenKind decl_kind, + bool is_def_allowed) -> void { + // TODO: This could better handle things like: + // base: { } + // var n: i32; + // ^ Ends up at `n`, instead of `var`. + if (state.has_error) { + RecoverFromDeclError(state, node_kind, + /*skip_past_likely_end=*/true); + return; + } + + if (auto semi = ConsumeIf(Lex::TokenKind::Semi)) { + AddNode(node_kind, *semi, state.subtree_start, /*has_error=*/false); + } else { + if (is_def_allowed) { + DiagnoseExpectedDeclSemiOrDefinition(decl_kind); + } else { + DiagnoseExpectedDeclSemi(decl_kind); + } + RecoverFromDeclError(state, node_kind, + /*skip_past_likely_end=*/true); + } +} + auto Context::RecoverFromDeclError(StateStackEntry state, NodeKind node_kind, bool skip_past_likely_end) -> void { auto token = state.token; diff --git a/toolchain/parse/context.h b/toolchain/parse/context.h index 92c33ff20390..da6736a5b835 100644 --- a/toolchain/parse/context.h +++ b/toolchain/parse/context.h @@ -281,6 +281,13 @@ class Context { // Propagates an error up the state stack, to the parent state. auto ReturnErrorOnState() -> void { state_stack_.back().has_error = true; } + // Adds a node for a declaration's semicolon. Includes error recovery when the + // token is not a semicolon, using `decl_kind` and `is_def_allowed` to inform + // diagnostics. + auto AddNodeExpectingDeclSemi(StateStackEntry state, NodeKind node_kind, + Lex::TokenKind decl_kind, bool is_def_allowed) + -> void; + // Emits a diagnostic for a declaration missing a semi. auto DiagnoseExpectedDeclSemi(Lex::TokenKind expected_kind) -> void; diff --git a/toolchain/parse/handle_adapt.cpp b/toolchain/parse/handle_adapt.cpp index 2e36bb9a4113..ac10df2a0b0d 100644 --- a/toolchain/parse/handle_adapt.cpp +++ b/toolchain/parse/handle_adapt.cpp @@ -13,20 +13,9 @@ auto HandleAdaptDecl(Context& context) -> void { // We should factor out this common work. auto state = context.PopState(); - auto semi = context.ConsumeIf(Lex::TokenKind::Semi); - if (!semi && !state.has_error) { - context.DiagnoseExpectedDeclSemi(context.tokens().GetKind(state.token)); - state.has_error = true; - } - - if (state.has_error) { - context.RecoverFromDeclError(state, NodeKind::AdaptDecl, - /*skip_past_likely_end=*/true); - return; - } - - context.AddNode(NodeKind::AdaptDecl, *semi, state.subtree_start, - state.has_error); + context.AddNodeExpectingDeclSemi(state, NodeKind::AdaptDecl, + Lex::TokenKind::Adapt, + /*is_def_allowed=*/false); } } // namespace Carbon::Parse diff --git a/toolchain/parse/handle_alias.cpp b/toolchain/parse/handle_alias.cpp index d56c81238e37..7810dc11fcc8 100644 --- a/toolchain/parse/handle_alias.cpp +++ b/toolchain/parse/handle_alias.cpp @@ -42,20 +42,9 @@ auto HandleAliasAfterName(Context& context) -> void { auto HandleAliasFinish(Context& context) -> void { auto state = context.PopState(); - if (state.has_error) { - context.RecoverFromDeclError(state, NodeKind::Alias, - /*skip_past_likely_end=*/true); - return; - } - - if (auto semi = context.ConsumeIf(Lex::TokenKind::Semi)) { - context.AddNode(NodeKind::Alias, *semi, state.subtree_start, - state.has_error); - } else { - context.DiagnoseExpectedDeclSemi(Lex::TokenKind::Alias); - context.RecoverFromDeclError(state, NodeKind::Alias, - /*skip_past_likely_end=*/true); - } + context.AddNodeExpectingDeclSemi(state, NodeKind::Alias, + Lex::TokenKind::Alias, + /*is_def_allowed=*/false); } } // namespace Carbon::Parse diff --git a/toolchain/parse/handle_base.cpp b/toolchain/parse/handle_base.cpp index df6c46912ff7..4fd7d337e109 100644 --- a/toolchain/parse/handle_base.cpp +++ b/toolchain/parse/handle_base.cpp @@ -10,20 +10,9 @@ namespace Carbon::Parse { auto HandleBaseDecl(Context& context) -> void { auto state = context.PopState(); - auto semi = context.ConsumeIf(Lex::TokenKind::Semi); - if (!semi && !state.has_error) { - context.DiagnoseExpectedDeclSemi(context.tokens().GetKind(state.token)); - state.has_error = true; - } - - if (state.has_error) { - context.RecoverFromDeclError(state, NodeKind::BaseDecl, - /*skip_past_likely_end=*/true); - return; - } - - context.AddNode(NodeKind::BaseDecl, *semi, state.subtree_start, - state.has_error); + context.AddNodeExpectingDeclSemi(state, NodeKind::BaseDecl, + Lex::TokenKind::Base, + /*is_def_allowed=*/false); } } // namespace Carbon::Parse diff --git a/toolchain/parse/handle_decl_definition.cpp b/toolchain/parse/handle_decl_definition.cpp index 525668569710..4d33f978e4a0 100644 --- a/toolchain/parse/handle_decl_definition.cpp +++ b/toolchain/parse/handle_decl_definition.cpp @@ -13,22 +13,10 @@ static auto HandleDeclOrDefinition(Context& context, NodeKind decl_kind, State definition_finish_state) -> void { auto state = context.PopState(); - if (state.has_error) { - context.RecoverFromDeclError(state, decl_kind, - /*skip_past_likely_end=*/true); - return; - } - - if (auto semi = context.ConsumeIf(Lex::TokenKind::Semi)) { - context.AddNode(decl_kind, *semi, state.subtree_start, state.has_error); - return; - } - - if (!context.PositionIs(Lex::TokenKind::OpenCurlyBrace)) { - context.DiagnoseExpectedDeclSemiOrDefinition( - context.tokens().GetKind(state.token)); - context.RecoverFromDeclError(state, decl_kind, - /*skip_past_likely_end=*/true); + if (state.has_error || !context.PositionIs(Lex::TokenKind::OpenCurlyBrace)) { + context.AddNodeExpectingDeclSemi(state, decl_kind, + context.tokens().GetKind(state.token), + /*is_def_allowed=*/true); return; } diff --git a/toolchain/parse/handle_namespace.cpp b/toolchain/parse/handle_namespace.cpp index 33ee69f79d06..3a1d4dbfc09e 100644 --- a/toolchain/parse/handle_namespace.cpp +++ b/toolchain/parse/handle_namespace.cpp @@ -15,20 +15,9 @@ auto HandleNamespace(Context& context) -> void { auto HandleNamespaceFinish(Context& context) -> void { auto state = context.PopState(); - if (state.has_error) { - context.RecoverFromDeclError(state, NodeKind::Namespace, - /*skip_past_likely_end=*/true); - return; - } - - if (auto semi = context.ConsumeIf(Lex::TokenKind::Semi)) { - context.AddNode(NodeKind::Namespace, *semi, state.subtree_start, - state.has_error); - } else { - context.DiagnoseExpectedDeclSemi(Lex::TokenKind::Namespace); - context.RecoverFromDeclError(state, NodeKind::Namespace, - /*skip_past_likely_end=*/true); - } + context.AddNodeExpectingDeclSemi(state, NodeKind::Namespace, + Lex::TokenKind::Namespace, + /*is_def_allowed=*/false); } } // namespace Carbon::Parse diff --git a/toolchain/parse/testdata/class/fail_base.carbon b/toolchain/parse/testdata/class/fail_base.carbon index 7c0c1fc6d3df..40ba64b1b5bc 100644 --- a/toolchain/parse/testdata/class/fail_base.carbon +++ b/toolchain/parse/testdata/class/fail_base.carbon @@ -17,11 +17,15 @@ class A { // CHECK:STDERR: base: ; - // CHECK:STDERR: fail_base.carbon:[[@LINE+3]]:7: ERROR: Unrecognized declaration introducer. - // CHECK:STDERR: base: { } - // CHECK:STDERR: ^ base: { } // We should resume parsing here after the previous error. + // CHECK:STDERR: fail_base.carbon:[[@LINE+7]]:3: ERROR: `base` declarations must end with a `;`. + // CHECK:STDERR: var n: i32; + // CHECK:STDERR: ^~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_base.carbon:[[@LINE+3]]:7: ERROR: Unrecognized declaration introducer. + // CHECK:STDERR: var n: i32; + // CHECK:STDERR: ^ var n: i32; } @@ -37,14 +41,14 @@ class A { // CHECK:STDOUT: {kind: 'BaseIntroducer', text: 'base'}, // CHECK:STDOUT: {kind: 'BaseColon', text: ':'}, // CHECK:STDOUT: {kind: 'InvalidParse', text: ';', has_error: yes}, -// CHECK:STDOUT: {kind: 'BaseDecl', text: 'base', has_error: yes, subtree_size: 4}, -// CHECK:STDOUT: {kind: 'InvalidParseStart', text: ':', has_error: yes}, -// CHECK:STDOUT: {kind: 'InvalidParseSubtree', text: '}', has_error: yes, subtree_size: 2}, -// CHECK:STDOUT: {kind: 'VariableIntroducer', text: 'var'}, -// CHECK:STDOUT: {kind: 'IdentifierName', text: 'n'}, -// CHECK:STDOUT: {kind: 'IntTypeLiteral', text: 'i32'}, -// CHECK:STDOUT: {kind: 'BindingPattern', text: ':', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'VariableDecl', text: ';', subtree_size: 5}, +// CHECK:STDOUT: {kind: 'BaseDecl', text: ';', has_error: yes, subtree_size: 4}, +// CHECK:STDOUT: {kind: 'BaseIntroducer', text: 'base'}, +// CHECK:STDOUT: {kind: 'BaseColon', text: ':'}, +// CHECK:STDOUT: {kind: 'StructLiteralStart', text: '{'}, +// CHECK:STDOUT: {kind: 'StructLiteral', text: '}', subtree_size: 2}, +// CHECK:STDOUT: {kind: 'BaseDecl', text: 'var', has_error: yes, subtree_size: 5}, +// CHECK:STDOUT: {kind: 'InvalidParseStart', text: 'n', has_error: yes}, +// CHECK:STDOUT: {kind: 'InvalidParseSubtree', text: ';', has_error: yes, subtree_size: 2}, // CHECK:STDOUT: {kind: 'ClassDefinition', text: '}', subtree_size: 18}, // CHECK:STDOUT: {kind: 'FileEnd', text: ''}, // CHECK:STDOUT: ]