Refactor addition of imported locations and placeholders (#6354)

- Makes a little more use of `MakeImportedLocIdAndInst` instead of
`UncheckedLoc`
- Requires use of `MakeImportedLocIdAndInst` with `ImportIRInstId`;
previously optional
- Relevant `if constexpr` moves to `AddPlaceholderImportedInst`, but is
more narrowly scoped there.
- Refactors out `AddPlaceholderImportedInstInNoBlock` to reduce how many
spots do an explicit `imports().push_back(...)`

I'd also considered removing `MakeImportedLocIdAndInst` where possible,
but went this route so that changes to the expected parse node wouldn't
affect callers. When it's required, `MakeImportedLocIdAndInst` is always
there; when it's conditionally present, changing `Parse::NodeId` between
enforceable and not-enforceable would require refactoring any callsites
that assumed one or the other.

toolchain/check/cpp/import.cpp

@@ -681,10 +681,9 @@ static auto BuildClassDecl(Context& context,
   auto class_decl = SemIR::ClassDecl{.type_id = SemIR::TypeType::TypeId,
                                      .class_id = SemIR::ClassId::None,
                                      .decl_block_id = SemIR::InstBlockId::None};
-  auto class_decl_id = AddPlaceholderInstInNoBlock(
+  auto class_decl_id = AddPlaceholderImportedInstInNoBlock(
       context,
-      SemIR::LocIdAndInst::UncheckedLoc(import_ir_inst_id, class_decl));
-  context.imports().push_back(class_decl_id);
+      MakeImportedLocIdAndInst(context, import_ir_inst_id, class_decl));
 
   SemIR::Class class_info = {
       {.name_id = name_id,
@@ -951,11 +950,12 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
 
   // TODO: Add a field to prevent tail padding reuse if necessary.
 
-  return AddTypeInst<SemIR::CustomLayoutType>(
-      context, import_ir_inst_id,
-      {.type_id = SemIR::TypeType::TypeId,
-       .fields_id = context.struct_type_fields().Add(fields),
-       .layout_id = context.custom_layouts().Add(layout)});
+  return AddTypeInst(context,
+                     MakeImportedLocIdAndInst<SemIR::CustomLayoutType>(
+                         context, import_ir_inst_id,
+                         {.type_id = SemIR::TypeType::TypeId,
+                          .fields_id = context.struct_type_fields().Add(fields),
+                          .layout_id = context.custom_layouts().Add(layout)}));
 }
 
 // Creates a Carbon class definition based on the information in the given Clang
@@ -976,10 +976,12 @@ static auto BuildClassDefinition(Context& context,
   // Compute the class's object representation.
   auto object_repr_id = ImportClassObjectRepr(
       context, class_id, import_ir_inst_id, class_inst_id, clang_def);
-  class_info.complete_type_witness_id = AddInst<SemIR::CompleteTypeWitness>(
-      context, import_ir_inst_id,
-      {.type_id = GetSingletonType(context, SemIR::WitnessType::TypeInstId),
-       .object_repr_type_inst_id = object_repr_id});
+  class_info.complete_type_witness_id = AddInst(
+      context,
+      MakeImportedLocIdAndInst<SemIR::CompleteTypeWitness>(
+          context, import_ir_inst_id,
+          {.type_id = GetSingletonType(context, SemIR::WitnessType::TypeInstId),
+           .object_repr_type_inst_id = object_repr_id}));
 
   class_info.body_block_id = context.inst_block_stack().Pop();
 }
@@ -996,11 +998,13 @@ static auto ImportEnumObjectRepresentation(
 
   auto int_kind = int_type->isSignedIntegerType() ? SemIR::IntKind::Signed
                                                   : SemIR::IntKind::Unsigned;
-  auto bit_width_id = GetOrAddInst<SemIR::IntValue>(
-      context, import_ir_inst_id,
-      {.type_id = GetSingletonType(context, SemIR::IntLiteralType::TypeInstId),
-       .int_id = context.ints().AddUnsigned(
-           llvm::APInt(64, context.ast_context().getIntWidth(int_type)))});
+  auto bit_width_id = GetOrAddInst(
+      context, MakeImportedLocIdAndInst<SemIR::IntValue>(
+                   context, import_ir_inst_id,
+                   {.type_id = GetSingletonType(
+                        context, SemIR::IntLiteralType::TypeInstId),
+                    .int_id = context.ints().AddUnsigned(llvm::APInt(
+                        64, context.ast_context().getIntWidth(int_type)))}));
   return context.types().GetAsTypeInstId(
       GetOrAddInst(context, SemIR::LocIdAndInst::NoLoc(SemIR::IntType{
                                 .type_id = SemIR::TypeType::TypeId,
@@ -1029,13 +1033,15 @@ static auto BuildEnumDefinition(Context& context,
   auto object_repr_id =
       ImportEnumObjectRepresentation(context, import_ir_inst_id, enum_decl);
   class_info.adapt_id = AddInst(
-      context, SemIR::LocIdAndInst::UncheckedLoc(
-                   import_ir_inst_id,
+      context, MakeImportedLocIdAndInst(
+                   context, import_ir_inst_id,
                    SemIR::AdaptDecl{.adapted_type_inst_id = object_repr_id}));
-  class_info.complete_type_witness_id = AddInst<SemIR::CompleteTypeWitness>(
-      context, import_ir_inst_id,
-      {.type_id = GetSingletonType(context, SemIR::WitnessType::TypeInstId),
-       .object_repr_type_inst_id = object_repr_id});
+  class_info.complete_type_witness_id = AddInst(
+      context,
+      MakeImportedLocIdAndInst<SemIR::CompleteTypeWitness>(
+          context, import_ir_inst_id,
+          {.type_id = GetSingletonType(context, SemIR::WitnessType::TypeInstId),
+           .object_repr_type_inst_id = object_repr_id}));
 
   class_info.body_block_id = context.inst_block_stack().Pop();
 }
@@ -1055,10 +1061,12 @@ static auto ImportEnumConstantDecl(Context& context,
 
   // Build a corresponding IntValue.
   auto int_id = context.ints().Add(enumerator_decl->getInitVal());
-  auto loc_id =
+  auto import_ir_inst_id =
       AddImportIRInst(context.sem_ir(), enumerator_decl->getLocation());
-  auto inst_id = AddInstInNoBlock<SemIR::IntValue>(
-      context, loc_id, {.type_id = type_id, .int_id = int_id});
+  auto inst_id = AddInstInNoBlock(
+      context,
+      MakeImportedLocIdAndInst<SemIR::IntValue>(
+          context, import_ir_inst_id, {.type_id = type_id, .int_id = int_id}));
   context.imports().push_back(inst_id);
   context.clang_decls().Add({.key = key, .inst_id = inst_id});
   return inst_id;
@@ -1667,9 +1675,8 @@ static auto ImportFunction(Context& context, SemIR::LocId loc_id,
 
   auto function_decl = SemIR::FunctionDecl{
       SemIR::TypeId::None, SemIR::FunctionId::None, decl_block_id};
-  auto decl_id =
-      AddPlaceholderInstInNoBlock(context, Parse::NodeId::None, function_decl);
-  context.imports().push_back(decl_id);
+  auto decl_id = AddPlaceholderImportedInstInNoBlock(
+      context, SemIR::LocIdAndInst::NoLoc(function_decl));
 
   auto virtual_modifier = SemIR::Function::VirtualModifier::None;
   int32_t virtual_index = -1;
@@ -1886,7 +1893,7 @@ static auto ImportVarDecl(Context& context, SemIR::LocId loc_id,
   // We can't use the convenience for `AddPlaceholderInstInNoBlock()` with typed
   // nodes because it doesn't support insts with cleanup.
   SemIR::InstId var_storage_inst_id =
-      AddPlaceholderInstInNoBlock(context, {loc_id, var_storage});
+      AddPlaceholderImportedInstInNoBlock(context, {loc_id, var_storage});
 
   auto clang_decl_id = context.clang_decls().Add(
       {.key = SemIR::ClangDeclKey(var_decl), .inst_id = var_storage_inst_id});
@@ -1915,7 +1922,6 @@ static auto ImportVarDecl(Context& context, SemIR::LocId loc_id,
 
   // Finalize the `VarStorage` instruction.
   ReplaceInstBeforeConstantUse(context, var_storage_inst_id, var_storage);
-  context.imports().push_back(var_storage_inst_id);
 
   return var_storage_inst_id;
 }

toolchain/check/import.cpp

@@ -144,8 +144,8 @@ auto AddImportNamespace(Context& context, SemIR::TypeId namespace_type_id,
       MakeImportedNamespaceLocIdAndInst(context, import_id, namespace_inst);
   AddImportNamespaceResult result = {
       .name_scope_id = SemIR::NameScopeId::None,
-      .inst_id = AddPlaceholderInstInNoBlock(context, namespace_inst_and_loc)};
-  context.imports().push_back(result.inst_id);
+      .inst_id =
+          AddPlaceholderImportedInstInNoBlock(context, namespace_inst_and_loc)};
   namespace_inst.name_scope_id =
       context.name_scopes().Add(result.inst_id, name_id, parent_scope_id);
   result.name_scope_id = namespace_inst.name_scope_id;

toolchain/check/import.h

@@ -114,7 +114,7 @@ auto MakeImportedLocIdAndInst(Context& context,
     Internal::CheckCompatibleImportedNodeKind(context, imported_loc_id,
                                               InstT::Kind);
   }
-  return SemIR::LocIdAndInst::UncheckedLoc(imported_loc_id, inst);
+  return SemIR::LocIdAndInst::UncheckedLoc(SemIR::LocId(imported_loc_id), inst);
 }
 
 }  // namespace Carbon::Check

toolchain/check/import_ref.cpp

@@ -89,15 +89,12 @@ auto AddImportRef(Context& context, SemIR::ImportIRInst import_ir_inst,
                   SemIR::EntityNameId entity_name_id =
                       SemIR::EntityNameId::None) -> SemIR::InstId {
   auto import_ir_inst_id = context.import_ir_insts().Add(import_ir_inst);
-  SemIR::ImportRefUnloaded inst = {.import_ir_inst_id = import_ir_inst_id,
-                                   .entity_name_id = entity_name_id};
-  auto import_ref_id = AddPlaceholderInstInNoBlock(
-      context, MakeImportedLocIdAndInst(context, import_ir_inst_id, inst));
-
-  // ImportRefs have a dedicated block because this may be called during
-  // processing where the instruction shouldn't be inserted in the current inst
-  // block.
-  context.imports().push_back(import_ref_id);
+  auto import_ref_id = AddPlaceholderImportedInstInNoBlock(
+      context,
+      MakeImportedLocIdAndInst(
+          context, import_ir_inst_id,
+          SemIR::ImportRefUnloaded{.import_ir_inst_id = import_ir_inst_id,
+                                   .entity_name_id = entity_name_id}));
   return import_ref_id;
 }
 
@@ -515,11 +512,10 @@ static auto AddLoadedImportRef(ImportContext& context,
   SemIR::ImportRefLoaded inst = {.type_id = local_type_id,
                                  .import_ir_inst_id = import_ir_inst_id,
                                  .entity_name_id = SemIR::EntityNameId::None};
-  auto inst_id = AddPlaceholderInstInNoBlock(
+  auto inst_id = AddPlaceholderImportedInstInNoBlock(
       context.local_context(),
       MakeImportedLocIdAndInst(context.local_context(), import_ir_inst_id,
                                inst));
-  context.local_context().imports().push_back(inst_id);
 
   context.local_constant_values().Set(inst_id, local_const_id);
   context.local_constant_values_for_import_insts().Set(import_inst_id,
@@ -561,14 +557,11 @@ template <typename InstT>
 static auto AddPlaceholderImportedInst(ImportContext& context,
                                        SemIR::InstId import_inst_id, InstT inst)
     -> SemIR::InstId {
-  auto inst_id = context.local_insts().AddInNoBlock(MakeImportedLocIdAndInst(
-      context.local_context(), AddImportIRInst(context, import_inst_id), inst));
-  CARBON_VLOG_TO(context.local_context().vlog_stream(),
-                 "AddPlaceholderImportedInst: {0}\n", inst);
-  // Track the instruction in the imports block so that it's included in
-  // formatted SemIR if it's referenced.
-  context.local_context().imports().push_back(inst_id);
-  return inst_id;
+  auto import_ir_inst_id = AddImportIRInst(context, import_inst_id);
+  return AddPlaceholderImportedInstInNoBlock(
+      context.local_context(),
+      MakeImportedLocIdAndInst(context.local_context(), import_ir_inst_id,
+                               inst));
 }
 
 // Replace an imported instruction that was added by
@@ -583,7 +576,7 @@ static auto ReplacePlaceholderImportedInst(ImportContext& context,
   context.local_insts().Set(inst_id, inst);
 
   CARBON_CHECK(context.local_constant_values().Get(inst_id) ==
-               SemIR::ConstantId::NotConstant);
+               SemIR::ConstantId::None);
   return SetConstantValue(context.local_context(), inst_id, inst);
 }
 

toolchain/check/inst.cpp

@@ -203,6 +203,14 @@ auto AddPlaceholderInstInNoBlock(Context& context,
   return inst_id;
 }
 
+auto AddPlaceholderImportedInstInNoBlock(Context& context,
+                                         SemIR::LocIdAndInst loc_id_and_inst)
+    -> SemIR::InstId {
+  auto inst_id = AddPlaceholderInstInNoBlock(context, loc_id_and_inst);
+  context.imports().push_back(inst_id);
+  return inst_id;
+}
+
 auto AddPlaceholderInst(Context& context, SemIR::LocIdAndInst loc_id_and_inst)
     -> SemIR::InstId {
   auto inst_id = AddPlaceholderInstInNoBlock(context, loc_id_and_inst);

toolchain/check/inst.h

@@ -29,12 +29,16 @@ auto AddInst(Context& context, LocT loc, InstT inst) -> SemIR::InstId {
 
 // Like AddInst, but for instructions with a type_id of `TypeType`, which is
 // encoded in the return type of `TypeInstId`.
+inline auto AddTypeInst(Context& context, SemIR::LocIdAndInst loc_id_and_inst)
+    -> SemIR::TypeInstId {
+  return context.types().GetAsTypeInstId(AddInst(context, loc_id_and_inst));
+}
+
 template <typename InstT, typename LocT>
   requires(!InstT::Kind.has_cleanup() &&
            std::convertible_to<LocT, SemIR::LocId>)
 auto AddTypeInst(Context& context, LocT loc, InstT inst) -> SemIR::TypeInstId {
-  return context.types().GetAsTypeInstId(
-      AddInst(context, SemIR::LocIdAndInst(loc, inst)));
+  return AddTypeInst(context, SemIR::LocIdAndInst(loc, inst));
 }
 
 // Pushes a parse tree node onto the stack, storing the SemIR::Inst as the
@@ -173,6 +177,12 @@ auto AddPlaceholderInstInNoBlock(Context& context, LocT loc, InstT inst)
   return AddPlaceholderInstInNoBlock(context, SemIR::LocIdAndInst(loc, inst));
 }
 
+// Similar to `AddPlaceholderInstInNoBlock`, but also tracks the instruction as
+// an import.
+auto AddPlaceholderImportedInstInNoBlock(Context& context,
+                                         SemIR::LocIdAndInst loc_id_and_inst)
+    -> SemIR::InstId;
+
 // Replaces the instruction at `inst_id` with `loc_id_and_inst`. The
 // instruction is required to not have been used in any constant evaluation,
 // either because it's newly created and entirely unused, or because it's only

toolchain/check/testdata/basics/raw_sem_ir/cpp_interop.carbon

@@ -281,8 +281,8 @@ fn G(x: Cpp.X) {
 // CHECK:STDOUT:       4:               inst6000002F
 // CHECK:STDOUT:       5:               inst6000003A
 // CHECK:STDOUT:       6:               inst60000042
-// CHECK:STDOUT:       7:               inst6000004E
-// CHECK:STDOUT:       8:               inst6000004B
+// CHECK:STDOUT:       7:               inst6000004B
+// CHECK:STDOUT:       8:               inst6000004E
 // CHECK:STDOUT:     global_init:     {}
 // CHECK:STDOUT:     inst_block60000004:
 // CHECK:STDOUT:       0:               inst60000011

toolchain/sem_ir/inst.h

@@ -411,8 +411,12 @@ struct LocIdAndInst {
   }
 
   // Unsafely form a pair of a location and an instruction. Used in the cases
-  // where we can't statically enforce the type matches.
-  static auto UncheckedLoc(LocId loc_id, Inst inst) -> LocIdAndInst {
+  // where we can't statically enforce the type matches. For `ImportIRInstId`,
+  // use `MakeImportedLocIdAndInst` in `import.h`.
+  template <typename LocT>
+    requires(std::convertible_to<LocT, LocId> &&
+             !std::same_as<LocT, ImportIRInstId>)
+  static auto UncheckedLoc(LocT loc_id, Inst inst) -> LocIdAndInst {
     return LocIdAndInst(loc_id, inst, /*is_unchecked=*/true);
   }
 
@@ -428,6 +432,10 @@ struct LocIdAndInst {
     requires(Internal::HasUntypedNodeId<InstT>)
   LocIdAndInst(LocId loc_id, InstT inst) : loc_id(loc_id), inst(inst) {}
 
+  // For `ImportIRInstId`, use `MakeImportedLocIdAndInst` in `import.h`.
+  template <typename InstT>
+  LocIdAndInst(ImportIRInstId loc_id, InstT inst) = delete;
+
   LocId loc_id;
   Inst inst;