Treat `(pattern)` as grouping parens. (#6976)

Do not treat it as a 1-tuple pattern as we used to. The design indicates
that `(pattern)` is invalid, but this appears to be an oversight, and
grouping parens appear to be the intended interpretation.

---------

Co-authored-by: Geoff Romer <gromer@google.com>

toolchain/check/handle_pattern_list.cpp

@@ -66,6 +66,16 @@ auto HandleParseNode(Context& context, Parse::ExplicitParamListId node_id)
                             Parse::NodeKind::ExplicitParamListStart);
 }
 
+auto HandleParseNode(Context& context, Parse::ParenPatternId node_id) -> bool {
+  EndSubpatternAsNonExpr(context);
+  auto pattern_id = context.node_stack().PopPattern();
+  context.param_and_arg_refs_stack().PopAndDiscard();
+  context.node_stack()
+      .PopAndDiscardSoloNodeId<Parse::NodeKind::TuplePatternStart>();
+  context.node_stack().Push(node_id, pattern_id);
+  return true;
+}
+
 auto HandleParseNode(Context& context, Parse::TuplePatternId node_id) -> bool {
   if (context.node_stack().PeekIs(Parse::NodeKind::TuplePatternStart)) {
     // End the subpattern started by a trailing comma, or the opening delimiter

toolchain/check/testdata/patterns/tuple.carbon

@@ -40,6 +40,24 @@ var t: ((), ()) = ((), ());
 let (a: (), b: ()) = t;
 //@dump-sem-ir-end
 
+// --- grouping_parens.carbon
+
+library "[[@TEST_NAME]]";
+
+//@dump-sem-ir-begin
+let (a: ()) = ();
+let (((b: ()))) = ();
+//@dump-sem-ir-end
+
+// --- one_tuple.carbon
+
+library "[[@TEST_NAME]]";
+
+//@dump-sem-ir-begin
+let (a: (),) = ((),);
+let (((b: ()),)) = ((),);
+//@dump-sem-ir-end
+
 // CHECK:STDOUT: --- literal.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
@@ -169,3 +187,86 @@ let (a: (), b: ()) = t;
 // CHECK:STDOUT:   <elided>
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
+// CHECK:STDOUT: --- grouping_parens.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %empty_tuple.type: type = tuple_type () [concrete]
+// CHECK:STDOUT:   %empty_tuple: %empty_tuple.type = tuple_value () [concrete]
+// CHECK:STDOUT:   %pattern_type: type = pattern_type %empty_tuple.type [concrete]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   name_binding_decl {
+// CHECK:STDOUT:     %a.patt: %pattern_type = value_binding_pattern a [concrete]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %.loc5_10.1: type = splice_block %.loc5_10.3 [concrete = constants.%empty_tuple.type] {
+// CHECK:STDOUT:     %.loc5_10.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:     %.loc5_10.3: type = converted %.loc5_10.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %empty_tuple.loc5: %empty_tuple.type = tuple_value () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc5_16: %empty_tuple.type = converted @__global_init.%.loc5, %empty_tuple.loc5 [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %a: %empty_tuple.type = value_binding a, %.loc5_16
+// CHECK:STDOUT:   name_binding_decl {
+// CHECK:STDOUT:     %b.patt: %pattern_type = value_binding_pattern b [concrete]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %.loc6_12.1: type = splice_block %.loc6_12.3 [concrete = constants.%empty_tuple.type] {
+// CHECK:STDOUT:     %.loc6_12.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:     %.loc6_12.3: type = converted %.loc6_12.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %empty_tuple.loc6: %empty_tuple.type = tuple_value () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc6_20: %empty_tuple.type = converted @__global_init.%.loc6, %empty_tuple.loc6 [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %b: %empty_tuple.type = value_binding b, %.loc6_20
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @__global_init() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   %.loc5: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc6: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   <elided>
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: --- one_tuple.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %empty_tuple.type: type = tuple_type () [concrete]
+// CHECK:STDOUT:   %empty_tuple: %empty_tuple.type = tuple_value () [concrete]
+// CHECK:STDOUT:   %pattern_type.cb1: type = pattern_type %empty_tuple.type [concrete]
+// CHECK:STDOUT:   %tuple.type: type = tuple_type (%empty_tuple.type) [concrete]
+// CHECK:STDOUT:   %pattern_type.559: type = pattern_type %tuple.type [concrete]
+// CHECK:STDOUT:   %tuple: %tuple.type = tuple_value (%empty_tuple) [concrete]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   name_binding_decl {
+// CHECK:STDOUT:     %a.patt: %pattern_type.cb1 = value_binding_pattern a [concrete]
+// CHECK:STDOUT:     %.loc5_12: %pattern_type.559 = tuple_pattern (%a.patt) [concrete]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %.loc5_10.1: type = splice_block %.loc5_10.3 [concrete = constants.%empty_tuple.type] {
+// CHECK:STDOUT:     %.loc5_10.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:     %.loc5_10.3: type = converted %.loc5_10.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %empty_tuple.loc5: %empty_tuple.type = tuple_value () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc5_18: %empty_tuple.type = converted @__global_init.%.loc5_18, %empty_tuple.loc5 [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %a: %empty_tuple.type = value_binding a, %.loc5_18
+// CHECK:STDOUT:   name_binding_decl {
+// CHECK:STDOUT:     %b.patt: %pattern_type.cb1 = value_binding_pattern b [concrete]
+// CHECK:STDOUT:     %.loc6_15: %pattern_type.559 = tuple_pattern (%b.patt) [concrete]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %.loc6_12.1: type = splice_block %.loc6_12.3 [concrete = constants.%empty_tuple.type] {
+// CHECK:STDOUT:     %.loc6_12.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:     %.loc6_12.3: type = converted %.loc6_12.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %empty_tuple.loc6: %empty_tuple.type = tuple_value () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc6_22: %empty_tuple.type = converted @__global_init.%.loc6_22, %empty_tuple.loc6 [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %b: %empty_tuple.type = value_binding b, %.loc6_22
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @__global_init() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   %.loc5_18: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc5_20: %tuple.type = tuple_literal (%.loc5_18) [concrete = constants.%tuple]
+// CHECK:STDOUT:   %.loc6_22: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
+// CHECK:STDOUT:   %.loc6_24: %tuple.type = tuple_literal (%.loc6_22) [concrete = constants.%tuple]
+// CHECK:STDOUT:   <elided>
+// CHECK:STDOUT: }
+// CHECK:STDOUT:

toolchain/parse/handle_pattern_list.cpp

@@ -43,9 +43,20 @@ static auto HandlePatternListElementFinish(Context& context,
     context.ReturnErrorOnState();
   }
 
-  if (context.ConsumeListToken(NodeKind::PatternListComma, close_token,
-                               state.has_error) ==
-      Context::ListTokenKind::Comma) {
+  auto list_token_kind = context.ConsumeListToken(NodeKind::PatternListComma,
+                                                  close_token, state.has_error);
+
+  // If we have a comma, the parent is now a tuple pattern not a parenthesized
+  // pattern.
+  if (list_token_kind != Context::ListTokenKind::Close &&
+      param_state_kind == StateKind::PatternListElementAsTuple) {
+    auto parent_state = context.PopState();
+    CARBON_CHECK(parent_state.kind == StateKind::PatternListFinishAsTuple ||
+                 parent_state.kind == StateKind::PatternListFinishAsParen);
+    context.PushState(parent_state, StateKind::PatternListFinishAsTuple);
+  }
+
+  if (list_token_kind == Context::ListTokenKind::Comma) {
     context.PushStateForPattern(param_state_kind, state.in_var_pattern,
                                 state.in_unused_pattern);
   }
@@ -70,31 +81,38 @@ auto HandlePatternListElementFinishAsImplicit(Context& context) -> void {
 static auto HandlePatternList(Context& context, NodeKind node_kind,
                               Lex::TokenKind open_token_kind,
                               Lex::TokenKind close_token_kind,
-                              StateKind param_state, StateKind finish_state)
-    -> void {
+                              StateKind param_state,
+                              StateKind finish_state_empty,
+                              StateKind finish_state_nonempty) -> void {
   auto state = context.PopState();
+  auto open_token = context.ConsumeChecked(open_token_kind);
+  bool empty = context.PositionIs(close_token_kind);
 
-  context.PushStateForPattern(finish_state, state.in_var_pattern,
-                              state.in_unused_pattern);
-  context.AddLeafNode(node_kind, context.ConsumeChecked(open_token_kind));
+  context.PushStateForPattern(
+      empty ? finish_state_empty : finish_state_nonempty, state.in_var_pattern,
+      state.in_unused_pattern);
+  context.AddLeafNode(node_kind, open_token);
 
-  if (!context.PositionIs(close_token_kind)) {
+  if (!empty) {
     context.PushStateForPattern(param_state, state.in_var_pattern,
                                 state.in_unused_pattern);
   }
 }
 
 auto HandlePatternListAsTuple(Context& context) -> void {
-  HandlePatternList(context, NodeKind::TuplePatternStart,
-                    Lex::TokenKind::OpenParen, Lex::TokenKind::CloseParen,
-                    StateKind::PatternListElementAsTuple,
-                    StateKind::PatternListFinishAsTuple);
+  // If the list is nonempty, use PatternListFinishAsParen as the parent. This
+  // will be replaced by PatternListFinishAsTuple if we see a comma.
+  HandlePatternList(
+      context, NodeKind::TuplePatternStart, Lex::TokenKind::OpenParen,
+      Lex::TokenKind::CloseParen, StateKind::PatternListElementAsTuple,
+      StateKind::PatternListFinishAsTuple, StateKind::PatternListFinishAsParen);
 }
 
 auto HandlePatternListAsExplicit(Context& context) -> void {
   HandlePatternList(context, NodeKind::ExplicitParamListStart,
                     Lex::TokenKind::OpenParen, Lex::TokenKind::CloseParen,
                     StateKind::PatternListElementAsExplicit,
+                    StateKind::PatternListFinishAsExplicit,
                     StateKind::PatternListFinishAsExplicit);
 }
 
@@ -103,10 +121,11 @@ auto HandlePatternListAsImplicit(Context& context) -> void {
                     Lex::TokenKind::OpenSquareBracket,
                     Lex::TokenKind::CloseSquareBracket,
                     StateKind::PatternListElementAsImplicit,
+                    StateKind::PatternListFinishAsImplicit,
                     StateKind::PatternListFinishAsImplicit);
 }
 
-// Handles PatternListFinishAs(Tuple|Explicit|Implicit).
+// Handles PatternListFinishAs(Paren|Tuple|Explicit|Implicit).
 static auto HandlePatternListFinish(Context& context, NodeKind node_kind,
                                     Lex::TokenKind token_kind) -> void {
   auto state = context.PopState();
@@ -115,6 +134,11 @@ static auto HandlePatternListFinish(Context& context, NodeKind node_kind,
                   state.has_error);
 }
 
+auto HandlePatternListFinishAsParen(Context& context) -> void {
+  HandlePatternListFinish(context, NodeKind::ParenPattern,
+                          Lex::TokenKind::CloseParen);
+}
+
 auto HandlePatternListFinishAsTuple(Context& context) -> void {
   HandlePatternListFinish(context, NodeKind::TuplePattern,
                           Lex::TokenKind::CloseParen);

toolchain/parse/node_kind.def

@@ -162,6 +162,7 @@ CARBON_PARSE_NODE_KIND(Alias)
 
 CARBON_PARSE_NODE_KIND(TuplePatternStart)
 CARBON_PARSE_NODE_KIND(PatternListComma)
+CARBON_PARSE_NODE_KIND(ParenPattern)
 CARBON_PARSE_NODE_KIND(TuplePattern)
 
 CARBON_PARSE_NODE_KIND(ExplicitParamListStart)

toolchain/parse/state.def

@@ -983,6 +983,7 @@ CARBON_PARSE_STATE_VARIANTS3(PatternListElement, Tuple, Explicit, Implicit)
 // ... , )  (variant is Tuple)
 //     ^
 //   (state done)
+//   SPECIAL: parent becomes PatternListFinishAsTuple
 //
 // ... , ]  (variant is Implicit)
 //     ^
@@ -991,6 +992,7 @@ CARBON_PARSE_STATE_VARIANTS3(PatternListElement, Tuple, Explicit, Implicit)
 // ... , ...
 //     ^
 //   1. PatternListElementAs(Tuple|Explicit|Implicit)
+//   SPECIAL (variant is Tuple): parent becomes PatternListFinishAsTuple
 //
 // ...
 //    ^
@@ -1007,22 +1009,28 @@ CARBON_PARSE_STATE_VARIANTS3(PatternListElementFinish, Tuple, Explicit,
 // ^
 //   1. PatternListFinishAs(Tuple|Explicit|Implicit)
 //
-// ( ... )    (variant is Tuple or Explicit)
+// ( ... )    (variant is Explicit)
 // ^
 // [ ... ]    (variant is Implicit)
 // ^
-//   1. PatternListElementAs(Tuple|Explicit|Implicit)
-//   2. PatternListFinishAs(Tuple|Explicit|Implicit)
+//   1. PatternListElementAs(Explicit|Implicit)
+//   2. PatternListFinishAs(Explicit|Implicit)
+//
+// ( ... )    (variant is Tuple)
+// ^
+//   1. PatternListElementAsTuple
+//   2. PatternListFinishAsParen             (SPECIAL: may be replaced)
 CARBON_PARSE_STATE_VARIANTS3(PatternList, Tuple, Explicit, Implicit)
 
 // Handles processing of a parameter list `]` or `)`.
 //
-// ( ... )    (variant is Tuple or Explicit)
+// ( ... )    (variant is Paren, Tuple or Explicit)
 //       ^
 // [ ... ]    (variant is Implicit)
 //       ^
 //   (state done)
-CARBON_PARSE_STATE_VARIANTS3(PatternListFinish, Tuple, Explicit, Implicit)
+CARBON_PARSE_STATE_VARIANTS4(PatternListFinish, Paren, Tuple, Explicit,
+                             Implicit)
 
 // Handles the processing of a `(condition)` up through the expression.
 //

toolchain/parse/testdata/match/match.carbon

@@ -66,7 +66,7 @@ fn f() -> i32 {
 // CHECK:STDOUT:                   {kind: 'IdentifierNameNotBeforeSignature', text: 'a'},
 // CHECK:STDOUT:                   {kind: 'IntTypeLiteral', text: 'i32'},
 // CHECK:STDOUT:                 {kind: 'LetBindingPattern', text: ':', subtree_size: 3},
-// CHECK:STDOUT:               {kind: 'TuplePattern', text: ')', subtree_size: 5},
+// CHECK:STDOUT:               {kind: 'ParenPattern', text: ')', subtree_size: 5},
 // CHECK:STDOUT:                 {kind: 'MatchCaseGuardIntroducer', text: 'if'},
 // CHECK:STDOUT:                 {kind: 'MatchCaseGuardStart', text: '('},
 // CHECK:STDOUT:                   {kind: 'IdentifierNameExpr', text: 'a'},

toolchain/parse/testdata/var/unused.carbon

@@ -354,7 +354,7 @@ var unused (unused y: i32) = (0,);
 // CHECK:STDOUT:                   {kind: 'IdentifierNameNotBeforeSignature', text: 'a'},
 // CHECK:STDOUT:                   {kind: 'IntTypeLiteral', text: 'i32'},
 // CHECK:STDOUT:                 {kind: 'LetBindingPattern', text: ':', subtree_size: 3},
-// CHECK:STDOUT:               {kind: 'TuplePattern', text: ')', subtree_size: 5},
+// CHECK:STDOUT:               {kind: 'ParenPattern', text: ')', subtree_size: 5},
 // CHECK:STDOUT:                 {kind: 'MatchCaseGuardIntroducer', text: 'if'},
 // CHECK:STDOUT:                 {kind: 'MatchCaseGuardStart', text: '('},
 // CHECK:STDOUT:                   {kind: 'IdentifierNameExpr', text: 'a'},
@@ -457,7 +457,7 @@ var unused (unused y: i32) = (0,);
 // CHECK:STDOUT:                 {kind: 'IntTypeLiteral', text: 'i32'},
 // CHECK:STDOUT:               {kind: 'VarBindingPattern', text: ':', subtree_size: 3},
 // CHECK:STDOUT:             {kind: 'UnusedPattern', text: 'unused', subtree_size: 4},
-// CHECK:STDOUT:           {kind: 'TuplePattern', text: ')', subtree_size: 6},
+// CHECK:STDOUT:           {kind: 'ParenPattern', text: ')', subtree_size: 6},
 // CHECK:STDOUT:         {kind: 'UnusedPattern', text: 'unused', subtree_size: 7},
 // CHECK:STDOUT:       {kind: 'VariablePattern', text: 'var', subtree_size: 8},
 // CHECK:STDOUT:       {kind: 'VariableInitializer', text: '='},

toolchain/parse/typed_nodes.h

@@ -414,6 +414,18 @@ using TuplePatternStart =
 using PatternListComma =
     LeafNode<NodeKind::PatternListComma, Lex::CommaTokenIndex>;
 
+// A parenthesized pattern that isn't an explicit parameter list.
+struct ParenPattern {
+  static constexpr auto Kind =
+      NodeKind::ParenPattern.Define({.category = NodeCategory::Pattern,
+                                     .bracketed_by = TuplePatternStart::Kind,
+                                     .child_count = 2});
+
+  TuplePatternStartId left_paren;
+  AnyPatternId inner;
+  Lex::CloseParenTokenIndex token;
+};
+
 // A tuple pattern that isn't an explicit parameter list: `(a: i32, b: i32)`.
 struct TuplePattern {
   static constexpr auto Kind =