Diagnose cycles in impl lookup (#4947)

Cycles are defined as reaching two independent lookups in a chain that
have all the same types involved. The acyclic rule states that this is
not possible and results in an error:
https://docs.carbon-lang.dev/docs/design/generics/details.html#acyclic-rule

To do this we need to track the types involved in impl lookup. The
interface constant includes the whole facet type being looked up, which
includes any specific types for generics or where constraints. Thus we
just need to compare the constant ids to look for this condition.

---------

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

toolchain/check/context.h

@@ -43,6 +43,12 @@ class Context {
   // add contextual notes as appropriate.
   using BuildDiagnosticFn = llvm::function_ref<auto()->DiagnosticBuilder>;
 
+  // An ongoing impl lookup, used to ensure termination.
+  struct ImplLookupStackEntry {
+    SemIR::ConstantId type_const_id;
+    SemIR::ConstantId interface_const_id;
+  };
+
   // Stores references for work.
   explicit Context(DiagnosticEmitter* emitter,
                    Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter,
@@ -236,6 +242,10 @@ class Context {
     return scope_stack_.full_pattern_stack();
   }
 
+  auto impl_lookup_stack() -> llvm::SmallVector<ImplLookupStackEntry>& {
+    return impl_lookup_stack_;
+  }
+
  private:
   // A FoldingSet node for a type.
   class TypeNode : public llvm::FastFoldingSetNode {
@@ -350,6 +360,10 @@ class Context {
 
   // Stack of single-entry regions being built.
   RegionStack region_stack_;
+
+  // Tracks all ongoing impl lookups in order to ensure that lookup terminates
+  // via the acyclic rule and the termination rule.
+  llvm::SmallVector<ImplLookupStackEntry> impl_lookup_stack_;
 };
 
 }  // namespace Carbon::Check

toolchain/check/impl_lookup.cpp

@@ -8,6 +8,8 @@
 #include "toolchain/check/generic.h"
 #include "toolchain/check/import_ref.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/impl.h"
+#include "toolchain/sem_ir/inst.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
@@ -110,6 +112,65 @@ static auto FindAssociatedImportIRs(Context& context,
   return result;
 }
 
+static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
+                                SemIR::ConstantId type_const_id,
+                                SemIR::ConstantId interface_const_id,
+                                const SemIR::Impl& impl) -> SemIR::InstId {
+  // If impl.constraint_id is not symbolic, and doesn't match the query, then
+  // we don't need to proceed.
+  auto impl_interface_const_id =
+      context.constant_values().Get(impl.constraint_id);
+  if (!impl_interface_const_id.is_symbolic() &&
+      interface_const_id != impl_interface_const_id) {
+    return SemIR::InstId::None;
+  }
+
+  // TODO: If the interface id of the `impl` and the query are not the same,
+  // then we can skip this `impl`. (The interface id is the root of the
+  // constraint, the unique `interface` declaration.)
+
+  auto specific_id = SemIR::SpecificId::None;
+  // This check comes first to avoid deduction with an invalid impl. We use an
+  // error value to indicate an error during creation of the impl, such as a
+  // recursive impl which will cause deduction to recurse infinitely.
+  if (impl.witness_id == SemIR::ErrorInst::SingletonInstId) {
+    return SemIR::InstId::None;
+  }
+  if (impl.generic_id.has_value()) {
+    specific_id = DeduceImplArguments(context, loc_id, impl, type_const_id,
+                                      interface_const_id);
+    if (!specific_id.has_value()) {
+      return SemIR::InstId::None;
+    }
+  }
+  if (!context.constant_values().AreEqualAcrossDeclarations(
+          SemIR::GetConstantValueInSpecific(context.sem_ir(), specific_id,
+                                            impl.self_id),
+          type_const_id)) {
+    return SemIR::InstId::None;
+  }
+  if (!context.constant_values().AreEqualAcrossDeclarations(
+          SemIR::GetConstantValueInSpecific(context.sem_ir(), specific_id,
+                                            impl.constraint_id),
+          interface_const_id)) {
+    // TODO: An impl of a constraint type should be treated as implementing
+    // the constraint's interfaces.
+    return SemIR::InstId::None;
+  }
+  if (!impl.witness_id.has_value()) {
+    // TODO: Diagnose if the impl isn't defined yet?
+    return SemIR::InstId::None;
+  }
+  LoadImportRef(context, impl.witness_id);
+  if (specific_id.has_value()) {
+    // We need a definition of the specific `impl` so we can access its
+    // witness.
+    ResolveSpecificDefinition(context, loc_id, specific_id);
+  }
+  return context.constant_values().GetInstId(SemIR::GetConstantValueInSpecific(
+      context.sem_ir(), specific_id, impl.witness_id));
+}
+
 auto LookupImplWitness(Context& context, SemIR::LocId loc_id,
                        SemIR::ConstantId type_const_id,
                        SemIR::ConstantId interface_const_id) -> SemIR::InstId {
@@ -130,63 +191,50 @@ auto LookupImplWitness(Context& context, SemIR::LocId loc_id,
     }
   }
 
-  for (const auto& impl : context.impls().array_ref()) {
-    // If impl.constraint_id is not symbolic, and doesn't match the query, then
-    // we don't need to proceed.
-    auto impl_interface_const_id =
-        context.constant_values().Get(impl.constraint_id);
-    if (!impl_interface_const_id.is_symbolic() &&
-        interface_const_id != impl_interface_const_id) {
-      continue;
+  auto& stack = context.impl_lookup_stack();
+  // Deduction of the interface parameters can do further impl lookups, and we
+  // need to ensure we terminate.
+  //
+  // https://docs.carbon-lang.dev/docs/design/generics/details.html#acyclic-rule
+  // - We look for violations of the acyclic rule by seeing if a previous lookup
+  //   had all the same type inputs.
+  // - The `interface_const_id` encodes the entire facet type being looked up,
+  //   including any specific parameters for a generic interface.
+  //
+  // TODO: Implement the termination rule, which requires looking at the
+  // complexity of the types on the top of (or throughout?) the stack:
+  // https://docs.carbon-lang.dev/docs/design/generics/details.html#termination-rule
+  for (auto entry : stack) {
+    if (entry.type_const_id == type_const_id &&
+        entry.interface_const_id == interface_const_id) {
+      CARBON_DIAGNOSTIC(ImplLookupCycle, Error,
+                        "cycle found in lookup of interface {0} for type {1}",
+                        std::string, SemIR::TypeId);
+      context.emitter()
+          .Build(loc_id, ImplLookupCycle, "<TODO: interface name>",
+                 context.types().GetTypeIdForTypeConstantId(type_const_id))
+          .Emit();
+      return SemIR::ErrorInst::SingletonInstId;
     }
+  }
 
-    // TODO: If the interface id of the `impl` and the query are not the same,
-    // then we can skip this `impl`. (The interface id is the root of the
-    // constraint, the unique `interface` declaration.)
+  auto witness_id = SemIR::InstId::None;
 
-    auto specific_id = SemIR::SpecificId::None;
-    // This check comes first to avoid deduction with an invalid impl. We use an
-    // error value to indicate an error during creation of the impl, such as a
-    // recursive impl which will cause deduction to recurse infinitely.
-    if (impl.witness_id == SemIR::ErrorInst::SingletonInstId) {
-      continue;
-    }
-    if (impl.generic_id.has_value()) {
-      specific_id = DeduceImplArguments(context, loc_id, impl, type_const_id,
-                                        interface_const_id);
-      if (!specific_id.has_value()) {
-        continue;
-      }
-    }
-    if (!context.constant_values().AreEqualAcrossDeclarations(
-            SemIR::GetConstantValueInSpecific(context.sem_ir(), specific_id,
-                                              impl.self_id),
-            type_const_id)) {
-      continue;
-    }
-    if (!context.constant_values().AreEqualAcrossDeclarations(
-            SemIR::GetConstantValueInSpecific(context.sem_ir(), specific_id,
-                                              impl.constraint_id),
-            interface_const_id)) {
-      // TODO: An impl of a constraint type should be treated as implementing
-      // the constraint's interfaces.
-      continue;
-    }
-    if (!impl.witness_id.has_value()) {
-      // TODO: Diagnose if the impl isn't defined yet?
-      return SemIR::InstId::None;
-    }
-    LoadImportRef(context, impl.witness_id);
-    if (specific_id.has_value()) {
-      // We need a definition of the specific `impl` so we can access its
-      // witness.
-      ResolveSpecificDefinition(context, loc_id, specific_id);
+  stack.push_back({
+      .type_const_id = type_const_id,
+      .interface_const_id = interface_const_id,
+  });
+  for (const auto& impl : context.impls().array_ref()) {
+    witness_id = GetWitnessIdForImpl(context, loc_id, type_const_id,
+                                     interface_const_id, impl);
+    if (witness_id.has_value()) {
+      // We found a matching impl, don't keep looking.
+      break;
     }
-    return context.constant_values().GetInstId(
-        SemIR::GetConstantValueInSpecific(context.sem_ir(), specific_id,
-                                          impl.witness_id));
   }
-  return SemIR::InstId::None;
+  stack.pop_back();
+
+  return witness_id;
 }
 
 }  // namespace Carbon::Check

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

@@ -0,0 +1,196 @@
+// 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_cycle.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_cycle.carbon
+
+// --- core.carbon
+package Core;
+
+interface As(Dest:! type) {
+  fn Convert[self: Self]() -> Dest;
+}
+
+interface ImplicitAs(Dest:! type) {
+  fn Convert[self: Self]() -> Dest;
+}
+
+// --- fail_impl_simple_cycle.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {}
+
+// This creates a dependency cycle with itself.
+impl forall [T:! Z] T as Z {}
+
+class Point {
+  impl as Z {}
+}
+
+fn F() {
+  // CHECK:STDERR: fail_impl_simple_cycle.carbon:[[@LINE+7]]:21: error: cycle found in lookup of interface <TODO: interface name> for type `Point` [ImplLookupCycle]
+  // CHECK:STDERR:   ({} as Point) as (Point as Z);
+  // CHECK:STDERR:                     ^~~~~~~~~~
+  // CHECK:STDERR: fail_impl_simple_cycle.carbon:[[@LINE-10]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: impl forall [T:! Z] T as Z {}
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  ({} as Point) as (Point as Z);
+}
+
+// --- fail_impl_cycle_one_generic_param.carbon
+library "[[@TEST_NAME]]";
+
+import Core;
+
+interface ComparableWith(T:! type) {}
+
+// This creates a dependency cycle with itself.
+impl forall [U:! type, T:! ComparableWith(U)]
+    U as ComparableWith(T) {}
+
+class C {}
+class D {}
+impl C as ComparableWith(D) {}
+
+fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}
+
+fn F() {
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE+20]]:3: error: cycle found in lookup of interface <TODO: interface name> for type `C` [ImplLookupCycle]
+  // CHECK:STDERR:   Compare({} as C, {} as C);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE-13]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: impl forall [U:! type, T:! ComparableWith(U)]
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE-9]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE+10]]:3: error: cannot implicitly convert from `type` to `ComparableWith(C)` [ImplicitAsConversionFailure]
+  // CHECK:STDERR:   Compare({} as C, {} as C);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE+7]]:3: note: type `type` does not implement interface `Core.ImplicitAs(ComparableWith(C))` [MissingImplInMemberAccessNote]
+  // CHECK:STDERR:   Compare({} as C, {} as C);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE-19]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  Compare({} as C, {} as C);
+
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE+13]]:3: error: cycle found in lookup of interface <TODO: interface name> for type `D` [ImplLookupCycle]
+  // CHECK:STDERR:   Compare({} as C, {} as D);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE-35]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: impl forall [U:! type, T:! ComparableWith(U)]
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE-38]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: impl forall [U:! type, T:! ComparableWith(U)]
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_impl_cycle_one_generic_param.carbon:[[@LINE-34]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  Compare({} as C, {} as D);
+}
+
+// --- impl_recurse_with_simpler_type_no_cycle.carbon
+library "[[@TEST_NAME]]";
+
+import Core;
+
+class Wraps(T:! type) {}
+
+interface Printable {}
+
+// If T is printable, so is Wraps(T).
+impl forall [T:! Printable] Wraps(T) as Printable {}
+
+class C {}
+impl C as Printable {}
+
+fn F() {
+  ({} as C) as (C as Printable);
+  ({} as Wraps(C)) as (Wraps(C) as Printable);
+}
+
+// --- impl_recurse_with_simpler_type_in_generic_param_no_cycle.carbon
+library "[[@TEST_NAME]]";
+
+import Core;
+
+class Wraps(T:! type) {}
+
+interface ComparableTo(T:! type) {}
+
+// If U is comparable to T, then U is comparable to Wraps(T).
+impl forall [T:! type, U:! ComparableTo(T)] U as ComparableTo(Wraps(T)) {}
+// If U is comparable to T then Wraps(U) is comparable to T.
+impl forall [T:! type, U:! ComparableTo(T)] Wraps(U) as ComparableTo(T) {}
+
+class C {}
+class D {}
+impl C as ComparableTo(D) {}
+
+fn F() {
+  ({} as C) as (C as ComparableTo(D));
+  ({} as C) as (C as ComparableTo(Wraps(D)));
+  ({} as Wraps(C)) as (Wraps(C) as ComparableTo(D));
+  ({} as Wraps(C)) as (Wraps(C) as ComparableTo(Wraps(D)));
+}
+
+// --- fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon
+library "[[@TEST_NAME]]";
+
+import Core;
+
+// Implement this for a type in one direction.
+interface ComparableTo(T:! type) {}
+// Use this as a bound with two types in any direction.
+interface ComparableWith(T:! type) {}
+
+class C {}
+class D {}
+// C is comparable to D. This should imply:
+// - C is ComparableWith(C).
+// - C is ComparableWith(D).
+// - D is ComparableWith(C).
+impl C as ComparableTo(D) {}
+
+// T is always ComparableWith(T).
+impl forall [T:! type] T as ComparableWith(T) {}
+// If U is ComparableTo(T), U is ComparableWith(T).
+//
+// TODO: When we look for the impl of `D as ComparableTo(C)` here, we find that
+// it's not implemented. We then go to the next impl which does match, but
+// deduction generates an error here regardless.
+impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {}
+// If U is ComparableTo(T), T is ComparableWith(U).
+impl forall [T:! type, U:! ComparableTo(T)] T as ComparableWith(U) {}
+
+fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}
+
+fn F() {
+  Compare({} as C, {} as C);
+  // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE+13]]:3: error: cannot implicitly convert from `type` to `ComparableTo(C)` [ImplicitAsConversionFailure]
+  // CHECK:STDERR:   Compare({} as C, {} as D);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE+10]]:3: note: type `type` does not implement interface `Core.ImplicitAs(ComparableTo(C))` [MissingImplInMemberAccessNote]
+  // CHECK:STDERR:   Compare({} as C, {} as D);
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE-14]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {}
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE-13]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere]
+  // CHECK:STDERR: fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}
+  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  Compare({} as C, {} as D);
+  Compare({} as D, {} as C);
+}

toolchain/diagnostics/diagnostic_kind.def

@@ -291,6 +291,7 @@ CARBON_DIAGNOSTIC_KIND(ImplMissingFunction)
 CARBON_DIAGNOSTIC_KIND(ImplPreviousDefinition)
 CARBON_DIAGNOSTIC_KIND(ImplRedefinition)
 CARBON_DIAGNOSTIC_KIND(ImplOfUndefinedInterface)
+CARBON_DIAGNOSTIC_KIND(ImplLookupCycle)
 
 // Impl lookup.
 CARBON_DIAGNOSTIC_KIND(MissingImplInMemberAccess)