Save WhereOperand node instead of performing a scan

toolchain/check/handle_impl.cpp

@@ -191,8 +191,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
 // Pops the parameters of an `impl`, forming a `NameComponent` with no
 // associated name that describes them.
 static auto PopImplIntroducerAndParamsAsNameComponent(
-    Context& context, Parse::AnyImplDeclId end_of_decl_node_id)
-    -> NameComponent {
+    Context& context, Parse::NodeId last_param_node_id) -> NameComponent {
   auto [implicit_params_loc_id, implicit_param_patterns_id] =
       context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ImplForall>();
 
@@ -213,44 +212,11 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
   Parse::NodeId first_param_node_id =
       context.node_stack().PopForSoloNodeId<Parse::NodeKind::ImplIntroducer>();
 
-  // Subtracting 1 since we don't want to include the final `{` or `;` of the
-  // declaration when performing syntactic match.
-  auto end_node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
-  CARBON_CHECK(end_node_kind == Parse::NodeKind::ImplDefinitionStart ||
-               end_node_kind == Parse::NodeKind::ImplDecl);
-  Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id);
-  --last_param_iter;
-
-  // Following proposal #3763, exclude a final `where` clause, if present. See:
-  // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations
-
-  // Caches the NodeKind for the current value of *last_param_iter so
-  if (context.parse_tree().node_kind(*last_param_iter) ==
-      Parse::NodeKind::WhereExpr) {
-    int where_operands_to_skip = 1;
-    --last_param_iter;
-    CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
-                 last_param_iter);
-    do {
-      auto node_kind = context.parse_tree().node_kind(*last_param_iter);
-      if (node_kind == Parse::NodeKind::WhereExpr) {
-        // If we have a nested `where`, we need to see another `WhereOperand`
-        // before we find the one that matches our original `WhereExpr` node.
-        ++where_operands_to_skip;
-      } else if (node_kind == Parse::NodeKind::WhereOperand) {
-        --where_operands_to_skip;
-      }
-      --last_param_iter;
-      CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
-                   last_param_iter);
-    } while (where_operands_to_skip > 0);
-  }
-
   return {
       .name_loc_id = Parse::NodeId::Invalid,
       .name_id = SemIR::NameId::Invalid,
       .first_param_node_id = first_param_node_id,
-      .last_param_node_id = *last_param_iter,
+      .last_param_node_id = last_param_node_id,
       .implicit_params_loc_id = implicit_params_loc_id,
       .implicit_param_patterns_id =
           implicit_param_patterns_id.value_or(SemIR::InstBlockId::Invalid),
@@ -310,11 +276,35 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
     -> std::pair<SemIR::ImplId, SemIR::InstId> {
   auto [constraint_node, constraint_id] =
       context.node_stack().PopExprWithNodeId();
+
+  // If the constraint is a `where` constraint, we need to stop syntactic
+  // matching at it. For this case, `WhereExpr` handling will have left the
+  // `WhereOperand` on the node stack.
+  Parse::NodeId last_param_node_id = Parse::NodeId::Invalid;
+  if (context.parse_tree().node_kind(constraint_node) ==
+      Parse::NodeKind::WhereExpr) {
+    // Only keep the node ID, not the inst ID.
+    auto [operand_node, _] =
+        context.node_stack().PopWithNodeId<Parse::NodeKind::WhereOperand>();
+    // Exclude the `where` itself from params.
+    last_param_node_id = *--Parse::Tree::PostorderIterator(operand_node);
+  } else {
+    // Exclude the `AnyImplDeclId` (either a `;` or `{`) from params.
+    last_param_node_id = *--Parse::Tree::PostorderIterator(node_id);
+  }
+
   auto [self_type_node, self_inst_id] =
       context.node_stack().PopWithNodeId<Parse::NodeCategory::ImplAs>();
   auto self_type_id = context.GetTypeIdForTypeInst(self_inst_id);
+
+  // Finish processing the name, which should be empty, but might have
+  // parameters.
+  auto name_context = context.decl_name_stack().FinishImplName();
+  CARBON_CHECK(name_context.state == DeclNameStack::NameContext::State::Empty);
+
   // Pop the `impl` introducer and any `forall` parameters as a "name".
-  auto name = PopImplIntroducerAndParamsAsNameComponent(context, node_id);
+  auto name =
+      PopImplIntroducerAndParamsAsNameComponent(context, last_param_node_id);
   auto decl_block_id = context.inst_block_stack().Pop();
 
   // Convert the constraint expression to a type.
@@ -334,11 +324,6 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
       context.decl_introducer_state_stack().Pop<Lex::TokenKind::Impl>();
   LimitModifiersOnDecl(context, introducer, KeywordModifierSet::ImplDecl);
 
-  // Finish processing the name, which should be empty, but might have
-  // parameters.
-  auto name_context = context.decl_name_stack().FinishImplName();
-  CARBON_CHECK(name_context.state == DeclNameStack::NameContext::State::Empty);
-
   // TODO: Check for an orphan `impl`.
 
   // Add the impl declaration.

toolchain/check/handle_paren_expr.cpp

@@ -4,19 +4,23 @@
 
 #include "toolchain/check/context.h"
 #include "toolchain/check/handle.h"
+#include "toolchain/parse/typed_nodes.h"
 
 namespace Carbon::Check {
 
-auto HandleParseNode(Context& /*context*/, Parse::ParenExprStartId /*node_id*/)
+auto HandleParseNode(Context& context, Parse::ParenExprStartId node_id)
     -> bool {
-  // The open paren is unused.
+  // Push the start to help track nesting.
+  context.node_stack().Push(node_id);
   return true;
 }
 
 auto HandleParseNode(Context& context, Parse::ParenExprId node_id) -> bool {
-  // We re-push because the ParenExpr is valid for member expressions, whereas
-  // the child expression might not be.
-  context.node_stack().Push(node_id, context.node_stack().PopExpr());
+  auto expr = context.node_stack().PopExpr();
+  context.node_stack().PopForSoloNodeId<Parse::NodeKind::ParenExprStart>();
+  // We push with the ParenExpr node because it's valid for member expressions,
+  // whereas the child expression might not be.
+  context.node_stack().Push(node_id, expr);
   return true;
 }
 

toolchain/check/handle_where.cpp

@@ -122,8 +122,15 @@ auto HandleParseNode(Context& context, Parse::WhereExprId node_id) -> bool {
   // Remove `PeriodSelf` from name lookup, undoing the `Push` done for the
   // `WhereOperand`.
   context.scope_stack().Pop();
-  SemIR::InstId period_self_id =
-      context.node_stack().Pop<Parse::NodeKind::WhereOperand>();
+
+  auto period_self_id =
+      context.node_stack().Peek<Parse::NodeKind::WhereOperand>();
+  // In most expressions, pop the `WhereOperand` node. In the `impl ... as ...
+  // where ...` case, leave the `WhereOperand` node on the stack so that part
+  // can be trimmed off by impl's syntactic decl matching.
+  if (!context.node_stack().PeekNextIs(Parse::NodeCategory::ImplAs)) {
+    context.node_stack().Pop<Parse::NodeKind::WhereOperand>();
+  }
   SemIR::InstBlockId requirements_id = context.args_type_info_stack().Pop();
   context.AddInstAndPush<SemIR::WhereExpr>(
       node_id, {.type_id = SemIR::TypeType::SingletonTypeId,

toolchain/check/node_stack.h

@@ -134,15 +134,28 @@ class NodeStack {
            NodeKindToIdKind(PeekNodeKind()) == Id::KindFor<IdT>();
   }
 
-  // Returns whether the *next* node on the stack is a given kind. This doesn't
-  // have the breadth of support versus other Peek functions because it's
-  // expected to be used in narrow circumstances when determining how to treat
-  // the *current* top of the stack.
+  // Returns whether the *next* node on the stack is a given kind.
+  //
+  // This doesn't have the breadth of support versus other Peek functions
+  // because it's expected to be used in narrow circumstances when determining
+  // how to treat the *current* top of the stack.
   template <const Parse::NodeKind& RequiredParseKind>
   auto PeekNextIs() const -> bool {
     CARBON_CHECK(stack_.size() >= 2);
-    return parse_tree_->node_kind(stack_[stack_.size() - 2].node_id) ==
-           RequiredParseKind;
+    auto next_kind = parse_tree_->node_kind(stack_[stack_.size() - 2].node_id);
+    return next_kind == RequiredParseKind;
+  }
+
+  // Returns whether the *next* node on the stack has an overlapping
+  // category.
+  //
+  // This doesn't have the breadth of support versus other Peek functions
+  // because it's expected to be used in narrow circumstances when determining
+  // how to treat the *current* top of the stack.
+  auto PeekNextIs(Parse::NodeCategory category) const -> bool {
+    CARBON_CHECK(stack_.size() >= 2);
+    auto next_kind = parse_tree_->node_kind(stack_[stack_.size() - 2].node_id);
+    return next_kind.category().HasAnyOf(category);
   }
 
   // Pops the top of the stack without any verification.
@@ -458,6 +471,7 @@ class NodeStack {
         case Parse::NodeKind::InterfaceIntroducer:
         case Parse::NodeKind::LetInitializer:
         case Parse::NodeKind::LetIntroducer:
+        case Parse::NodeKind::ParenExprStart:
         case Parse::NodeKind::ReturnedModifier:
         case Parse::NodeKind::ReturnStatementStart:
         case Parse::NodeKind::ReturnVarModifier:
@@ -602,7 +616,6 @@ class NodeStack {
         case Parse::NodeKind::PackageIntroducer:
         case Parse::NodeKind::PackageName:
         case Parse::NodeKind::ParenExpr:
-        case Parse::NodeKind::ParenExprStart:
         case Parse::NodeKind::PatternListComma:
         case Parse::NodeKind::Placeholder:
         case Parse::NodeKind::PointerMemberAccessExpr: