Fearlessly hold references into ValueStore again (carbon-language#5589)

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

Author: danakj

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,21 +283,17 @@ 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));
 
   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,28 +268,22 @@ 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
     // they can only be cached in a limited way, or the cache needs to
     // 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);