Parse tree for `impl` that is better for check stage (#3678)

Use two different nodes for "<type> followed by `as`" and "<type>
omitted before `as`, use `self`", so it is easier to determine which
case. Later the second case will push the type id for `self` onto the
node stack, making the two paths more similar.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/check/handle_impl.cpp

@@ -32,20 +32,24 @@ auto HandleImplForall(Context& context, Parse::ImplForallId parse_node)
   return true;
 }
 
-auto HandleImplAs(Context& /*context*/, Parse::ImplAsId /*parse_node*/)
+auto HandleTypeImplAs(Context& context, Parse::TypeImplAsId parse_node)
     -> bool {
+  auto self_id = context.node_stack().PopExpr();
+  context.node_stack().Push(parse_node, self_id);
+  return true;
+}
+
+auto HandleDefaultSelfImplAs(Context& /*context*/,
+                             Parse::DefaultSelfImplAsId /*parse_node*/)
+    -> bool {
+  // TODO: Determine self_id and push it onto node stack.
   return true;
 }
 
 static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId /*parse_node*/)
     -> SemIR::InstId {
   auto interface_id = context.node_stack().PopExpr();
-  auto self_id = SemIR::InstId::Invalid;
-  if (!context.node_stack().PeekIs<Parse::NodeKind::ImplForall>() &&
-      !context.node_stack().PeekIs<Parse::NodeKind::ImplIntroducer>()) {
-    self_id = context.node_stack().PopExpr();
-  }
-
+  auto self_id = context.node_stack().PopIf<Parse::NodeKind::TypeImplAs>();
   auto params_id = context.node_stack().PopIf<Parse::NodeKind::ImplForall>();
   auto decl_block_id = context.inst_block_stack().Pop();
   context.node_stack()

toolchain/check/node_stack.h

@@ -488,6 +488,7 @@ class NodeStack {
         case Parse::NodeKind::ShortCircuitOperandOr:
         case Parse::NodeKind::StructFieldValue:
         case Parse::NodeKind::StructFieldType:
+        case Parse::NodeKind::TypeImplAs:
         case Parse::NodeKind::VariableInitializer:
           return IdKind::InstId;
         case Parse::NodeKind::IfCondition:

toolchain/parse/handle_impl.cpp

@@ -9,7 +9,7 @@ namespace Carbon::Parse {
 static auto ExpectAsOrTypeExpression(Carbon::Parse::Context& context) -> void {
   if (context.PositionIs(Lex::TokenKind::As)) {
     // as <expression> ...
-    context.AddLeafNode(NodeKind::ImplAs, context.Consume());
+    context.AddLeafNode(NodeKind::DefaultSelfImplAs, context.Consume());
     context.PushState(State::Expr);
   } else {
     // <expression> as <expression>...
@@ -55,7 +55,6 @@ auto HandleImplAfterForall(Carbon::Parse::Context& context) -> void {
   }
   context.AddNode(NodeKind::ImplForall, state.token, state.subtree_start,
                   state.has_error);
-
   // One of:
   //   as <expression> ...
   //   <expression> as <expression>...
@@ -64,17 +63,16 @@ auto HandleImplAfterForall(Carbon::Parse::Context& context) -> void {
 
 auto HandleImplBeforeAs(Carbon::Parse::Context& context) -> void {
   auto state = context.PopState();
-  if (state.has_error) {
-    context.ReturnErrorOnState();
-    return;
-  }
   if (auto as = context.ConsumeIf(Lex::TokenKind::As)) {
-    context.AddLeafNode(NodeKind::ImplAs, *as);
+    context.AddNode(NodeKind::TypeImplAs, *as, state.subtree_start,
+                    state.has_error);
     context.PushState(State::Expr);
   } else {
-    CARBON_DIAGNOSTIC(ImplExpectedAs, Error,
-                      "Expected `as` in `impl` declaration.");
-    context.emitter().Emit(*context.position(), ImplExpectedAs);
+    if (!state.has_error) {
+      CARBON_DIAGNOSTIC(ImplExpectedAs, Error,
+                        "Expected `as` in `impl` declaration.");
+      context.emitter().Emit(*context.position(), ImplExpectedAs);
+    }
     context.ReturnErrorOnState();
   }
 }

toolchain/parse/node_kind.def

@@ -666,9 +666,9 @@ CARBON_PARSE_NODE_KIND_BRACKET(InterfaceDecl, InterfaceIntroducer,
 // `impl ... as`:
 //     ImplIntroducer
 //     _repeated_ _external_: modifier
-//     _optional_ ImplForall (see below)
+//     _optional_ _external_: ImplForall
 //     _optional_ _external_: expression
-//     ImplAs
+//     _external_: DefaultSelfImplAs or TypeImplAs
 //     _external_: expression
 //   ImplDefinitionStart
 //   _external_: declarations
@@ -678,18 +678,24 @@ CARBON_PARSE_NODE_KIND_BRACKET(InterfaceDecl, InterfaceIntroducer,
 // ImplDefinitionStart and later nodes are removed and replaced by
 // ImplDecl.
 CARBON_PARSE_NODE_KIND_CHILD_COUNT(ImplIntroducer, 0, Impl)
-
-// `forall ...`:
-//   _external_: ImplicitParamList
-// ImplForall
-CARBON_PARSE_NODE_KIND_CHILD_COUNT(ImplForall, 1, Forall)
-CARBON_PARSE_NODE_KIND_CHILD_COUNT(ImplAs, 0, As)
 CARBON_PARSE_NODE_KIND_BRACKET(ImplDefinitionStart, ImplIntroducer,
                                OpenCurlyBrace)
 CARBON_PARSE_NODE_KIND_BRACKET(ImplDefinition, ImplDefinitionStart,
                                CloseCurlyBrace)
 CARBON_PARSE_NODE_KIND_BRACKET(ImplDecl, ImplIntroducer, CARBON_IF_VALID(Semi))
 
+// `forall ...`:
+//   _external_: ImplicitParamList
+// ImplForall
+CARBON_PARSE_NODE_KIND_CHILD_COUNT(ImplForall, 1, Forall)
+
+// `... as`:
+//   _external_: expression
+// TypeImplAs
+CARBON_PARSE_NODE_KIND_CHILD_COUNT(TypeImplAs, 1, As)
+// `as` without a type before it
+CARBON_PARSE_NODE_KIND_CHILD_COUNT(DefaultSelfImplAs, 0, As)
+
 // `constraint`:
 //     NamedConstraintIntroducer
 //     _repeated_ _external_: modifier

toolchain/parse/testdata/generics/impl/basic.carbon

@@ -12,8 +12,8 @@ impl i32 as Interface {
 // CHECK:STDOUT:   parse_tree: [
 // CHECK:STDOUT:     {kind: 'FileStart', text: ''},
 // CHECK:STDOUT:         {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:         {kind: 'IntTypeLiteral', text: 'i32'},
-// CHECK:STDOUT:         {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:           {kind: 'IntTypeLiteral', text: 'i32'},
+// CHECK:STDOUT:         {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'Interface'},
 // CHECK:STDOUT:       {kind: 'ImplDefinitionStart', text: '{', subtree_size: 5},
 // CHECK:STDOUT:         {kind: 'FunctionIntroducer', text: 'fn'},

toolchain/parse/testdata/generics/impl/class.carbon

@@ -22,23 +22,23 @@ class C {
 // CHECK:STDOUT:         {kind: 'IdentifierName', text: 'C'},
 // CHECK:STDOUT:       {kind: 'ClassDefinitionStart', text: '{', subtree_size: 3},
 // CHECK:STDOUT:         {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:         {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:         {kind: 'DefaultSelfImplAs', text: 'as'},
 // CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'ForwardDeclared'},
 // CHECK:STDOUT:       {kind: 'ImplDecl', text: ';', subtree_size: 4},
 // CHECK:STDOUT:           {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'C'},
-// CHECK:STDOUT:           {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:             {kind: 'IdentifierNameExpr', text: 'C'},
+// CHECK:STDOUT:           {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'ExplicitType'},
 // CHECK:STDOUT:         {kind: 'ImplDefinitionStart', text: '{', subtree_size: 5},
 // CHECK:STDOUT:       {kind: 'ImplDefinition', text: '}', subtree_size: 6},
 // CHECK:STDOUT:         {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:         {kind: 'SelfTypeNameExpr', text: 'Self'},
-// CHECK:STDOUT:         {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:           {kind: 'SelfTypeNameExpr', text: 'Self'},
+// CHECK:STDOUT:         {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'ExplicitSelf'},
 // CHECK:STDOUT:       {kind: 'ImplDecl', text: ';', subtree_size: 5},
 // CHECK:STDOUT:           {kind: 'ImplIntroducer', text: 'impl'},
 // CHECK:STDOUT:           {kind: 'ExtendModifier', text: 'extend'},
-// CHECK:STDOUT:           {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:           {kind: 'DefaultSelfImplAs', text: 'as'},
 // CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'ExtendImpl'},
 // CHECK:STDOUT:         {kind: 'ImplDefinitionStart', text: '{', subtree_size: 5},
 // CHECK:STDOUT:           {kind: 'FunctionIntroducer', text: 'fn'},
@@ -49,8 +49,8 @@ class C {
 // CHECK:STDOUT:       {kind: 'ImplDefinition', text: '}', subtree_size: 11},
 // CHECK:STDOUT:         {kind: 'ImplIntroducer', text: 'impl'},
 // CHECK:STDOUT:         {kind: 'ExtendModifier', text: 'extend'},
-// CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'C'},
-// CHECK:STDOUT:         {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'C'},
+// CHECK:STDOUT:         {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'InvalidButDiagnosedInCheck'},
 // CHECK:STDOUT:       {kind: 'ImplDecl', text: ';', subtree_size: 6},
 // CHECK:STDOUT:     {kind: 'ClassDefinition', text: '}', subtree_size: 36},

toolchain/parse/testdata/generics/impl/declaration.carbon

@@ -10,8 +10,8 @@ impl bool as Interface;
 // CHECK:STDOUT:   parse_tree: [
 // CHECK:STDOUT:     {kind: 'FileStart', text: ''},
 // CHECK:STDOUT:       {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:       {kind: 'BoolTypeLiteral', text: 'bool'},
-// CHECK:STDOUT:       {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:         {kind: 'BoolTypeLiteral', text: 'bool'},
+// CHECK:STDOUT:       {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:       {kind: 'IdentifierNameExpr', text: 'Interface'},
 // CHECK:STDOUT:     {kind: 'ImplDecl', text: ';', subtree_size: 5},
 // CHECK:STDOUT:     {kind: 'FileEnd', text: ''},

toolchain/parse/testdata/generics/impl/empty_body.carbon

@@ -11,8 +11,8 @@ impl String as Interface {
 // CHECK:STDOUT:   parse_tree: [
 // CHECK:STDOUT:     {kind: 'FileStart', text: ''},
 // CHECK:STDOUT:         {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:         {kind: 'StringTypeLiteral', text: 'String'},
-// CHECK:STDOUT:         {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:           {kind: 'StringTypeLiteral', text: 'String'},
+// CHECK:STDOUT:         {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'Interface'},
 // CHECK:STDOUT:       {kind: 'ImplDefinitionStart', text: '{', subtree_size: 5},
 // CHECK:STDOUT:     {kind: 'ImplDefinition', text: '}', subtree_size: 6},

toolchain/parse/testdata/generics/impl/fail_impl.carbon

@@ -19,6 +19,16 @@ impl i32 as;
 // CHECK:STDERR:                  ^~~~~~~~~~
 impl bool as bar unexpected;
 
+// CHECK:STDERR: fail_impl.carbon:[[@LINE+3]]:6: ERROR: Expected expression.
+// CHECK:STDERR: impl return as A;
+// CHECK:STDERR:      ^~~~~~
+impl return as A;
+
+// CHECK:STDERR: fail_impl.carbon:[[@LINE+3]]:6: ERROR: Expected expression.
+// CHECK:STDERR: impl return B;
+// CHECK:STDERR:      ^~~~~~
+impl return B;
+
 // CHECK:STDERR: fail_impl.carbon:[[@LINE+6]]:13: ERROR: Expected `[` after `forall` in `impl` declaration.
 // CHECK:STDERR: impl forall f32;
 // CHECK:STDERR:             ^~~
@@ -67,7 +77,7 @@ impl;
 
 impl
 
-// CHECK:STDERR: fail_impl.carbon:[[@LINE+82]]:21: ERROR: Expected expression.
+// CHECK:STDERR: fail_impl.carbon:[[@LINE+88]]:21: ERROR: Expected expression.
 // CHECK:STDERR: // CHECK:STDOUT:   ]
 // CHECK:STDERR:                     ^
 // CHECK:STDOUT: - filename: fail_impl.carbon
@@ -77,16 +87,22 @@ impl
 // CHECK:STDOUT:       {kind: 'IdentifierNameExpr', text: 'foo'},
 // CHECK:STDOUT:     {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 3},
 // CHECK:STDOUT:       {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:       {kind: 'IntTypeLiteral', text: 'i32'},
-// CHECK:STDOUT:       {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:         {kind: 'IntTypeLiteral', text: 'i32'},
+// CHECK:STDOUT:       {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:       {kind: 'InvalidParse', text: ';', has_error: yes},
 // CHECK:STDOUT:     {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 5},
 // CHECK:STDOUT:       {kind: 'ImplIntroducer', text: 'impl'},
-// CHECK:STDOUT:       {kind: 'BoolTypeLiteral', text: 'bool'},
-// CHECK:STDOUT:       {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:         {kind: 'BoolTypeLiteral', text: 'bool'},
+// CHECK:STDOUT:       {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:       {kind: 'IdentifierNameExpr', text: 'bar'},
 // CHECK:STDOUT:     {kind: 'ImplDecl', text: ';', has_error: yes, subtree_size: 5},
 // CHECK:STDOUT:       {kind: 'ImplIntroducer', text: '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: '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: 'FloatTypeLiteral', text: 'f32'},
@@ -109,8 +125,8 @@ impl
 // 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: 'FloatTypeLiteral', text: 'f16'},
-// CHECK:STDOUT:       {kind: 'ImplAs', text: 'as'},
+// 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'},
@@ -138,8 +154,8 @@ impl
 // CHECK:STDOUT:           {kind: 'GenericBindingPattern', text: ':!', subtree_size: 3},
 // CHECK:STDOUT:         {kind: 'ImplicitParamList', text: ']', subtree_size: 5},
 // CHECK:STDOUT:       {kind: 'ImplForall', text: 'forall', subtree_size: 6},
-// CHECK:STDOUT:       {kind: 'IdentifierNameExpr', text: 'T'},
-// CHECK:STDOUT:       {kind: 'ImplAs', text: 'as'},
+// 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: ';', has_error: yes, subtree_size: 11},
 // CHECK:STDOUT:       {kind: 'ImplIntroducer', text: 'impl'},

toolchain/parse/testdata/generics/impl/forall.carbon

@@ -19,8 +19,8 @@ impl forall [T:! type, U:! Interface] U as Interface(T) {
 // CHECK:STDOUT:           {kind: 'GenericBindingPattern', text: ':!', subtree_size: 3},
 // CHECK:STDOUT:         {kind: 'ImplicitParamList', text: ']', subtree_size: 5},
 // CHECK:STDOUT:       {kind: 'ImplForall', text: 'forall', subtree_size: 6},
-// CHECK:STDOUT:       {kind: 'IdentifierNameExpr', text: 'T'},
-// CHECK:STDOUT:       {kind: 'ImplAs', text: 'as'},
+// 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'},
@@ -34,8 +34,8 @@ impl forall [T:! type, U:! Interface] U as Interface(T) {
 // CHECK:STDOUT:             {kind: 'GenericBindingPattern', text: ':!', subtree_size: 3},
 // CHECK:STDOUT:           {kind: 'ImplicitParamList', text: ']', subtree_size: 9},
 // CHECK:STDOUT:         {kind: 'ImplForall', text: 'forall', subtree_size: 10},
-// CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'U'},
-// CHECK:STDOUT:         {kind: 'ImplAs', text: 'as'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'U'},
+// CHECK:STDOUT:         {kind: 'TypeImplAs', text: 'as', subtree_size: 2},
 // CHECK:STDOUT:             {kind: 'IdentifierNameExpr', text: 'Interface'},
 // CHECK:STDOUT:           {kind: 'CallExprStart', text: '(', subtree_size: 2},
 // CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'T'},

toolchain/parse/typed_nodes.h

@@ -849,8 +849,6 @@ struct InterfaceDefinition {
 
 // `impl`
 using ImplIntroducer = LeafNode<NodeKind::ImplIntroducer>;
-// `as`
-using ImplAs = LeafNode<NodeKind::ImplAs>;
 
 // `forall [...]`
 struct ImplForall {
@@ -859,6 +857,16 @@ struct ImplForall {
   ImplicitParamListId params;
 };
 
+// `as` with no type before it
+using DefaultSelfImplAs = LeafNode<NodeKind::DefaultSelfImplAs>;
+
+// `<type> as`
+struct TypeImplAs {
+  static constexpr auto Kind = NodeKind::TypeImplAs.Define();
+
+  std::optional<AnyExprId> type_expr;
+};
+
 // `impl T as I`
 template <const NodeKind& KindT, NodeCategory Category>
 struct ImplSignature {
@@ -867,8 +875,7 @@ struct ImplSignature {
   ImplIntroducerId introducer;
   llvm::SmallVector<AnyModifierId> modifiers;
   std::optional<ImplForallId> forall;
-  std::optional<AnyExprId> type_expr;
-  ImplAsId as;
+  NodeIdOneOf<DefaultSelfImplAs, TypeImplAs> as;
   AnyExprId interface;
 };