Save WhereOperand node instead of performing a scan

toolchain/check/decl_name_stack.cpp

@@ -81,6 +81,10 @@ auto DeclNameStack::FinishImplName() -> NameContext {
   return result;
 }
 
+auto DeclNameStack::SetImplWhereOperandNodeId(Parse::NodeId node_id) -> void {
+  decl_name_stack_.back().where_operand_node_id = node_id;
+}
+
 auto DeclNameStack::PopScope() -> void {
   CARBON_CHECK(decl_name_stack_.back().state == NameContext::State::Finished,
                "Missing call to FinishName before PopScope");

toolchain/check/decl_name_stack.h

@@ -151,6 +151,11 @@ class DeclNameStack {
       // The ID of an unresolved identifier.
       SemIR::NameId unresolved_name_id = SemIR::NameId::Invalid;
     };
+
+    // The `WhereOperand` node_id for the constraint in an impl decl, if any.
+    // FIXME: Can this be in a union with some other field that isn't used for
+    // impl decls?
+    Parse::NodeId where_operand_node_id = Parse::NodeId::Invalid;
   };
 
   // Information about a declaration name that has been temporarily removed from
@@ -200,6 +205,9 @@ class DeclNameStack {
   // logic such as building parameter scopes, so are a special case.
   auto FinishImplName() -> NameContext;
 
+  // Set the position of the `WhereOperand` node in an `impl...as...where...`.
+  auto SetImplWhereOperandNodeId(Parse::NodeId node_id) -> void;
+
   // Pops the declaration name from the declaration name stack, and pops all
   // scopes that were entered as part of handling the declaration name. These
   // are the scopes corresponding to name qualifiers in the name, for example

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 end_of_decl_node_id) -> NameComponent {
   auto [implicit_params_loc_id, implicit_param_patterns_id] =
       context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ImplForall>();
 
@@ -213,39 +212,15 @@ 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);
+  // Subtracting 1 since we don't want to include the final `{`, `;`, or `where`
+  // of the declaration when performing syntactic match.
+  auto node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
+  CARBON_CHECK(node_kind == Parse::NodeKind::ImplDefinitionStart ||
+               node_kind == Parse::NodeKind::ImplDecl ||
+               node_kind == Parse::NodeKind::WhereOperand);
   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,
@@ -313,8 +288,18 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId 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);
+  // Stop at the `where` node, if present, to exclude it from syntactic match.
+  auto name = PopImplIntroducerAndParamsAsNameComponent(
+      context, name_context.where_operand_node_id.is_valid()
+                   ? name_context.where_operand_node_id
+                   : node_id);
   auto decl_block_id = context.inst_block_stack().Pop();
 
   // Convert the constraint expression to a type.
@@ -334,11 +319,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_where.cpp

@@ -122,9 +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 [operand_node_id, period_self_id] =
+      context.node_stack().PopWithNodeId<Parse::NodeKind::WhereOperand>();
   SemIR::InstBlockId requirements_id = context.args_type_info_stack().Pop();
+  if (context.node_stack().PeekIs<Parse::NodeCategory::ImplAs>()) {
+    // In the `impl ... as ... where ...` case, save the `WhereOperand` node in
+    // the impl declaration so that part can be trimmed off for syntactic decl
+    // matching.
+    context.decl_name_stack().SetImplWhereOperandNodeId(operand_node_id);
+  }
   context.AddInstAndPush<SemIR::WhereExpr>(
       node_id, {.type_id = SemIR::TypeType::SingletonTypeId,
                 .period_self_id = period_self_id,

toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon

@@ -32,6 +32,10 @@ interface K {}
 
 // `impl` matching only ignores a root-level `where` clause.
 
+// CHECK:STDERR: parens_other_nesting.carbon:[[@LINE+4]]:1: error: impl declared but not defined [MissingImplDefinition]
+// CHECK:STDERR: impl {} as (K where .Self impls type) where .Self impls type;
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl {} as (K where .Self impls type) where .Self impls type;
 impl {} as (K where .Self impls type) {}