Syntactic `impl` declaration matching updates (#4762)

* Implement ignoring the difference between `Self as` and `as`, as well
as `where` clauses at the end of an `impl` declaration when checking
whether `impl` declarations match, from #3763.
* Allow impl declarations with different constraint ids to match, as
long as the facet type of the constraint has the same interface_id and
specific_id.
* Add some TODOs reflecting future facet type resolution.

---------

Co-authored-by: Josh L <josh11b@users.noreply.github.com>

toolchain/check/handle_impl.cpp

@@ -212,21 +212,45 @@ 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.
-  // TODO: Following proposal #3763, we should exclude any `where` clause, and
-  // add `Self` before `as` if needed, see:
+  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
-  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);
-  Parse::NodeId last_param_node_id(end_of_decl_node_id.index - 1);
+
+  // 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_node_id,
+      .last_param_node_id = *last_param_iter,
       .implicit_params_loc_id = implicit_params_loc_id,
       .implicit_param_patterns_id =
           implicit_param_patterns_id.value_or(SemIR::InstBlockId::Invalid),
@@ -297,6 +321,11 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
   // TODO: Check that its constant value is a constraint.
   auto [constraint_inst_id, constraint_type_id] =
       ExprAsType(context, constraint_node, constraint_id);
+  // TODO: Do facet type resolution here.
+  // TODO: Determine `interface_id` and `specific_id` once and save it in the
+  // resolved facet type, instead of in multiple functions called below.
+  // TODO: Skip work below if facet type resolution fails, so we don't have a
+  // valid/non-error `interface_id` at all.
 
   // Process modifiers.
   // TODO: Should we somehow permit access specifiers on `impl`s?

toolchain/check/merge.cpp

@@ -394,18 +394,38 @@ static auto CheckRedeclParamSyntax(Context& context,
   CARBON_CHECK(prev_last_param_node_id.is_valid(),
                "prev_last_param_node_id.is_valid should match "
                "prev_first_param_node_id.is_valid");
-
-  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
-  // brackets. As a consequence, we don't explicitly handle different range
-  // sizes here.
-  for (auto [new_node_id, prev_node_id] : llvm::zip(new_range, prev_range)) {
+  Parse::Tree::PostorderIterator new_iter(new_first_param_node_id);
+  Parse::Tree::PostorderIterator new_end(new_last_param_node_id);
+  Parse::Tree::PostorderIterator prev_iter(prev_first_param_node_id);
+  Parse::Tree::PostorderIterator prev_end(prev_last_param_node_id);
+  // Done when one past the last node to check.
+  ++new_end;
+  ++prev_end;
+
+  // Compare up to the shortest length.
+  for (; new_iter != new_end && prev_iter != prev_end;
+       ++new_iter, ++prev_iter) {
+    auto new_node_id = *new_iter;
+    auto prev_node_id = *prev_iter;
     if (!IsNodeSyntaxEqual(context, new_node_id, prev_node_id)) {
+      // Skip difference if it is `Self as` vs. `as` in an `impl` declaration.
+      // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations
+      auto new_node_kind = context.parse_tree().node_kind(new_node_id);
+      auto prev_node_kind = context.parse_tree().node_kind(prev_node_id);
+      if (new_node_kind == Parse::NodeKind::DefaultSelfImplAs &&
+          prev_node_kind == Parse::NodeKind::SelfTypeNameExpr &&
+          context.parse_tree().node_kind(prev_iter[1]) ==
+              Parse::NodeKind::TypeImplAs) {
+        ++prev_iter;
+        continue;
+      }
+      if (prev_node_kind == Parse::NodeKind::DefaultSelfImplAs &&
+          new_node_kind == Parse::NodeKind::SelfTypeNameExpr &&
+          context.parse_tree().node_kind(new_iter[1]) ==
+              Parse::NodeKind::TypeImplAs) {
+        ++new_iter;
+        continue;
+      }
       if (!diagnose) {
         return false;
       }
@@ -420,6 +440,17 @@ static auto CheckRedeclParamSyntax(Context& context,
       return false;
     }
   }
+  // The prefixes are the same, but the lengths may still be different. This is
+  // only relevant for `impl` declarations where the final bracketing node is
+  // not included in the range of nodes being compared, and in those cases
+  // `diagnose` is false.
+  if (new_iter != new_end) {
+    CARBON_CHECK(!diagnose);
+    return false;
+  } else if (prev_iter != prev_end) {
+    CARBON_CHECK(!diagnose);
+    return false;
+  }
 
   return true;
 }

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

@@ -0,0 +1,180 @@
+// 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
+//
+// EXTRA-ARGS: --no-dump-sem-ir
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon
+
+// --- match.carbon
+library "[[@TEST_NAME]]";
+
+interface I1 {}
+interface I2 {}
+interface J1(T1:! type) {}
+interface J2(T2:! type) {}
+
+// `impl Self as` should match `impl as`, so these should not trigger impl
+// declaration without definition diagnostics.
+
+class C1 {
+  impl Self as I1;
+  impl as I1 {}
+
+  impl as I2;
+  impl Self as I2 {}
+
+  impl forall [U:! type] Self as J1(U);
+  impl forall [U:! type] as J1(U) {}
+
+  impl forall [V:! type] as J2(V);
+  impl forall [V:! type] Self as J2(V) {}
+}
+
+class C2(W:! type) {
+  impl Self as I1;
+  impl as I1 {}
+
+  impl as I2;
+  impl Self as I2 {}
+
+  impl forall [X:! type] Self as J1(X);
+  impl forall [X:! type] as J1(X) {}
+
+  impl forall [Y:! type] as J2(Y);
+  impl forall [Y:! type] Self as J2(Y) {}
+}
+
+
+// --- fail_no_match.carbon
+library "[[@TEST_NAME]]";
+
+interface I3 {}
+interface I4 {}
+interface I5 {}
+interface I6 {}
+interface J3(T3:! type) {}
+interface J4(T4:! type) {}
+interface J5(T5:! type) {}
+interface J6(T6:! type) {}
+
+// `impl C as` should not match `impl Self as` or `impl as`.
+
+class C3 {
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl C3 as I3;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl C3 as I3;
+  impl as I3 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl C3 as I4;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl C3 as I4;
+  impl Self as I4 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl as I5;
+  // CHECK:STDERR:   ^~~~~~~~~~~
+  // CHECK:STDERR:
+  impl as I5;
+  impl C3 as I5 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl Self as I6;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl Self as I6;
+  impl C3 as I6 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [Z3:! type] C3 as J3(Z3);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [Z3:! type] C3 as J3(Z3);
+  impl forall [Z3:! type] as J3(Z3) {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [Z4:! type] C3 as J4(Z4);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [Z4:! type] C3 as J4(Z4);
+  impl forall [Z4:! type] Self as J4(Z4) {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [Z5:! type] as J5(Z5);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [Z5:! type] as J5(Z5);
+  impl forall [Z5:! type] C3 as J5(Z5) {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [Z6:! type] Self as J6(Z6);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [Z6:! type] Self as J6(Z6);
+  impl forall [Z6:! type] C3 as J6(Z6) {}
+}
+
+class C4(A:! type) {
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl C4(A) as I3;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl C4(A) as I3;
+  impl as I3 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl C4(A) as I4;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl C4(A) as I4;
+  impl Self as I4 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl as I5;
+  // CHECK:STDERR:   ^~~~~~~~~~~
+  // CHECK:STDERR:
+  impl as I5;
+  impl C4(A) as I5 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl Self as I6;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl Self as I6;
+  impl C4(A) as I6 {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [B3:! type] C4(A) as J3(B3);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [B3:! type] C4(A) as J3(B3);
+  impl forall [B3:! type] as J3(B3) {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [B4:! type] C4(A) as J4(B4);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [B4:! type] C4(A) as J4(B4);
+  impl forall [B4:! type] Self as J4(B4) {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [B5:! type] as J5(B5);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  impl forall [B5:! type] as J5(B5);
+  impl forall [B5:! type] C4(A) as J5(B5) {}
+
+  // CHECK:STDERR: fail_no_match.carbon:[[@LINE+3]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   impl forall [B6:! type] Self as J6(B6);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  impl forall [B6:! type] Self as J6(B6);
+  impl forall [B6:! type] C4(A) as J6(B6) {}
+}

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

@@ -0,0 +1,47 @@
+// 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
+//
+// EXTRA-ARGS: --no-dump-sem-ir
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon
+
+// --- match.carbon
+library "[[@TEST_NAME]]";
+
+interface J {}
+
+// `impl` matching ignores the `where` clause. It should not get confused by
+// nested `where`, so the following should not trigger impl declaration without
+// definition diagnostics.
+
+impl () as J;
+impl () as J where .Self impls type and .Self impls (type where .Self impls type) {}
+
+impl {} as J where .Self impls type and .Self impls (type where .Self impls type);
+impl {} as J {}
+
+// --- parens_other_nesting.carbon
+library "[[@TEST_NAME]]";
+
+interface K {}
+
+// `impl` matching only ignores a root-level `where` clause.
+
+impl {} as (K where .Self impls type) where .Self impls type;
+impl {} as (K where .Self impls type) {}
+
+// --- fail_other_nesting.carbon
+library "[[@TEST_NAME]]";
+
+interface L {}
+
+// CHECK:STDERR: fail_other_nesting.carbon:[[@LINE+3]]:1: error: impl declared but not defined [MissingImplDefinition]
+// CHECK:STDERR: impl () as (L where .Self impls type) where .Self impls type;
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+impl () as (L where .Self impls type) where .Self impls type;
+impl () as L {}

toolchain/sem_ir/impl.cpp

@@ -10,11 +10,22 @@ namespace Carbon::SemIR {
 
 auto ImplStore::GetOrAddLookupBucket(const Impl& impl) -> LookupBucketRef {
   auto self_id = sem_ir_.constant_values().GetConstantInstId(impl.self_id);
-  auto constraint_id =
-      sem_ir_.constant_values().GetConstantInstId(impl.constraint_id);
+  InterfaceId interface_id = InterfaceId::Invalid;
+  SpecificId specific_id = SpecificId::Invalid;
+  auto facet_type_id = TypeId::ForTypeConstant(
+      sem_ir_.constant_values().Get(impl.constraint_id));
+  if (auto facet_type =
+          sem_ir_.types().TryGetAs<SemIR::FacetType>(facet_type_id)) {
+    const SemIR::FacetTypeInfo& facet_type_info =
+        sem_ir_.facet_types().Get(facet_type->facet_type_id);
+    if (auto interface_type = facet_type_info.TryAsSingleInterface()) {
+      interface_id = interface_type->interface_id;
+      specific_id = interface_type->specific_id;
+    }
+  }
   return LookupBucketRef(
       *this, lookup_
-                 .Insert(std::pair{self_id, constraint_id},
+                 .Insert(std::tuple{self_id, interface_id, specific_id},
                          [] { return ImplOrLookupBucketId::Invalid; })
                  .value());
 }

toolchain/sem_ir/impl.h

@@ -189,7 +189,8 @@ class ImplStore {
  private:
   File& sem_ir_;
   ValueStore<ImplId> values_;
-  Map<std::pair<InstId, InstId>, ImplOrLookupBucketId> lookup_;
+  Map<std::tuple<InstId, InterfaceId, SpecificId>, ImplOrLookupBucketId>
+      lookup_;
   // Buckets with at least 2 entries, which will be rare; see LookupBucketRef.
   llvm::SmallVector<llvm::SmallVector<ImplId, 2>> lookup_buckets_;
 };