Avoid resolving the decl block for specifics in imported instructions (#5517)

Move the operation of resolving the specific decl block from
`GetConstantValue()` to `TryEvalTypedInst()`, with is now happening
after replacing the fields of the instruction with new constant values,
but before running the evaluation of the instruction. Since imported
instructions are not evaluated, this avoids resolving the specific decl
block from imported instructions, resolving a TODO in
`AddImportedConstant()`. Now `AddImportedConstant()` can replace
constant values in its fields without having to worry about that
operation resolving any specific decl blocks.

We get to add a new TODO however, to explain why we still need a special
case in resolving specific decl blocks for handling `Impl` construction.
The witness table contains instructions with specifics referring to the
generic self of the impl declaration. But the table must be constructed
before the impl's generic is finished, in order to make the instructions
dependent for the generic. But then resolving the specific decl block
can't be done when the instructions are created and evaluated, as that
requires a finished generic.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/check/BUILD

@@ -107,6 +107,7 @@ cc_library(
         ":node_stack",
         "//common:array_stack",
         "//common:check",
+        "//common:concepts",
         "//common:find",
         "//common:map",
         "//common:ostream",

toolchain/check/eval.cpp

@@ -180,6 +180,7 @@ class EvalContext {
   auto facet_types() -> CanonicalValueStore<SemIR::FacetTypeId>& {
     return sem_ir().facet_types();
   }
+  auto generics() -> const SemIR::GenericStore& { return sem_ir().generics(); }
   auto specifics() -> const SemIR::SpecificStore& {
     return sem_ir().specifics();
   }
@@ -542,29 +543,21 @@ static auto GetConstantValue(EvalContext& eval_context,
     return SemIR::SpecificId::None;
   }
 
+  // Generally, when making a new specific, it's done through MakeSpecific(),
+  // which will ensure the declaration is resolved.
+  //
+  // However, the SpecificId returned here is intentionally left without its
+  // declaration resolved. Imported instructions with SpecificIds should not
+  // have the specific's declaration resolved, but other instructions which
+  // include a new SpecificId should.
+  //
+  // The resolving of the specific's declaration will be ensured later when
+  // evaluating the instruction containing the SpecificId.
   if (args_id == specific.args_id) {
-    const auto& specific = eval_context.specifics().Get(specific_id);
-    // A constant specific_id should always have a resolved declaration. The
-    // specific_id from the instruction may coincidentally be canonical, and so
-    // constant evaluation gives the same value. In that case, we still need to
-    // ensure its declaration is resolved.
-    //
-    // However, don't resolve the declaration if the generic's eval block hasn't
-    // been set yet. This happens when building the eval block during import.
-    //
-    // TODO: Change importing of generic eval blocks to be less fragile and
-    // remove this `if` so we unconditionally call `ResolveSpecificDeclaration`.
-    if (!specific.decl_block_id.has_value() && eval_context.context()
-                                                   .generics()
-                                                   .Get(specific.generic_id)
-                                                   .decl_block_id.has_value()) {
-      ResolveSpecificDeclaration(eval_context.context(),
-                                 eval_context.fallback_loc_id(), specific_id);
-    }
     return specific_id;
   }
-  return MakeSpecific(eval_context.context(), eval_context.fallback_loc_id(),
-                      specific.generic_id, args_id);
+  return eval_context.context().specifics().GetOrAdd(specific.generic_id,
+                                                     args_id);
 }
 
 static auto GetConstantValue(EvalContext& eval_context,
@@ -768,6 +761,99 @@ static auto ReplaceTypeWithConstantValue(EvalContext& eval_context,
   return IsConstant(*phase);
 }
 
+template <typename... T>
+struct KindHasGetConstantValueOverload;
+
+template <typename... Types>
+struct KindHasGetConstantValueOverload<TypeEnum<Types...>> {
+  static constexpr auto Value(TypeEnum<Types...> e) -> bool {
+    constexpr auto Values = [] {
+      std::array<bool, SemIR::IdKind::NumValues> values = {false};
+      ((values[SemIR::IdKind::template For<Types>.ToIndex()] =
+            HasGetConstantValueOverload<Types>),
+       ...);
+      return values;
+    }();
+    return Values[e.ToIndex()];
+  }
+};
+
+static auto ResolveSpecificDeclForSpecificId(EvalContext& eval_context,
+                                             SemIR::SpecificId specific_id)
+    -> void {
+  if (!specific_id.has_value()) {
+    return;
+  }
+
+  const auto& specific = eval_context.specifics().Get(specific_id);
+  const auto& generic = eval_context.generics().Get(specific.generic_id);
+  if (specific_id == generic.self_specific_id) {
+    // Impl witness table construction happens before its generic decl is
+    // finish, in order to make the table's instructions dependent
+    // instructions of the Impl's generic. But those instructions can refer to
+    // the generic's self specific. We can not resolve the specific
+    // declaration for the self specific until the generic is finished, but it
+    // is explicitly resolved at that time in `FinishGenericDecl()`.
+    return;
+  }
+  ResolveSpecificDecl(eval_context.context(), eval_context.fallback_loc_id(),
+                      specific_id);
+}
+
+// Resolves the specific declarations for a specific id in any field of the
+// `inst` instruction.
+static auto ResolveSpecificDeclForInst(EvalContext& eval_context,
+                                       const SemIR::Inst& inst) -> void {
+  for (auto arg_and_kind : {inst.arg0_and_kind(), inst.arg1_and_kind()}) {
+    // This switch must handle any field type that has a GetConstantValue()
+    // overload which canonicalizes a specific (and thus potentially forms a new
+    // specific) as part of forming its constant value.
+    CARBON_KIND_SWITCH(arg_and_kind) {
+      case CARBON_KIND(SemIR::FacetTypeId facet_type_id): {
+        const auto& info =
+            eval_context.context().facet_types().Get(facet_type_id);
+        for (const auto& interface : info.extend_constraints) {
+          ResolveSpecificDeclForSpecificId(eval_context, interface.specific_id);
+        }
+        for (const auto& interface : info.self_impls_constraints) {
+          ResolveSpecificDeclForSpecificId(eval_context, interface.specific_id);
+        }
+        break;
+      }
+      case CARBON_KIND(SemIR::SpecificId specific_id): {
+        ResolveSpecificDeclForSpecificId(eval_context, specific_id);
+        break;
+      }
+      case CARBON_KIND(SemIR::SpecificInterfaceId specific_interface_id): {
+        ResolveSpecificDeclForSpecificId(eval_context,
+                                         eval_context.specific_interfaces()
+                                             .Get(specific_interface_id)
+                                             .specific_id);
+        break;
+      }
+
+        // These id types have a GetConstantValue() overload but that overload
+        // does not canonicalize any SpecificId in the value type.
+      case SemIR::IdKind::For<SemIR::DestInstId>:
+      case SemIR::IdKind::For<SemIR::EntityNameId>:
+      case SemIR::IdKind::For<SemIR::InstBlockId>:
+      case SemIR::IdKind::For<SemIR::InstId>:
+      case SemIR::IdKind::For<SemIR::MetaInstId>:
+      case SemIR::IdKind::For<SemIR::StructTypeFieldsId>:
+      case SemIR::IdKind::For<SemIR::TypeInstId>:
+        break;
+
+      default:
+        CARBON_CHECK(
+            !KindHasGetConstantValueOverload<SemIR::IdKind>::Value(
+                arg_and_kind.kind()),
+            "Missing case for {0} which has a GetConstantValue() overload",
+            arg_and_kind.kind());
+        break;
+    }
+  }
+}
+
 auto AddImportedConstant(Context& context, SemIR::Inst inst)
     -> SemIR::ConstantId {
   EvalContext eval_context(&context, SemIR::LocId::None);
@@ -775,8 +861,6 @@ auto AddImportedConstant(Context& context, SemIR::Inst inst)
                inst.kind());
   Phase phase = GetPhase(context.constant_values(),
                          context.types().GetConstantId(inst.type_id()));
-  // TODO: Can we avoid doing this replacement? It may do things that are
-  // undesirable during importing, such as resolving specifics.
   if (!ReplaceAllFieldsWithConstantValues(eval_context, &inst, &phase)) {
     return SemIR::ConstantId::NotConstant;
   }
@@ -1769,6 +1853,12 @@ static auto TryEvalTypedInst(EvalContext& eval_context, SemIR::InstId inst_id,
       }
       return MakeNonConstantResult(phase);
     }
+
+    // When canonicalizing a SpecificId, we defer resolving the specific's
+    // declaration until here, to avoid resolving declarations from imported
+    // specifics. (Imported instructions are not evaluated.)
+    ResolveSpecificDeclForInst(eval_context, inst);
+
     if constexpr (ConstantKind == SemIR::InstConstantKind::Always ||
                   ConstantKind == SemIR::InstConstantKind::WheneverPossible) {
       return MakeConstantResult(eval_context.context(), inst, phase);

toolchain/check/generic.cpp

@@ -475,8 +475,8 @@ auto FinishGenericDecl(Context& context, SemIR::LocId loc_id,
   context.generic_region_stack().Pop();
   context.generics().Get(generic_id).decl_block_id = decl_block_id;
 
-  ResolveSpecificDeclaration(context, loc_id,
-                             context.generics().GetSelfSpecific(generic_id));
+  ResolveSpecificDecl(context, loc_id,
+                      context.generics().GetSelfSpecific(generic_id));
 }
 
 auto BuildGenericDecl(Context& context, SemIR::InstId decl_id)
@@ -643,8 +643,8 @@ auto FinishGenericDefinition(Context& context, SemIR::GenericId generic_id)
   context.generics().Get(generic_id).definition_block_id = definition_block_id;
 }
 
-auto ResolveSpecificDeclaration(Context& context, SemIR::LocId loc_id,
-                                SemIR::SpecificId specific_id) -> void {
+auto ResolveSpecificDecl(Context& context, SemIR::LocId loc_id,
+                         SemIR::SpecificId specific_id) -> void {
   // If this is the first time we've formed this specific, evaluate its decl
   // block to form information about the specific.
   if (!context.specifics().Get(specific_id).decl_block_id.has_value()) {
@@ -666,7 +666,7 @@ auto MakeSpecific(Context& context, SemIR::LocId loc_id,
                   SemIR::GenericId generic_id, SemIR::InstBlockId args_id)
     -> SemIR::SpecificId {
   auto specific_id = context.specifics().GetOrAdd(generic_id, args_id);
-  ResolveSpecificDeclaration(context, loc_id, specific_id);
+  ResolveSpecificDecl(context, loc_id, specific_id);
   return specific_id;
 }
 
@@ -703,7 +703,7 @@ auto MakeSelfSpecific(Context& context, SemIR::LocId loc_id,
   // TODO: This could be made more efficient. We don't need to perform
   // substitution here; we know we want identity mappings for all constants and
   // types. We could also consider not storing the mapping at all in this case.
-  ResolveSpecificDeclaration(context, loc_id, specific_id);
+  ResolveSpecificDecl(context, loc_id, specific_id);
   return specific_id;
 }
 

toolchain/check/generic.h

@@ -102,8 +102,8 @@ auto MakeSelfSpecific(Context& context, SemIR::LocId loc_id,
 // Resolve the declaration of the given specific, by evaluating the eval block
 // of the corresponding generic and storing a corresponding value block in the
 // specific.
-auto ResolveSpecificDeclaration(Context& context, SemIR::LocId loc_id,
-                                SemIR::SpecificId specific_id) -> void;
+auto ResolveSpecificDecl(Context& context, SemIR::LocId loc_id,
+                         SemIR::SpecificId specific_id) -> void;
 
 // Attempts to resolve the definition of the given specific, by evaluating the
 // eval block of the corresponding generic and storing a corresponding value

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

@@ -699,12 +699,6 @@ var n: Int(64) = MakeFromClass(FromLiteral(64) as OtherInt);
 // CHECK:STDOUT:   %pattern_type => constants.%pattern_type.a71
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: specific @Op.1(constants.%Add.facet.5a7) {
-// CHECK:STDOUT:   %Self => constants.%Add.facet.5a7
-// CHECK:STDOUT:   %Self.as_type => constants.%MyInt.09f
-// CHECK:STDOUT:   %pattern_type => constants.%pattern_type.a71
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
 // CHECK:STDOUT: specific @Double(constants.%int_64) {
 // CHECK:STDOUT:   %N => constants.%int_64
 // CHECK:STDOUT:   %MyInt => constants.%MyInt.f30