Enforce that the parse node for an instruction has the kind specified…

… in the instruction definition (carbon-language#4264)

Remove `ReusingLoc` and add enforcement that even for imported
locations, the kind of the parse node for an instruction matches the
kind specified in the instruction definition.

Change the node kind for a few instructions to `NodeId`:

- A couple of instructions had a typed node but could be created
implicitly with any node as part of a builtin implicit conversion. This
happened for `AddrOf`, `ArrayIndex`, and `Deref`.
- A bunch of instructions had `InvalidNodeId` as their associated parse
node kind but were actually always created with a location.

---------

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

toolchain/check/check.cpp

@@ -282,9 +282,10 @@ static auto ImportOtherPackages(Context& context, UnitInfo& unit_info,
         auto import_ir_inst_id = context.import_ir_insts().Add(
             {.ir_id = SemIR::ImportIRId::ApiForImpl,
              .inst_id = api_imports->import_decl_id});
-        import_decl_id = context.AddInstReusingLoc<SemIR::ImportDecl>(
-            import_ir_inst_id, {.package_id = SemIR::NameId::ForIdentifier(
-                                    api_imports_entry.first)});
+        import_decl_id =
+            context.AddInst(context.MakeImportedLocAndInst<SemIR::ImportDecl>(
+                import_ir_inst_id, {.package_id = SemIR::NameId::ForIdentifier(
+                                        api_imports_entry.first)}));
         package_id = api_imports_entry.first;
       }
       has_load_error |= api_imports->has_load_error;

toolchain/check/context.cpp

@@ -118,6 +118,34 @@ auto Context::FinishInst(SemIR::InstId inst_id, SemIR::Inst inst) -> void {
   }
 }
 
+// Returns whether a parse node associated with an imported instruction of kind
+// `imported_kind` is usable as the location of a corresponding local
+// instruction of kind `local_kind`.
+static auto HasCompatibleImportedNodeKind(SemIR::InstKind imported_kind,
+                                          SemIR::InstKind local_kind) -> bool {
+  if (imported_kind == local_kind) {
+    return true;
+  }
+  if (imported_kind == SemIR::ImportDecl::Kind &&
+      local_kind == SemIR::Namespace::Kind) {
+    static_assert(
+        std::is_convertible_v<decltype(SemIR::ImportDecl::Kind)::TypedNodeId,
+                              decltype(SemIR::Namespace::Kind)::TypedNodeId>);
+    return true;
+  }
+  return false;
+}
+
+auto Context::CheckCompatibleImportedNodeKind(
+    SemIR::ImportIRInstId imported_loc_id, SemIR::InstKind kind) -> void {
+  auto& import_ir_inst = import_ir_insts().Get(imported_loc_id);
+  const auto* import_ir = import_irs().Get(import_ir_inst.ir_id).sem_ir;
+  auto imported_kind = import_ir->insts().Get(import_ir_inst.inst_id).kind();
+  CARBON_CHECK(HasCompatibleImportedNodeKind(imported_kind, kind))
+      << "Node of kind " << kind
+      << " created with location of imported node of kind " << imported_kind;
+}
+
 auto Context::AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst)
     -> SemIR::InstId {
   auto inst_id = sem_ir().insts().AddInNoBlock(loc_id_and_inst);

toolchain/check/context.h

@@ -24,6 +24,7 @@
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/import_ir.h"
 #include "toolchain/sem_ir/inst.h"
+#include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
 
@@ -68,37 +69,34 @@ class Context {
   auto AddInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
 
   // Convenience for AddInst with typed nodes.
-  template <typename InstT>
-    requires(SemIR::Internal::HasNodeId<InstT>)
-  auto AddInst(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst)
-      -> SemIR::InstId {
-    return AddInst(SemIR::LocIdAndInst(node_id, inst));
+  template <typename InstT, typename LocT>
+  auto AddInst(LocT loc, InstT inst)
+      -> decltype(AddInst(SemIR::LocIdAndInst(loc, inst))) {
+    return AddInst(SemIR::LocIdAndInst(loc, inst));
   }
 
-  // Convenience for AddInst when reusing a location, which any instruction can
-  // do.
+  // Returns a LocIdAndInst for an instruction with an imported location. Checks
+  // that the imported location is compatible with the kind of instruction being
+  // created.
   template <typename InstT>
-  auto AddInstReusingLoc(SemIR::LocId loc_id, InstT inst) -> SemIR::InstId {
-    return AddInst(SemIR::LocIdAndInst::ReusingLoc<InstT>(loc_id, inst));
+    requires SemIR::Internal::HasNodeId<InstT>
+  auto MakeImportedLocAndInst(SemIR::ImportIRInstId imported_loc_id, InstT inst)
+      -> SemIR::LocIdAndInst {
+    if constexpr (!SemIR::Internal::HasUntypedNodeId<InstT>) {
+      CheckCompatibleImportedNodeKind(imported_loc_id, InstT::Kind);
+    }
+    return SemIR::LocIdAndInst::UncheckedLoc(imported_loc_id, inst);
   }
 
   // Adds an instruction in no block, returning the produced ID. Should be used
   // rarely.
   auto AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
 
   // Convenience for AddInstInNoBlock with typed nodes.
-  template <typename InstT>
-    requires(SemIR::Internal::HasNodeId<InstT>)
-  auto AddInstInNoBlock(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst)
-      -> SemIR::InstId {
-    return AddInstInNoBlock(SemIR::LocIdAndInst(node_id, inst));
-  }
-
-  // Convenience for AddInstInNoBlock on imported instructions.
-  template <typename InstT>
-  auto AddInstInNoBlock(SemIR::ImportIRInstId import_ir_inst_id, InstT inst)
-      -> SemIR::InstId {
-    return AddInstInNoBlock(SemIR::LocIdAndInst(import_ir_inst_id, inst));
+  template <typename InstT, typename LocT>
+  auto AddInstInNoBlock(LocT loc, InstT inst)
+      -> decltype(AddInstInNoBlock(SemIR::LocIdAndInst(loc, inst))) {
+    return AddInstInNoBlock(SemIR::LocIdAndInst(loc, inst));
   }
 
   // Adds an instruction to the current block, returning the produced ID. The
@@ -500,6 +498,11 @@ class Context {
     SemIR::TypeId type_id_;
   };
 
+  // Checks that the provided imported location has a node kind that is
+  // compatible with that of the given instruction.
+  auto CheckCompatibleImportedNodeKind(SemIR::ImportIRInstId imported_loc_id,
+                                       SemIR::InstKind kind) -> void;
+
   // Finish producing an instruction. Set its constant value, and register it in
   // any applicable instruction lists.
   auto FinishInst(SemIR::InstId inst_id, SemIR::Inst inst) -> void;

toolchain/check/convert.cpp

@@ -104,10 +104,10 @@ static auto FinalizeTemporary(Context& context, SemIR::InstId init_id,
         << "initialized multiple times? Have "
         << sem_ir.insts().Get(return_slot_id);
     auto init = sem_ir.insts().Get(init_id);
-    return context.AddInstReusingLoc<SemIR::Temporary>(
-        sem_ir.insts().GetLocId(init_id), {.type_id = init.type_id(),
-                                           .storage_id = return_slot_id,
-                                           .init_id = init_id});
+    return context.AddInst<SemIR::Temporary>(sem_ir.insts().GetLocId(init_id),
+                                             {.type_id = init.type_id(),
+                                              .storage_id = return_slot_id,
+                                              .init_id = init_id});
   }
 
   if (discarded) {
@@ -122,12 +122,11 @@ static auto FinalizeTemporary(Context& context, SemIR::InstId init_id,
   // instructions.
   auto init = sem_ir.insts().Get(init_id);
   auto loc_id = sem_ir.insts().GetLocId(init_id);
-  auto temporary_id = context.AddInstReusingLoc<SemIR::TemporaryStorage>(
+  auto temporary_id = context.AddInst<SemIR::TemporaryStorage>(
       loc_id, {.type_id = init.type_id()});
-  return context.AddInstReusingLoc<SemIR::Temporary>(
-      loc_id, {.type_id = init.type_id(),
-               .storage_id = temporary_id,
-               .init_id = init_id});
+  return context.AddInst<SemIR::Temporary>(loc_id, {.type_id = init.type_id(),
+                                                    .storage_id = temporary_id,
+                                                    .init_id = init_id});
 }
 
 // Materialize a temporary to hold the result of the given expression if it is
@@ -151,14 +150,14 @@ static auto MakeElementAccessInst(Context& context, SemIR::LocId loc_id,
     // TODO: Add a new instruction kind for indexing an array at a constant
     // index so that we don't need an integer literal instruction here, and
     // remove this special case.
-    auto index_id = block.template AddInstReusingLoc<SemIR::IntLiteral>(
+    auto index_id = block.template AddInst<SemIR::IntLiteral>(
         loc_id,
         {.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::IntType),
          .int_id = context.ints().Add(llvm::APInt(32, i))});
-    return block.template AddInstReusingLoc<AccessInstT>(
+    return block.template AddInst<AccessInstT>(
         loc_id, {elem_type_id, aggregate_id, index_id});
   } else {
-    return block.template AddInstReusingLoc<AccessInstT>(
+    return block.template AddInst<AccessInstT>(
         loc_id, {elem_type_id, aggregate_id, SemIR::ElementIndex(i)});
   }
 }
@@ -257,7 +256,7 @@ static auto ConvertTupleToArray(Context& context, SemIR::TupleType tuple_type,
   // destination for the array initialization if we weren't given one.
   SemIR::InstId return_slot_id = target.init_id;
   if (!target.init_id.is_valid()) {
-    return_slot_id = target_block->AddInstReusingLoc<SemIR::TemporaryStorage>(
+    return_slot_id = target_block->AddInst<SemIR::TemporaryStorage>(
         value_loc_id, {.type_id = target.type_id});
   }
 
@@ -284,7 +283,7 @@ static auto ConvertTupleToArray(Context& context, SemIR::TupleType tuple_type,
   // Flush the temporary here if we didn't insert it earlier, so we can add a
   // reference to the return slot.
   target_block->InsertHere();
-  return context.AddInstReusingLoc<SemIR::ArrayInit>(
+  return context.AddInst<SemIR::ArrayInit>(
       value_loc_id, {.type_id = target.type_id,
                      .inits_id = sem_ir.inst_blocks().Add(inits),
                      .dest_id = return_slot_id});
@@ -364,12 +363,12 @@ static auto ConvertTupleToTuple(Context& context, SemIR::TupleType src_type,
 
   if (is_init) {
     target.init_block->InsertHere();
-    return context.AddInstReusingLoc<SemIR::TupleInit>(
-        value_loc_id, {.type_id = target.type_id,
-                       .elements_id = new_block.id(),
-                       .dest_id = target.init_id});
+    return context.AddInst<SemIR::TupleInit>(value_loc_id,
+                                             {.type_id = target.type_id,
+                                              .elements_id = new_block.id(),
+                                              .dest_id = target.init_id});
   } else {
-    return context.AddInstReusingLoc<SemIR::TupleValue>(
+    return context.AddInst<SemIR::TupleValue>(
         value_loc_id,
         {.type_id = target.type_id, .elements_id = new_block.id()});
   }
@@ -498,18 +497,18 @@ static auto ConvertStructToStructOrClass(Context& context,
     target.init_block->InsertHere();
     CARBON_CHECK(is_init)
         << "Converting directly to a class value is not supported";
-    return context.AddInstReusingLoc<SemIR::ClassInit>(
-        value_loc_id, {.type_id = target.type_id,
-                       .elements_id = new_block.id(),
-                       .dest_id = target.init_id});
+    return context.AddInst<SemIR::ClassInit>(value_loc_id,
+                                             {.type_id = target.type_id,
+                                              .elements_id = new_block.id(),
+                                              .dest_id = target.init_id});
   } else if (is_init) {
     target.init_block->InsertHere();
-    return context.AddInstReusingLoc<SemIR::StructInit>(
-        value_loc_id, {.type_id = target.type_id,
-                       .elements_id = new_block.id(),
-                       .dest_id = target.init_id});
+    return context.AddInst<SemIR::StructInit>(value_loc_id,
+                                              {.type_id = target.type_id,
+                                               .elements_id = new_block.id(),
+                                               .dest_id = target.init_id});
   } else {
-    return context.AddInstReusingLoc<SemIR::StructValue>(
+    return context.AddInst<SemIR::StructValue>(
         value_loc_id,
         {.type_id = target.type_id, .elements_id = new_block.id()});
   }
@@ -556,7 +555,7 @@ static auto ConvertStructToClass(Context& context, SemIR::StructType src_type,
   if (need_temporary) {
     target.kind = ConversionTarget::Initializer;
     target.init_block = &target_block;
-    target.init_id = target_block.AddInstReusingLoc<SemIR::TemporaryStorage>(
+    target.init_id = target_block.AddInst<SemIR::TemporaryStorage>(
         context.insts().GetLocId(value_id), {.type_id = target.type_id});
   }
 
@@ -565,7 +564,7 @@ static auto ConvertStructToClass(Context& context, SemIR::StructType src_type,
 
   if (need_temporary) {
     target_block.InsertHere();
-    result_id = context.AddInstReusingLoc<SemIR::Temporary>(
+    result_id = context.AddInst<SemIR::Temporary>(
         context.insts().GetLocId(value_id), {.type_id = target.type_id,
                                              .storage_id = target.init_id,
                                              .init_id = result_id});
@@ -623,7 +622,7 @@ static auto ConvertDerivedToBase(Context& context, SemIR::LocId loc_id,
   // Add a series of `.base` accesses.
   for (auto base_id : path) {
     auto base_decl = context.insts().GetAs<SemIR::BaseDecl>(base_id);
-    value_id = context.AddInstReusingLoc<SemIR::ClassElementAccess>(
+    value_id = context.AddInst<SemIR::ClassElementAccess>(
         loc_id, {.type_id = base_decl.base_type_id,
                  .base_id = value_id,
                  .index = base_decl.index});
@@ -638,14 +637,14 @@ static auto ConvertDerivedPointerToBasePointer(
     const InheritancePath& path) -> SemIR::InstId {
   // Form `*p`.
   ptr_id = ConvertToValueExpr(context, ptr_id);
-  auto ref_id = context.AddInstReusingLoc<SemIR::Deref>(
+  auto ref_id = context.AddInst<SemIR::Deref>(
       loc_id, {.type_id = src_ptr_type.pointee_id, .pointer_id = ptr_id});
 
   // Convert as a reference expression.
   ref_id = ConvertDerivedToBase(context, loc_id, ref_id, path);
 
   // Take the address.
-  return context.AddInstReusingLoc<SemIR::AddrOf>(
+  return context.AddInst<SemIR::AddrOf>(
       loc_id, {.type_id = dest_ptr_type_id, .lvalue_id = ref_id});
 }
 
@@ -744,7 +743,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id,
         // The initializer produces an object representation by copy, and the
         // value representation is a copy of the object representation, so we
         // already have a value of the right form.
-        return context.AddInstReusingLoc<SemIR::ValueOfInitializer>(
+        return context.AddInst<SemIR::ValueOfInitializer>(
             loc_id, {.type_id = value_type_id, .init_id = value_id});
       }
     }
@@ -765,7 +764,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id,
             ConversionTarget{.kind = ConversionTarget::Value,
                              .type_id = value_type_id});
       }
-      return context.AddInstReusingLoc<SemIR::AsCompatible>(
+      return context.AddInst<SemIR::AsCompatible>(
           loc_id, {.type_id = target.type_id, .source_id = value_id});
     }
   }
@@ -871,7 +870,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id,
     // TODO: Support converting tuple and struct values to facet types,
     // combining the above conversions and this one in a single conversion.
     if (sem_ir.types().Is<SemIR::InterfaceType>(value_type_id)) {
-      return context.AddInstReusingLoc<SemIR::FacetTypeAccess>(
+      return context.AddInst<SemIR::FacetTypeAccess>(
           loc_id, {.type_id = target.type_id, .facet_id = value_id});
     }
   }
@@ -982,10 +981,10 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
 
   // Track that we performed a type conversion, if we did so.
   if (orig_expr_id != expr_id) {
-    expr_id = context.AddInstReusingLoc<SemIR::Converted>(
-        loc_id, {.type_id = target.type_id,
-                 .original_id = orig_expr_id,
-                 .result_id = expr_id});
+    expr_id =
+        context.AddInst<SemIR::Converted>(loc_id, {.type_id = target.type_id,
+                                                   .original_id = orig_expr_id,
+                                                   .result_id = expr_id});
   }
 
   // For `as`, don't perform any value category conversions. In particular, an
@@ -1035,7 +1034,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
 
       // If we have a reference and don't want one, form a value binding.
       // TODO: Support types with custom value representations.
-      expr_id = context.AddInstReusingLoc<SemIR::BindValue>(
+      expr_id = context.AddInst<SemIR::BindValue>(
           context.insts().GetLocId(expr_id),
           {.type_id = expr.type_id(), .value_id = expr_id});
       // We now have a value expression.
@@ -1054,7 +1053,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
     if (auto init_rep = SemIR::InitRepr::ForType(sem_ir, target.type_id);
         init_rep.kind == SemIR::InitRepr::ByCopy) {
       target.init_block->InsertHere();
-      expr_id = context.AddInstReusingLoc<SemIR::InitializeFrom>(
+      expr_id = context.AddInst<SemIR::InitializeFrom>(
           loc_id, {.type_id = target.type_id,
                    .src_id = expr_id,
                    .dest_id = target.init_id});
@@ -1163,7 +1162,7 @@ static auto ConvertSelf(Context& context, SemIR::LocId call_loc_id,
         return SemIR::InstId::BuiltinError;
     }
     auto loc_id = context.insts().GetLocId(self_or_addr_id);
-    self_or_addr_id = context.AddInstReusingLoc<SemIR::AddrOf>(
+    self_or_addr_id = context.AddInst<SemIR::AddrOf>(
         loc_id, {.type_id = context.GetPointerType(self.type_id()),
                  .lvalue_id = self_or_addr_id});
   }

toolchain/check/generic.cpp

@@ -431,11 +431,9 @@ auto RequireGenericParams(Context& context, SemIR::InstBlockId block_id)
       // constructing a generic based on it. Note this is updating the param
       // refs block, not the actual params block, so will not be directly
       // reflected in SemIR output.
-      inst_id = context.AddInstInNoBlock(
-          SemIR::LocIdAndInst::ReusingLoc<SemIR::Param>(
-              context.insts().GetLocId(inst_id),
-              {.type_id = SemIR::TypeId::Error,
-               .name_id = SemIR::NameId::Base}));
+      inst_id = context.AddInstInNoBlock<SemIR::Param>(
+          context.insts().GetLocId(inst_id),
+          {.type_id = SemIR::TypeId::Error, .name_id = SemIR::NameId::Base});
     }
   }
 }

toolchain/check/handle_alias.cpp

@@ -59,7 +59,7 @@ auto HandleParseNode(Context& context, Parse::AliasId /*node_id*/) -> bool {
     alias_type_id = SemIR::TypeId::Error;
     alias_value_id = SemIR::InstId::BuiltinError;
   }
-  auto alias_id = context.AddInstReusingLoc<SemIR::BindAlias>(
+  auto alias_id = context.AddInst<SemIR::BindAlias>(
       name_context.loc_id, {.type_id = alias_type_id,
                             .entity_name_id = entity_name_id,
                             .value_id = alias_value_id});

toolchain/check/impl.cpp

@@ -198,7 +198,7 @@ static auto BuildInterfaceWitness(
   }
 
   auto table_id = context.inst_blocks().Add(table);
-  return context.AddInstReusingLoc<SemIR::InterfaceWitness>(
+  return context.AddInst<SemIR::InterfaceWitness>(
       context.insts().GetLocId(impl.definition_id),
       {.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::WitnessType),
        .elements_id = table_id});