Fearlessly hold references into ValueStore again (#5589)

Undo changes that were meant to prevent use of a reference into
`ValueStore` after being invalidated. After #5576, the `ValueStore`
makes such references stable, so there's no need to worry about
invalidation.

toolchain/check/convert.cpp

@@ -567,21 +567,13 @@ static auto ConvertStructToClass(
     SemIR::InstId value_id, ConversionTarget target,
     SemIR::InstId dest_vtable_id = SemIR::InstId::None) -> SemIR::InstId {
   PendingBlock target_block(&context);
-  auto object_repr_id = SemIR::TypeId::None;
-
-  {
-    auto& dest_class_info = context.classes().Get(dest_type.class_id);
-    CARBON_CHECK(dest_class_info.inheritance_kind != SemIR::Class::Abstract);
-    if (!dest_vtable_id.has_value()) {
-      dest_vtable_id = dest_class_info.vtable_id;
-    }
-    object_repr_id =
-        dest_class_info.GetObjectRepr(context.sem_ir(), dest_type.specific_id);
-    if (object_repr_id == SemIR::ErrorInst::TypeId) {
-      return SemIR::ErrorInst::InstId;
-    }
+  auto& dest_class_info = context.classes().Get(dest_type.class_id);
+  CARBON_CHECK(dest_class_info.inheritance_kind != SemIR::Class::Abstract);
+  auto object_repr_id =
+      dest_class_info.GetObjectRepr(context.sem_ir(), dest_type.specific_id);
+  if (object_repr_id == SemIR::ErrorInst::TypeId) {
+    return SemIR::ErrorInst::InstId;
   }
-
   auto dest_struct_type =
       context.types().GetAs<SemIR::StructType>(object_repr_id);
 
@@ -596,7 +588,8 @@ static auto ConvertStructToClass(
   }
 
   auto result_id = ConvertStructToStructOrClass<SemIR::ClassElementAccess>(
-      context, src_type, dest_struct_type, value_id, target, dest_vtable_id);
+      context, src_type, dest_struct_type, value_id, target,
+      dest_vtable_id.has_value() ? dest_vtable_id : dest_class_info.vtable_id);
 
   if (need_temporary) {
     target_block.InsertHere();
@@ -1461,9 +1454,6 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id,
                      const SemIR::Function& callee,
                      SemIR::SpecificId callee_specific_id)
     -> SemIR::InstBlockId {
-  // The callee reference can be invalidated by conversions, so ensure all reads
-  // from it are done before conversion calls.
-  auto callee_decl_id = callee.latest_decl_id();
   auto param_patterns =
       context.inst_blocks().GetOrEmpty(callee.param_patterns_id);
   auto return_slot_pattern_id = callee.return_slot_pattern_id;
@@ -1477,7 +1467,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id,
     CARBON_DIAGNOSTIC(InCallToFunction, Note, "calling function declared here");
     context.emitter()
         .Build(call_loc_id, MissingObjectInMethodCall)
-        .Note(callee_decl_id, InCallToFunction)
+        .Note(callee.latest_decl_id(), InCallToFunction)
         .Emit();
     self_id = SemIR::ErrorInst::InstId;
   }

toolchain/check/deduce.cpp

@@ -602,8 +602,8 @@ auto DeduceGenericCallArguments(
   return deduction.MakeSpecific();
 }
 
-auto DeduceImplArguments(Context& context, SemIR::LocId loc_id, DeduceImpl impl,
-                         SemIR::ConstantId self_id,
+auto DeduceImplArguments(Context& context, SemIR::LocId loc_id,
+                         const SemIR::Impl& impl, SemIR::ConstantId self_id,
                          SemIR::SpecificId constraint_specific_id)
     -> SemIR::SpecificId {
   DeductionContext deduction(&context, loc_id, impl.generic_id,
@@ -613,7 +613,7 @@ auto DeduceImplArguments(Context& context, SemIR::LocId loc_id, DeduceImpl impl,
 
   // Prepare to perform deduction of the type and interface.
   deduction.Add(impl.self_id, context.constant_values().GetInstId(self_id));
-  deduction.Add(impl.specific_id, constraint_specific_id);
+  deduction.Add(impl.interface.specific_id, constraint_specific_id);
 
   if (!deduction.Deduce() || !deduction.CheckDeductionIsComplete()) {
     return SemIR::SpecificId::None;

toolchain/check/deduce.h

@@ -18,21 +18,10 @@ auto DeduceGenericCallArguments(
     SemIR::InstBlockId param_patterns_id, SemIR::InstId self_id,
     llvm::ArrayRef<SemIR::InstId> arg_ids) -> SemIR::SpecificId;
 
-// Data from the `Impl` that is used by deduce.
-//
-// We don't use a reference to an `Impl` as deduction can invalidate the
-// reference by causing impl declarations to be imported from `Core` during
-// conversion.
-struct DeduceImpl {
-  SemIR::InstId self_id;
-  SemIR::GenericId generic_id;
-  SemIR::SpecificId specific_id;
-};
-
 // Deduces the impl arguments to use in a use of a parameterized impl. Returns
 // `None` if deduction fails.
-auto DeduceImplArguments(Context& context, SemIR::LocId loc_id, DeduceImpl impl,
-                         SemIR::ConstantId self_id,
+auto DeduceImplArguments(Context& context, SemIR::LocId loc_id,
+                         const SemIR::Impl& impl, SemIR::ConstantId self_id,
                          SemIR::SpecificId constraint_specific_id)
     -> SemIR::SpecificId;
 

toolchain/check/handle_choice.cpp

@@ -283,12 +283,8 @@ auto HandleParseNode(Context& context, Parse::ChoiceDefinitionId node_id)
           .type_id = GetSingletonType(context, SemIR::WitnessType::TypeInstId),
           .object_repr_type_inst_id =
               context.types().GetInstId(GetStructType(context, fields_id))});
-  // Note: avoid storing a reference to the returned Class, since it may be
-  // invalidated by other type constructions.
-  context.classes().Get(class_id).complete_type_witness_id = choice_witness_id;
-
-  auto self_type_id = context.classes().Get(class_id).self_type_id;
-  auto name_scope_id = context.classes().Get(class_id).scope_id;
+  auto& class_info = context.classes().Get(class_id);
+  class_info.complete_type_witness_id = choice_witness_id;
 
   auto self_struct_type_id = GetStructType(
       context, context.struct_type_fields().AddCanonical(struct_type_fields));
@@ -296,8 +292,8 @@ auto HandleParseNode(Context& context, Parse::ChoiceDefinitionId node_id)
   for (auto [i, deferred_binding] :
        llvm::enumerate(context.choice_deferred_bindings())) {
     MakeLetBinding(context,
-                   ChoiceInfo{.self_type_id = self_type_id,
-                              .name_scope_id = name_scope_id,
+                   ChoiceInfo{.self_type_id = class_info.self_type_id,
+                              .name_scope_id = class_info.scope_id,
                               .self_struct_type_id = self_struct_type_id,
                               .discriminant_type_id = discriminant_type_id,
                               .num_alternative_bits = num_alternative_bits},
@@ -310,7 +306,7 @@ auto HandleParseNode(Context& context, Parse::ChoiceDefinitionId node_id)
   context.scope_stack().Pop();
   context.decl_name_stack().PopScope();
 
-  FinishGenericDefinition(context, context.classes().Get(class_id).generic_id);
+  FinishGenericDefinition(context, class_info.generic_id);
 
   context.choice_deferred_bindings().clear();
   return true;

toolchain/check/handle_class.cpp

@@ -366,9 +366,10 @@ auto HandleParseNode(Context& context, Parse::AdaptDeclId node_id) -> bool {
     return true;
   }
 
-  auto adapt_id = context.classes().Get(parent_class_decl->class_id).adapt_id;
-  if (adapt_id.has_value()) {
-    DiagnoseClassSpecificDeclRepeated(context, node_id, SemIR::LocId(adapt_id),
+  auto& class_info = context.classes().Get(parent_class_decl->class_id);
+  if (class_info.adapt_id.has_value()) {
+    DiagnoseClassSpecificDeclRepeated(context, node_id,
+                                      SemIR::LocId(class_info.adapt_id),
                                       Lex::TokenKind::Adapt);
     return true;
   }
@@ -395,10 +396,8 @@ auto HandleParseNode(Context& context, Parse::AdaptDeclId node_id) -> bool {
   }
 
   // Build a SemIR representation for the declaration.
-  adapt_id = AddInst<SemIR::AdaptDecl>(
+  class_info.adapt_id = AddInst<SemIR::AdaptDecl>(
       context, node_id, {.adapted_type_inst_id = adapted_type_inst_id});
-  auto& class_info = context.classes().Get(parent_class_decl->class_id);
-  class_info.adapt_id = adapt_id;
 
   // Extend the class scope with the adapted type's scope if requested.
   if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
@@ -502,9 +501,10 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
     return true;
   }
 
-  auto base_id = context.classes().Get(parent_class_decl->class_id).base_id;
-  if (base_id.has_value()) {
-    DiagnoseClassSpecificDeclRepeated(context, node_id, SemIR::LocId(base_id),
+  auto& class_info = context.classes().Get(parent_class_decl->class_id);
+  if (class_info.base_id.has_value()) {
+    DiagnoseClassSpecificDeclRepeated(context, node_id,
+                                      SemIR::LocId(class_info.base_id),
                                       Lex::TokenKind::Base);
     return true;
   }
@@ -525,16 +525,13 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
   // The `base` value in the class scope has an unbound element type. Instance
   // binding will be performed when it's found by name lookup into an instance.
   auto field_type_id = GetUnboundElementType(
-      context,
-      context.types().GetInstId(
-          context.classes().Get(parent_class_decl->class_id).self_type_id),
+      context, context.types().GetInstId(class_info.self_type_id),
       base_info.inst_id);
-  base_id = AddInst<SemIR::BaseDecl>(context, node_id,
-                                     {.type_id = field_type_id,
-                                      .base_type_inst_id = base_info.inst_id,
-                                      .index = SemIR::ElementIndex::None});
-  auto& class_info = context.classes().Get(parent_class_decl->class_id);
-  class_info.base_id = base_id;
+  class_info.base_id =
+      AddInst<SemIR::BaseDecl>(context, node_id,
+                               {.type_id = field_type_id,
+                                .base_type_inst_id = base_info.inst_id,
+                                .index = SemIR::ElementIndex::None});
 
   if (base_info.type_id != SemIR::ErrorInst::TypeId) {
     auto base_class_info = context.classes().Get(

toolchain/check/handle_impl.cpp

@@ -352,30 +352,22 @@ static auto DiagnoseUnusedGenericBinding(Context& context,
                                          SemIR::ImplId impl_id) -> void {
   auto deduced_specific_id = SemIR::SpecificId::None;
 
-  {
-    auto& stored_impl_info = context.impls().Get(impl_id);
-    if (!stored_impl_info.generic_id.has_value() ||
-        stored_impl_info.witness_id == SemIR::ErrorInst::InstId) {
-      return;
-    }
-
-    // TODO: Deduce has side effects in the semir by generating `Converted`
-    // instructions which we will not use here. We should stop generating
-    // those when deducing for impl lookup, but for now we discard them by
-    // pushing an InstBlock on the stack and dropping it right after.
-    context.inst_block_stack().Push();
-    // Deduction can invalidate references to ValueStores; we can't use
-    // `stored_impl_info` after.
-    deduced_specific_id = DeduceImplArguments(
-        context, node_id,
-        DeduceImpl{.self_id = stored_impl_info.self_id,
-                   .generic_id = stored_impl_info.generic_id,
-                   .specific_id = stored_impl_info.interface.specific_id},
-        context.constant_values().Get(stored_impl_info.self_id),
-        stored_impl_info.interface.specific_id);
-    context.inst_block_stack().PopAndDiscard();
+  auto& impl = context.impls().Get(impl_id);
+  if (!impl.generic_id.has_value() ||
+      impl.witness_id == SemIR::ErrorInst::InstId) {
+    return;
   }
 
+  // TODO: Deduce has side effects in the semir by generating `Converted`
+  // instructions which we will not use here. We should stop generating
+  // those when deducing for impl lookup, but for now we discard them by
+  // pushing an InstBlock on the stack and dropping it right after.
+  context.inst_block_stack().Push();
+  deduced_specific_id = DeduceImplArguments(
+      context, node_id, impl, context.constant_values().Get(impl.self_id),
+      impl.interface.specific_id);
+  context.inst_block_stack().PopAndDiscard();
+
   if (deduced_specific_id.has_value()) {
     // Deduction succeeded, all bindings were used.
     return;

toolchain/check/impl.cpp

@@ -164,9 +164,7 @@ auto ImplWitnessStartDefinition(Context& context, SemIR::Impl& impl) -> void {
 // Adds functions to the witness that the specified impl implements the given
 // interface.
 auto FinishImplWitness(Context& context, SemIR::ImplId impl_id) -> void {
-  // Make a copy of the impl. We're going to reference it a lot, and `impl`s
-  // could get invalidated by some of the things we do.
-  const auto impl = context.impls().Get(impl_id);
+  const auto& impl = context.impls().Get(impl_id);
 
   CARBON_CHECK(impl.is_being_defined());
   CARBON_CHECK(impl.witness_id.has_value());

toolchain/check/impl_lookup.cpp

@@ -206,31 +206,20 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
                                 SemIR::ConstantId query_self_const_id,
                                 const SemIR::SpecificInterface& interface,
                                 SemIR::ImplId impl_id) -> EvalImplLookupResult {
+  const SemIR::Impl& impl = context.impls().Get(impl_id);
+
   // The impl may have generic arguments, in which case we need to deduce them
   // to find what they are given the specific type and interface query. We use
   // that specific to map values in the impl to the deduced values.
   auto specific_id = SemIR::SpecificId::None;
-  {
-    // DeduceImplArguments can import new impls which can invalidate any
-    // pointers into `context.impls()`.
-    const SemIR::Impl& impl = context.impls().Get(impl_id);
-
-    if (impl.generic_id.has_value()) {
-      specific_id =
-          DeduceImplArguments(context, loc_id,
-                              {.self_id = impl.self_id,
-                               .generic_id = impl.generic_id,
-                               .specific_id = impl.interface.specific_id},
-                              query_self_const_id, interface.specific_id);
-      if (!specific_id.has_value()) {
-        return EvalImplLookupResult::MakeNone();
-      }
+  if (impl.generic_id.has_value()) {
+    specific_id = DeduceImplArguments(
+        context, loc_id, impl, query_self_const_id, interface.specific_id);
+    if (!specific_id.has_value()) {
+      return EvalImplLookupResult::MakeNone();
     }
   }
 
-  // Get a pointer again after DeduceImplArguments() is complete.
-  const SemIR::Impl& impl = context.impls().Get(impl_id);
-
   // The self type of the impl must match the type in the query, or this is an
   // `impl T as ...` for some other type `T` and should not be considered.
   auto deduced_self_const_id = SemIR::GetConstantValueInSpecific(
@@ -279,20 +268,14 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
     return EvalImplLookupResult::MakeNone();
   }
 
-  bool is_effectively_final = query_is_concrete || impl.is_final;
-  auto witness_id = impl.witness_id;
-
-  // Note that this invalidates our `impl` reference. Don't use it again after
-  // this point.
-  LoadImportRef(context, witness_id);
-
+  LoadImportRef(context, impl.witness_id);
   if (specific_id.has_value()) {
     // We need a definition of the specific `impl` so we can access its
     // witness.
     ResolveSpecificDefinition(context, loc_id, specific_id);
   }
 
-  if (is_effectively_final) {
+  if (query_is_concrete || impl.is_final) {
     // TODO: These final results should be cached somehow. Positive (non-None)
     // results could be cached globally, as they can not change. But
     // negative results can change after a final impl is written, so
@@ -300,7 +283,7 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
     // be invalidated by writing a final impl that would match.
     return EvalImplLookupResult::MakeFinal(
         context.constant_values().GetInstId(SemIR::GetConstantValueInSpecific(
-            context.sem_ir(), specific_id, witness_id)));
+            context.sem_ir(), specific_id, impl.witness_id)));
   } else {
     return EvalImplLookupResult::MakeNonFinal();
   }
@@ -583,11 +566,6 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
                                  SemIR::InstId non_canonical_query_self_inst_id,
                                  bool poison_concrete_results)
     -> EvalImplLookupResult {
-  // NOTE: Do not retain this reference to the SpecificInterface obtained from a
-  // value store by SpecificInterfaceId. Doing impl lookup does deduce which can
-  // do more impl lookups, and impl lookup can add a new SpecificInterface to
-  // the store which can reallocate and invalidate any references held here into
-  // the store.
   auto query_specific_interface =
       context.specific_interfaces().Get(eval_query.query_specific_interface_id);
 
@@ -656,8 +634,6 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
       context.impl_lookup_stack().back().impl_loc = candidate.loc_inst_id;
     }
 
-    // NOTE: GetWitnessIdForImpl() does deduction, which can cause new impls
-    // to be imported, invalidating any pointer into `context.impls()`.
     auto result = GetWitnessIdForImpl(
         context, loc_id, query_is_concrete, query_self_const_id,
         query_specific_interface, candidate.impl_id);

toolchain/check/impl_validation.cpp

@@ -22,15 +22,9 @@ namespace Carbon::Check {
 
 namespace {
 
-struct ImplAndInterface {
-  SemIR::ImplId impl_id;
-  SemIR::SpecificInterface interface;
-};
-
-// We avoid holding a reference into the ImplStore, as the act of verifying the
-// diagnostic checks can invalidate such a reference. We collect what we need to
-// know about the the `Impl` for validation into this struct.
+// All information about a `SemIR::Impl` needed for validation.
 struct ImplInfo {
+  SemIR::ImplId impl_id;
   bool is_final;
   SemIR::InstId witness_id;
   SemIR::TypeInstId self_id;
@@ -56,7 +50,8 @@ static auto GetIRId(Context& context, SemIR::InstId owning_inst_id)
 auto GetImplInfo(Context& context, SemIR::ImplId impl_id) -> ImplInfo {
   const auto& impl = context.impls().Get(impl_id);
   auto ir_id = GetIRId(context, impl.first_owning_decl_id);
-  return {.is_final = impl.is_final,
+  return {.impl_id = impl_id,
+          .is_final = impl.is_final,
           .witness_id = impl.witness_id,
           .self_id = impl.self_id,
           .latest_decl_id = impl.latest_decl_id(),
@@ -146,15 +141,15 @@ static auto DiagnoseNonFinalImplsWithSameTypeStructure(Context& context,
 // final impl.
 //
 // Returns true if an error was diagnosed.
-static auto DiagnoseUnmatchableNonFinalImplWithFinalImpl(
-    Context& context, SemIR::ImplId impl_a_id, const ImplInfo& impl_a,
-    SemIR::ImplId impl_b_id, const ImplInfo& impl_b) -> bool {
+static auto DiagnoseUnmatchableNonFinalImplWithFinalImpl(Context& context,
+                                                         const ImplInfo& impl_a,
+                                                         const ImplInfo& impl_b)
+    -> bool {
   auto diagnose_unmatchable_impl = [&](const ImplInfo& query_impl,
-                                       SemIR::ImplId final_impl_id,
                                        const ImplInfo& final_impl) -> bool {
     if (LookupMatchesImpl(context, SemIR::LocId(query_impl.latest_decl_id),
                           context.constant_values().Get(query_impl.self_id),
-                          query_impl.interface, final_impl_id)) {
+                          query_impl.interface, final_impl.impl_id)) {
       CARBON_DIAGNOSTIC(ImplFinalOverlapsNonFinal, Error,
                         "`impl` will never be used");
       auto builder = context.emitter().Build(query_impl.latest_decl_id,
@@ -172,9 +167,9 @@ static auto DiagnoseUnmatchableNonFinalImplWithFinalImpl(
   CARBON_CHECK(impl_a.is_final || impl_b.is_final);
 
   if (impl_b.is_final) {
-    return diagnose_unmatchable_impl(impl_a, impl_b_id, impl_b);
+    return diagnose_unmatchable_impl(impl_a, impl_b);
   } else {
-    return diagnose_unmatchable_impl(impl_b, impl_a_id, impl_a);
+    return diagnose_unmatchable_impl(impl_b, impl_a);
   }
 }
 
@@ -236,27 +231,17 @@ static auto DiagnoseFinalImplsOverlapOutsideMatchFirst(Context& context,
   return false;
 }
 
-static auto ValidateImplsForInterface(
-    Context& context, llvm::ArrayRef<ImplAndInterface> impls_and_interface)
-    -> void {
-  // Range over `SemIR::ImplId` only. Caller has ensured all of these are
-  // for the same interface.
-  auto impl_ids = llvm::map_range(impls_and_interface,
-                                  [=](ImplAndInterface impl_and_interface) {
-                                    return impl_and_interface.impl_id;
-                                  });
-
+static auto ValidateImplsForInterface(Context& context,
+                                      llvm::ArrayRef<ImplInfo> impls) -> void {
   // All `impl`s we look at here have the same `InterfaceId` (though different
   // `SpecificId`s in their `SpecificInterface`s). So we can grab the
   // `ImportIRId` for the interface a single time up front.
-  auto interface_decl_id =
-      context.interfaces()
-          .Get(impls_and_interface[0].interface.interface_id)
-          .first_owning_decl_id;
+  auto interface_decl_id = context.interfaces()
+                               .Get(impls[0].interface.interface_id)
+                               .first_owning_decl_id;
   auto interface_ir_id = GetIRId(context, interface_decl_id);
 
-  for (auto impl_id : impl_ids) {
-    auto impl = GetImplInfo(context, impl_id);
+  for (const auto& impl : impls) {
     if (impl.is_final && impl.is_local) {
       // =======================================================================
       /// Rules for an individual final impl.
@@ -273,11 +258,8 @@ static auto ValidateImplsForInterface(
   // For each impl, we compare it pair-wise which each impl found before it, so
   // that diagnostics are attached to the later impl, as the earlier impl on its
   // own does not generate a diagnostic.
-  size_t num_impl_ids = impls_and_interface.size();
-  for (auto [split_point, impl_b_id] :
-       llvm::drop_begin(llvm::enumerate(impl_ids))) {
-    auto impl_b = GetImplInfo(context, impl_b_id);
-
+  size_t num_impls = impls.size();
+  for (auto [split_point, impl_b] : llvm::drop_begin(llvm::enumerate(impls))) {
     // Prevent diagnosing the same error multiple times for the same `impl_b`
     // against different impls before it. But still ensure we do give one of
     // each diagnostic when they are different errors.
@@ -286,10 +268,8 @@ static auto ValidateImplsForInterface(
     bool did_diagnose_final_impls_overlap_in_different_files = false;
     bool did_diagnose_final_impls_overlap_outside_match_first = false;
 
-    auto impls_before = llvm::drop_end(impl_ids, num_impl_ids - split_point);
-    for (auto impl_a_id : impls_before) {
-      auto impl_a = GetImplInfo(context, impl_a_id);
-
+    auto impls_before = llvm::drop_end(impls, num_impls - split_point);
+    for (const auto& impl_a : impls_before) {
       // Only enforce rules when at least one of the impls was written in this
       // file.
       if (!impl_a.is_local && !impl_b.is_local) {
@@ -318,8 +298,8 @@ static auto ValidateImplsForInterface(
         // Rules between final impl and non-final impl.
         // =====================================================================
         if (!did_diagnose_unmatchable_non_final_impl_with_final_impl) {
-          if (DiagnoseUnmatchableNonFinalImplWithFinalImpl(
-                  context, impl_a_id, impl_a, impl_b_id, impl_b)) {
+          if (DiagnoseUnmatchableNonFinalImplWithFinalImpl(context, impl_a,
+                                                           impl_b)) {
             did_diagnose_unmatchable_non_final_impl_with_final_impl = true;
           }
         }
@@ -432,23 +412,18 @@ auto ValidateImplsInFile(Context& context) -> void {
   // about them anyhow. We also verify the impl has an `InterfaceId` since it
   // can be missing, in which case a diagnostic would have been generated
   // already as well.
-  //
-  // Don't hold Impl pointers here because the process of looking for
-  // diagnostics may cause imports and may invalidate pointers into the
-  // ImplStore.
-  llvm::SmallVector<ImplAndInterface> impl_ids_by_interface(llvm::map_range(
+  llvm::SmallVector<ImplInfo> impl_ids_by_interface(llvm::map_range(
       llvm::make_filter_range(
           context.impls().enumerate(),
           [](std::pair<SemIR::ImplId, const SemIR::Impl&> pair) {
             return pair.second.witness_id != SemIR::ErrorInst::InstId &&
                    pair.second.interface.interface_id.has_value();
           }),
-      [](std::pair<SemIR::ImplId, const SemIR::Impl&> pair) {
-        return ImplAndInterface{.impl_id = pair.first,
-                                .interface = pair.second.interface};
+      [&](std::pair<SemIR::ImplId, const SemIR::Impl&> pair) {
+        return GetImplInfo(context, pair.first);
       }));
-  llvm::stable_sort(impl_ids_by_interface, [](const ImplAndInterface& lhs,
-                                              const ImplAndInterface& rhs) {
+  llvm::stable_sort(impl_ids_by_interface, [](const ImplInfo& lhs,
+                                              const ImplInfo& rhs) {
     return lhs.interface.interface_id.index < rhs.interface.interface_id.index;
   });
 

toolchain/check/import.cpp

@@ -152,13 +152,13 @@ auto AddImportNamespaceToScope(
     SemIR::NameScopeId parent_scope_id, bool diagnose_duplicate_namespace,
     llvm::function_ref<SemIR::InstId()> make_import_id)
     -> AddImportNamespaceToScopeResult {
-  auto* parent_scope = &context.name_scopes().Get(parent_scope_id);
-  auto [inserted, entry_id] = parent_scope->LookupOrAdd(
+  auto& parent_scope = context.name_scopes().Get(parent_scope_id);
+  auto [inserted, entry_id] = parent_scope.LookupOrAdd(
       name_id,
       // This InstId is temporary and would be overridden if used.
       SemIR::InstId::None, SemIR::AccessKind::Public);
   if (!inserted) {
-    const auto& prev_entry = parent_scope->GetEntry(entry_id);
+    const auto& prev_entry = parent_scope.GetEntry(entry_id);
     if (!prev_entry.result.is_poisoned()) {
       auto prev_inst_id = prev_entry.result.target_inst_id();
       if (auto namespace_inst =
@@ -186,14 +186,11 @@ auto AddImportNamespaceToScope(
                                        parent_scope_id, import_id),
       .is_duplicate_of_namespace_in_current_package = false};
 
-  // Note we have to get the parent scope freshly, creating the imported
-  // namespace may invalidate the pointer above.
-  parent_scope = &context.name_scopes().Get(parent_scope_id);
   // Diagnose if there's a name conflict, but still produce the namespace to
   // supersede the name conflict in order to avoid repeat diagnostics. Names
   // are poisoned optimistically by name lookup before checking for imports,
   // so we may be overwriting a poisoned entry here.
-  auto& lookup_result = parent_scope->GetEntry(entry_id).result;
+  auto& lookup_result = parent_scope.GetEntry(entry_id).result;
   if (!lookup_result.is_poisoned() && !inserted) {
     // TODO: Pass the import namespace name location instead of the namespace
     // id to get more accurate location.

toolchain/check/name_lookup.cpp

@@ -192,9 +192,7 @@ auto LookupNameInExactScope(Context& context, SemIR::LocId loc_id,
     if (imported_inst_id.has_value()) {
       SemIR::ScopeLookupResult result = SemIR::ScopeLookupResult::MakeFound(
           imported_inst_id, SemIR::AccessKind::Public);
-      // `ImportNameFromCpp()` can invalidate `scope`, so we do a scope lookup.
-      context.name_scopes().Get(scope_id).AddRequired(
-          {.name_id = name_id, .result = result});
+      scope.AddRequired({.name_id = name_id, .result = result});
       return result;
     }
   }

toolchain/check/return.cpp

@@ -159,7 +159,6 @@ auto BuildReturnWithExpr(Context& context, SemIR::LocId loc_id,
   } else if (return_info.has_return_slot()) {
     return_slot_id = GetCurrentReturnSlot(context);
     CARBON_CHECK(return_slot_id.has_value());
-    // Note that this can import a function and invalidate `function`.
     expr_id = Initialize(context, loc_id, return_slot_id, expr_id);
   } else {
     expr_id =

toolchain/check/thunk.cpp

@@ -161,18 +161,16 @@ static auto CloneFunctionDecl(Context& context, SemIR::LocId loc_id,
     -> std::pair<SemIR::FunctionId, SemIR::InstId> {
   StartGenericDecl(context);
 
-  // Clone the signature. Note that we re-get the function after each of these,
-  // because they might trigger imports that invalidate the function.
+  const auto& signature = context.functions().Get(signature_id);
+
+  // Clone the signature.
   context.pattern_block_stack().Push();
   auto implicit_param_patterns_id = ClonePatternBlock(
-      context, signature_specific_id,
-      context.functions().Get(signature_id).implicit_param_patterns_id);
-  auto param_patterns_id = ClonePatternBlock(
-      context, signature_specific_id,
-      context.functions().Get(signature_id).param_patterns_id);
-  auto return_slot_pattern_id = ClonePattern(
-      context, signature_specific_id,
-      context.functions().Get(signature_id).return_slot_pattern_id);
+      context, signature_specific_id, signature.implicit_param_patterns_id);
+  auto param_patterns_id = ClonePatternBlock(context, signature_specific_id,
+                                             signature.param_patterns_id);
+  auto return_slot_pattern_id = ClonePattern(context, signature_specific_id,
+                                             signature.return_slot_pattern_id);
   auto self_param_id = FindSelfPattern(context, implicit_param_patterns_id);
   auto pattern_block_id = context.pattern_block_stack().Pop();
 
@@ -191,7 +189,6 @@ static auto CloneFunctionDecl(Context& context, SemIR::LocId loc_id,
   auto generic_id = BuildGenericDecl(context, decl_id);
 
   // Create the `Function` object.
-  auto& signature = context.functions().Get(signature_id);
   auto& callee = context.functions().Get(callee_id);
   function_decl.function_id = context.functions().Add(SemIR::Function{
       {.name_id = signature.name_id,

toolchain/sem_ir/name_scope.h

@@ -151,9 +151,6 @@ class NameScope : public Printable<NameScope> {
   auto entries() const -> llvm::ArrayRef<Entry> { return names_; }
 
   // Get a specific Name entry based on an EntryId that return from a lookup.
-  //
-  // The Entry could become invalidated if the scope object is invalidated or if
-  // a name is added.
   auto GetEntry(EntryId entry_id) const -> const Entry& {
     return names_[entry_id.index];
   }
@@ -258,9 +255,6 @@ class NameScope : public Printable<NameScope> {
  private:
   // Names in the scope, including poisoned names.
   //
-  // Entries could become invalidated if the scope object is invalidated or if a
-  // name is added.
-  //
   // We store both an insertion-ordered vector for iterating
   // and a map from `NameId` to the index of that vector for name lookup.
   //