From 1c673041f0f2fa4990a0a4172a2ad5278d71c1b9 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 27 Mar 2024 16:25:41 -0700 Subject: [PATCH] Provide locations for indirectly imported instructions. (#3811) This replaces all invalid node IDs in import_ref.cpp with references to the imported instruction. This splits out ReplaceInstBeforeConstantUse into a separate function when the LocationId is replaced, as for splicing. That's the less common case, whereas others would need to provide the LocationId in order just to not change the value. --- toolchain/check/check.cpp | 68 +++++++++---- toolchain/check/context.cpp | 21 +++- toolchain/check/context.h | 12 ++- toolchain/check/handle_class.cpp | 2 +- toolchain/check/handle_function.cpp | 3 +- toolchain/check/handle_interface.cpp | 3 +- toolchain/check/handle_let.cpp | 5 +- toolchain/check/handle_namespace.cpp | 2 +- toolchain/check/import.cpp | 2 +- toolchain/check/import_ref.cpp | 95 +++++++++++-------- toolchain/check/pending_block.h | 6 +- .../class/cross_package_import.carbon | 11 ++- .../testdata/class/fail_import_misuses.carbon | 9 +- .../function/definition/import.carbon | 33 +++++-- .../packages/cross_package_import.carbon | 9 +- toolchain/sem_ir/inst.h | 15 ++- 16 files changed, 198 insertions(+), 98 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index 529326e529c2..4460a057b54d 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -9,6 +9,7 @@ #include "toolchain/check/context.h" #include "toolchain/check/diagnostic_helpers.h" #include "toolchain/check/import.h" +#include "toolchain/diagnostics/diagnostic.h" #include "toolchain/diagnostics/diagnostic_emitter.h" #include "toolchain/lex/token_kind.h" #include "toolchain/parse/node_ids.h" @@ -36,38 +37,69 @@ class SemIRDiagnosticConverter : public DiagnosticConverter { const SemIR::File* sem_ir) : node_converters_(node_converters), sem_ir_(sem_ir) {} + // Converts an instruction's location to a diagnostic location, which will be + // the underlying line of code. Adds context for any imports used in the + // current SemIR to get to the underlying code. auto ConvertLocation(SemIRLocation loc, ContextFnT context_fn) const -> DiagnosticLocation override { - // Parse nodes always refer to the current IR. - if (!loc.is_inst_id) { - CARBON_CHECK(loc.loc_id.is_node_id() || !loc.loc_id.is_valid()) - << "TODO: Handle non-NodeId locs"; - return ConvertLocationInFile(sem_ir_, loc.loc_id.node_id(), - loc.token_only, context_fn); + // Cursors for the current IR and instruction in that IR. + const auto* cursor_ir = sem_ir_; + auto cursor_inst_id = SemIR::InstId::Invalid; + + // Notes an import on the diagnostic and updates cursors to point at the + // imported IR. + auto follow_import_ref = [&](SemIR::ImportIRId ir_id, + SemIR::InstId inst_id) { + const auto& import_ir = cursor_ir->import_irs().Get(ir_id); + auto context_loc = ConvertLocationInFile(cursor_ir, import_ir.node_id, + loc.token_only, context_fn); + CARBON_DIAGNOSTIC(InImport, Note, "In import."); + context_fn(context_loc, InImport); + cursor_ir = import_ir.sem_ir; + cursor_inst_id = inst_id; + }; + + // If the location is is an import, follows it and returns nullopt. + // Otherwise, it's a parse node, so return the final location. + auto handle_loc = + [&](SemIR::LocationId loc_id) -> std::optional { + if (loc_id.is_import_ir_inst_id()) { + auto import_ir_inst = + cursor_ir->import_ir_insts().Get(loc_id.import_ir_inst_id()); + follow_import_ref(import_ir_inst.ir_id, import_ir_inst.inst_id); + return std::nullopt; + } else { + // Parse nodes always refer to the current IR. + return ConvertLocationInFile(cursor_ir, loc_id.node_id(), + loc.token_only, context_fn); + } + }; + + // Handle the base location. + if (loc.is_inst_id) { + cursor_inst_id = loc.inst_id; + } else { + if (auto diag_loc = handle_loc(loc.loc_id)) { + return *diag_loc; + } + CARBON_CHECK(cursor_inst_id.is_valid()) << "Should have been set"; } - const auto* cursor_ir = sem_ir_; - auto cursor_inst_id = loc.inst_id; while (true) { // If the parse node is valid, use it for the location. if (auto loc_id = cursor_ir->insts().GetLocationId(cursor_inst_id); loc_id.is_valid()) { - CARBON_CHECK(loc_id.is_node_id()) << "TODO: Handle non-NodeId locs"; - return ConvertLocationInFile(cursor_ir, loc_id.node_id(), - loc.token_only, context_fn); + if (auto diag_loc = handle_loc(loc_id)) { + return *diag_loc; + } + continue; } // If the parse node was invalid, recurse through import references when // possible. if (auto import_ref = cursor_ir->insts().TryGetAs( cursor_inst_id)) { - const auto& import_ir = cursor_ir->import_irs().Get(import_ref->ir_id); - auto context_loc = ConvertLocationInFile(cursor_ir, import_ir.node_id, - loc.token_only, context_fn); - CARBON_DIAGNOSTIC(InImport, Note, "In import."); - context_fn(context_loc, InImport); - cursor_ir = import_ir.sem_ir; - cursor_inst_id = import_ref->inst_id; + follow_import_ref(import_ref->ir_id, import_ref->inst_id); continue; } diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 13e59a77ce16..054d66ee5868 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -117,9 +117,9 @@ auto Context::AddInstAndPush(SemIR::LocationIdAndInst loc_id_and_inst) -> void { node_stack_.Push(loc_id_and_inst.loc_id.node_id(), inst_id); } -auto Context::ReplaceInstBeforeConstantUse( +auto Context::ReplaceLocationIdAndInstBeforeConstantUse( SemIR::InstId inst_id, SemIR::LocationIdAndInst loc_id_and_inst) -> void { - sem_ir().insts().Set(inst_id, loc_id_and_inst); + sem_ir().insts().SetLocationIdAndInst(inst_id, loc_id_and_inst); CARBON_VLOG() << "ReplaceInst: " << inst_id << " -> " << loc_id_and_inst.inst << "\n"; @@ -135,6 +135,23 @@ auto Context::ReplaceInstBeforeConstantUse( constant_values().Set(inst_id, const_id); } +auto Context::ReplaceInstBeforeConstantUse(SemIR::InstId inst_id, + SemIR::Inst inst) -> void { + sem_ir().insts().Set(inst_id, inst); + + CARBON_VLOG() << "ReplaceInst: " << inst_id << " -> " << inst << "\n"; + + // Redo evaluation. This is only safe to do if this instruction has not + // already been used as a constant, which is the caller's responsibility to + // ensure. + auto const_id = TryEvalInst(*this, inst_id, inst); + if (const_id.is_constant()) { + CARBON_VLOG() << "Constant: " << inst << " -> " << const_id.inst_id() + << "\n"; + } + constant_values().Set(inst_id, const_id); +} + auto Context::AddImportRef(SemIR::ImportIRId ir_id, SemIR::InstId inst_id) -> SemIR::InstId { auto import_ref_id = diff --git a/toolchain/check/context.h b/toolchain/check/context.h index c66493604ff9..11fbc2b76976 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -67,13 +67,19 @@ class Context { // result. Only valid if the LocationId is for a NodeId. auto AddInstAndPush(SemIR::LocationIdAndInst loc_id_and_inst) -> void; - // Replaces the value of the instruction `inst_id` with `loc_id_and_inst`. + // Replaces the instruction `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 used + // in a position that constant evaluation ignores, such as a return slot. + auto ReplaceLocationIdAndInstBeforeConstantUse( + SemIR::InstId inst_id, SemIR::LocationIdAndInst loc_id_and_inst) -> void; + + // Replaces the instruction `inst_id` with `inst`, not affecting location. // 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 used in a position that constant evaluation ignores, such // as a return slot. - auto ReplaceInstBeforeConstantUse(SemIR::InstId inst_id, - SemIR::LocationIdAndInst loc_id_and_inst) + auto ReplaceInstBeforeConstantUse(SemIR::InstId inst_id, SemIR::Inst inst) -> void; // Adds an import_ref instruction for the specified instruction in the diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index 430c16a675cb..78255f38f6f2 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -108,7 +108,7 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id) } // Write the class ID into the ClassDecl. - context.ReplaceInstBeforeConstantUse(class_decl_id, {node_id, class_decl}); + context.ReplaceInstBeforeConstantUse(class_decl_id, class_decl); if (is_new_class) { // Build the `Self` type using the resulting type constant. diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 01027804244a..399202750e99 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -198,8 +198,7 @@ static auto BuildFunctionDecl(Context& context, } // Write the function ID into the FunctionDecl. - context.ReplaceInstBeforeConstantUse(function_info.decl_id, - {node_id, function_decl}); + context.ReplaceInstBeforeConstantUse(function_info.decl_id, function_decl); if (SemIR::IsEntryPoint(context.sem_ir(), function_decl.function_id)) { // TODO: Update this once valid signatures for the entry point are decided. diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index c7454921cb87..61847fe3a914 100644 --- a/toolchain/check/handle_interface.cpp +++ b/toolchain/check/handle_interface.cpp @@ -88,8 +88,7 @@ static auto BuildInterfaceDecl(Context& context, } // Write the interface ID into the InterfaceDecl. - context.ReplaceInstBeforeConstantUse(interface_decl_id, - {node_id, interface_decl}); + context.ReplaceInstBeforeConstantUse(interface_decl_id, interface_decl); return {interface_decl.interface_id, interface_decl_id}; } diff --git a/toolchain/check/handle_let.cpp b/toolchain/check/handle_let.cpp index d875e3e4f0a1..7d5a0c17c426 100644 --- a/toolchain/check/handle_let.cpp +++ b/toolchain/check/handle_let.cpp @@ -46,7 +46,7 @@ static auto BuildAssociatedConstantDecl( // declaration. auto name_id = context.bind_names().Get(binding_pattern->bind_name_id).name_id; - context.ReplaceInstBeforeConstantUse( + context.ReplaceLocationIdAndInstBeforeConstantUse( pattern_id, {node_id, SemIR::AssociatedConstantDecl{ binding_pattern->type_id, name_id}}); auto decl_id = pattern_id; @@ -130,8 +130,7 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId node_id) -> bool { CARBON_CHECK(!bind_name.value_id.is_valid()) << "Binding should not already have a value!"; bind_name.value_id = *value_id; - pattern.inst = bind_name; - context.ReplaceInstBeforeConstantUse(pattern_id, pattern); + context.ReplaceInstBeforeConstantUse(pattern_id, bind_name); context.inst_block_stack().AddInstId(pattern_id); // Add the name of the binding to the current scope. diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp index b6dc0e9d0e53..50be9dddb308 100644 --- a/toolchain/check/handle_namespace.cpp +++ b/toolchain/check/handle_namespace.cpp @@ -28,7 +28,7 @@ auto HandleNamespace(Context& context, Parse::NamespaceId node_id) -> bool { namespace_inst.name_scope_id = context.name_scopes().Add( namespace_id, name_context.name_id_for_new_inst(), name_context.enclosing_scope_id_for_new_inst()); - context.ReplaceInstBeforeConstantUse(namespace_id, {node_id, namespace_inst}); + context.ReplaceInstBeforeConstantUse(namespace_id, namespace_inst); auto existing_inst_id = context.decl_name_stack().LookupOrAddName(name_context, namespace_id); diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 68ae266d76df..8890a2959bf7 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -102,7 +102,7 @@ static auto AddNamespace( auto namespace_id = context.AddPlaceholderInst({node_id, namespace_inst}); namespace_inst.name_scope_id = context.name_scopes().Add(namespace_id, name_id, enclosing_scope_id); - context.ReplaceInstBeforeConstantUse(namespace_id, {node_id, namespace_inst}); + context.ReplaceInstBeforeConstantUse(namespace_id, namespace_inst); // Diagnose if there's a name conflict, but still produce the namespace to // supersede the name conflict in order to avoid repeat diagnostics. diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 5377b37c5e1c..7277ad8534eb 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -158,6 +158,11 @@ class ImportRefResolver { return initial_work < work_stack_.size(); } + auto AddImportIRInst(SemIR::InstId inst_id) -> SemIR::LocationId { + return context_.import_ir_insts().Add( + {.ir_id = import_ir_id_, .inst_id = inst_id}); + } + // Returns the ConstantId for an InstId. Adds unresolved constants to // work_stack_. auto GetLocalConstantId(SemIR::InstId inst_id) -> SemIR::ConstantId { @@ -217,14 +222,20 @@ class ImportRefResolver { // TODO: Consider a different parameter handling to simplify import logic. auto inst = import_ir_.insts().Get(ref_id); auto addr_inst = inst.TryAs(); + auto bind_id = ref_id; + auto param_id = ref_id; + if (addr_inst) { bind_id = addr_inst->inner_id; + param_id = bind_id; inst = import_ir_.insts().Get(bind_id); } + auto bind_inst = inst.TryAs(); if (bind_inst) { - inst = import_ir_.insts().Get(bind_inst->value_id); + param_id = bind_inst->value_id; + inst = import_ir_.insts().Get(param_id); } auto param_inst = inst.As(); @@ -233,7 +244,7 @@ class ImportRefResolver { auto type_id = context_.GetTypeIdForTypeConstant(const_id); auto new_param_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, SemIR::Param{type_id, name_id}}); + {AddImportIRInst(param_id), SemIR::Param{type_id, name_id}}); if (bind_inst) { switch (bind_inst->kind) { case SemIR::InstKind::BindName: { @@ -241,7 +252,7 @@ class ImportRefResolver { {.name_id = name_id, .enclosing_scope_id = SemIR::NameScopeId::Invalid}); new_param_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, + {AddImportIRInst(bind_id), SemIR::BindName{type_id, bind_name_id, new_param_id}}); break; } @@ -254,8 +265,7 @@ class ImportRefResolver { new_bind_inst.value_id = new_param_id; // This is not before constant use, but doesn't change the // constant value of the instruction. - context_.ReplaceInstBeforeConstantUse( - bind_id, {Parse::NodeId::Invalid, new_bind_inst}); + context_.ReplaceInstBeforeConstantUse(bind_id, new_bind_inst); break; } default: { @@ -264,9 +274,10 @@ class ImportRefResolver { } } if (addr_inst) { - new_param_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, - SemIR::AddrPattern{type_id, new_param_id}}); + new_param_id = + context_.AddInstInNoBlock(SemIR::LocationIdAndInst::Untyped( + AddImportIRInst(ref_id), + SemIR::AddrPattern{type_id, new_param_id})); } new_param_refs.push_back(new_param_id); } @@ -374,7 +385,7 @@ class ImportRefResolver { return TryResolveTypedInst(inst.As()); case SemIR::InstKind::BaseDecl: - return TryResolveTypedInst(inst.As()); + return TryResolveTypedInst(inst.As(), inst_id); case SemIR::InstKind::BindAlias: return TryResolveTypedInst(inst.As()); @@ -389,7 +400,7 @@ class ImportRefResolver { return TryResolveTypedInst(inst.As()); case SemIR::InstKind::FieldDecl: - return TryResolveTypedInst(inst.As()); + return TryResolveTypedInst(inst.As(), inst_id); case SemIR::InstKind::FunctionDecl: return TryResolveTypedInst(inst.As()); @@ -404,7 +415,7 @@ class ImportRefResolver { return TryResolveTypedInst(inst.As()); case SemIR::InstKind::StructType: - return TryResolveTypedInst(inst.As()); + return TryResolveTypedInst(inst.As(), inst_id); case SemIR::InstKind::TupleType: return TryResolveTypedInst(inst.As()); @@ -417,11 +428,11 @@ class ImportRefResolver { return {TryEvalInst(context_, inst_id, inst)}; case SemIR::InstKind::BindSymbolicName: - return TryResolveTypedInst(inst.As()); + return TryResolveTypedInst(inst.As(), inst_id); default: context_.TODO( - Parse::NodeId(Parse::NodeId::Invalid), + AddImportIRInst(inst_id), llvm::formatv("TryResolveInst on {0}", inst.kind()).str()); return {SemIR::ConstantId::Error}; } @@ -438,7 +449,7 @@ class ImportRefResolver { auto decl_id = context_.AddImportRef(import_ir_id_, inst.decl_id); auto inst_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, + {AddImportIRInst(inst.decl_id), SemIR::AssociatedEntity{ context_.GetTypeIdForTypeConstant(type_const_id), inst.index, decl_id}}); @@ -465,7 +476,8 @@ class ImportRefResolver { return {context_.constant_values().Get(inst_id)}; } - auto TryResolveTypedInst(SemIR::BaseDecl inst) -> ResolveResult { + auto TryResolveTypedInst(SemIR::BaseDecl inst, SemIR::InstId import_inst_id) + -> ResolveResult { auto initial_work = work_stack_.size(); auto type_const_id = GetLocalConstantId(inst.type_id); auto base_type_const_id = GetLocalConstantId(inst.base_type_id); @@ -474,11 +486,11 @@ class ImportRefResolver { } // Import the instruction in order to update contained base_type_id. - auto inst_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, - SemIR::BaseDecl{context_.GetTypeIdForTypeConstant(type_const_id), - context_.GetTypeIdForTypeConstant(base_type_const_id), - inst.index}}); + auto inst_id = context_.AddInstInNoBlock(SemIR::LocationIdAndInst::Untyped( + AddImportIRInst(import_inst_id), + SemIR::BaseDecl{context_.GetTypeIdForTypeConstant(type_const_id), + context_.GetTypeIdForTypeConstant(base_type_const_id), + inst.index})); return {context_.constant_values().Get(inst_id)}; } @@ -491,7 +503,8 @@ class ImportRefResolver { return {value_id}; } - auto TryResolveTypedInst(SemIR::BindSymbolicName inst) -> ResolveResult { + auto TryResolveTypedInst(SemIR::BindSymbolicName inst, + SemIR::InstId import_inst_id) -> ResolveResult { auto initial_work = work_stack_.size(); auto type_id = GetLocalConstantId(inst.type_id); if (HasNewWork(initial_work)) { @@ -504,7 +517,7 @@ class ImportRefResolver { {.name_id = name_id, .enclosing_scope_id = SemIR::NameScopeId::Invalid}); auto new_bind_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, + {AddImportIRInst(import_inst_id), SemIR::BindSymbolicName{context_.GetTypeIdForTypeConstant(type_id), bind_name_id, SemIR::InstId::Invalid}}); return {context_.constant_values().Get(new_bind_id)}; @@ -519,7 +532,8 @@ class ImportRefResolver { SemIR::ClassDecl{SemIR::TypeId::Invalid, SemIR::ClassId::Invalid, SemIR::InstBlockId::Empty}; auto class_decl_id = - context_.AddPlaceholderInst({Parse::NodeId::Invalid, class_decl}); + context_.AddPlaceholderInst(SemIR::LocationIdAndInst::Untyped( + AddImportIRInst(import_class.decl_id), class_decl)); // Regardless of whether ClassDecl is a complete type, we first need an // incomplete type so that any references have something to point at. class_decl.class_id = context_.classes().Add({ @@ -533,8 +547,7 @@ class ImportRefResolver { }); // Write the class ID into the ClassDecl. - context_.ReplaceInstBeforeConstantUse(class_decl_id, - {Parse::NodeId::Invalid, class_decl}); + context_.ReplaceInstBeforeConstantUse(class_decl_id, class_decl); auto self_const_id = context_.constant_values().Get(class_decl_id); // Build the `Self` type using the resulting type constant. @@ -644,16 +657,17 @@ class ImportRefResolver { SemIR::ConstType{SemIR::TypeId::TypeType, inner_type_id})}; } - auto TryResolveTypedInst(SemIR::FieldDecl inst) -> ResolveResult { + auto TryResolveTypedInst(SemIR::FieldDecl inst, SemIR::InstId import_inst_id) + -> ResolveResult { auto initial_work = work_stack_.size(); auto const_id = GetLocalConstantId(inst.type_id); if (HasNewWork(initial_work)) { return ResolveResult::Retry(); } - auto inst_id = context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, - SemIR::FieldDecl{context_.GetTypeIdForTypeConstant(const_id), - GetLocalNameId(inst.name_id), inst.index}}); + auto inst_id = context_.AddInstInNoBlock(SemIR::LocationIdAndInst::Untyped( + AddImportIRInst(import_inst_id), + SemIR::FieldDecl{context_.GetTypeIdForTypeConstant(const_id), + GetLocalNameId(inst.name_id), inst.index})); return {context_.constant_values().Get(inst_id)}; } @@ -684,8 +698,9 @@ class ImportRefResolver { auto function_decl = SemIR::FunctionDecl{ context_.GetTypeIdForTypeConstant(type_const_id), SemIR::FunctionId::Invalid, SemIR::InstBlockId::Empty}; - auto function_decl_id = context_.AddPlaceholderInstInNoBlock( - {Parse::NodeId::Invalid, function_decl}); + auto function_decl_id = + context_.AddPlaceholderInstInNoBlock(SemIR::LocationIdAndInst::Untyped( + AddImportIRInst(function.decl_id), function_decl)); auto new_return_type_id = return_type_const_id.is_valid() @@ -710,10 +725,7 @@ class ImportRefResolver { .is_extern = function.is_extern, .builtin_kind = function.builtin_kind}); // Write the function ID into the FunctionDecl. - // TODO: The Invalid NodeId means it's hard to associate diagnostics. We - // should replace this. - context_.ReplaceInstBeforeConstantUse( - function_decl_id, {Parse::NodeId::Invalid, function_decl}); + context_.ReplaceInstBeforeConstantUse(function_decl_id, function_decl); return {context_.constant_values().Get(function_decl_id)}; } @@ -725,7 +737,8 @@ class ImportRefResolver { SemIR::InterfaceId::Invalid, SemIR::InstBlockId::Empty}; auto interface_decl_id = - context_.AddPlaceholderInst({Parse::NodeId::Invalid, interface_decl}); + context_.AddPlaceholderInst(SemIR::LocationIdAndInst::Untyped( + AddImportIRInst(import_interface.decl_id), interface_decl)); // Start with an incomplete interface. SemIR::Interface new_interface = { @@ -737,8 +750,7 @@ class ImportRefResolver { // Write the interface ID into the InterfaceDecl. interface_decl.interface_id = context_.interfaces().Add(new_interface); - context_.ReplaceInstBeforeConstantUse( - interface_decl_id, {Parse::NodeId::Invalid, interface_decl}); + context_.ReplaceInstBeforeConstantUse(interface_decl_id, interface_decl); // Set the constant value for the imported interface. return context_.constant_values().Get(interface_decl_id); @@ -825,7 +837,8 @@ class ImportRefResolver { context_.GetPointerType(pointee_type_id))}; } - auto TryResolveTypedInst(SemIR::StructType inst) -> ResolveResult { + auto TryResolveTypedInst(SemIR::StructType inst, SemIR::InstId import_inst_id) + -> ResolveResult { // Collect all constants first, locating unresolved ones in a single pass. auto initial_work = work_stack_.size(); CARBON_CHECK(inst.type_id == SemIR::TypeId::TypeType); @@ -851,7 +864,7 @@ class ImportRefResolver { auto name_id = GetLocalNameId(field.name_id); auto field_type_id = context_.GetTypeIdForTypeConstant(field_const_id); fields.push_back(context_.AddInstInNoBlock( - {Parse::NodeId::Invalid, + {AddImportIRInst(import_inst_id), SemIR::StructTypeField{.name_id = name_id, .field_type_id = field_type_id}})); } diff --git a/toolchain/check/pending_block.h b/toolchain/check/pending_block.h index 33f24e49a693..39e639a92749 100644 --- a/toolchain/check/pending_block.h +++ b/toolchain/check/pending_block.h @@ -63,17 +63,17 @@ class PendingBlock { if (insts_.empty()) { // 1) The block is empty. Replace `target_id` with an empty splice // pointing at `value_id`. - context_.ReplaceInstBeforeConstantUse( + context_.ReplaceLocationIdAndInstBeforeConstantUse( target_id, {value.loc_id, SemIR::SpliceBlock{value.inst.type_id(), SemIR::InstBlockId::Empty, value_id}}); } else if (insts_.size() == 1 && insts_[0] == value_id) { // 2) The block is {value_id}. Replace `target_id` with the instruction // referred to by `value_id`. This is intended to be the common case. - context_.ReplaceInstBeforeConstantUse(target_id, value); + context_.ReplaceLocationIdAndInstBeforeConstantUse(target_id, value); } else { // 3) Anything else: splice it into the IR, replacing `target_id`. - context_.ReplaceInstBeforeConstantUse( + context_.ReplaceLocationIdAndInstBeforeConstantUse( target_id, {value.loc_id, SemIR::SpliceBlock{value.inst.type_id(), diff --git a/toolchain/check/testdata/class/cross_package_import.carbon b/toolchain/check/testdata/class/cross_package_import.carbon index b185436797e1..247547f96dea 100644 --- a/toolchain/check/testdata/class/cross_package_import.carbon +++ b/toolchain/check/testdata/class/cross_package_import.carbon @@ -45,10 +45,15 @@ library "extern" api; import Other library "extern"; -// CHECK:STDERR: fail_extern.carbon:[[@LINE+5]]:8: ERROR: Variable has incomplete type `C`. +// CHECK:STDERR: fail_extern.carbon:[[@LINE+10]]:8: ERROR: Variable has incomplete type `C`. // CHECK:STDERR: var c: Other.C = {}; // CHECK:STDERR: ^~~~~~~ -// CHECK:STDERR: fail_extern.carbon: Class was forward declared here. +// CHECK:STDERR: fail_extern.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import Other library "extern"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: other_extern.carbon:5:1: Class was forward declared here. +// CHECK:STDERR: class C; +// CHECK:STDERR: ^~~~~~~~ // CHECK:STDERR: var c: Other.C = {}; @@ -213,7 +218,7 @@ var c: Other.C = {}; // CHECK:STDOUT: // CHECK:STDOUT: fn @__global_init() { // CHECK:STDOUT: !entry: -// CHECK:STDOUT: %.loc11: {} = struct_literal () +// CHECK:STDOUT: %.loc16: {} = struct_literal () // CHECK:STDOUT: assign file.%c.var, // CHECK:STDOUT: return // CHECK:STDOUT: } diff --git a/toolchain/check/testdata/class/fail_import_misuses.carbon b/toolchain/check/testdata/class/fail_import_misuses.carbon index 2edb61eec4eb..2b53467d73d5 100644 --- a/toolchain/check/testdata/class/fail_import_misuses.carbon +++ b/toolchain/check/testdata/class/fail_import_misuses.carbon @@ -32,10 +32,15 @@ import library "a"; class Empty { } -// CHECK:STDERR: fail_b.carbon:[[@LINE+4]]:8: ERROR: Variable has incomplete type `Incomplete`. +// CHECK:STDERR: fail_b.carbon:[[@LINE+9]]:8: ERROR: Variable has incomplete type `Incomplete`. // CHECK:STDERR: var a: Incomplete; // CHECK:STDERR: ^~~~~~~~~~ -// CHECK:STDERR: fail_b.carbon: Class was forward declared here. +// CHECK:STDERR: fail_b.carbon:[[@LINE-18]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: a.carbon:7:1: Class was forward declared here. +// CHECK:STDERR: class Incomplete; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~ var a: Incomplete; // CHECK:STDOUT: --- a.carbon diff --git a/toolchain/check/testdata/function/definition/import.carbon b/toolchain/check/testdata/function/definition/import.carbon index 28f4261d9c69..e9cfc63ffa91 100644 --- a/toolchain/check/testdata/function/definition/import.carbon +++ b/toolchain/check/testdata/function/definition/import.carbon @@ -37,16 +37,26 @@ library "def_ownership" api; import library "a"; -// CHECK:STDERR: fail_def_ownership.carbon:[[@LINE+5]]:1: ERROR: Only one library can declare function A without `extern`. +// CHECK:STDERR: fail_def_ownership.carbon:[[@LINE+10]]:1: ERROR: Only one library can declare function A without `extern`. // CHECK:STDERR: fn A() {}; // CHECK:STDERR: ^~~~~~~~ -// CHECK:STDERR: fail_def_ownership.carbon: Previously declared here. +// CHECK:STDERR: fail_def_ownership.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: a.carbon:4:1: Previously declared here. +// CHECK:STDERR: fn A() {} +// CHECK:STDERR: ^~~~~~~~ // CHECK:STDERR: fn A() {}; -// CHECK:STDERR: fail_def_ownership.carbon:[[@LINE+5]]:1: ERROR: Only one library can declare function B without `extern`. +// CHECK:STDERR: fail_def_ownership.carbon:[[@LINE+10]]:1: ERROR: Only one library can declare function B without `extern`. // CHECK:STDERR: fn B(b: i32) -> i32; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~ -// CHECK:STDERR: fail_def_ownership.carbon: Previously declared here. +// CHECK:STDERR: fail_def_ownership.carbon:[[@LINE-16]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: a.carbon:5:1: Previously declared here. +// CHECK:STDERR: fn B(b: i32) -> i32 { return b; } +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~ // CHECK:STDERR: fn B(b: i32) -> i32; @@ -57,10 +67,15 @@ library "mix_extern_decl" api; import library "a"; extern fn D(); -// CHECK:STDERR: fail_mix_extern_decl.carbon:[[@LINE+4]]:1: ERROR: Only one library can declare function D without `extern`. +// CHECK:STDERR: fail_mix_extern_decl.carbon:[[@LINE+9]]:1: ERROR: Only one library can declare function D without `extern`. // CHECK:STDERR: fn D() {} // CHECK:STDERR: ^~~~~~~~ -// CHECK:STDERR: fail_mix_extern_decl.carbon: Previously declared here. +// CHECK:STDERR: fail_mix_extern_decl.carbon:[[@LINE-6]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: a.carbon:7:1: Previously declared here. +// CHECK:STDERR: fn D(); +// CHECK:STDERR: ^~~~~~~ fn D() {} // CHECK:STDOUT: --- a.carbon @@ -195,8 +210,8 @@ fn D() {} // CHECK:STDOUT: %import_ref.4 = import_ref ir1, inst+30, unused // CHECK:STDOUT: %A: = fn_decl @A [template] {} // CHECK:STDOUT: %B: = fn_decl @B [template] { -// CHECK:STDOUT: %b.loc17_6.1: i32 = param b -// CHECK:STDOUT: %b.loc17_6.2: i32 = bind_name b, %b.loc17_6.1 +// CHECK:STDOUT: %b.loc27_6.1: i32 = param b +// CHECK:STDOUT: %b.loc27_6.2: i32 = bind_name b, %b.loc27_6.1 // CHECK:STDOUT: %return.var: ref i32 = var // CHECK:STDOUT: } // CHECK:STDOUT: } @@ -222,7 +237,7 @@ fn D() {} // CHECK:STDOUT: %import_ref.3 = import_ref ir1, inst+20, unused // CHECK:STDOUT: %import_ref.4: = import_ref ir1, inst+30, used [template = imports.%D] // CHECK:STDOUT: %D.loc6: = fn_decl @D [template] {} -// CHECK:STDOUT: %D.loc11: = fn_decl @D [template] {} +// CHECK:STDOUT: %D.loc16: = fn_decl @D [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: fn @D() { diff --git a/toolchain/check/testdata/packages/cross_package_import.carbon b/toolchain/check/testdata/packages/cross_package_import.carbon index f0ff127d0f7c..89f806f532f9 100644 --- a/toolchain/check/testdata/packages/cross_package_import.carbon +++ b/toolchain/check/testdata/packages/cross_package_import.carbon @@ -132,10 +132,15 @@ import library "other_ns"; // CHECK:STDERR: import Other library "fn"; -// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE+5]]:1: ERROR: Only one library can declare function F without `extern`. +// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE+10]]:1: ERROR: Only one library can declare function F without `extern`. // CHECK:STDERR: fn Other.F() {} // CHECK:STDERR: ^~~~~~~~~~~~~~ -// CHECK:STDERR: fail_main_namespace_conflict.carbon: Previously declared here. +// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import Other library "fn"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: other_fn.carbon:4:1: Previously declared here. +// CHECK:STDERR: fn F() {} +// CHECK:STDERR: ^~~~~~~~ // CHECK:STDERR: fn Other.F() {} diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index 3ad8f2c46ec3..dcb13b5cfbac 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -390,16 +390,21 @@ class InstStore { return loc_ids_[inst_id.index]; } - // Overwrites a given instruction and location ID with a new value. - auto Set(InstId inst_id, LocationIdAndInst loc_id_and_inst) -> void { - values_.Get(inst_id) = loc_id_and_inst.inst; - loc_ids_[inst_id.index] = loc_id_and_inst.loc_id; - } + // Overwrites a given instruction with a new value. + auto Set(InstId inst_id, Inst inst) -> void { values_.Get(inst_id) = inst; } + // Overwrites a given instruction's location with a new value. auto SetLocationId(InstId inst_id, LocationId loc_id) -> void { loc_ids_[inst_id.index] = loc_id; } + // Overwrites a given instruction and location ID with a new value. + auto SetLocationIdAndInst(InstId inst_id, LocationIdAndInst loc_id_and_inst) + -> void { + Set(inst_id, loc_id_and_inst.inst); + SetLocationId(inst_id, loc_id_and_inst.loc_id); + } + // Reserves space. auto Reserve(size_t size) -> void { loc_ids_.reserve(size);