Use earlier require decls inside a named constraint to provide witnesses for Self (#6915)

Performing a lookup against `Self` inside the definition of the named
constraint leads to cycles, as described in the document [Self
contradictions in Named
Constraints](https://docs.google.com/document/d/17rn2XmME8o2MM4OJqatSVuMa1iYZ1PAgcNrf0PXR9Q4/edit?tab=t.0).
To prevent those cycles, this change introduces a large refactoring of
impl lookup.

The impl lookup done inside eval is reduced to only performing
monomorphization. That is it:
- Only looks for an provides final witnesses.
- Is not allowed to identify the facet type of the query self.
- Returns either a final witness or None (or an error)

The paths for finding non-final witnesses are now done outside of eval,
directly in the initial `LookupImplWitness()` function. If no final
witness it found through eval, the resulting non-final
`LookupImplWitness` instruction witness is returned. It does not produce
cycles to identify the facet type of query self outside of eval, since
that does not result in repeating the identification when resolving
specifics of the named constraint or require decl.

Move the ArrayStack for Context::require_impls_stack into a new class
which tracks a NamedConstraintId (or InterfaceId) for each frame of
RequireImplsIds, so that in type completion we always can find the
correct frame for a given named constraint which is still being defined,
in order to find the RequireImplsIds in the in-progress definition.

Author: danakj

toolchain/check/BUILD

@@ -66,6 +66,7 @@ cc_library(
         "pattern.cpp",
         "pattern_match.cpp",
         "pointer_dereference.cpp",
+        "require_impls_stack.cpp",
         "return.cpp",
         "scope_stack.cpp",
         "subst.cpp",
@@ -134,6 +135,7 @@ cc_library(
         "pending_block.h",
         "pointer_dereference.h",
         "region_stack.h",
+        "require_impls_stack.h",
         "return.h",
         "scope_index.h",
         "scope_stack.h",

toolchain/check/check_unit.cpp

@@ -530,20 +530,21 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
   auto poisoned_queries =
       std::exchange(context_.poisoned_concrete_impl_lookup_queries(), {});
   for (const auto& poison : poisoned_queries) {
-    auto witness_result = EvalLookupSingleImplWitness(
-        context_, poison.loc_id, poison.query, poison.query.query_self_inst_id,
+    auto found_witness_id = EvalLookupSingleFinalWitness(
+        context_, poison.loc_id, poison.query, SemIR::InstId::None,
         EvalImplLookupMode::RecheckPoisonedLookup);
-    CARBON_CHECK(witness_result.has_final_value());
-    auto found_witness_id = witness_result.final_witness();
-    if (found_witness_id == SemIR::ErrorInst::InstId) {
+    CARBON_CHECK(found_witness_id.has_value());
+    if (found_witness_id == SemIR::ErrorInst::ConstantId) {
       // Errors may have been diagnosed with the impl used in the poisoned query
       // in the meantime (such as a missing definition).
       continue;
     }
-    if (found_witness_id != poison.impl_witness) {
-      auto witness_to_impl_id = [&](SemIR::InstId witness_id) {
+    if (found_witness_id != poison.witness_id) {
+      auto witness_to_impl_id = [&](SemIR::ConstantId witness_id) {
+        // TODO: Add and use constant_values().GetAs<SemIR::FacetType>().
+        auto inst_id = context_.constant_values().GetInstId(witness_id);
         auto table_id = context_.insts()
-                            .GetAs<SemIR::ImplWitness>(witness_id)
+                            .GetAs<SemIR::ImplWitness>(inst_id)
                             .witness_table_id;
         return context_.insts()
             .GetAs<SemIR::ImplWitnessTable>(table_id)
@@ -555,7 +556,7 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
       auto bad_impl_id = witness_to_impl_id(found_witness_id);
       const auto& bad_impl = context_.impls().Get(bad_impl_id);
 
-      auto prev_impl_id = witness_to_impl_id(poison.impl_witness);
+      auto prev_impl_id = witness_to_impl_id(poison.witness_id);
       const auto& prev_impl = context_.impls().Get(prev_impl_id);
 
       CARBON_DIAGNOSTIC(

toolchain/check/context.h

@@ -25,6 +25,7 @@
 #include "toolchain/check/node_stack.h"
 #include "toolchain/check/param_and_arg_refs_stack.h"
 #include "toolchain/check/region_stack.h"
+#include "toolchain/check/require_impls_stack.h"
 #include "toolchain/check/scope_stack.h"
 #include "toolchain/diagnostics/emitter.h"
 #include "toolchain/parse/node_ids.h"
@@ -125,7 +126,7 @@ class Context {
     return field_decls_stack_;
   }
 
-  auto require_impls_stack() -> ArrayStack<SemIR::RequireImplsId>& {
+  auto require_impls_stack() -> RequireImplsStack& {
     return require_impls_stack_;
   }
 
@@ -221,6 +222,9 @@ class Context {
     SemIR::ConstantId query_facet_type_const_id;
     // The location of the impl being looked at for the stack entry.
     SemIR::InstId impl_loc = SemIR::InstId::None;
+    // TODO: If we make a proper stack class, we only need one `diagnosed_cycle`
+    // field, which resets to false when the stack becomes empty.
+    bool diagnosed_cycle = false;
   };
   auto impl_lookup_stack() -> llvm::SmallVector<ImplLookupStackEntry>& {
     return impl_lookup_stack_;
@@ -229,7 +233,7 @@ class Context {
   // A map from a (self, interface) pair to a final witness.
   using ImplLookupCacheKey =
       std::pair<SemIR::ConstantId, SemIR::SpecificInterfaceId>;
-  using ImplLookupCacheMap = Map<ImplLookupCacheKey, SemIR::InstId>;
+  using ImplLookupCacheMap = Map<ImplLookupCacheKey, SemIR::ConstantId>;
   auto impl_lookup_cache() -> ImplLookupCacheMap& { return impl_lookup_cache_; }
 
   // An impl lookup query that resulted in a concrete witness from finding an
@@ -240,8 +244,8 @@ class Context {
     SemIR::LocId loc_id;
     // The query for a witness of an impl for an interface.
     SemIR::LookupImplWitness query;
-    // The resulting ImplWitness.
-    SemIR::InstId impl_witness;
+    // The resulting final witness.
+    SemIR::ConstantId witness_id;
   };
   auto poisoned_concrete_impl_lookup_queries()
       -> llvm::SmallVector<PoisonedConcreteImplLookupQuery>& {
@@ -425,7 +429,7 @@ class Context {
 
   // The stack of RequireImpls for in-progress Interface and Constraint
   // definitions.
-  ArrayStack<SemIR::RequireImplsId> require_impls_stack_;
+  RequireImplsStack require_impls_stack_;
 
   // The stack used for qualified declaration name construction.
   DeclNameStack decl_name_stack_;

toolchain/check/convert.cpp

@@ -1512,7 +1512,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id,
     // by finding impl witnesses for the target FacetType.
     auto lookup_result = LookupImplWitness(
         context, loc_id, sem_ir.constant_values().Get(type_inst_id),
-        sem_ir.types().GetConstantId(target.type_id));
+        sem_ir.types().GetConstantId(target.type_id), target.diagnose);
     if (lookup_result.has_value()) {
       if (lookup_result.has_error_value()) {
         return SemIR::ErrorInst::InstId;

toolchain/check/custom_witness.cpp

@@ -377,7 +377,7 @@ static auto TypeCanDestroy(Context& context,
 static auto MakeDestroyWitness(
     Context& context, SemIR::LocId loc_id,
     SemIR::ConstantId query_self_const_id,
-    SemIR::SpecificInterfaceId query_specific_interface_id)
+    SemIR::SpecificInterfaceId query_specific_interface_id, bool build_witness)
     -> std::optional<SemIR::InstId> {
   auto query_specific_interface =
       context.specific_interfaces().Get(query_specific_interface_id);
@@ -387,6 +387,10 @@ static auto MakeDestroyWitness(
     return std::nullopt;
   }
 
+  if (!build_witness) {
+    return SemIR::InstId::None;
+  }
+
   if (query_self_const_id.is_symbolic()) {
     return SemIR::InstId::None;
   }
@@ -407,7 +411,7 @@ static auto MakeDestroyWitness(
 static auto MakeIntFitsInWitness(
     Context& context, SemIR::LocId loc_id,
     SemIR::ConstantId query_self_const_id,
-    SemIR::SpecificInterfaceId query_specific_interface_id)
+    SemIR::SpecificInterfaceId query_specific_interface_id, bool build_witness)
     -> std::optional<SemIR::InstId> {
   auto query_specific_interface =
       context.specific_interfaces().Get(query_specific_interface_id);
@@ -470,22 +474,26 @@ static auto MakeIntFitsInWitness(
     return std::nullopt;
   }
 
+  if (!build_witness) {
+    return SemIR::InstId::None;
+  }
+
   return BuildCustomWitness(context, loc_id, query_self_const_id,
                             query_specific_interface_id, {});
 }
 
 auto LookupCustomWitness(Context& context, SemIR::LocId loc_id,
                          CoreInterface core_interface,
                          SemIR::ConstantId query_self_const_id,
-                         SemIR::SpecificInterfaceId query_specific_interface_id)
-    -> std::optional<SemIR::InstId> {
+                         SemIR::SpecificInterfaceId query_specific_interface_id,
+                         bool build_witness) -> std::optional<SemIR::InstId> {
   switch (core_interface) {
     case CoreInterface::Destroy:
       return MakeDestroyWitness(context, loc_id, query_self_const_id,
-                                query_specific_interface_id);
+                                query_specific_interface_id, build_witness);
     case CoreInterface::IntFitsIn:
       return MakeIntFitsInWitness(context, loc_id, query_self_const_id,
-                                  query_specific_interface_id);
+                                  query_specific_interface_id, build_witness);
     case CoreInterface::Copy:
     case CoreInterface::CppUnsafeDeref:
     case CoreInterface::Default:

toolchain/check/custom_witness.h

@@ -39,11 +39,15 @@ auto GetCoreInterface(Context& context, SemIR::InterfaceId interface_id)
 // Returns a witness for a `CoreInterface` `CustomWitness`. A return value of
 // `None` indicates a non-final witness should be produced, while `std::nullopt`
 // indicates the query is final and no witness can be produced.
+//
+// If `build_witness` is false, this function always returns `None` as the
+// witness, whether it would be final or not. It is used to indicate the
+// presence of such a witness without adding instructions for it.
 auto LookupCustomWitness(Context& context, SemIR::LocId loc_id,
                          CoreInterface core_interface,
                          SemIR::ConstantId query_self_const_id,
-                         SemIR::SpecificInterfaceId query_specific_interface_id)
-    -> std::optional<SemIR::InstId>;
+                         SemIR::SpecificInterfaceId query_specific_interface_id,
+                         bool build_witness) -> std::optional<SemIR::InstId>;
 
 }  // namespace Carbon::Check
 

toolchain/check/eval_inst.cpp

@@ -281,48 +281,35 @@ auto EvalConstantInst(Context& /*context*/, SemIR::FunctionDecl inst)
 
 auto EvalConstantInst(Context& context, SemIR::InstId inst_id,
                       SemIR::LookupImplWitness inst) -> ConstantEvalResult {
+  // If the monomorphized query self is a FacetValue, we may get a witness from
+  // it under limited circumstances. If no final witness is found though, we
+  // don't need to preserve it for future evaluations, so we strip it from the
+  // LookupImplWitness instruction to reduce the number of distinct constant
+  // values.
+  auto self_facet_value_inst_id = SemIR::InstId::None;
+  if (auto facet_value = context.insts().TryGetAs<SemIR::FacetValue>(
+          inst.query_self_inst_id)) {
+    self_facet_value_inst_id =
+        std::exchange(inst.query_self_inst_id, facet_value->type_inst_id);
+  }
+
   // The self value is canonicalized in order to produce a canonical
   // LookupImplWitness instruction, avoiding multiple constant values for
   // `<facet value>` and `<facet value>` as type, which always have the same
   // lookup result.
-  auto self_facet_value_inst_id =
+  inst.query_self_inst_id =
       GetCanonicalFacetOrTypeValue(context, inst.query_self_inst_id);
 
-  // When we look for a witness in the (facet) type of self, we may get a
-  // concrete witness from a `FacetValue` (which is `self_facet_value_inst_id`)
-  // in which case this instruction evaluates to that witness.
-  //
-  // If we only get a symbolic witness result though, then this instruction
-  // evaluates to a `LookupImplWitness`. Since there was no concrete result in
-  // the `FacetValue`, we don't need to preserve it. By looking through the
-  // `FacetValue` at the type value it wraps to generate a more canonical value
-  // for a symbolic `LookupImplWitness`. This makes us produce the same constant
-  // value for symbolic lookups in `FacetValue(T)` and `T`, since they will
-  // always have the same lookup result later, when `T` is replaced in a
-  // specific by something that can provide a concrete witness.
-  if (auto facet_value = context.insts().TryGetAs<SemIR::FacetValue>(
-          self_facet_value_inst_id)) {
-    inst.query_self_inst_id =
-        GetCanonicalFacetOrTypeValue(context, facet_value->type_inst_id);
-  } else {
-    inst.query_self_inst_id = self_facet_value_inst_id;
-  }
-
-  auto result = EvalLookupSingleImplWitness(context, SemIR::LocId(inst_id),
-                                            inst, self_facet_value_inst_id,
-                                            EvalImplLookupMode::Normal);
-  if (!result.has_value()) {
-    // We use NotConstant to communicate back to impl lookup that the lookup
-    // failed. This can not happen for a deferred symbolic lookup in a generic
-    // eval block, since we only add the deferred lookup instruction (being
-    // evaluated here) to the SemIR if the lookup succeeds.
-    return ConstantEvalResult::NotConstant;
+  auto witness_id = EvalLookupSingleFinalWitness(context, SemIR::LocId(inst_id),
+                                                 inst, self_facet_value_inst_id,
+                                                 EvalImplLookupMode::Normal);
+  if (witness_id == SemIR::ErrorInst::ConstantId) {
+    return ConstantEvalResult::Error;
   }
-  if (result.has_final_value()) {
-    return ConstantEvalResult::Existing(
-        context.constant_values().Get(result.final_witness()));
+  if (witness_id.has_value()) {
+    return ConstantEvalResult::Existing(witness_id);
   }
-
+  // Try again when the query is modified by a specific.
   return ConstantEvalResult::NewSamePhase(inst);
 }
 

toolchain/check/generic.cpp

@@ -147,8 +147,21 @@ class RebuildGenericConstantInEvalBlockCallbacks : public SubstInstCallbacks {
 
   auto ReuseUnchanged(SemIR::InstId orig_inst_id) -> SemIR::InstId override {
     auto inst = context().insts().Get(orig_inst_id);
+
+    auto const_id = context().constant_values().Get(orig_inst_id);
+    const auto& symbolic =
+        context().constant_values().GetSymbolicConstant(const_id);
+    // Template actions are inserted into the eval block directly, instead of
+    // adding a new instruction, in `AddTemplateActionToEvalBlock`. This means
+    // any instruction that is symbolic because it contains a template action as
+    // an operand would Rebuild that operand with the same instruction. Then the
+    // dependent instruction would be reused unchanged.
+    bool is_template =
+        symbolic.dependence == SemIR::ConstantDependence::Template;
+
     CARBON_CHECK(
-        (inst.IsOneOf<SemIR::SymbolicBinding, SemIR::SymbolicBindingPattern>()),
+        is_template || (inst.IsOneOf<SemIR::SymbolicBinding,
+                                     SemIR::SymbolicBindingPattern>()),
         "Instruction {0} has symbolic constant value but no symbolic operands",
         inst);
 

toolchain/check/handle_interface.cpp

@@ -184,7 +184,7 @@ auto HandleParseNode(Context& context,
       context.inst_block_stack().PeekOrAdd();
 
   context.inst_block_stack().Push();
-  context.require_impls_stack().PushArray();
+  context.require_impls_stack().Push(interface_id);
   // We use the arg stack to build the witness table type.
   context.args_type_info_stack().Push();
 
@@ -213,8 +213,8 @@ auto HandleParseNode(Context& context, Parse::InterfaceDefinitionId /*node_id*/)
   auto associated_entities_id = context.args_type_info_stack().Pop();
 
   auto require_impls_block_id = context.require_impls_blocks().Add(
-      context.require_impls_stack().PeekArray());
-  context.require_impls_stack().PopArray();
+      context.require_impls_stack().PeekTop());
+  context.require_impls_stack().Pop();
 
   auto& interface_info = context.interfaces().Get(interface_id);
   if (!interface_info.associated_entities_id.has_value()) {

toolchain/check/handle_named_constraint.cpp

@@ -184,7 +184,7 @@ auto HandleParseNode(Context& context,
   constraint_info.body_block_with_self_id =
       context.inst_block_stack().PeekOrAdd();
 
-  context.require_impls_stack().PushArray();
+  context.require_impls_stack().Push(named_constraint_id);
 
   context.node_stack().Push(node_id, named_constraint_id);
   return true;
@@ -199,8 +199,8 @@ auto HandleParseNode(Context& context,
   context.inst_block_stack().Pop();
 
   auto require_impls_block_id = context.require_impls_blocks().Add(
-      context.require_impls_stack().PeekArray());
-  context.require_impls_stack().PopArray();
+      context.require_impls_stack().PeekTop());
+  context.require_impls_stack().Pop();
 
   auto& constraint_info = context.named_constraints().Get(named_constraint_id);
   if (!constraint_info.complete) {