Refactor Pattern and FunctionParameter handling for Parser consistency (#2385)

While the Parser has similar divergent states, lists of Expressions tend to be handled more like this. I'm keeping the divergent start state in order to continue support of a distinct error, but I think this organization of FunctionParameter/FunctionParameterFinish will be less surprising.

toolchain/parser/parser.cpp

@@ -1004,6 +1004,27 @@ auto Parser::HandleFunctionIntroducerState() -> void {
   // Advance past the open paren.
   ++position_;
   if (!PositionIs(TokenKind::CloseParen())) {
+    PushState(ParserState::FunctionParameter());
+  }
+}
+
+auto Parser::HandleFunctionParameterState() -> void {
+  PopAndDiscardState();
+
+  PushState(ParserState::FunctionParameterFinish());
+  PushState(ParserState::PatternAsFunctionParameter());
+}
+
+auto Parser::HandleFunctionParameterFinishState() -> void {
+  auto state = PopState();
+
+  if (state.has_error) {
+    ReturnErrorOnState();
+  }
+
+  if (ConsumeListToken(ParseNodeKind::ParameterListComma(),
+                       TokenKind::CloseParen(),
+                       state.has_error) == ListTokenKind::Comma) {
     PushState(ParserState::PatternAsFunctionParameter());
   }
 }
@@ -1270,20 +1291,11 @@ auto Parser::HandleParenExpressionFinishAsTupleState() -> void {
           state.has_error);
 }
 
-auto Parser::HandlePatternStart(PatternKind pattern_kind) -> void {
+auto Parser::HandlePattern(PatternKind pattern_kind) -> void {
   auto state = PopState();
 
   // Ensure the finish state always follows.
-  switch (pattern_kind) {
-    case PatternKind::Parameter: {
-      state.state = ParserState::PatternAsFunctionParameterFinish();
-      break;
-    }
-    case PatternKind::Variable: {
-      state.state = ParserState::PatternAsVariableFinish();
-      break;
-    }
-  }
+  state.state = ParserState::PatternFinish();
 
   // Handle an invalid pattern introducer.
   if (!PositionIs(TokenKind::Identifier()) ||
@@ -1316,41 +1328,26 @@ auto Parser::HandlePatternStart(PatternKind pattern_kind) -> void {
   position_ += 2;
 }
 
-auto Parser::HandlePatternFinish() -> bool {
+auto Parser::HandlePatternAsFunctionParameterState() -> void {
+  HandlePattern(PatternKind::Parameter);
+}
+
+auto Parser::HandlePatternAsVariableState() -> void {
+  HandlePattern(PatternKind::Variable);
+}
+
+auto Parser::HandlePatternFinishState() -> void {
   auto state = PopState();
 
   // If an error was encountered, propagate it without adding a node.
   if (state.has_error) {
     ReturnErrorOnState();
-    return true;
+    return;
   }
 
   // TODO: may need to mark has_error if !type.
   AddNode(ParseNodeKind::PatternBinding(), state.token, state.subtree_start,
           /*has_error=*/false);
-  return false;
-}
-
-auto Parser::HandlePatternAsFunctionParameterState() -> void {
-  HandlePatternStart(PatternKind::Parameter);
-}
-
-auto Parser::HandlePatternAsFunctionParameterFinishState() -> void {
-  bool has_error = HandlePatternFinish();
-
-  if (ConsumeListToken(ParseNodeKind::ParameterListComma(),
-                       TokenKind::CloseParen(),
-                       has_error) == ListTokenKind::Comma) {
-    PushState(ParserState::PatternAsFunctionParameter());
-  }
-}
-
-auto Parser::HandlePatternAsVariableState() -> void {
-  HandlePatternStart(PatternKind::Variable);
-}
-
-auto Parser::HandlePatternAsVariableFinishState() -> void {
-  HandlePatternFinish();
 }
 
 auto Parser::HandleStatementState() -> void {

toolchain/parser/parser.h

@@ -16,6 +16,8 @@
 
 namespace Carbon {
 
+// This parser uses a stack for state transitions. See parser_state.def for
+// state documentation.
 class Parser {
  public:
   // Parses the tokens into a parse tree, emitting any errors encountered.
@@ -265,16 +267,11 @@ class Parser {
   auto HandleFunctionError(StateStackEntry state, bool skip_past_likely_end)
       -> void;
 
-  // Handles ParenExpressionParameterFinish(AsUnknown|AsTuple)
+  // Handles ParenExpressionParameterFinish(AsUnknown|AsTuple).
   auto HandleParenExpressionParameterFinish(bool as_tuple) -> void;
 
-  // Handles the start of a pattern.
-  // If the start of the pattern is invalid, it's the responsibility of the
-  // outside context to advance past the pattern.
-  auto HandlePatternStart(PatternKind pattern_kind) -> void;
-
-  // Handles the end of a pattern.
-  auto HandlePatternFinish() -> bool;
+  // Handles PatternAs(FunctionParameter|Variable).
+  auto HandlePattern(PatternKind pattern_kind) -> void;
 
   // Handles the `;` after a keyword statement.
   auto HandleStatementKeywordFinish(TokenKind token_kind,

toolchain/parser/parser_state.def

@@ -218,6 +218,22 @@ CARBON_PARSER_STATE(ExpressionStatementFinish)
 //   2. FunctionAfterParameterList
 CARBON_PARSER_STATE(FunctionIntroducer)
 
+// Starts function parameter processing.
+//
+// Always:
+//   1. PatternAsFunctionParameter
+//   2. FunctionParameterFinish
+CARBON_PARSER_STATE(FunctionParameter)
+
+// Finishes function parameter processing, including `,`. If there are more
+// parameters, enqueues another parameter processing state.
+//
+// If `Comma` without `CloseParen`:
+//   1. FunctionParameter
+// Else:
+//   (state done)
+CARBON_PARSER_STATE(FunctionParameterFinish)
+
 // Handles processing of a function's parameter list `)`.
 //
 // Always:
@@ -312,41 +328,22 @@ CARBON_PARSER_STATE(ParenExpressionParameterFinishAsTuple)
 CARBON_PARSER_STATE(ParenExpressionFinish)
 CARBON_PARSER_STATE(ParenExpressionFinishAsTuple)
 
-// Handles pattern parsing for a function parameter, enqueuing type expression
-// processing. Proceeds to the matching Finish state when done.
+// Handles pattern parsing for a pattern, enqueuing type expression processing.
+// This covers function parameter and `var` support.
 //
 // If valid:
 //   1. Expression
-//   2. PatternAsFunctionParameterFinish
+//   2. PatternFinish
 // Else:
-//   1. PatternAsFunctionParameterFinish
+//   1. PatternFinish
 CARBON_PARSER_STATE(PatternAsFunctionParameter)
-
-// Finishes function parameter processing, including `,`. If there are more
-// parameters, enqueues another parameter processing state.
-//
-// If `Comma` without `CloseParen`:
-//   1. PatternAsFunctionParameter
-// Else:
-//   (state done)
-CARBON_PARSER_STATE(PatternAsFunctionParameterFinish)
-
-// Handles pattern parsing for a `var` statement, enqueuing type expression
-// processing. Proceeds to the matching Finish state when done.
-//
-//
-// If valid:
-//   1. Expression
-//   2. PatternAsVariableFinish
-// Else:
-//   1. PatternAsVariableFinish
 CARBON_PARSER_STATE(PatternAsVariable)
 
-// Finishes `var` pattern processing.
+// Finishes pattern processing.
 //
 // Always:
 //   (state done)
-CARBON_PARSER_STATE(PatternAsVariableFinish)
+CARBON_PARSER_STATE(PatternFinish)
 
 // Handles a single statement. While typically within a statement block, this
 // can also be used for error recovery where we expect a statement block and