Avoid cyclic lookup of an impl inside its own definition (#6629)

An interface A requiring another interface B means that an impl of A
must verify that the self-type also impls B. The instructions created
from this can involved a lookup that the self-type impls A, which end up
finding the impl being defined. This is not problematic of itself, but
it is problematic if these lookup instructions become part of the impl's
generic definition. When we find a specific of that `impl as A` during
impl lookup of A, and we resolve the specific definition, those lookup
instructions are replayed. Doing so does another lookup for `impl as A`,
which creates an infinitely recursive loop.

To break this loop we move the lookup instructions done to verify that
the self-type impls B outside of the definition of `impl as A`. This
prevents them from being specialized. But it doesn't prevent us from
diagnosing monomorphization errors properly. They just get diagnosed at
the use of that invalid specific, instead of inside the verification of
`impl as B` in the definition of `impl as A`.

toolchain/check/handle_impl.cpp

@@ -335,6 +335,8 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
   auto [impl_id, impl_decl_id] = BuildImplDecl(context, node_id);
   auto& impl = context.impls().Get(impl_id);
 
+  CheckRequireDeclsSatisfied(context, node_id, impl);
+
   CARBON_CHECK(!impl.has_definition_started());
   impl.definition_id = impl_decl_id;
   impl.scope_id =
@@ -346,7 +348,6 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
       context.generics().GetSelfSpecific(impl.generic_id));
   StartGenericDefinition(context, impl.generic_id);
   ImplWitnessStartDefinition(context, impl);
-  CheckRequireDeclsSatisfied(context, impl);
   context.inst_block_stack().Push();
   context.node_stack().Push(node_id, impl_id);
 

toolchain/check/impl.cpp

@@ -663,12 +663,21 @@ auto FinishImplWitness(Context& context, const SemIR::Impl& impl) -> void {
   // TODO: Diagnose if any declarations in the impl are not in used_decl_ids.
 }
 
-auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void {
+auto CheckRequireDeclsSatisfied(Context& context, SemIR::LocId loc_id,
+                                SemIR::Impl& impl) -> void {
   if (impl.witness_id == SemIR::ErrorInst::InstId) {
     return;
   }
 
   const auto& interface = context.interfaces().Get(impl.interface.interface_id);
+  if (!interface.is_complete()) {
+    // This will be diagnosed later. We check for required decls before starting
+    // the definition to avoid inserting these lookups into the definition, as
+    // the lookups can end up looking for the impl being defined, which creates
+    // a cycle.
+    return;
+  }
+
   auto require_ids =
       context.require_impls_blocks().Get(interface.require_impls_block_id);
   if (require_ids.empty()) {
@@ -690,8 +699,7 @@ auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void {
         context, require_specific, require.facet_type_inst_id);
 
     auto result =
-        LookupImplWitness(context, SemIR::LocId(impl.latest_decl_id()),
-                          self_const_id, facet_type_const_id);
+        LookupImplWitness(context, loc_id, self_const_id, facet_type_const_id);
     // TODO: If the facet type contains 2 interfaces, and one is not `impl`ed,
     // it would be nice to diagnose which one was not `impl`ed, but that
     // requires LookupImplWitness to return a partial result, or take a
@@ -708,7 +716,7 @@ auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void {
                           SemIR::SpecificInterface, SemIR::TypeId,
                           SemIR::FacetTypeId);
         context.emitter().Emit(
-            impl.latest_decl_id(), RequireImplsNotImplemented, impl.interface,
+            loc_id, RequireImplsNotImplemented, impl.interface,
             context.types().GetTypeIdForTypeConstantId(self_const_id),
             context.insts()
                 .GetAs<SemIR::FacetType>(facet_type_inst_id)

toolchain/check/impl.h

@@ -64,7 +64,8 @@ auto FinishImplWitness(Context& context, const SemIR::Impl& impl_id) -> void;
 // Checks that any `require` declarations in the interface being implemented by
 // `impl` are satisfied. Otherwise, a diagnostic is issued and the `impl` is
 // made invalid.
-auto CheckRequireDeclsSatisfied(Context& context, SemIR::Impl& impl) -> void;
+auto CheckRequireDeclsSatisfied(Context& context, SemIR::LocId loc_id,
+                                SemIR::Impl& impl) -> void;
 
 // Sets all unset members of the witness for `impl` to the error instruction and
 // sets the witness id in the `Impl` to an error.

toolchain/check/testdata/facet/require_satisified.carbon

@@ -2,7 +2,7 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
-// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/convert.carbon
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon
 //
 // AUTOUPDATE
 // TIP: To test this file alone, run:
@@ -184,3 +184,75 @@ impl C as Z(());
 impl () as Y(Self) {}
 
 impl C as Z(()) {}
+
+// --- impl_requires_itself_cycle.carbon
+library "[[@TEST_NAME]]";
+
+interface A { fn AA(); }
+interface B {
+  require impls A;
+}
+interface C {
+  require impls B;
+}
+
+impl forall [T:! B] T as A { fn AA() {} }
+
+// When we get to the definition we check that anything B requires is satisfied.
+// - The interface B requires A, so we must check T impls A.
+// - The impl for A requires T impls B.
+// - This impl is what provides T impls B.
+impl forall [T:! C] T as B {}
+
+fn F(T:! C) {
+  // If things go wrong, we find the impl `T as B`, but inside its definition is
+  // a lookup for `T as A`, which (through deducing `T`) includes a lookup for
+  // `T as B`. This creates a cycle of evaluating `T as B` recursively.
+  //
+  // This was solved by doing the check for requirements of `B` outside the
+  // definition of `T as B`.
+  // https://discord.com/channels/655572317891461132/941071822756143115/1463189087598022861
+  T.(A.AA)();
+}
+
+// --- fail_impl_requires_itself_cycle_with_monomorphization_error.carbon
+library "[[@TEST_NAME]]";
+
+class W(T:! type) {
+  adapt {};
+}
+
+interface A(N:! Core.IntLiteral()) {
+  // CHECK:STDERR: fail_impl_requires_itself_cycle_with_monomorphization_error.carbon:[[@LINE+3]]:26: error: array bound of -1 is negative [ArrayBoundNegative]
+  // CHECK:STDERR:   fn AA() -> W(array((), N));
+  // CHECK:STDERR:                          ^
+  fn AA() -> W(array((), N));
+}
+interface B(N:! Core.IntLiteral()) {
+  require impls A(N);
+}
+interface C(N:! Core.IntLiteral()) {
+  require impls B(N);
+}
+
+impl forall [N:! Core.IntLiteral(), T:! B(N)] T as A(N) {
+  fn AA() -> W(array((), N)) { return {} as W(array((), N)); }
+}
+
+impl forall [N:! Core.IntLiteral(), T:! C(N)] T as B(N) {
+  // The definition here does not contain the lookups verifying that C(N) impls
+  // `A(N)`, so they do not get re-evaluated for a specific `N`. That doesn't
+  // prevent us from producing a reasonable diagnostic when that `N` causes an
+  // error in the specific use of this impl, which we use to get from `C(N)` to
+  // `A(N)` (in the deduction of `T` in the impl as `A(N)`). The error just
+  // happens where we use `N` in `A(N)` instead of inside the verification that
+  // `T as B(N)` implies `T as A(N)`.
+}
+
+fn F(T:! C(-1)) {
+  // CHECK:STDERR: fail_impl_requires_itself_cycle_with_monomorphization_error.carbon:[[@LINE+4]]:3: note: in `AA()` used here [ResolvingSpecificHere]
+  // CHECK:STDERR:   T.(A(-1).AA)();
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  T.(A(-1).AA)();
+}

toolchain/check/testdata/impl/import_generic.carbon

@@ -325,10 +325,10 @@ impl forall [T:! type] D as N(T*) {}
 // CHECK:STDOUT:   %require_complete: <witness> = require_complete_type %I.type.1ab [symbolic]
 // CHECK:STDOUT:   %J.type.d6f: type = facet_type <@J, @J(%empty_struct_type)> [concrete]
 // CHECK:STDOUT:   %J.impl_witness: <witness> = impl_witness @C.as.J.impl.%J.impl_witness_table [concrete]
-// CHECK:STDOUT:   %Self.9e4: %J.type.d6f = symbolic_binding Self, 1 [symbolic]
-// CHECK:STDOUT:   %complete_type.5b1: <witness> = complete_type_witness %I.type.ab5 [concrete]
 // CHECK:STDOUT:   %type: type = facet_type <type> [concrete]
 // CHECK:STDOUT:   %C.type.facet: %type = facet_value %C, () [concrete]
+// CHECK:STDOUT:   %Self.9e4: %J.type.d6f = symbolic_binding Self, 1 [symbolic]
+// CHECK:STDOUT:   %complete_type.5b1: <witness> = complete_type_witness %I.type.ab5 [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {