Always import canonical instructions except for exceptional Decl cases (#6009)

There are a few instructions that import in multiple phases, which
receive the `const_id` and use it to construct multiple constants until
building the final constant value. These include
`AssociatedConstantDecl`, `FunctionDecl`, and `InterfaceDecl`.

Other instructions just construct a constant value in a single attempt,
once all their dependencies are imported. For these instruction types,
avoid importing the non-canonical instruction. Always get the canonical
constant instruction and import that.

Since the constant value of an instruction can have a very different
structure than its non-canonical value, this ensures import has a
consistent structure to work with, by only working with canonical values
as much as possible.

The `VtableDecl` and `VtablePtr` were set up to pass along `const_id`
but do not actually require multiple phases, so they have been changed
to stop passing along the unused (and always empty) `const_id`.

---------

Co-authored-by: Jon Ross-Perkins <[email protected]>

Author: danakj

toolchain/check/import_ref.cpp

@@ -2046,9 +2046,7 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
 }
 
 static auto TryResolveTypedInst(ImportRefResolver& resolver,
-                                SemIR::VtableDecl inst,
-                                SemIR::ConstantId /*vtable_const_id*/)
-    -> ResolveResult {
+                                SemIR::VtableDecl inst) -> ResolveResult {
   const auto& import_vtable = resolver.import_vtables().Get(inst.vtable_id);
   auto class_const_id =
       GetLocalConstantId(resolver, resolver.import_classes()
@@ -2117,9 +2115,7 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
 }
 
 static auto TryResolveTypedInst(ImportRefResolver& resolver,
-                                SemIR::VtablePtr inst,
-                                SemIR::ConstantId /*vtable_const_id*/)
-    -> ResolveResult {
+                                SemIR::VtablePtr inst) -> ResolveResult {
   auto specific_data = GetLocalSpecificData(resolver, inst.specific_id);
 
   auto vtable_const_id = GetLocalConstantId(
@@ -2147,6 +2143,7 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
   auto fn_val_id = GetLocalConstantInstId(
       resolver,
       resolver.import_functions().Get(inst.function_id).first_decl_id());
+
   auto specific_data = GetLocalSpecificData(resolver, inst.specific_id);
   if (resolver.HasNewWork()) {
     return ResolveResult::Retry();
@@ -3058,40 +3055,84 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
                                     SemIR::InstId inst_id,
                                     SemIR::ConstantId const_id)
     -> ResolveResult {
-  if (SemIR::IsSingletonInstId(inst_id)) {
-    CARBON_CHECK(!const_id.has_value());
+  // These instruction types are imported across multiple phases to arrive at
+  // their constant value. We can't just import their constant value instruction
+  // directly.
+  auto untyped_inst = resolver.import_insts().GetWithAttachedType(inst_id);
+  CARBON_KIND_SWITCH(untyped_inst) {
+    case CARBON_KIND(SemIR::AssociatedConstantDecl inst): {
+      return TryResolveTypedInst(resolver, inst, const_id);
+    }
+    case CARBON_KIND(SemIR::ClassDecl inst): {
+      return TryResolveTypedInst(resolver, inst, const_id);
+    }
+    case CARBON_KIND(SemIR::FunctionDecl inst): {
+      return TryResolveTypedInst(resolver, inst, const_id);
+    }
+    case CARBON_KIND(SemIR::ImplDecl inst): {
+      return TryResolveTypedInst(resolver, inst, const_id);
+    }
+    case CARBON_KIND(SemIR::InterfaceDecl inst): {
+      return TryResolveTypedInst(resolver, inst, const_id);
+    }
+    default:
+      break;
+  }
+
+  // Other instructions are imported in a single phase (once their dependencies
+  // are all imported).
+  CARBON_CHECK(!const_id.has_value());
+
+  auto inst_constant_id = resolver.import_constant_values().Get(inst_id);
+  if (!inst_constant_id.is_constant()) {
+    // TODO: Import of non-constant BindNames happens when importing `let`
+    // declarations.
+    CARBON_CHECK(resolver.import_insts().Is<SemIR::BindName>(inst_id),
+                 "TryResolveInst on non-constant instruction {0}", inst_id);
+    return ResolveResult::Done(SemIR::ConstantId::NotConstant);
+  }
+
+  // Import the canonical constant value instruction for `inst_id` directly. We
+  // don't try to import the non-canonical `inst_id`.
+  auto constant_inst_id =
+      resolver.import_constant_values().GetInstId(inst_constant_id);
+  CARBON_DCHECK(resolver.import_constant_values().GetConstantInstId(
+                    constant_inst_id) == constant_inst_id,
+                "Constant value of constant instruction should refer to "
+                "the same instruction");
+
+  if (SemIR::IsSingletonInstId(constant_inst_id)) {
     // Constants for builtins can be directly copied.
-    return ResolveResult::Done(resolver.local_constant_values().Get(inst_id));
+    return ResolveResult::Done(
+        resolver.local_constant_values().Get(constant_inst_id));
   }
 
-  auto untyped_inst = resolver.import_insts().GetWithAttachedType(inst_id);
-  CARBON_KIND_SWITCH(untyped_inst) {
+  auto untyped_constant_inst =
+      resolver.import_insts().GetWithAttachedType(constant_inst_id);
+  CARBON_KIND_SWITCH(untyped_constant_inst) {
     case CARBON_KIND(SemIR::AdaptDecl inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::AddrPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::ArrayType inst): {
       return TryResolveTypedInst(resolver, inst);
     }
-    case CARBON_KIND(SemIR::AssociatedConstantDecl inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
-    }
     case CARBON_KIND(SemIR::AssociatedEntity inst): {
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::AssociatedEntityType inst): {
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::BaseDecl inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::BindAlias inst): {
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::BindingPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::BindSymbolicName inst): {
       return TryResolveTypedInst(resolver, inst);
@@ -3108,9 +3149,6 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
     case CARBON_KIND(SemIR::CharLiteralValue inst): {
       return TryResolveTypedInst(resolver, inst);
     }
-    case CARBON_KIND(SemIR::ClassDecl inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
-    }
     case CARBON_KIND(SemIR::ClassType inst): {
       return TryResolveTypedInst(resolver, inst);
     }
@@ -3133,7 +3171,7 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::FieldDecl inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::FloatLiteralValue inst): {
       return TryResolveTypedInst(resolver, inst);
@@ -3144,9 +3182,6 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
     case CARBON_KIND(SemIR::FloatValue inst): {
       return TryResolveTypedInst(resolver, inst);
     }
-    case CARBON_KIND(SemIR::FunctionDecl inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
-    }
     case CARBON_KIND(SemIR::FunctionType inst): {
       return TryResolveTypedInst(resolver, inst);
     }
@@ -3159,9 +3194,6 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
     case CARBON_KIND(SemIR::GenericInterfaceType inst): {
       return TryResolveTypedInst(resolver, inst);
     }
-    case CARBON_KIND(SemIR::ImplDecl inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
-    }
     case CARBON_KIND(SemIR::LookupImplWitness inst): {
       return TryResolveTypedInst(resolver, inst);
     }
@@ -3172,13 +3204,10 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::ImplWitnessTable inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::ImportRefLoaded inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
-    }
-    case CARBON_KIND(SemIR::InterfaceDecl inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::IntValue inst): {
       return TryResolveTypedInst(resolver, inst);
@@ -3190,10 +3219,10 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::Namespace inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::OutParamPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::PartialType inst): {
       return TryResolveTypedInst(resolver, inst);
@@ -3205,13 +3234,13 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::RefParamPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::RequireCompleteType inst): {
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::ReturnSlotPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::SpecificFunction inst): {
       return TryResolveTypedInst(resolver, inst);
@@ -3229,13 +3258,13 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::SymbolicBindingPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::TupleAccess inst): {
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::TuplePattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::TupleType inst): {
       return TryResolveTypedInst(resolver, inst);
@@ -3247,51 +3276,28 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
       return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::ValueParamPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::VarPattern inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::VarStorage inst): {
-      return TryResolveTypedInst(resolver, inst, inst_id);
+      return TryResolveTypedInst(resolver, inst, constant_inst_id);
     }
     case CARBON_KIND(SemIR::VtableDecl inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
+      return TryResolveTypedInst(resolver, inst);
     }
     case CARBON_KIND(SemIR::VtablePtr inst): {
-      return TryResolveTypedInst(resolver, inst, const_id);
-    }
-    default: {
-      auto inst_constant_id = resolver.import_constant_values().Get(inst_id);
-      if (!inst_constant_id.is_constant()) {
-        // TODO: Import of non-constant BindNames happens when importing `let`
-        // declarations.
-        CARBON_CHECK(untyped_inst.Is<SemIR::BindName>(),
-                     "TryResolveInst on non-constant instruction {0}",
-                     untyped_inst);
-        return ResolveResult::Done(SemIR::ConstantId::NotConstant);
-      }
-
-      // This instruction might have a constant value of a different kind.
-      auto constant_inst_id =
-          resolver.import_constant_values().GetInstId(inst_constant_id);
-      if (constant_inst_id == inst_id) {
-        // Produce a diagnostic to provide a source location with the CHECK
-        // failure.
-        resolver.local_context().TODO(
-            SemIR::LocId(AddImportIRInst(resolver, inst_id)),
-            llvm::formatv("TryResolveInst on {0}", untyped_inst.kind()).str());
-        CARBON_FATAL("TryResolveInst on unsupported instruction kind {0}",
-                     untyped_inst.kind());
-      }
-      // Try to resolve the constant value instead. Note that this can only
-      // retry once.
-      CARBON_DCHECK(resolver.import_constant_values().GetConstantInstId(
-                        constant_inst_id) == constant_inst_id,
-                    "Constant value of constant instruction should refer to "
-                    "the same instruction");
-      return TryResolveInstCanonical(resolver, constant_inst_id, const_id);
+      return TryResolveTypedInst(resolver, inst);
     }
+    default:
+      // Found a canonical instruction which needs to be resolved, but which is
+      // not yet handled.
+      //
+      // TODO: Could we turn this into a compile-time error?
+      CARBON_FATAL(
+          "Missing case in TryResolveInstCanonical for instruction kind {0}",
+          untyped_constant_inst.kind());
   }
 }