Refactor some check-phase postorder iterator use. (#4175)

Allow directly constructing a PostorderIterator, to get rid of
`tree.postorder(node_id).end()` indirect construction. For ranges that
don't need tree data, make it clearer that they're not validated.

Note, this subtly gets rid of a subtree size use in the
`tree.postorder(node_id).end()` case (to get the discarded `begin()`
value).

toolchain/check/check.cpp

@@ -647,9 +647,10 @@ class NodeIdTraversal {
       : context_(context),
         next_deferred_definition_(&context.parse_tree()),
         worklist_(vlog_stream) {
+    auto range = context.parse_tree().postorder();
     chunks_.push_back(
-        {.it = context.parse_tree().postorder().begin(),
-         .end = context.parse_tree().postorder().end(),
+        {.it = range.begin(),
+         .end = range.end(),
          .next_definition = Parse::DeferredDefinitionIndex::Invalid});
   }
 
@@ -717,12 +718,11 @@ class NodeIdTraversal {
         context_.parse_tree().deferred_definitions().Get(definition_index);
     HandleFunctionDefinitionResume(context_, definition_info.start_id,
                                    std::move(suspended_fn));
-    chunks_.push_back(
-        {.it = context_.parse_tree().postorder(definition_info.start_id).end(),
-         .end = context_.parse_tree()
-                    .postorder(definition_info.definition_id)
-                    .end(),
-         .next_definition = next_deferred_definition_.index()});
+    auto range = Parse::Tree::PostorderIterator::MakeRange(
+        definition_info.start_id, definition_info.definition_id);
+    chunks_.push_back({.it = range.begin() + 1,
+                       .end = range.end(),
+                       .next_definition = next_deferred_definition_.index()});
     ++definition_index.index;
     next_deferred_definition_.SkipTo(definition_index);
   }
@@ -776,7 +776,7 @@ auto NodeIdTraversal::Next() -> std::optional<Parse::NodeId> {
 
       // Continue type-checking the parse tree after the end of the definition.
       chunks_.back().it =
-          context_.parse_tree().postorder(definition_info.definition_id).end();
+          Parse::Tree::PostorderIterator(definition_info.definition_id) + 1;
       next_deferred_definition_.SkipTo(definition_info.next_definition_index);
       continue;
     }

toolchain/check/merge.cpp

@@ -312,10 +312,10 @@ static auto CheckRedeclParamSyntax(Context& context,
       << "prev_last_param_node_id.is_valid should match "
          "prev_first_param_node_id.is_valid";
 
-  auto new_range = context.parse_tree().postorder(new_first_param_node_id,
-                                                  new_last_param_node_id);
-  auto prev_range = context.parse_tree().postorder(prev_first_param_node_id,
-                                                   prev_last_param_node_id);
+  auto new_range = Parse::Tree::PostorderIterator::MakeRange(
+      new_first_param_node_id, new_last_param_node_id);
+  auto prev_range = Parse::Tree::PostorderIterator::MakeRange(
+      prev_first_param_node_id, prev_last_param_node_id);
 
   // zip is using the shortest range. If they differ in length, there should be
   // some difference inside the range because the range includes parameter

toolchain/parse/tree.cpp

@@ -22,21 +22,10 @@ auto Tree::postorder() const -> llvm::iterator_range<PostorderIterator> {
 
 auto Tree::postorder(NodeId n) const
     -> llvm::iterator_range<PostorderIterator> {
-  CARBON_CHECK(n.is_valid());
   // The postorder ends after this node, the root, and begins at the start of
   // its subtree.
-  int end_index = n.index + 1;
-  int start_index = end_index - node_impls_[n.index].subtree_size;
-  return llvm::iterator_range<PostorderIterator>(
-      PostorderIterator(NodeId(start_index)),
-      PostorderIterator(NodeId(end_index)));
-}
-
-auto Tree::postorder(NodeId begin, NodeId end) const
-    -> llvm::iterator_range<PostorderIterator> {
-  CARBON_CHECK(begin.is_valid() && end.is_valid());
-  return llvm::iterator_range<PostorderIterator>(
-      PostorderIterator(begin), PostorderIterator(NodeId(end.index + 1)));
+  int start_index = n.index - node_impls_[n.index].subtree_size + 1;
+  return PostorderIterator::MakeRange(NodeId(start_index), n);
 }
 
 auto Tree::children(NodeId n) const -> llvm::iterator_range<SiblingIterator> {
@@ -307,6 +296,13 @@ auto Tree::Verify() const -> ErrorOr<Success> {
   return Success();
 }
 
+auto Tree::PostorderIterator::MakeRange(NodeId begin, NodeId end)
+    -> llvm::iterator_range<PostorderIterator> {
+  CARBON_CHECK(begin.is_valid() && end.is_valid());
+  return llvm::iterator_range<PostorderIterator>(
+      PostorderIterator(begin), PostorderIterator(NodeId(end.index + 1)));
+}
+
 auto Tree::PostorderIterator::Print(llvm::raw_ostream& output) const -> void {
   output << node_;
 }

toolchain/parse/tree.h

@@ -118,11 +118,6 @@ class Tree : public Printable<Tree> {
   // descendants in depth-first postorder.
   auto postorder(NodeId n) const -> llvm::iterator_range<PostorderIterator>;
 
-  // Returns an iterable range between the two parse tree nodes, in depth-first
-  // postorder. The range is inclusive of the bounds: [begin, end].
-  auto postorder(NodeId begin, NodeId end) const
-      -> llvm::iterator_range<PostorderIterator>;
-
   // Returns an iterable range over the direct children of a node in the parse
   // tree. This is a forward range, but is constant time to increment. The order
   // of children is the same as would be found in a reverse postorder traversal.
@@ -408,6 +403,15 @@ class Tree::PostorderIterator
                                         int, const NodeId*, NodeId>,
       public Printable<Tree::PostorderIterator> {
  public:
+  // Returns an iterable range between the two parse tree nodes, in depth-first
+  // postorder. The range is inclusive of the bounds: [begin, end].
+  static auto MakeRange(NodeId begin, NodeId end)
+      -> llvm::iterator_range<PostorderIterator>;
+
+  // Prefer using the `postorder` range calls, but direct construction is
+  // allowed if needed.
+  explicit PostorderIterator(NodeId n) : node_(n) {}
+
   PostorderIterator() = delete;
 
   auto operator==(const PostorderIterator& rhs) const -> bool {
@@ -442,8 +446,6 @@ class Tree::PostorderIterator
  private:
   friend class Tree;
 
-  explicit PostorderIterator(NodeId n) : node_(n) {}
-
   NodeId node_;
 };