From e0305684b0352dccae8e06337120a54f4c8e2428 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 24 Mar 2026 13:56:44 -0700 Subject: [PATCH] Add MakeVerifiedLocIdAndInst for runtime validation (#6942) This follows up on a discussion about wanting to use `Any*` inst clusters to handle boilerplate construction, with the issue that `UncheckedLoc` use removes validation. Some context is at https://github.com/carbon-language/carbon-lang/pull/6930#discussion_r2963157428. This folds in `MakeImportedLocIdAndInst` because the logic is related, particularly for `LocId` values which are `ImportIRInstId`, and it eliminates questions of what the right function is to use. This uncovers an error in the `NodeKind` associated with `FormBindingPattern`. For now I'm just adding a TODO regarding that. Assisted-by: Google Antigravity with Gemini --- toolchain/check/check_unit.cpp | 11 ++-- toolchain/check/convert.cpp | 16 ++--- toolchain/check/cpp/import.cpp | 97 ++++++++++++++++--------------- toolchain/check/cpp/operators.cpp | 11 ++-- toolchain/check/function.cpp | 3 +- toolchain/check/generic.cpp | 5 +- toolchain/check/import.cpp | 54 +++-------------- toolchain/check/import.h | 24 -------- toolchain/check/import_ref.cpp | 12 ++-- toolchain/check/inst.cpp | 1 + toolchain/check/pattern.cpp | 27 ++++----- toolchain/check/subst.cpp | 5 +- toolchain/check/thunk.cpp | 5 +- toolchain/sem_ir/inst.cpp | 62 ++++++++++++++++++++ toolchain/sem_ir/inst.h | 24 ++++---- toolchain/sem_ir/inst_kind.cpp | 8 +++ toolchain/sem_ir/inst_kind.h | 58 ++++++++++++++++++ toolchain/sem_ir/typed_insts.h | 17 ++++-- 18 files changed, 262 insertions(+), 178 deletions(-) diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index 4087d2b13276..be9027053bec 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -355,11 +355,12 @@ auto CheckUnit::ImportOtherPackages(SemIR::TypeId namespace_type_id) -> void { auto import_ir_inst_id = context_.import_ir_insts().Add(SemIR::ImportIRInst( SemIR::ImportIRId::ApiForImpl, api_imports->import_decl_id)); - import_decl_id = - AddInst(context_, MakeImportedLocIdAndInst( - context_, import_ir_inst_id, - {.package_id = SemIR::NameId::ForPackageName( - api_imports_entry.first)})); + import_decl_id = AddInst( + context_, + SemIR::LocIdAndInst::RuntimeVerified( + context_.sem_ir(), import_ir_inst_id, + SemIR::ImportDecl{.package_id = SemIR::NameId::ForPackageName( + api_imports_entry.first)})); package_id = api_imports_entry.first; } has_load_error |= api_imports->has_load_error; diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 7db8192734ee..fcc8c5ae7169 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -2276,8 +2276,8 @@ auto ReturnExprAsForm(Context& context, SemIR::LocId loc_id, } form_inst_id = AddInst( context, - SemIR::LocIdAndInst::UncheckedLoc( - loc_id, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), loc_id, SemIR::RefForm{.type_id = SemIR::FormType::TypeId, .type_component_inst_id = context.types().GetAsTypeInstId(type_inst_id)})); @@ -2292,12 +2292,12 @@ auto ReturnExprAsForm(Context& context, SemIR::LocId loc_id, return Context::FormExpr::Error; } form_inst_id = AddInst( - context, - SemIR::LocIdAndInst::UncheckedLoc( - loc_id, SemIR::InitForm{ - .type_id = SemIR::FormType::TypeId, - .type_component_inst_id = - context.types().GetAsTypeInstId(type_inst_id)})); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), loc_id, + SemIR::InitForm{ + .type_id = SemIR::FormType::TypeId, + .type_component_inst_id = + context.types().GetAsTypeInstId(type_inst_id)})); } auto type_const_id = context.constant_values().Get(type_inst_id); diff --git a/toolchain/check/cpp/import.cpp b/toolchain/check/cpp/import.cpp index 0fc92eb250e2..df1820da3351 100644 --- a/toolchain/check/cpp/import.cpp +++ b/toolchain/check/cpp/import.cpp @@ -318,8 +318,8 @@ static auto BuildClassDecl(Context& context, .class_id = SemIR::ClassId::None, .decl_block_id = SemIR::InstBlockId::None}; auto class_decl_id = AddPlaceholderImportedInstInNoBlock( - context, - MakeImportedLocIdAndInst(context, import_ir_inst_id, class_decl)); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, class_decl)); SemIR::Class class_info = { {.name_id = name_id, @@ -438,8 +438,8 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id, clang_def->isAggregate()) { return context.types().GetAsTypeInstId(AddInst( context, - MakeImportedLocIdAndInst( - context, import_ir_inst_id, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, SemIR::StructType{.type_id = SemIR::TypeType::TypeId, .fields_id = SemIR::StructTypeFieldsId::Empty}))); } @@ -491,8 +491,8 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id, auto base_decl_id = AddInst( context, - MakeImportedLocIdAndInst( - context, import_ir_inst_id, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, SemIR::BaseDecl{.type_id = GetUnboundElementType( context, class_type_inst_id, base_type_inst_id), .base_type_inst_id = base_type_inst_id, @@ -574,8 +574,8 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id, // Create a field now, as we know the index to use. // TODO: Consider doing this lazily instead. auto field_decl_id = AddInst( - context, MakeImportedLocIdAndInst( - context, import_ir_inst_id, + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, SemIR::FieldDecl{ .type_id = GetUnboundElementType( context, class_type_inst_id, field_type_inst_id), @@ -607,12 +607,13 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id, // TODO: Add a field to prevent tail padding reuse if necessary. - return AddTypeInst(context, - MakeImportedLocIdAndInst( - 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, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, + SemIR::CustomLayoutType{ + .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 @@ -633,12 +634,13 @@ 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( - context, - MakeImportedLocIdAndInst( - 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, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, + SemIR::CompleteTypeWitness{ + .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(); } @@ -656,12 +658,13 @@ static auto ImportEnumObjectRepresentation( auto int_kind = int_type->isSignedIntegerType() ? SemIR::IntKind::Signed : SemIR::IntKind::Unsigned; auto bit_width_id = GetOrAddInst( - context, MakeImportedLocIdAndInst( - 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)))})); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, + SemIR::IntValue{ + .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, @@ -690,15 +693,16 @@ static auto BuildEnumDefinition(Context& context, auto object_repr_id = ImportEnumObjectRepresentation(context, import_ir_inst_id, enum_decl); class_info.adapt_id = AddInst( - context, MakeImportedLocIdAndInst( - context, import_ir_inst_id, + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, SemIR::AdaptDecl{.adapted_type_inst_id = object_repr_id})); - class_info.complete_type_witness_id = AddInst( - context, - MakeImportedLocIdAndInst( - 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, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, + SemIR::CompleteTypeWitness{ + .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(); } @@ -721,9 +725,9 @@ static auto ImportEnumConstantDecl(Context& context, auto import_ir_inst_id = AddImportIRInst(context.sem_ir(), enumerator_decl->getLocation()); auto inst_id = AddInstInNoBlock( - context, - MakeImportedLocIdAndInst( - context, import_ir_inst_id, {.type_id = type_id, .int_id = int_id})); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, + SemIR::IntValue{.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; @@ -1207,10 +1211,10 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id, auto tuple_pattern_type_id = GetPatternType(context, GetTupleType(context, param_type_ids)); SemIR::InstId pattern_id = AddPatternInst( - context, - SemIR::LocIdAndInst::UncheckedLoc( - loc_id, SemIR::TuplePattern{.type_id = tuple_pattern_type_id, - .elements_id = param_block_id})); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), loc_id, + SemIR::TuplePattern{.type_id = tuple_pattern_type_id, + .elements_id = param_block_id})); param_ids = {pattern_id}; break; } @@ -1320,15 +1324,15 @@ static auto GetReturnInfo(Context& context, SemIR::LocId loc_id, if (auto init_form = context.insts().TryGetAs(form_inst_id)) { auto param_pattern_id = AddPatternInst( - context, MakeImportedLocIdAndInst( - context, return_type_import_ir_inst_id, + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), return_type_import_ir_inst_id, SemIR::OutParamPattern( {.type_id = pattern_type_id, .pretty_name_id = SemIR::NameId::ReturnSlot}))); SemIR::InstId return_slot_pattern_id = AddPatternInst( context, - MakeImportedLocIdAndInst( - context, return_type_import_ir_inst_id, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), return_type_import_ir_inst_id, SemIR::ReturnSlotPattern({.type_id = pattern_type_id, .subpattern_id = param_pattern_id, .type_inst_id = type_inst_id}))); @@ -1773,7 +1777,8 @@ static auto ImportTemplateDecl(Context& context, SemIR::StructValue value = {.type_id = SemIR::TypeId::None, .elements_id = SemIR::InstBlockId::Empty}; auto inst_id = AddPlaceholderImportedInstInNoBlock( - context, MakeImportedLocIdAndInst(context, import_loc_id, value)); + context, SemIR::LocIdAndInst::RuntimeVerified(context.sem_ir(), + import_loc_id, value)); // Create a type for the constant value. auto name_id = context.entity_names().Add( diff --git a/toolchain/check/cpp/operators.cpp b/toolchain/check/cpp/operators.cpp index 65305b094a7d..efc01231f81d 100644 --- a/toolchain/check/cpp/operators.cpp +++ b/toolchain/check/cpp/operators.cpp @@ -247,11 +247,12 @@ static auto MakeCppStdInitializerListMake(Context& context, SemIR::LocId loc_id, context, SemIR::IntLiteralType::TypeInstId), .int_id = context.ints().Add(size)})); auto array_type_inst_id = AddTypeInst( - context, SemIR::LocIdAndInst::UncheckedLoc( - loc_id, SemIR::ArrayType{ - .type_id = SemIR::TypeType::TypeId, - .bound_id = bound_id, - .element_type_inst_id = element_type_inst_id})); + context, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), loc_id, + SemIR::ArrayType{.type_id = SemIR::TypeType::TypeId, + .bound_id = bound_id, + .element_type_inst_id = element_type_inst_id})); auto array_type_id = context.types().GetTypeIdForTypeInstId(array_type_inst_id); diff --git a/toolchain/check/function.cpp b/toolchain/check/function.cpp index facf7aa2a280..012e7b727ac1 100644 --- a/toolchain/check/function.cpp +++ b/toolchain/check/function.cpp @@ -454,7 +454,8 @@ auto MakeFunctionDecl(Context& context, SemIR::LocId loc_id, SemIR::FunctionDecl function_decl = {SemIR::TypeId::None, SemIR::FunctionId::None, decl_block_id}; auto decl_id = AddPlaceholderInstInNoBlock( - context, SemIR::LocIdAndInst::UncheckedLoc(loc_id, function_decl)); + context, SemIR::LocIdAndInst::RuntimeVerified(context.sem_ir(), loc_id, + function_decl)); function.first_owning_decl_id = decl_id; if (is_definition) { function.definition_id = decl_id; diff --git a/toolchain/check/generic.cpp b/toolchain/check/generic.cpp index a2523a9be13a..6b1d5f341429 100644 --- a/toolchain/check/generic.cpp +++ b/toolchain/check/generic.cpp @@ -134,10 +134,9 @@ class RebuildGenericConstantInEvalBlockCallbacks : public SubstInstCallbacks { // TODO: Add a function on `Context` to add the instruction without // inserting it into the dependent instructions list or computing a // constant value for it. - // TODO: Is the location we pick here always appropriate for the new - // instruction? auto inst_id = context().sem_ir().insts().AddInNoBlock( - SemIR::LocIdAndInst::UncheckedLoc(loc_id_, new_inst)); + SemIR::LocIdAndInst::RuntimeVerified(context().sem_ir(), loc_id_, + new_inst)); auto const_id = AddGenericConstantInstToEvalBlock( context(), const_inst_id, inst_id, dependence); context().constant_values().Set(inst_id, const_id); diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 7bee6e1b2f13..ddca30be4c60 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -112,7 +112,8 @@ static auto MakeImportedNamespaceLocIdAndInst(Context& context, // If the import was itself imported, use its location. if (auto import_ir_inst_id = context.insts().GetImportSource(import_id); import_ir_inst_id.has_value()) { - return MakeImportedLocIdAndInst(context, import_ir_inst_id, namespace_inst); + return SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, namespace_inst); } // Otherwise we should have a node location for some kind of namespace @@ -237,11 +238,12 @@ static auto CopySingleNameScopeFromImportIR( auto import_ir_inst_id = context.import_ir_insts().Add( SemIR::ImportIRInst(ir_id, import_inst_id)); auto inst_id = AddInstInNoBlock( - context, MakeImportedLocIdAndInst( - context, import_ir_inst_id, - {.type_id = namespace_type_id, - .import_ir_inst_id = import_ir_inst_id, - .entity_name_id = entity_name_id})); + context, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, + SemIR::ImportRefLoaded{.type_id = namespace_type_id, + .import_ir_inst_id = import_ir_inst_id, + .entity_name_id = entity_name_id})); context.imports().push_back(inst_id); return inst_id; }; @@ -712,44 +714,4 @@ auto ImportNameFromOtherPackage( return result_id; } -// 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); - return true; - } - return false; -} - -namespace Internal { - -auto CheckCompatibleImportedNodeKind(Context& context, - SemIR::ImportIRInstId imported_loc_id, - SemIR::InstKind kind) -> void { - auto& import_ir_inst = context.import_ir_insts().Get(imported_loc_id); - if (import_ir_inst.ir_id() == SemIR::ImportIRId::Cpp) { - // We don't require a matching node kind if the location is in C++, because - // there isn't a node. - return; - } - const auto* import_ir = - context.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 {0} created with location of imported node of kind {1}", - kind, imported_kind); -} - -} // namespace Internal - } // namespace Carbon::Check diff --git a/toolchain/check/import.h b/toolchain/check/import.h index f9ce93979127..6c1e53bceded 100644 --- a/toolchain/check/import.h +++ b/toolchain/check/import.h @@ -93,30 +93,6 @@ auto ImportNameFromOtherPackage( import_ir_scopes, SemIR::NameId name_id) -> SemIR::InstId; -namespace Internal { - -// Checks that the provided imported location has a node kind that is -// compatible with that of the given instruction. -auto CheckCompatibleImportedNodeKind(Context& context, - SemIR::ImportIRInstId imported_loc_id, - SemIR::InstKind kind) -> void; -} // namespace Internal - -// 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 - requires SemIR::Internal::HasNodeId -auto MakeImportedLocIdAndInst(Context& context, - SemIR::ImportIRInstId imported_loc_id, InstT inst) - -> SemIR::LocIdAndInst { - if constexpr (!SemIR::Internal::HasUntypedNodeId) { - Internal::CheckCompatibleImportedNodeKind(context, imported_loc_id, - InstT::Kind); - } - return SemIR::LocIdAndInst::UncheckedLoc(SemIR::LocId(imported_loc_id), inst); -} - } // namespace Carbon::Check #endif // CARBON_TOOLCHAIN_CHECK_IMPORT_H_ diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index a25148286f1c..5be74f39fa84 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -94,8 +94,8 @@ auto AddImportRef(Context& context, SemIR::ImportIRInst import_ir_inst, auto import_ir_inst_id = context.import_ir_insts().Add(import_ir_inst); auto import_ref_id = AddPlaceholderImportedInstInNoBlock( context, - MakeImportedLocIdAndInst( - context, import_ir_inst_id, + SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), import_ir_inst_id, SemIR::ImportRefUnloaded{.import_ir_inst_id = import_ir_inst_id, .entity_name_id = entity_name_id})); return import_ref_id; @@ -512,8 +512,8 @@ static auto AddLoadedImportRef(ImportContext& context, .entity_name_id = SemIR::EntityNameId::None}; auto inst_id = AddPlaceholderImportedInstInNoBlock( context.local_context(), - MakeImportedLocIdAndInst(context.local_context(), import_ir_inst_id, - inst)); + SemIR::LocIdAndInst::RuntimeVerified(context.local_context().sem_ir(), + import_ir_inst_id, inst)); context.local_constant_values().Set(inst_id, local_const_id); context.local_constant_values_for_import_insts().Set(import_inst_id, @@ -558,8 +558,8 @@ static auto AddPlaceholderImportedInst(ImportContext& context, auto import_ir_inst_id = AddImportIRInst(context, import_inst_id); return AddPlaceholderImportedInstInNoBlock( context.local_context(), - MakeImportedLocIdAndInst(context.local_context(), import_ir_inst_id, - inst)); + SemIR::LocIdAndInst::RuntimeVerified(context.local_context().sem_ir(), + import_ir_inst_id, inst)); } // Replace an imported instruction that was added by diff --git a/toolchain/check/inst.cpp b/toolchain/check/inst.cpp index f1bef4e68bdd..49be0861583d 100644 --- a/toolchain/check/inst.cpp +++ b/toolchain/check/inst.cpp @@ -8,6 +8,7 @@ #include "toolchain/check/context.h" #include "toolchain/check/eval.h" #include "toolchain/check/generic.h" +#include "toolchain/check/import.h" #include "toolchain/sem_ir/constant.h" #include "toolchain/sem_ir/ids.h" #include "toolchain/sem_ir/inst_kind.h" diff --git a/toolchain/check/pattern.cpp b/toolchain/check/pattern.cpp index 781314cee048..6f1639e7eaf0 100644 --- a/toolchain/check/pattern.cpp +++ b/toolchain/check/pattern.cpp @@ -106,15 +106,16 @@ auto AddBindingPattern(Context& context, SemIR::LocId name_loc, auto type_id = SemIR::ExtractScrutineeType(context.sem_ir(), pattern.type_id); auto bind_id = AddInstInNoBlock( - context, - SemIR::LocIdAndInst::UncheckedLoc( - name_loc, SemIR::AnyBinding{.kind = bind_name_kind, - .type_id = type_id, - .entity_name_id = pattern.entity_name_id, - .value_id = SemIR::InstId::None})); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), name_loc, + SemIR::AnyBinding{.kind = bind_name_kind, + .type_id = type_id, + .entity_name_id = pattern.entity_name_id, + .value_id = SemIR::InstId::None})); - auto binding_pattern_id = AddPatternInst( - context, SemIR::LocIdAndInst::UncheckedLoc(name_loc, pattern)); + auto binding_pattern_id = + AddPatternInst(context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), name_loc, pattern)); if (pattern.kind == SemIR::SymbolicBindingPattern::Kind) { context.scope_stack().PushCompileTimeBinding(bind_id); @@ -173,11 +174,11 @@ auto AddParamPattern(Context& context, SemIR::LocId loc_id, const auto& param_pattern_kind = is_ref ? SemIR::RefParamPattern::Kind : SemIR::ValueParamPattern::Kind; auto pattern_id = AddPatternInst( - context, - SemIR::LocIdAndInst::UncheckedLoc( - loc_id, SemIR::AnyLeafParamPattern{.kind = param_pattern_kind, - .type_id = pattern_type_id, - .pretty_name_id = name_id})); + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), loc_id, + SemIR::AnyLeafParamPattern{.kind = param_pattern_kind, + .type_id = pattern_type_id, + .pretty_name_id = name_id})); auto entity_name_id = AddBindingEntityName(context, name_id, diff --git a/toolchain/check/subst.cpp b/toolchain/check/subst.cpp index e3ea7dff01af..fafd86e43073 100644 --- a/toolchain/check/subst.cpp +++ b/toolchain/check/subst.cpp @@ -23,8 +23,9 @@ auto SubstInstCallbacks::RebuildType(SemIR::TypeInstId type_inst_id) const auto SubstInstCallbacks::RebuildNewInst(SemIR::LocId loc_id, SemIR::Inst new_inst) const -> SemIR::InstId { - auto const_id = EvalOrAddInst( - context(), SemIR::LocIdAndInst::UncheckedLoc(loc_id, new_inst)); + auto const_id = + EvalOrAddInst(context(), SemIR::LocIdAndInst::RuntimeVerified( + context().sem_ir(), loc_id, new_inst)); CARBON_CHECK(const_id.has_value(), "Substitution into constant produced non-constant"); CARBON_CHECK(const_id.is_constant(), diff --git a/toolchain/check/thunk.cpp b/toolchain/check/thunk.cpp index 8b86e1166df7..8089e3a5d2ae 100644 --- a/toolchain/check/thunk.cpp +++ b/toolchain/check/thunk.cpp @@ -41,8 +41,9 @@ static auto RebuildPatternInst(Context& context, SemIR::InstId orig_inst_id, CARBON_CHECK(context.insts().Get(orig_inst_id).kind() == new_inst.kind(), "Rebuilt pattern with the wrong kind: {0} -> {1}", context.insts().Get(orig_inst_id), new_inst); - return AddPatternInst(context, SemIR::LocIdAndInst::UncheckedLoc( - SemIR::LocId(orig_inst_id), new_inst)); + return AddPatternInst( + context, SemIR::LocIdAndInst::RuntimeVerified( + context.sem_ir(), SemIR::LocId(orig_inst_id), new_inst)); } // Wrapper to allow the type to be specified as a template argument for API diff --git a/toolchain/sem_ir/inst.cpp b/toolchain/sem_ir/inst.cpp index b85ed71a2dfa..fea982df7192 100644 --- a/toolchain/sem_ir/inst.cpp +++ b/toolchain/sem_ir/inst.cpp @@ -61,4 +61,66 @@ auto InstStore::GetUnattachedType(TypeId type_id) const -> TypeId { return file_->types().GetUnattachedType(type_id); } +// 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(InstKind imported_kind, + InstKind local_kind) -> bool { + if (imported_kind == local_kind) { + return true; + } + if (imported_kind == ImportDecl::Kind && local_kind == Namespace::Kind) { + static_assert( + std::is_convertible_v); + return true; + } + return false; +} + +auto LocIdAndInst::RuntimeVerified(const File& file, LocId loc_id, Inst inst) + -> LocIdAndInst { + switch (loc_id.kind()) { + case LocId::Kind::ImportIRInstId: { + CARBON_CHECK(!inst.kind().disallow_all_node_kinds(), + "Should never import builtins/singletons: {0}", inst); + + if (inst.kind().allow_all_node_kinds()) { + break; + } + + const auto& import_ir_inst = + file.import_ir_insts().Get(loc_id.import_ir_inst_id()); + // We don't require a matching node kind if the location is in C++, + // because there isn't a node. + if (import_ir_inst.ir_id() == ImportIRId::Cpp) { + break; + } + const auto* import_ir = + file.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, inst.kind()), + "Unexpected imported `InstKind` {0} for {1}", imported_kind, + inst); + break; + } + + case LocId::Kind::InstId: + // TODO: Figure out right verification. + break; + + case LocId::Kind::NodeId: { + auto node_kind = file.parse_tree().node_kind(loc_id.node_id()); + CARBON_CHECK(inst.kind().IsAllowedNodeKind(node_kind), + "Unexpected `NodeKind` {0} for {1}", node_kind, inst); + break; + } + + case LocId::Kind::None: + break; + } + return LocIdAndInst(loc_id, inst, /*is_unchecked=*/true); +} + } // namespace Carbon::SemIR diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index 2f0c500f6e1e..91728bdae7bb 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -28,6 +28,8 @@ namespace Carbon::SemIR { template struct CategoryOf; +class File; + // InstLikeTypeInfo is an implementation detail, and not public API. namespace Internal { @@ -417,15 +419,12 @@ struct LocIdAndInst { return LocIdAndInst(LocId::None, inst, /*is_unchecked=*/true); } - // Unsafely form a pair of a location and an instruction. Used in the cases - // where we can't statically enforce the type matches. For `ImportIRInstId`, - // use `MakeImportedLocIdAndInst` in `import.h`. - template - requires(std::convertible_to && - !std::same_as) - static auto UncheckedLoc(LocT loc_id, Inst inst) -> LocIdAndInst { - return LocIdAndInst(loc_id, inst, /*is_unchecked=*/true); - } + // Constructs a `LocIdAndInst` with a runtime verification of the location. + // + // Prefer `LocIdAndInst` constructors with compile-time verification, + // or `AddInst` overloads which make use of those constructors. + static auto RuntimeVerified(const File& file, LocId loc_id, Inst inst) + -> LocIdAndInst; // Construction for the common case with a typed node. template @@ -439,7 +438,7 @@ struct LocIdAndInst { requires(Internal::HasUntypedNodeId) LocIdAndInst(LocId loc_id, InstT inst) : loc_id(loc_id), inst(inst) {} - // For `ImportIRInstId`, use `MakeImportedLocIdAndInst` in `import.h`. + // For `ImportIRInstId`, use `RuntimeVerified`. template LocIdAndInst(ImportIRInstId loc_id, InstT inst) = delete; @@ -447,6 +446,9 @@ struct LocIdAndInst { Inst inst; private: + // For `InstStore::GetWithLocId` to construct unchecked values. + friend class InstStore; + // Note `is_unchecked` serves to disambiguate from public constructors. explicit LocIdAndInst(LocId loc_id, Inst inst, bool /*is_unchecked*/) : loc_id(loc_id), inst(inst) {} @@ -503,7 +505,7 @@ class InstStore { // Returns the requested instruction and its location ID. auto GetWithLocId(InstId inst_id) const -> LocIdAndInst { - return LocIdAndInst::UncheckedLoc(LocId(inst_id), Get(inst_id)); + return LocIdAndInst(LocId(inst_id), Get(inst_id), /*is_unchecked=*/true); } // Returns whether the requested instruction is the specified type. diff --git a/toolchain/sem_ir/inst_kind.cpp b/toolchain/sem_ir/inst_kind.cpp index 180a33deae1d..bba8a4a9e960 100644 --- a/toolchain/sem_ir/inst_kind.cpp +++ b/toolchain/sem_ir/inst_kind.cpp @@ -29,4 +29,12 @@ auto InstKind::has_type() const -> bool { return Table[AsInt()]; } +auto InstKind::IsAllowedNodeKind(Parse::NodeKind node_kind) const -> bool { + const auto& def = definition_info(*this); + if (def.internal_allow_all_node_kinds) { + return true; + } + return llvm::is_contained(def.internal_allowed_node_kinds, node_kind); +} + } // namespace Carbon::SemIR diff --git a/toolchain/sem_ir/inst_kind.h b/toolchain/sem_ir/inst_kind.h index 73f104d17c68..a1723c7c0eec 100644 --- a/toolchain/sem_ir/inst_kind.h +++ b/toolchain/sem_ir/inst_kind.h @@ -5,6 +5,7 @@ #ifndef CARBON_TOOLCHAIN_SEM_IR_INST_KIND_H_ #define CARBON_TOOLCHAIN_SEM_IR_INST_KIND_H_ +#include #include #include @@ -290,6 +291,13 @@ class InstKind : public CARBON_ENUM_BASE(InstKind) { bool is_lowered = true; bool deduce_through = false; bool has_cleanup = false; + + // The inst's allowed node kinds, for `IsAllowedNodeKind`. + // + // Do not set these directly. They are set by the `TypedNodeId` template + // parameter of `Define`. + bool internal_allow_all_node_kinds = false; + llvm::ArrayRef internal_allowed_node_kinds; }; // Provides a definition for this instruction kind. Should only be called @@ -365,6 +373,21 @@ class InstKind : public CARBON_ENUM_BASE(InstKind) { return definition_info(*this).has_cleanup; } + // Returns true if the passed `NodeKind` is allowed. + auto IsAllowedNodeKind(Parse::NodeKind node_kind) const -> bool; + + // Returns true if all `NodeKind`s are allowed. + auto allow_all_node_kinds() const -> bool { + return definition_info(*this).internal_allow_all_node_kinds; + } + + // Returns true if no `NodeKind`s are allowed. + auto disallow_all_node_kinds() const -> bool { + const auto& def = definition_info(*this); + return !def.internal_allow_all_node_kinds && + def.internal_allowed_node_kinds.empty(); + } + private: // Returns the DefinitionInfo for the kind. static auto definition_info(InstKind kind) -> const DefinitionInfo&; @@ -453,9 +476,44 @@ class InstKind::Definition : public InstKind { InstKind::DefinitionInfo info_; }; +namespace Internal { + +// Storage for `internal_allowed_node_kinds` where there's a list of kinds. +template +constexpr std::array Kinds = {T...}; + +// `NoneNodeId` uses should never have a node associated; it's mainly for +// builtins. +constexpr auto GetAllowedNodeKinds(Parse::NoneNodeId* /*unused*/) + -> llvm::ArrayRef { + return {}; +} + +// For a regular `NodeId`, returns an array of just its kind. +template +constexpr auto GetAllowedNodeKinds(Parse::NodeIdForKind* /*unused*/) + -> llvm::ArrayRef { + return Kinds(Kind)>; +} + +// For `NodeIdOneOf`, returns an array of each kind. +template +constexpr auto GetAllowedNodeKinds(Parse::NodeIdOneOf* /*unused*/) + -> llvm::ArrayRef { + return Kinds; +} + +} // namespace Internal + template constexpr auto InstKind::Define(DefinitionInfo info) const -> Definition { + if constexpr (std::same_as) { + info.internal_allow_all_node_kinds = true; + } else { + info.internal_allowed_node_kinds = + Internal::GetAllowedNodeKinds(static_cast(nullptr)); + } return Definition(*this, info); } diff --git a/toolchain/sem_ir/typed_insts.h b/toolchain/sem_ir/typed_insts.h index 72570bbe50a2..476fbc415802 100644 --- a/toolchain/sem_ir/typed_insts.h +++ b/toolchain/sem_ir/typed_insts.h @@ -749,8 +749,10 @@ struct FloatValue { // A form binding, such as the `x` declared by `x:? F`. See `AnyBinding` for // member documentation. struct FormBinding { + // TODO: Should this have been associated with the `FormBindingPatternId` + // instead? static constexpr auto Kind = - InstKind::FormBinding.Define( + InstKind::FormBinding.Define( {.ir_name = "form_binding", .expr_category = ComputedExprCategory::DependsOnOperands, .constant_kind = InstConstantKind::Never}); @@ -763,12 +765,15 @@ struct FormBinding { // A form binding pattern, such as `x:? F`, that is not a parameter. See // `AnyBindingPattern` for member documentation. struct FormBindingPattern { + // TODO: Should this have been associated with the `FormBindingPatternId` + // instead? static constexpr auto Kind = - InstKind::FormBindingPattern.Define( - {.ir_name = "form_binding_pattern", - .expr_category = ExprCategory::Pattern, - .constant_kind = InstConstantKind::AlwaysUnique, - .is_lowered = false}); + InstKind::FormBindingPattern + .Define( + {.ir_name = "form_binding_pattern", + .expr_category = ExprCategory::Pattern, + .constant_kind = InstConstantKind::AlwaysUnique, + .is_lowered = false}); TypeId type_id; // Note that the EntityName's `form_id` represents the scrutinee form, so it