Fix handling of compatible conversions in initialization. (#6797)

Stop using "performed builtin conversion" as a proxy for whether we
created an initializing expression with a correctly-set storage
argument. That isn't correct in the case where the builtin conversion
creates a new initializing expression without setting its storage, such
as by creating an `AsCompatible` wrapper around an existing initializing
expression.

Instead look at whether the storage argument is a `TemporaryStorage`,
and only overwrite in that case, otherwise assuming that the storage
argument has been set correctly.

This fixes a miscompile that was already visible in our lowering tests!

toolchain/check/convert.cpp

@@ -43,28 +43,28 @@
 
 namespace Carbon::Check {
 
-// Overwrites the contents of the storage arg of the initializing expression
-// `init_id` with the inst at `target.storage_id`, and returns the ID that
-// should now be used to refer to `init_id`'s storage. Has no effect and returns
-// `target.storage_id` unchanged if `target.storage_id` is None or `init_id`
-// doesn't have a storage arg.
-static auto OverwriteStorageArg(SemIR::File& sem_ir, SemIR::InstId init_id,
-                                const ConversionTarget& target)
+// If the initializing expression `init_id` has a storage argument that refers
+// to a temporary, overwrites it with the inst at `target.storage_id`, and
+// returns the ID that should now be used to refer to `init_id`'s storage. Has
+// no effect and returns `target.storage_id` unchanged if `target.storage_id` is
+// None, if `init_id` doesn't have a storage arg, or if the storage argument
+// doesn't point to a temporary. In the latter case, we assume it was set
+// correctly when the instruction was created.
+static auto OverwriteTemporaryStorageArg(SemIR::File& sem_ir,
+                                         SemIR::InstId init_id,
+                                         const ConversionTarget& target)
     -> SemIR::InstId {
   CARBON_CHECK(target.is_initializer());
   if (!target.storage_id.has_value()) {
     return SemIR::InstId::None;
   }
   auto storage_arg_id = FindStorageArgForInitializer(sem_ir, init_id);
-  if (!storage_arg_id.has_value()) {
+  if (!storage_arg_id.has_value() || storage_arg_id == target.storage_id ||
+      !sem_ir.insts().Is<SemIR::TemporaryStorage>(storage_arg_id)) {
     return target.storage_id;
   }
-  // Replace the temporary in the return slot with a reference to our target.
-  CARBON_CHECK(sem_ir.insts().Get(storage_arg_id).kind() ==
-                   SemIR::TemporaryStorage::Kind,
-               "Return slot for initializer does not contain a temporary; "
-               "initialized multiple times? Have {0}",
-               sem_ir.insts().Get(storage_arg_id));
+  // Replace the temporary in the storage argument with a reference to our
+  // target.
   return target.storage_access_block->MergeReplacing(storage_arg_id,
                                                      target.storage_id);
 }
@@ -1454,15 +1454,13 @@ auto PerformAction(Context& context, SemIR::LocId loc_id,
 class CategoryConverter {
  public:
   // Constructs a converter which converts an expression at the given location
-  // to the given conversion target. performed_builtin_conversion indicates
-  // whether builtin conversions were performed prior to this.
+  // to the given conversion target.
   CategoryConverter(Context& context, SemIR::LocId loc_id,
-                    ConversionTarget& target, bool performed_builtin_conversion)
+                    ConversionTarget& target)
       : context_(context),
         sem_ir_(context.sem_ir()),
         loc_id_(loc_id),
-        target_(target),
-        performed_builtin_conversion_(performed_builtin_conversion) {}
+        target_(target) {}
 
   // Converts expr_id to the target specified in the constructor, and returns
   // the converted inst.
@@ -1515,7 +1513,6 @@ class CategoryConverter {
   SemIR::File& sem_ir_;
   SemIR::LocId loc_id_;
   const ConversionTarget& target_;
-  bool performed_builtin_conversion_;
 };
 
 auto CategoryConverter::DoStep(const SemIR::InstId expr_id,
@@ -1543,16 +1540,14 @@ auto CategoryConverter::DoStep(const SemIR::InstId expr_id,
     case SemIR::ExprCategory::ReprInitializing:
       if (target_.is_initializer()) {
         // Overwrite the initializer's storage argument with the inst currently
-        // at target_.storage_id, if both are present. However, we skip this
-        // in certain cases:
-        // - If we created the expression through a builtin conversion, we
-        //   will have created it with the target already set.
-        // - If the type is a C++ enum, we don't actually have an initializing
-        //   expression, we're just pretending we do.
+        // at target_.storage_id, if both are present and the storage argument
+        // hasn't already been set. However, we skip this if the type is a C++
+        // enum: in that case, we don't actually have an initializing
+        // expression, we're just pretending we do.
         auto new_storage_id = target_.storage_id;
-        if (!performed_builtin_conversion_ &&
-            !IsCppEnum(context_, target_.type_id)) {
-          new_storage_id = OverwriteStorageArg(sem_ir_, expr_id, target_);
+        if (!IsCppEnum(context_, target_.type_id)) {
+          new_storage_id =
+              OverwriteTemporaryStorageArg(sem_ir_, expr_id, target_);
         }
 
         // If in-place initialization was requested, and it hasn't already
@@ -1802,7 +1797,6 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
   if (expr_id == SemIR::ErrorInst::InstId) {
     return expr_id;
   }
-  bool performed_builtin_conversion = expr_id != orig_expr_id;
 
   // Defer the action if it's dependent. We do this now rather than before
   // attempting any conversion so that we can still perform builtin conversions
@@ -1888,9 +1882,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
   }
 
   // Now perform any necessary value category conversions.
-  expr_id =
-      CategoryConverter(context, loc_id, target, performed_builtin_conversion)
-          .Convert(expr_id);
+  expr_id = CategoryConverter(context, loc_id, target).Convert(expr_id);
 
   return expr_id;
 }

toolchain/check/pending_block.h

@@ -77,6 +77,8 @@ class PendingBlock {
   // block, but that would be costlier to enforce.
   auto MergeReplacing(SemIR::InstId target_id, SemIR::InstId value_id)
       -> SemIR::InstId {
+    CARBON_CHECK(target_id != value_id);
+
     // TODO: consider adding an end-of-phase check that the SemIR::File is in
     // SSA form, and dropping this check and the ordering preconditions here and
     // on Initialize.

toolchain/lower/testdata/primitives/optional.carbon

@@ -110,9 +110,7 @@ fn AddOrRemoveConst(a: i32, b: const i32) {
 // CHECK:STDOUT:
 // CHECK:STDOUT: ; Function Attrs: nounwind
 // CHECK:STDOUT: define linkonce_odr void @"_CConvert.a44fce96e16342e7:ImplicitAs.ad22d1bbc0605210.Core.bd3a79eb4ffd3025"(ptr sret({ i32, i1 }) %return, i32 %self) #0 !dbg !56 {
-// CHECK:STDOUT:   %temp = alloca { i32, i1 }, align 8, !dbg !60
-// CHECK:STDOUT:   call void @llvm.lifetime.start.p0(ptr %temp), !dbg !60
-// CHECK:STDOUT:   call void @"_CConvert.e5cf8fcbb4feaae2:ImplicitAs.0f95c9e18c91e00a.Core.622e77b643a0d3a8"(ptr %temp, i32 %self), !dbg !60
+// CHECK:STDOUT:   call void @"_CConvert.e5cf8fcbb4feaae2:ImplicitAs.0f95c9e18c91e00a.Core.622e77b643a0d3a8"(ptr %return, i32 %self), !dbg !60
 // CHECK:STDOUT:   ret void, !dbg !61
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -124,9 +122,7 @@ fn AddOrRemoveConst(a: i32, b: const i32) {
 // CHECK:STDOUT:
 // CHECK:STDOUT: ; Function Attrs: nounwind
 // CHECK:STDOUT: define linkonce_odr void @"_CConvert.a44fce96e16342e7:ImplicitAs.ad22d1bbc0605210.Core.b37806f50d6ac6f2"(ptr sret({ i32, i1 }) %return, i32 %self) #0 !dbg !67 {
-// CHECK:STDOUT:   %temp = alloca { i32, i1 }, align 8, !dbg !70
-// CHECK:STDOUT:   call void @llvm.lifetime.start.p0(ptr %temp), !dbg !70
-// CHECK:STDOUT:   call void @"_CConvert.e5cf8fcbb4feaae2:ImplicitAs.0f95c9e18c91e00a.Core.a7b795aef3d7a0e5"(ptr %temp, i32 %self), !dbg !70
+// CHECK:STDOUT:   call void @"_CConvert.e5cf8fcbb4feaae2:ImplicitAs.0f95c9e18c91e00a.Core.a7b795aef3d7a0e5"(ptr %return, i32 %self), !dbg !70
 // CHECK:STDOUT:   ret void, !dbg !71
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -198,8 +194,6 @@ fn AddOrRemoveConst(a: i32, b: const i32) {
 // CHECK:STDOUT:
 // CHECK:STDOUT: ; Function Attrs: nounwind
 // CHECK:STDOUT: define linkonce_odr i32 @"_CConvert.a44fce96e16342e7:ImplicitAs.ad22d1bbc0605210.Core.b930bfdac0979466"(i32 %self) #0 !dbg !121 {
-// CHECK:STDOUT:   %temp = alloca i32, align 4, !dbg !124
-// CHECK:STDOUT:   call void @llvm.lifetime.start.p0(ptr %temp), !dbg !124
 // CHECK:STDOUT:   %1 = call i32 @"_CConvert.14b8745117b2bc54:ImplicitAs.a9271c1e04015f9c.Core.64ccbb8e5d9a0b8e"(i32 %self), !dbg !124
 // CHECK:STDOUT:   ret i32 %1, !dbg !125
 // CHECK:STDOUT: }
@@ -241,7 +235,7 @@ fn AddOrRemoveConst(a: i32, b: const i32) {
 // CHECK:STDOUT:
 // CHECK:STDOUT: ; uselistorder directives
 // CHECK:STDOUT: uselistorder ptr @"_CConvert.e5cf8fcbb4feaae2:ImplicitAs.0f95c9e18c91e00a.Core.622e77b643a0d3a8", { 0, 1, 3, 2 }
-// CHECK:STDOUT: uselistorder ptr @llvm.lifetime.start.p0, { 0, 1, 2, 3, 11, 10, 9, 8, 7, 6, 5, 4 }
+// CHECK:STDOUT: uselistorder ptr @llvm.lifetime.start.p0, { 0, 8, 7, 6, 5, 4, 3, 2, 1 }
 // CHECK:STDOUT:
 // CHECK:STDOUT: attributes #0 = { nounwind }
 // CHECK:STDOUT: attributes #1 = { nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }