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

@@ -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;

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;
 

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

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

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

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;
   }
 

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

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:   ]