Refactor the node stack into its own class. (#2505)

Currently, there's a mix of accessing node_stack_ both directly and indirectly, and there are already several Push/Pop methods to help wrap the behavior. However, there's also direct access because of shifting over time, as well as variations in _how_ the stack is used.

This migrates to a separate class in order to make a more specific contract for the API. It cleans up existing uses and adds APIs where needed.

toolchain/common/index_base.h

@@ -27,7 +27,13 @@ struct IndexBase {
   constexpr IndexBase() : index(InvalidIndex) {}
   constexpr explicit IndexBase(int index) : index(index) {}
 
-  auto Print(llvm::raw_ostream& output) const -> void { output << index; }
+  auto Print(llvm::raw_ostream& output) const -> void {
+    if (is_valid()) {
+      output << index;
+    } else {
+      output << "<invalid>";
+    }
+  }
 
   auto is_valid() const -> bool { return index != InvalidIndex; }
 

toolchain/semantics/BUILD

@@ -20,21 +20,49 @@ cc_library(
     deps = ["//toolchain/common:enum_base"],
 )
 
+cc_library(
+    name = "semantics_node",
+    srcs = ["semantics_node.cpp"],
+    hdrs = ["semantics_node.h"],
+    deps = [
+        ":semantics_builtin_kind",
+        ":semantics_node_kind",
+        "//common:check",
+        "//common:ostream",
+        "//toolchain/parser:parse_tree",
+        "@llvm-project//llvm:Support",
+    ],
+)
+
+cc_library(
+    name = "semantics_node_stack",
+    srcs = ["semantics_node_stack.cpp"],
+    hdrs = ["semantics_node_stack.h"],
+    deps = [
+        ":semantics_node",
+        "//common:check",
+        "//common:ostream",
+        "//common:vlog",
+        "//toolchain/parser:parse_node_kind",
+        "//toolchain/parser:parse_tree",
+        "@llvm-project//llvm:Support",
+    ],
+)
+
 cc_library(
     name = "semantics_ir",
     srcs = [
         "semantics_ir.cpp",
-        "semantics_node.cpp",
         "semantics_parse_tree_handler.cpp",
     ],
     hdrs = [
         "semantics_ir.h",
-        "semantics_node.h",
         "semantics_parse_tree_handler.h",
     ],
     deps = [
         ":semantics_builtin_kind",
-        ":semantics_node_kind",
+        ":semantics_node",
+        ":semantics_node_stack",
         "//common:check",
         "//common:ostream",
         "//common:vlog",

toolchain/semantics/semantics_node.h

@@ -28,31 +28,46 @@ struct SemanticsNodeId : public IndexBase {
   static auto MakeInvalid() -> SemanticsNodeId { return SemanticsNodeId(); }
 
   using IndexBase::IndexBase;
-  auto Print(llvm::raw_ostream& out) const -> void { out << "node" << index; }
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "node";
+    IndexBase::Print(out);
+  }
 };
 
 // The ID of a cross-referenced IR.
 struct SemanticsCrossReferenceIRId : public IndexBase {
   using IndexBase::IndexBase;
-  auto Print(llvm::raw_ostream& out) const -> void { out << "ir" << index; }
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "ir";
+    IndexBase::Print(out);
+  }
 };
 
 // Type-safe storage of integer literals.
 struct SemanticsIntegerLiteralId : public IndexBase {
   using IndexBase::IndexBase;
-  auto Print(llvm::raw_ostream& out) const -> void { out << "int" << index; }
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "int";
+    IndexBase::Print(out);
+  }
 };
 
 // Type-safe storage of node blocks.
 struct SemanticsNodeBlockId : public IndexBase {
   using IndexBase::IndexBase;
-  auto Print(llvm::raw_ostream& out) const -> void { out << "block" << index; }
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "block";
+    IndexBase::Print(out);
+  }
 };
 
 // Type-safe storage of strings.
 struct SemanticsStringId : public IndexBase {
   using IndexBase::IndexBase;
-  auto Print(llvm::raw_ostream& out) const -> void { out << "str" << index; }
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "str";
+    IndexBase::Print(out);
+  }
 };
 
 // The standard structure for nodes.

toolchain/semantics/semantics_node_stack.cpp

@@ -0,0 +1,140 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "toolchain/semantics/semantics_node_stack.h"
+
+#include "common/vlog.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "toolchain/semantics/semantics_node.h"
+
+namespace Carbon {
+
+auto SemanticsNodeStack::PushEntry(Entry entry, bool is_node_id) -> void {
+  CARBON_VLOG() << "Push " << stack_.size() << ": "
+                << parse_tree_->node_kind(entry.parse_node) << " -> ";
+  if (is_node_id) {
+    CARBON_VLOG() << entry.node_id;
+  } else {
+    CARBON_VLOG() << entry.name_id;
+  }
+  CARBON_VLOG() << "\n";
+  CARBON_CHECK(stack_.size() < (1 << 20))
+      << "Excessive stack size: likely infinite loop";
+  stack_.push_back(entry);
+}
+
+auto SemanticsNodeStack::PopEntry() -> Entry {
+  auto back = stack_.pop_back_val();
+  CARBON_VLOG() << "Pop " << stack_.size() << ": any ("
+                << parse_tree_->node_kind(back.parse_node) << ") -> "
+                << back.node_id << "\n";
+  return back;
+}
+
+auto SemanticsNodeStack::PopEntry(ParseNodeKind pop_parse_kind) -> Entry {
+  auto back = stack_.pop_back_val();
+  CARBON_VLOG() << "Pop " << stack_.size() << ": " << pop_parse_kind << " -> "
+                << back.node_id << "\n";
+  RequireParseKind(back, pop_parse_kind);
+  return back;
+}
+
+auto SemanticsNodeStack::RequireParseKind(Entry entry,
+                                          ParseNodeKind require_kind) -> void {
+  auto actual_kind = parse_tree_->node_kind(entry.parse_node);
+  CARBON_CHECK(require_kind == actual_kind)
+      << "Expected " << require_kind << ", found " << actual_kind;
+}
+
+auto SemanticsNodeStack::RequireSoloParseNode(Entry entry) -> void {
+  CARBON_CHECK(!entry.node_id.is_valid())
+      << "Expected invalid node_id on "
+      << parse_tree_->node_kind(entry.parse_node) << ", was " << entry.node_id;
+}
+
+auto SemanticsNodeStack::RequireNodeId(Entry entry) -> void {
+  CARBON_CHECK(entry.node_id.is_valid())
+      << "Expected valid node_id on "
+      << parse_tree_->node_kind(entry.parse_node);
+}
+
+auto SemanticsNodeStack::PopAndDiscardSoloParseNode(
+    ParseNodeKind pop_parse_kind) -> void {
+  auto back = PopEntry(pop_parse_kind);
+  RequireSoloParseNode(back);
+}
+
+auto SemanticsNodeStack::PopForSoloParseNode() -> ParseTree::Node {
+  auto back = PopEntry();
+  RequireSoloParseNode(back);
+  return back.parse_node;
+}
+
+auto SemanticsNodeStack::PopForSoloParseNode(ParseNodeKind pop_parse_kind)
+    -> ParseTree::Node {
+  auto back = PopEntry(pop_parse_kind);
+  RequireSoloParseNode(back);
+  return back.parse_node;
+}
+
+auto SemanticsNodeStack::PopForNodeId() -> SemanticsNodeId {
+  auto back = PopEntry();
+  RequireNodeId(back);
+  return back.node_id;
+}
+
+auto SemanticsNodeStack::PopForNodeId(ParseNodeKind pop_parse_kind)
+    -> SemanticsNodeId {
+  auto back = PopEntry(pop_parse_kind);
+  RequireNodeId(back);
+  return back.node_id;
+}
+
+auto SemanticsNodeStack::PopForParseNodeAndNodeId()
+    -> std::pair<ParseTree::Node, SemanticsNodeId> {
+  auto back = PopEntry();
+  RequireNodeId(back);
+  return {back.parse_node, back.node_id};
+}
+
+auto SemanticsNodeStack::PopForParseNodeAndNodeId(ParseNodeKind pop_parse_kind)
+    -> std::pair<ParseTree::Node, SemanticsNodeId> {
+  auto back = PopEntry(pop_parse_kind);
+  RequireNodeId(back);
+  return {back.parse_node, back.node_id};
+}
+
+auto SemanticsNodeStack::PopForParseNodeAndNameId()
+    -> std::pair<ParseTree::Node, SemanticsStringId> {
+  auto back = PopEntry(ParseNodeKind::PatternBinding);
+  RequireNodeId(back);
+  return {back.parse_node, back.name_id};
+}
+
+auto SemanticsNodeStack::PeekForNameId() -> SemanticsStringId {
+  auto back = stack_.back();
+  RequireParseKind(back, ParseNodeKind::PatternBinding);
+  RequireNodeId(back);
+  return back.name_id;
+}
+
+auto SemanticsNodeStack::PrintForStackDump(llvm::raw_ostream& output) const
+    -> void {
+  output << "node_stack_:\n";
+  for (int i = 0; i < static_cast<int>(stack_.size()); ++i) {
+    const auto& entry = stack_[i];
+    auto parse_node_kind = parse_tree_->node_kind(entry.parse_node);
+    output << "\t" << i << ".\t" << parse_node_kind;
+    if (parse_node_kind == ParseNodeKind::PatternBinding) {
+      output << " -> " << entry.name_id;
+    } else {
+      if (entry.node_id.is_valid()) {
+        output << " -> " << entry.node_id;
+      }
+    }
+    output << "\n";
+  }
+}
+
+}  // namespace Carbon

toolchain/semantics/semantics_node_stack.h

@@ -0,0 +1,145 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_TOOLCHAIN_SEMANTICS_SEMANTICS_NODE_STACK_H_
+#define CARBON_TOOLCHAIN_SEMANTICS_SEMANTICS_NODE_STACK_H_
+
+#include "llvm/ADT/SmallVector.h"
+#include "toolchain/parser/parse_node_kind.h"
+#include "toolchain/parser/parse_tree.h"
+#include "toolchain/semantics/semantics_node.h"
+
+namespace Carbon {
+
+// Wraps the stack of nodes for SemanticsParseTreeHandler.
+//
+// All pushes and pops will be vlogged.
+//
+// Pop APIs will run basic verification:
+//
+// - If receiving a pop_parse_kind, verify that the parse_node being popped is
+//   of pop_parse_kind.
+// - Validates presence of node_id based on whether it's a solo
+//   parse_node.
+//
+// These should be assumed API constraints unless otherwise mentioned on a
+// method. The main exception is PopAndIgnore, which doesn't do verification.
+class SemanticsNodeStack {
+ public:
+  SemanticsNodeStack(const ParseTree& parse_tree,
+                     llvm::raw_ostream* vlog_stream)
+      : parse_tree_(&parse_tree), vlog_stream_(vlog_stream) {}
+
+  // Pushes a solo parse tree node onto the stack. Used when there is no
+  // IR generated by the node.
+  auto Push(ParseTree::Node parse_node) -> void {
+    PushEntry(
+        {.parse_node = parse_node, .node_id = SemanticsNodeId::MakeInvalid()},
+        /*is_node_id=*/true);
+  }
+
+  // Pushes a parse tree node onto the stack.
+  auto Push(ParseTree::Node parse_node, SemanticsNodeId node_id) -> void {
+    PushEntry({.parse_node = parse_node, .node_id = node_id},
+              /*is_node_id=*/true);
+  }
+
+  // Pushes a PatternBinding parse tree node onto the stack with its name.
+  auto Push(ParseTree::Node parse_node, SemanticsStringId name_id) -> void {
+    CARBON_CHECK(parse_tree_->node_kind(parse_node) ==
+                 ParseNodeKind::PatternBinding);
+    PushEntry({.parse_node = parse_node, .name_id = name_id},
+              /*is_node_id=*/false);
+  }
+
+  // Pops the top of the stack without any verification.
+  auto PopAndIgnore() -> void { PopEntry(); }
+
+  // Pops the top of the stack.
+  auto PopAndDiscardSoloParseNode(ParseNodeKind pop_parse_kind) -> void;
+
+  // Pops the top of the stack and returns the parse_node.
+  auto PopForSoloParseNode() -> ParseTree::Node;
+
+  // Pops the top of the stack and returns the parse_node.
+  auto PopForSoloParseNode(ParseNodeKind pop_parse_kind) -> ParseTree::Node;
+
+  // Pops the top of the stack and returns the node_id.
+  auto PopForNodeId(ParseNodeKind pop_parse_kind) -> SemanticsNodeId;
+
+  // Pops the top of the stack and returns the parse_node and node_id.
+  auto PopForParseNodeAndNodeId()
+      -> std::pair<ParseTree::Node, SemanticsNodeId>;
+
+  // Pops the top of the stack and returns the parse_node and node_id.
+  auto PopForParseNodeAndNodeId(ParseNodeKind pop_parse_kind)
+      -> std::pair<ParseTree::Node, SemanticsNodeId>;
+
+  // Pops the top of the stack and returns the node_id.
+  auto PopForNodeId() -> SemanticsNodeId;
+
+  // Pops the top of the stack and returns the parse_node and name_id.
+  // Verifies that the parse_node is a PatternBinding.
+  auto PopForParseNodeAndNameId()
+      -> std::pair<ParseTree::Node, SemanticsStringId>;
+
+  // Peeks at the parse_node of the top of the stack.
+  auto PeekParseNode() -> ParseTree::Node { return stack_.back().parse_node; }
+
+  // Peeks at the name_id of the top of the stack.
+  // Verifies that the parse_node is a PatternBinding.
+  auto PeekForNameId() -> SemanticsStringId;
+
+  // Prints the stack for a stack dump.
+  auto PrintForStackDump(llvm::raw_ostream& output) const -> void;
+
+ private:
+  // An entry in node_stack_.
+  struct Entry {
+    ParseTree::Node parse_node;
+    union {
+      // The node_id will be invalid if and only if it's a solo parse_node.
+      SemanticsNodeId node_id;
+      // The name_id is provided for PatternBindings.
+      SemanticsStringId name_id;
+    };
+  };
+  static_assert(sizeof(Entry) == 8, "Unexpected Entry size");
+
+  // Pushes an entry onto the stack. is_node_id is provided for debug output
+  // only.
+  auto PushEntry(Entry entry, bool is_node_id) -> void;
+
+  // Pops an entry.
+  auto PopEntry() -> Entry;
+
+  // Pops an entry, requiring the specific kind.
+  auto PopEntry(ParseNodeKind pop_parse_kind) -> Entry;
+
+  // Require an entry to have the given ParseNodeKind.
+  auto RequireParseKind(Entry entry, ParseNodeKind require_kind) -> void;
+
+  // Requires an entry to have a invalid node_id.
+  // Also works with name_id in the union due to type compatibility.
+  auto RequireSoloParseNode(Entry entry) -> void;
+
+  // Requires an entry to have a valid node_id.
+  // Also works with name_id in the union due to type compatibility.
+  auto RequireNodeId(Entry entry) -> void;
+
+  // The file's parse tree.
+  const ParseTree* parse_tree_;
+
+  // Whether to print verbose output.
+  llvm::raw_ostream* vlog_stream_;
+
+  // The actual stack.
+  // PushEntry and PopEntry control modification in order to centralize
+  // vlogging.
+  llvm::SmallVector<Entry> stack_;
+};
+
+}  // namespace Carbon
+
+#endif  // CARBON_TOOLCHAIN_SEMANTICS_SEMANTICS_NODE_STACK_H_

toolchain/semantics/semantics_parse_tree_handler.cpp

@@ -22,20 +22,7 @@ class SemanticsParseTreeHandler::PrettyStackTraceNodeStack
   ~PrettyStackTraceNodeStack() override = default;
 
   auto print(llvm::raw_ostream& output) const -> void override {
-    output << "node_stack_:\n";
-    for (int i = 0; i < static_cast<int>(handler_->node_stack_.size()); ++i) {
-      const auto& entry = handler_->node_stack_[i];
-      auto parse_node_kind = handler_->parse_tree_->node_kind(entry.parse_node);
-      output << "\t" << i << ".\t" << parse_node_kind;
-      if (parse_node_kind == ParseNodeKind::PatternBinding) {
-        output << " -> " << entry.name_id;
-      } else {
-        if (entry.result_id.is_valid()) {
-          output << " -> " << entry.result_id;
-        }
-      }
-      output << "\n";
-    }
+    handler_->node_stack_.PrintForStackDump(output);
   }
 
  private:
@@ -95,6 +82,12 @@ auto SemanticsParseTreeHandler::AddNode(SemanticsNode node) -> SemanticsNodeId {
   return semantics_->AddNode(current_block_id(), node);
 }
 
+auto SemanticsParseTreeHandler::AddNodeAndPush(ParseTree::Node parse_node,
+                                               SemanticsNode node) -> void {
+  auto node_id = AddNode(node);
+  node_stack_.Push(parse_node, node_id);
+}
+
 auto SemanticsParseTreeHandler::BindName(ParseTree::Node name_node,
                                          SemanticsNodeId type_id,
                                          SemanticsNodeId target_id)
@@ -122,101 +115,6 @@ auto SemanticsParseTreeHandler::BindName(ParseTree::Node name_node,
   return name_id;
 }
 
-auto SemanticsParseTreeHandler::Push(ParseTree::Node parse_node) -> void {
-  CARBON_VLOG() << "Push " << node_stack_.size() << ": "
-                << parse_tree_->node_kind(parse_node) << "\n";
-  CARBON_CHECK(node_stack_.size() < (1 << 20))
-      << "Excessive stack size: likely infinite loop";
-  node_stack_.push_back(
-      {.parse_node = parse_node, .result_id = SemanticsNodeId::MakeInvalid()});
-}
-
-auto SemanticsParseTreeHandler::Push(ParseTree::Node parse_node,
-                                     SemanticsNode node) -> void {
-  CARBON_VLOG() << "Push " << node_stack_.size() << ": "
-                << parse_tree_->node_kind(parse_node) << " -> " << node.kind()
-                << "\n";
-  CARBON_CHECK(node_stack_.size() < (1 << 20))
-      << "Excessive stack size: likely infinite loop";
-  auto node_id = AddNode(node);
-  node_stack_.push_back({.parse_node = parse_node, .result_id = node_id});
-}
-
-auto SemanticsParseTreeHandler::Push(ParseTree::Node parse_node,
-                                     SemanticsNodeId node_id) -> void {
-  CARBON_VLOG() << "Push " << node_stack_.size() << ": "
-                << parse_tree_->node_kind(parse_node) << " -> " << node_id
-                << "\n";
-  CARBON_CHECK(node_stack_.size() < (1 << 20))
-      << "Excessive stack size: likely infinite loop";
-  node_stack_.push_back({.parse_node = parse_node, .result_id = node_id});
-}
-
-auto SemanticsParseTreeHandler::Push(ParseTree::Node parse_node,
-                                     SemanticsStringId name_id) -> void {
-  CARBON_CHECK(parse_tree_->node_kind(parse_node) ==
-               ParseNodeKind::PatternBinding);
-  CARBON_VLOG() << "Push " << node_stack_.size() << ": "
-                << parse_tree_->node_kind(parse_node) << " -> " << name_id
-                << "\n";
-  CARBON_CHECK(node_stack_.size() < (1 << 20))
-      << "Excessive stack size: likely infinite loop";
-  node_stack_.push_back({.parse_node = parse_node, .name_id = name_id});
-}
-
-auto SemanticsParseTreeHandler::Pop(ParseNodeKind pop_parse_kind) -> void {
-  auto back = node_stack_.pop_back_val();
-  auto parse_kind = parse_tree_->node_kind(back.parse_node);
-  CARBON_VLOG() << "Pop " << node_stack_.size() << ": " << pop_parse_kind
-                << "\n";
-  CARBON_CHECK(parse_kind == pop_parse_kind)
-      << "Expected " << pop_parse_kind << ", found " << parse_kind;
-  CARBON_CHECK(!back.result_id.is_valid())
-      << "Expected no result ID on " << parse_kind << ", was "
-      << back.result_id;
-}
-
-auto SemanticsParseTreeHandler::PopWithResult() -> SemanticsNodeId {
-  auto back = node_stack_.pop_back_val();
-  auto node_id = back.result_id;
-  CARBON_VLOG() << "Pop " << node_stack_.size() << ": any ("
-                << parse_tree_->node_kind(back.parse_node) << ") -> " << node_id
-                << "\n";
-  CARBON_CHECK(node_id.is_valid())
-      << "Invalid PopWithResult on " << parse_tree_->node_kind(back.parse_node);
-  return node_id;
-}
-
-auto SemanticsParseTreeHandler::PopWithResult(ParseNodeKind pop_parse_kind)
-    -> SemanticsNodeId {
-  auto back = node_stack_.pop_back_val();
-  auto parse_kind = parse_tree_->node_kind(back.parse_node);
-  auto node_id = back.result_id;
-  CARBON_VLOG() << "Pop " << node_stack_.size() << ": " << pop_parse_kind
-                << ") -> " << node_id << "\n";
-  CARBON_CHECK(parse_kind == pop_parse_kind)
-      << "Expected " << pop_parse_kind << ", found " << parse_kind;
-  CARBON_CHECK(node_id.is_valid())
-      << "Invalid PopWithResult with " << parse_kind;
-  return node_id;
-}
-
-auto SemanticsParseTreeHandler::PopWithResultIf(ParseNodeKind pop_parse_kind)
-    -> std::optional<SemanticsNodeId> {
-  auto parse_kind = parse_tree_->node_kind(node_stack_.back().parse_node);
-  if (parse_kind != pop_parse_kind) {
-    return std::nullopt;
-  }
-
-  auto back = node_stack_.pop_back_val();
-  auto node_id = back.result_id;
-  CARBON_VLOG() << "Pop " << node_stack_.size() << ": " << pop_parse_kind
-                << ") -> " << node_id << "\n";
-  CARBON_CHECK(node_id.is_valid())
-      << "Invalid PopWithResult with " << parse_kind;
-  return node_id;
-}
-
 auto SemanticsParseTreeHandler::PushScope() -> void {
   scope_stack_.push_back({});
 }
@@ -311,7 +209,7 @@ auto SemanticsParseTreeHandler::HandleContinueStatementStart(
 auto SemanticsParseTreeHandler::HandleDeclaredName(ParseTree::Node parse_node)
     -> void {
   // The parent is responsible for binding the name.
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleDeducedParameterList(
@@ -338,7 +236,7 @@ auto SemanticsParseTreeHandler::HandleEmptyDeclaration(
     ParseTree::Node parse_node) -> void {
   // Empty declarations have no actions associated, but we still balance the
   // tree.
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleExpressionStatement(
@@ -346,8 +244,8 @@ auto SemanticsParseTreeHandler::HandleExpressionStatement(
   // Pop the expression without investigating its contents.
   // TODO: This will probably eventually need to do some "do not discard"
   // analysis.
-  PopWithResult();
-  Push(parse_node);
+  node_stack_.PopAndIgnore();
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleFileEnd(ParseTree::Node /*parse_node*/)
@@ -383,23 +281,23 @@ auto SemanticsParseTreeHandler::HandleFunctionDeclaration(
 auto SemanticsParseTreeHandler::HandleFunctionDefinition(
     ParseTree::Node parse_node) -> void {
   // Merges code block children up under the FunctionDefinitionStart.
-  while (parse_tree_->node_kind(node_stack_.back().parse_node) !=
+  while (parse_tree_->node_kind(node_stack_.PeekParseNode()) !=
          ParseNodeKind::FunctionDefinitionStart) {
-    node_stack_.pop_back();
+    node_stack_.PopAndIgnore();
   }
-  Pop(ParseNodeKind::FunctionDefinitionStart);
+  node_stack_.PopAndIgnore();
+
   PopScope();
   node_block_stack_.pop_back();
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleFunctionDefinitionStart(
     ParseTree::Node parse_node) -> void {
-  Pop(ParseNodeKind::ParameterList);
-  auto name_node = node_stack_.back().parse_node;
-  node_stack_.pop_back();
-  auto fn_node = node_stack_.back().parse_node;
-  Pop(ParseNodeKind::FunctionIntroducer);
+  node_stack_.PopAndDiscardSoloParseNode(ParseNodeKind::ParameterList);
+  auto name_node = node_stack_.PopForSoloParseNode(ParseNodeKind::DeclaredName);
+  auto fn_node =
+      node_stack_.PopForSoloParseNode(ParseNodeKind::FunctionIntroducer);
 
   auto decl_id = AddNode(SemanticsNode::MakeFunctionDeclaration(fn_node));
   // TODO: Propagate the type of the function.
@@ -408,13 +306,13 @@ auto SemanticsParseTreeHandler::HandleFunctionDefinitionStart(
   AddNode(SemanticsNode::MakeFunctionDefinition(parse_node, decl_id, block_id));
   node_block_stack_.push_back(block_id);
   PushScope();
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleFunctionIntroducer(
     ParseTree::Node parse_node) -> void {
   // No action, just a bracketing node.
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleIfCondition(
@@ -439,8 +337,8 @@ auto SemanticsParseTreeHandler::HandleIfStatementElse(
 
 auto SemanticsParseTreeHandler::HandleInfixOperator(ParseTree::Node parse_node)
     -> void {
-  auto rhs_id = PopWithResult();
-  auto lhs_id = PopWithResult();
+  auto rhs_id = node_stack_.PopForNodeId();
+  auto lhs_id = node_stack_.PopForNodeId();
   SemanticsNodeId result_type =
       TryTypeConversion(parse_node, lhs_id, rhs_id, /*can_convert_lhs=*/true);
 
@@ -448,8 +346,8 @@ auto SemanticsParseTreeHandler::HandleInfixOperator(ParseTree::Node parse_node)
   auto token = parse_tree_->node_token(parse_node);
   switch (auto token_kind = tokens_->GetKind(token)) {
     case TokenKind::Plus:
-      Push(parse_node, SemanticsNode::MakeBinaryOperatorAdd(
-                           parse_node, result_type, lhs_id, rhs_id));
+      AddNodeAndPush(parse_node, SemanticsNode::MakeBinaryOperatorAdd(
+                                     parse_node, result_type, lhs_id, rhs_id));
       break;
     default:
       CARBON_FATAL() << "Unrecognized token kind: " << token_kind.name();
@@ -478,19 +376,20 @@ auto SemanticsParseTreeHandler::HandleLiteral(ParseTree::Node parse_node)
     case TokenKind::IntegerLiteral: {
       auto id =
           semantics_->AddIntegerLiteral(tokens_->GetIntegerLiteral(token));
-      Push(parse_node, SemanticsNode::MakeIntegerLiteral(parse_node, id));
+      AddNodeAndPush(parse_node,
+                     SemanticsNode::MakeIntegerLiteral(parse_node, id));
       break;
     }
     case TokenKind::RealLiteral: {
       // TODO: Add storage of the Real literal.
-      Push(parse_node, SemanticsNode::MakeRealLiteral(parse_node));
+      AddNodeAndPush(parse_node, SemanticsNode::MakeRealLiteral(parse_node));
       break;
     }
     case TokenKind::IntegerTypeLiteral: {
       auto text = tokens_->GetTokenText(token);
       CARBON_CHECK(text == "i32") << "Currently only i32 is allowed";
-      Push(parse_node, SemanticsNodeId::MakeBuiltinReference(
-                           SemanticsBuiltinKind::IntegerType));
+      node_stack_.Push(parse_node, SemanticsNodeId::MakeBuiltinReference(
+                                       SemanticsBuiltinKind::IntegerType));
       break;
     }
     default:
@@ -506,8 +405,8 @@ auto SemanticsParseTreeHandler::HandleNameReference(ParseTree::Node parse_node)
     CARBON_DIAGNOSTIC(NameNotFound, Error, "Name {0} not found",
                       llvm::StringRef);
     emitter_->Emit(parse_node, NameNotFound, name_str);
-    Push(parse_node, SemanticsNodeId::MakeBuiltinReference(
-                         SemanticsBuiltinKind::InvalidType));
+    node_stack_.Push(parse_node, SemanticsNodeId::MakeBuiltinReference(
+                                     SemanticsBuiltinKind::InvalidType));
   };
 
   auto name_id = semantics_->GetString(name_str);
@@ -524,7 +423,7 @@ auto SemanticsParseTreeHandler::HandleNameReference(ParseTree::Node parse_node)
   CARBON_CHECK(!it->second.empty()) << "Should have been erased: " << name_str;
 
   // TODO: Check for ambiguous lookups.
-  Push(parse_node, it->second.back());
+  node_stack_.Push(parse_node, it->second.back());
 }
 
 auto SemanticsParseTreeHandler::HandlePackageApi(ParseTree::Node /*parse_node*/)
@@ -556,8 +455,8 @@ auto SemanticsParseTreeHandler::HandleParameterList(ParseTree::Node parse_node)
     -> void {
   // TODO: This should transform into a usable parameter list. For now
   // it's unused and only stored so that node counts match.
-  Pop(ParseNodeKind::ParameterListStart);
-  Push(parse_node);
+  node_stack_.PopAndDiscardSoloParseNode(ParseNodeKind::ParameterListStart);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleParameterListComma(
@@ -568,7 +467,7 @@ auto SemanticsParseTreeHandler::HandleParameterListComma(
 auto SemanticsParseTreeHandler::HandleParameterListStart(
     ParseTree::Node parse_node) -> void {
   // TODO: See HandleParameterList.
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleParenExpression(
@@ -583,23 +482,21 @@ auto SemanticsParseTreeHandler::HandleParenExpressionOrTupleLiteralStart(
 
 auto SemanticsParseTreeHandler::HandlePatternBinding(ParseTree::Node parse_node)
     -> void {
-  auto type = node_stack_.pop_back_val();
-  CARBON_CHECK(type.result_id.is_valid());
+  auto type = node_stack_.PopForNodeId();
 
   // Get the name.
-  auto name_node = node_stack_.pop_back_val().parse_node;
+  auto name_node = node_stack_.PopForSoloParseNode();
 
   // Allocate storage, linked to the name for error locations.
-  auto storage_id =
-      AddNode(SemanticsNode::MakeVarStorage(name_node, type.result_id));
+  auto storage_id = AddNode(SemanticsNode::MakeVarStorage(name_node, type));
 
   // Bind the name to storage.
-  auto name_id = BindName(name_node, type.result_id, storage_id);
+  auto name_id = BindName(name_node, type, storage_id);
 
   // If this node's result is used, it'll be for either the name or the storage
   // address. The storage address can be found through the name, so we push the
   // name.
-  Push(parse_node, name_id);
+  node_stack_.Push(parse_node, name_id);
 }
 
 auto SemanticsParseTreeHandler::HandlePostfixOperator(
@@ -614,23 +511,23 @@ auto SemanticsParseTreeHandler::HandlePrefixOperator(
 
 auto SemanticsParseTreeHandler::HandleReturnStatement(
     ParseTree::Node parse_node) -> void {
-  if (parse_tree_->node_kind(node_stack_.back().parse_node) ==
+  if (parse_tree_->node_kind(node_stack_.PeekParseNode()) ==
       ParseNodeKind::ReturnStatementStart) {
-    Pop(ParseNodeKind::ReturnStatementStart);
-    Push(parse_node, SemanticsNode::MakeReturn(parse_node));
+    node_stack_.PopAndDiscardSoloParseNode(ParseNodeKind::ReturnStatementStart);
+    AddNodeAndPush(parse_node, SemanticsNode::MakeReturn(parse_node));
   } else {
-    auto arg = PopWithResult();
+    auto arg = node_stack_.PopForNodeId();
     auto arg_type = semantics_->GetType(arg);
-    Pop(ParseNodeKind::ReturnStatementStart);
-    Push(parse_node,
-         SemanticsNode::MakeReturnExpression(parse_node, arg_type, arg));
+    node_stack_.PopAndDiscardSoloParseNode(ParseNodeKind::ReturnStatementStart);
+    AddNodeAndPush(parse_node, SemanticsNode::MakeReturnExpression(
+                                   parse_node, arg_type, arg));
   }
 }
 
 auto SemanticsParseTreeHandler::HandleReturnStatementStart(
     ParseTree::Node parse_node) -> void {
   // No action, just a bracketing node.
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleReturnType(ParseTree::Node /*parse_node*/)
@@ -700,49 +597,45 @@ auto SemanticsParseTreeHandler::HandleTupleLiteralComma(
 
 auto SemanticsParseTreeHandler::HandleVariableDeclaration(
     ParseTree::Node parse_node) -> void {
-  auto last_child = node_stack_.pop_back_val();
-  CARBON_CHECK(last_child.result_id.is_valid());
+  auto last_child = node_stack_.PopForParseNodeAndNodeId();
 
-  if (parse_tree_->node_kind(last_child.parse_node) !=
+  if (parse_tree_->node_kind(last_child.first) !=
       ParseNodeKind::PatternBinding) {
-    SemanticsNodeId init_id = last_child.result_id;
-    auto storage_id = PopWithResult(ParseNodeKind::VariableInitializer);
+    auto storage_id =
+        node_stack_.PopForNodeId(ParseNodeKind::VariableInitializer);
 
-    auto binding = node_stack_.pop_back_val();
-    CARBON_CHECK(parse_tree_->node_kind(binding.parse_node) ==
+    auto binding = node_stack_.PopForParseNodeAndNameId();
+    CARBON_CHECK(parse_tree_->node_kind(binding.first) ==
                  ParseNodeKind::PatternBinding);
-    CARBON_CHECK(binding.name_id.is_valid());
+    CARBON_CHECK(binding.second.is_valid());
 
     // Restore the name now that the initializer is complete.
-    AddNameToLookup(binding.name_id, storage_id);
+    AddNameToLookup(binding.second, storage_id);
 
-    auto storage_type = TryTypeConversion(parse_node, storage_id, init_id,
-                                          /*can_convert_lhs=*/false);
+    auto storage_type =
+        TryTypeConversion(parse_node, storage_id, last_child.second,
+                          /*can_convert_lhs=*/false);
     AddNode(SemanticsNode::MakeAssign(parse_node, storage_type, storage_id,
-                                      init_id));
+                                      last_child.second));
   }
 
-  Pop(ParseNodeKind::VariableIntroducer);
-  Push(parse_node);
+  node_stack_.PopAndDiscardSoloParseNode(ParseNodeKind::VariableIntroducer);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleVariableIntroducer(
     ParseTree::Node parse_node) -> void {
   // No action, just a bracketing node.
-  Push(parse_node);
+  node_stack_.Push(parse_node);
 }
 
 auto SemanticsParseTreeHandler::HandleVariableInitializer(
     ParseTree::Node parse_node) -> void {
   // Temporarily remove name lookup entries added by the `var`. These will be
   // restored by `VariableDeclaration`.
-  auto back = node_stack_.back();
-  CARBON_CHECK(parse_tree_->node_kind(back.parse_node) ==
-               ParseNodeKind::PatternBinding)
-      << parse_tree_->node_kind(back.parse_node);
 
   // Save the storage ID.
-  auto it = name_lookup_.find(back.name_id);
+  auto it = name_lookup_.find(node_stack_.PeekForNameId());
   CARBON_CHECK(it != name_lookup_.end());
   CARBON_CHECK(!it->second.empty());
   auto storage_id = it->second.back();
@@ -755,7 +648,7 @@ auto SemanticsParseTreeHandler::HandleVariableInitializer(
     it->second.pop_back();
   }
 
-  Push(parse_node, storage_id);
+  node_stack_.Push(parse_node, storage_id);
 }
 
 auto SemanticsParseTreeHandler::HandleWhileCondition(

toolchain/semantics/semantics_parse_tree_handler.h

@@ -11,6 +11,7 @@
 #include "toolchain/parser/parse_tree.h"
 #include "toolchain/semantics/semantics_ir.h"
 #include "toolchain/semantics/semantics_node.h"
+#include "toolchain/semantics/semantics_node_stack.h"
 
 namespace Carbon {
 
@@ -26,7 +27,8 @@ class SemanticsParseTreeHandler {
         emitter_(&emitter),
         parse_tree_(&parse_tree),
         semantics_(&semantics),
-        vlog_stream_(vlog_stream) {}
+        vlog_stream_(vlog_stream),
+        node_stack_(parse_tree, vlog_stream) {}
 
   // Outputs the ParseTree information into SemanticsIR.
   auto Build() -> void;
@@ -56,18 +58,6 @@ class SemanticsParseTreeHandler {
     }
   };
 
-  // An entry in node_stack_.
-  struct NodeStackEntry {
-    ParseTree::Node parse_node;
-    union {
-      // The result_id may be invalid if there's no result.
-      SemanticsNodeId result_id;
-      // The name_id is provided for PatternBindings.
-      SemanticsStringId name_id;
-    };
-  };
-  static_assert(sizeof(NodeStackEntry) == 8, "Unexpected NodeStackEntry size");
-
   // An entry in scope_stack_.
   struct ScopeStackEntry {
     // Names which are registered with name_lookup_, and will need to be
@@ -80,6 +70,10 @@ class SemanticsParseTreeHandler {
   // Adds a node to the current block, returning the produced ID.
   auto AddNode(SemanticsNode node) -> SemanticsNodeId;
 
+  // Pushes a parse tree node onto the stack, storing the SemanticsNode as the
+  // result.
+  auto AddNodeAndPush(ParseTree::Node parse_node, SemanticsNode node) -> void;
+
   // Adds a name to name lookup. This is typically done through BindName, but
   // can also be used to restore removed names.
   auto AddNameToLookup(SemanticsStringId name_id, SemanticsNodeId storage_id)
@@ -91,36 +85,6 @@ class SemanticsParseTreeHandler {
   auto BindName(ParseTree::Node name_node, SemanticsNodeId type_id,
                 SemanticsNodeId target_id) -> SemanticsStringId;
 
-  // Pushes a parse tree node onto the stack. Used when there is no IR generated
-  // by the node.
-  auto Push(ParseTree::Node parse_node) -> void;
-
-  // Pushes a parse tree node onto the stack, storing the SemanticsNode as the
-  // result.
-  auto Push(ParseTree::Node parse_node, SemanticsNode node) -> void;
-
-  // Pushes a parse tree node onto the stack with an already-built node ID.
-  auto Push(ParseTree::Node parse_node, SemanticsNodeId node_id) -> void;
-
-  // Pushes a PatternBinding parse tree node onto the stack with its name.
-  auto Push(ParseTree::Node parse_node, SemanticsStringId name_id) -> void;
-
-  // Pops the top of the stack, verifying that it's the expected kind.
-  auto Pop(ParseNodeKind pop_parse_kind) -> void;
-
-  // Pops the top of the stack, returning the result_id. Must only be called for
-  // nodes that have results.
-  auto PopWithResult() -> SemanticsNodeId;
-
-  // Pops the top of the stack, verifying that it's the expected kind and
-  // returning the result_id. Must only be called for nodes that have results.
-  auto PopWithResult(ParseNodeKind pop_parse_kind) -> SemanticsNodeId;
-
-  // Pops the top of the stack, verifying that it's the expected kind and
-  // returning the result_id. Must only be called for nodes that have results.
-  auto PopWithResultIf(ParseNodeKind pop_parse_kind)
-      -> std::optional<SemanticsNodeId>;
-
   // Pushes a new scope onto scope_stack_.
   auto PushScope() -> void;
 
@@ -161,7 +125,7 @@ class SemanticsParseTreeHandler {
   llvm::raw_ostream* vlog_stream_;
 
   // The stack during Build. Will contain file-level parse nodes on return.
-  llvm::SmallVector<NodeStackEntry> node_stack_;
+  SemanticsNodeStack node_stack_;
 
   // The stack of node blocks during build. Only updated on ParseTree nodes that
   // affect the stack.

toolchain/semantics/testdata/basics/verbose.carbon

@@ -6,7 +6,7 @@
 // RUN: %{carbon} -v dump semantics-ir %s | %{FileCheck-allow-unmatched}
 //
 // Only checks a couple statements in order to minimize manual update churn.
-// CHECK:STDERR: Push 0: FunctionIntroducer
+// CHECK:STDERR: Push 0: FunctionIntroducer -> node<invalid>
 // CHECK:STDERR: AddNode block0: BindName(str0, node{{[0-9]+}})
 
 fn Foo() {