Clean up some uses of the node stack. (#3512)

Also minor cleanups for the stack itself.

toolchain/check/context.cpp

@@ -590,7 +590,7 @@ auto Context::ParamOrArgComma() -> void {
 }
 
 auto Context::ParamOrArgEndNoPop(Parse::NodeKind start_kind) -> void {
-  if (parse_tree_->node_kind(node_stack_.PeekParseNode()) != start_kind) {
+  if (!node_stack_.PeekIs(start_kind)) {
     ParamOrArgSave(node_stack_.PopExpr());
   }
 }

toolchain/check/decl_name_stack.cpp

@@ -27,13 +27,10 @@ auto DeclNameStack::PushScopeAndStartName() -> void {
 auto DeclNameStack::FinishName() -> NameContext {
   CARBON_CHECK(decl_name_stack_.back().state != NameContext::State::Finished)
       << "Finished name twice";
-  if (context_->parse_tree().node_kind(
-          context_->node_stack().PeekParseNode()) ==
-      Parse::NodeKind::QualifiedDecl) {
+  if (context_->node_stack()
+          .PopAndDiscardSoloParseNodeIf<Parse::NodeKind::QualifiedDecl>()) {
     // Any parts from a QualifiedDecl will already have been processed
     // into the name.
-    context_->node_stack()
-        .PopAndDiscardSoloParseNode<Parse::NodeKind::QualifiedDecl>();
   } else {
     // The name had no qualifiers, so we need to process the node now.
     auto [parse_node, name_id] = context_->node_stack().PopNameWithParseNode();

toolchain/check/handle_array.cpp

@@ -21,9 +21,8 @@ auto HandleArrayExprSemi(Context& context, Parse::NodeId parse_node) -> bool {
 
 auto HandleArrayExpr(Context& context, Parse::NodeId parse_node) -> bool {
   // TODO: Handle array type with undefined bound.
-  if (context.parse_tree().node_kind(context.node_stack().PeekParseNode()) ==
-      Parse::NodeKind::ArrayExprSemi) {
-    context.node_stack().PopAndIgnore();
+  if (context.node_stack()
+          .PopAndDiscardSoloParseNodeIf<Parse::NodeKind::ArrayExprSemi>()) {
     context.node_stack().PopAndIgnore();
     return context.TODO(parse_node, "HandleArrayExprWithoutBounds");
   }

toolchain/check/handle_binding_pattern.cpp

@@ -57,8 +57,8 @@ auto HandleBindingPattern(Context& context, Parse::NodeId parse_node) -> bool {
   // error locations.
   // TODO: The node stack is a fragile way of getting context information.
   // Get this information from somewhere else.
-  switch (auto context_parse_node_kind = context.parse_tree().node_kind(
-              context.node_stack().PeekParseNode())) {
+  switch (auto context_parse_node_kind =
+              context.node_stack().PeekParseNodeKind()) {
     case Parse::NodeKind::ReturnedModifier:
     case Parse::NodeKind::VariableIntroducer: {
       // A `var` declaration at class scope introduces a field.

toolchain/check/handle_function.cpp

@@ -57,18 +57,17 @@ static auto BuildFunctionDecl(Context& context, Parse::NodeId parse_node,
 
   auto return_type_id = SemIR::TypeId::Invalid;
   auto return_slot_id = SemIR::InstId::Invalid;
-  if (context.parse_tree().node_kind(context.node_stack().PeekParseNode()) ==
-      Parse::NodeKind::ReturnType) {
-    auto [return_node, return_storage_id] =
-        context.node_stack().PopWithParseNode<Parse::NodeKind::ReturnType>();
-    auto return_node_copy = return_node;
+  if (auto return_node_and_id =
+          context.node_stack()
+              .PopWithParseNodeIf<Parse::NodeKind::ReturnType>()) {
+    auto return_storage_id = return_node_and_id->second;
     return_type_id = context.insts().Get(return_storage_id).type_id();
 
     return_type_id = context.AsCompleteType(return_type_id, [&] {
       CARBON_DIAGNOSTIC(IncompleteTypeInFunctionReturnType, Error,
                         "Function returns incomplete type `{0}`.", std::string);
       return context.emitter().Build(
-          return_node_copy, IncompleteTypeInFunctionReturnType,
+          return_node_and_id->first, IncompleteTypeInFunctionReturnType,
           context.sem_ir().StringifyType(return_type_id));
     });
 

toolchain/check/handle_if_statement.cpp

@@ -46,8 +46,7 @@ auto HandleIfStatementElse(Context& context, Parse::NodeId parse_node) -> bool {
 }
 
 auto HandleIfStatement(Context& context, Parse::NodeId parse_node) -> bool {
-  switch (auto kind = context.parse_tree().node_kind(
-              context.node_stack().PeekParseNode())) {
+  switch (auto kind = context.node_stack().PeekParseNodeKind()) {
     case Parse::NodeKind::IfCondition: {
       // Branch from then block to else block, and start emitting the else
       // block.

toolchain/check/handle_return_statement.cpp

@@ -22,8 +22,7 @@ auto HandleReturnVarModifier(Context& context, Parse::NodeId parse_node)
 }
 
 auto HandleReturnStatement(Context& context, Parse::NodeId parse_node) -> bool {
-  switch (
-      context.parse_tree().node_kind(context.node_stack().PeekParseNode())) {
+  switch (context.node_stack().PeekParseNodeKind()) {
     case Parse::NodeKind::ReturnStatementStart:
       // This is a `return;` statement.
       context.node_stack()

toolchain/check/handle_variable.cpp

@@ -34,8 +34,7 @@ auto HandleVariableInitializer(Context& context, Parse::NodeId parse_node)
 auto HandleVariableDecl(Context& context, Parse::NodeId parse_node) -> bool {
   // Handle the optional initializer.
   auto init_id = SemIR::InstId::Invalid;
-  Parse::NodeKind next_kind =
-      context.parse_tree().node_kind(context.node_stack().PeekParseNode());
+  Parse::NodeKind next_kind = context.node_stack().PeekParseNodeKind();
   if (next_kind == Parse::NodeKind::TuplePattern) {
     return context.TODO(parse_node, "tuple pattern in var");
   }

toolchain/check/node_stack.h

@@ -39,10 +39,11 @@ class NodeStack {
   // Pushes a solo parse tree node onto the stack. Used when there is no
   // IR generated by the node.
   auto Push(Parse::NodeId parse_node) -> void {
-    CARBON_CHECK(ParseNodeIdKind(parse_node) == IdKind::SoloParseNode)
-        << "Parse kind expects an Id: " << parse_tree_->node_kind(parse_node);
-    CARBON_VLOG() << "Node Push " << stack_.size() << ": "
-                  << parse_tree_->node_kind(parse_node) << " -> <none>\n";
+    auto kind = parse_tree_->node_kind(parse_node);
+    CARBON_CHECK(ParseNodeKindToIdKind(kind) == IdKind::SoloParseNode)
+        << "Parse kind expects an Id: " << kind;
+    CARBON_VLOG() << "Node Push " << stack_.size() << ": " << kind
+                  << " -> <none>\n";
     CARBON_CHECK(stack_.size() < (1 << 20))
         << "Excessive stack size: likely infinite loop";
     stack_.push_back(Entry(parse_node, SemIR::InstId::Invalid));
@@ -51,47 +52,57 @@ class NodeStack {
   // Pushes a parse tree node onto the stack with an ID.
   template <typename IdT>
   auto Push(Parse::NodeId parse_node, IdT id) -> void {
-    CARBON_CHECK(ParseNodeIdKind(parse_node) == IdTypeToIdKind<IdT>())
-        << "Parse kind expected a different IdT: "
-        << parse_tree_->node_kind(parse_node) << " -> " << id << "\n";
+    auto kind = parse_tree_->node_kind(parse_node);
+    CARBON_CHECK(ParseNodeKindToIdKind(kind) == IdTypeToIdKind<IdT>())
+        << "Parse kind expected a different IdT: " << kind << " -> " << id
+        << "\n";
     CARBON_CHECK(id.is_valid()) << "Push called with invalid id: "
                                 << parse_tree_->node_kind(parse_node);
-    CARBON_VLOG() << "Node Push " << stack_.size() << ": "
-                  << parse_tree_->node_kind(parse_node) << " -> " << id << "\n";
+    CARBON_VLOG() << "Node Push " << stack_.size() << ": " << kind << " -> "
+                  << id << "\n";
     CARBON_CHECK(stack_.size() < (1 << 20))
         << "Excessive stack size: likely infinite loop";
     stack_.push_back(Entry(parse_node, id));
   }
 
-  // Returns whether the node on the top of the stack is the specified kind.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
-  auto PeekIs() const -> bool {
-    return !stack_.empty() &&
-           parse_tree_->node_kind(PeekParseNode()) == RequiredParseKind;
+  // Returns whether there is a node of the specified kind on top of the stack.
+  auto PeekIs(Parse::NodeKind kind) const -> bool {
+    return !stack_.empty() && PeekParseNodeKind() == kind;
   }
 
-  // Returns whether the node on the top of the stack is a name.
+  // Returns whether there is a node of the specified kind on top of the stack.
+  // Templated for consistency with other functions taking a parse node kind.
+  template <const Parse::NodeKind& RequiredParseKind>
+  auto PeekIs() const -> bool {
+    return PeekIs(RequiredParseKind);
+  }
+
+  // Returns whether there is a name on top of the stack.
   auto PeekIsName() const -> bool {
     return !stack_.empty() &&
-           ParseNodeIdKind(PeekParseNode()) == IdKind::NameId;
+           ParseNodeKindToIdKind(PeekParseNodeKind()) == IdKind::NameId;
   }
 
   // Pops the top of the stack without any verification.
-  auto PopAndIgnore() -> void { PopEntry<SemIR::InstId>(); }
+  auto PopAndIgnore() -> void {
+    Entry back = stack_.pop_back_val();
+    CARBON_VLOG() << "Node Pop " << stack_.size() << ": "
+                  << parse_tree_->node_kind(back.parse_node)
+                  << " -> <ignored>\n";
+  }
 
   // Pops the top of the stack and returns the parse_node.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto PopForSoloParseNode() -> Parse::NodeId {
     Entry back = PopEntry<SemIR::InstId>();
-    RequireIdKind(Parse::NodeKind::Create(RequiredParseKind),
-                  IdKind::SoloParseNode);
+    RequireIdKind(RequiredParseKind, IdKind::SoloParseNode);
     RequireParseKind<RequiredParseKind>(back.parse_node);
     return back.parse_node;
   }
 
   // Pops the top of the stack if it is the given kind, and returns the
   // parse_node. Otherwise, returns std::nullopt.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto PopForSoloParseNodeIf() -> std::optional<Parse::NodeId> {
     if (PeekIs<RequiredParseKind>()) {
       return PopForSoloParseNode<RequiredParseKind>();
@@ -100,14 +111,14 @@ class NodeStack {
   }
 
   // Pops the top of the stack.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto PopAndDiscardSoloParseNode() -> void {
     PopForSoloParseNode<RequiredParseKind>();
   }
 
   // Pops the top of the stack if it is the given kind. Returns `true` if a node
   // was popped.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto PopAndDiscardSoloParseNodeIf() -> bool {
     if (!PeekIs<RequiredParseKind>()) {
       return false;
@@ -129,10 +140,9 @@ class NodeStack {
   }
 
   // Pops the top of the stack and returns the parse_node and the ID.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto PopWithParseNode() -> auto {
-    constexpr IdKind RequiredIdKind =
-        ParseNodeKindToIdKind(Parse::NodeKind::Create(RequiredParseKind));
+    constexpr IdKind RequiredIdKind = ParseNodeKindToIdKind(RequiredParseKind);
     if constexpr (RequiredIdKind == IdKind::InstId) {
       auto back = PopWithParseNode<SemIR::InstId>();
       RequireParseKind<RequiredParseKind>(back.first);
@@ -168,11 +178,21 @@ class NodeStack {
       RequireParseKind<RequiredParseKind>(back.first);
       return back;
     }
-    CARBON_FATAL() << "Unpoppable IdKind for parse kind: "
-                   << Parse::NodeKind::Create(RequiredParseKind)
+    CARBON_FATAL() << "Unpoppable IdKind for parse kind: " << RequiredParseKind
                    << "; see value in ParseNodeKindToIdKind";
   }
 
+  // Pops the top of the stack and returns the parse_node and the ID if it is
+  // of the specified kind.
+  template <const Parse::NodeKind& RequiredParseKind>
+  auto PopWithParseNodeIf()
+      -> std::optional<decltype(PopWithParseNode<RequiredParseKind>())> {
+    if (!PeekIs<RequiredParseKind>()) {
+      return std::nullopt;
+    }
+    return PopWithParseNode<RequiredParseKind>();
+  }
+
   // Pops an expression from the top of the stack and returns the ID.
   // Expressions map multiple Parse::NodeKinds to SemIR::InstId always.
   auto PopExpr() -> SemIR::InstId { return PopExprWithParseNode().second; }
@@ -181,14 +201,14 @@ class NodeStack {
   auto PopName() -> SemIR::NameId { return PopNameWithParseNode().second; }
 
   // Pops the top of the stack and returns the ID.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto Pop() -> auto {
     return PopWithParseNode<RequiredParseKind>().second;
   }
 
   // Pops the top of the stack if it has the given kind, and returns the ID.
   // Otherwise returns std::nullopt.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto PopIf() -> std::optional<decltype(Pop<RequiredParseKind>())> {
     if (PeekIs<RequiredParseKind>()) {
       return Pop<RequiredParseKind>();
@@ -196,18 +216,22 @@ class NodeStack {
     return std::nullopt;
   }
 
-  // Peeks at the parse node of the top of the name stack.
+  // Peeks at the parse node of the top of the node stack.
   auto PeekParseNode() const -> Parse::NodeId {
     return stack_.back().parse_node;
   }
 
+  // Peeks at the kind of the parse node of the top of the node stack.
+  auto PeekParseNodeKind() const -> Parse::NodeKind {
+    return parse_tree_->node_kind(PeekParseNode());
+  }
+
   // Peeks at the ID associated with the top of the name stack.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto Peek() const -> auto {
     Entry back = stack_.back();
     RequireParseKind<RequiredParseKind>(back.parse_node);
-    constexpr IdKind RequiredIdKind =
-        ParseNodeKindToIdKind(Parse::NodeKind::Create(RequiredParseKind));
+    constexpr IdKind RequiredIdKind = ParseNodeKindToIdKind(RequiredParseKind);
     if constexpr (RequiredIdKind == IdKind::InstId) {
       return back.id<SemIR::InstId>();
     }
@@ -229,8 +253,7 @@ class NodeStack {
     if constexpr (RequiredIdKind == IdKind::TypeId) {
       return back.id<SemIR::TypeId>();
     }
-    CARBON_FATAL() << "Unpeekable IdKind for parse kind: "
-                   << Parse::NodeKind::Create(RequiredParseKind)
+    CARBON_FATAL() << "Unpeekable IdKind for parse kind: " << RequiredParseKind
                    << "; see value in ParseNodeKindToIdKind";
   }
 
@@ -405,10 +428,6 @@ class NodeStack {
     }
   }
 
-  auto ParseNodeIdKind(Parse::NodeId node) const -> IdKind {
-    return ParseNodeKindToIdKind(parse_tree_->node_kind(node));
-  }
-
   // Translates an ID type to the enum ID kind for comparison with
   // ParseNodeKindToIdKind.
   template <typename IdT>
@@ -462,12 +481,11 @@ class NodeStack {
   }
 
   // Require an entry to have the given Parse::NodeKind.
-  template <Parse::NodeKind::RawEnumType RequiredParseKind>
+  template <const Parse::NodeKind& RequiredParseKind>
   auto RequireParseKind(Parse::NodeId parse_node) const -> void {
     auto actual_kind = parse_tree_->node_kind(parse_node);
     CARBON_CHECK(RequiredParseKind == actual_kind)
-        << "Expected " << Parse::NodeKind::Create(RequiredParseKind)
-        << ", found " << actual_kind;
+        << "Expected " << RequiredParseKind << ", found " << actual_kind;
   }
 
   // The file's parse tree.