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

… in the instruction definition (#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 <jperkins@google.com>

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});