Model category conversion as a state machine. (#6535)

The fallthrough-based approach was unwieldy and error-prone, and
inherently couldn't support category conversions whose steps don't
follow the fixed order of the `switch` statement.

toolchain/check/convert.cpp

@@ -1450,6 +1450,178 @@ static auto CheckRefTag(Context& context, SemIR::InstId expr_id,
   }
 }
 
+// State machine for performing category conversions.
+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.
+  CategoryConverter(Context& context, SemIR::LocId loc_id,
+                    ConversionTarget& target, bool performed_builtin_conversion)
+      : context_(context),
+        sem_ir_(context.sem_ir()),
+        loc_id_(loc_id),
+        target_(target),
+        performed_builtin_conversion_(performed_builtin_conversion) {}
+
+  // Converts expr_id to the target specified in the constructor, and returns
+  // the converted inst.
+  auto Convert(SemIR::InstId expr_id) && -> SemIR::InstId {
+    auto category = SemIR::GetExprCategory(sem_ir_, expr_id);
+    while (true) {
+      if (expr_id == SemIR::ErrorInst::InstId) {
+        return expr_id;
+      }
+      CARBON_KIND_SWITCH(DoStep(expr_id, category)) {
+        case CARBON_KIND(NextStep next_step): {
+          CARBON_CHECK(next_step.expr_id != SemIR::InstId::None);
+          expr_id = next_step.expr_id;
+          category = next_step.category;
+          break;
+        }
+        case CARBON_KIND(Done done): {
+          return done.expr_id;
+        }
+      }
+    }
+  }
+
+ private:
+  // State that indicates there's more work to be done. As a convenience,
+  // if expr_id is SemIR::ErrorInst::InstId, this is equivalent to
+  // Done{SemIR::ErrorInst::InstId}.
+  struct NextStep {
+    // The inst to convert.
+    SemIR::InstId expr_id;
+    // The category of expr_id.
+    SemIR::ExprCategory category;
+  };
+
+  // State that indicates we've finished category conversion.
+  struct Done {
+    // The result of the conversion.
+    SemIR::InstId expr_id;
+  };
+
+  using State = std::variant<NextStep, Done>;
+
+  // Performs the first step of converting `expr_id` with category `category`
+  // to the target specified in the constructor, and returns the state after
+  // that step.
+  auto DoStep(SemIR::InstId expr_id, SemIR::ExprCategory category) const
+      -> State;
+
+  Context& context_;
+  SemIR::File& sem_ir_;
+  SemIR::LocId loc_id_;
+  ConversionTarget& target_;
+  bool performed_builtin_conversion_;
+};
+
+auto CategoryConverter::DoStep(const SemIR::InstId expr_id,
+                               const SemIR::ExprCategory category) const
+    -> State {
+  CARBON_DCHECK(SemIR::GetExprCategory(sem_ir_, expr_id) == category);
+  switch (category) {
+    case SemIR::ExprCategory::NotExpr:
+    case SemIR::ExprCategory::Mixed:
+    case SemIR::ExprCategory::Pattern:
+      CARBON_FATAL("Unexpected expression {0} after builtin conversions",
+                   sem_ir_.insts().Get(expr_id));
+
+    case SemIR::ExprCategory::Error:
+      return Done{SemIR::ErrorInst::InstId};
+
+    case SemIR::ExprCategory::Initializing:
+      if (target_.is_initializer()) {
+        if (!performed_builtin_conversion_) {
+          // Don't fill in the return slot if we created the expression through
+          // a builtin conversion. In that case, we will have created it with
+          // the target already set.
+          // TODO: Find a better way to track whether we need to do this,
+          // and then make target_ immutable if possible.
+          MarkInitializerFor(sem_ir_, expr_id, target_);
+        }
+        return Done{expr_id};
+      }
+
+      // Commit to using a temporary for this initializing expression.
+      // TODO: Don't create a temporary if the initializing representation
+      // is already a value representation.
+      // TODO: If the target is DurableRef, materialize a VarStorage instead of
+      // a TemporaryStorage to lifetime-extend.
+      if (target_.kind == ConversionTarget::Discarded) {
+        return Done{FinalizeTemporary(context_, expr_id, /*discarded=*/true)};
+      } else {
+        return NextStep{.expr_id = FinalizeTemporary(context_, expr_id,
+                                                     /*discarded=*/false),
+                        .category = SemIR::ExprCategory::EphemeralRef};
+      }
+
+    case SemIR::ExprCategory::DurableRef:
+      if (target_.kind == ConversionTarget::DurableRef) {
+        return Done{expr_id};
+      }
+      [[fallthrough]];
+
+    case SemIR::ExprCategory::EphemeralRef:
+      // If a reference expression is an acceptable result, we're done.
+      if (target_.kind == ConversionTarget::ValueOrRef ||
+          target_.kind == ConversionTarget::Discarded ||
+          target_.kind == ConversionTarget::CppThunkRef ||
+          target_.kind == ConversionTarget::RefParam) {
+        return Done{expr_id};
+      }
+
+      // If we have a reference and don't want one, form a value binding.
+      // TODO: Support types with custom value representations.
+      return NextStep{.expr_id = AddInst<SemIR::AcquireValue>(
+                          context_, SemIR::LocId(expr_id),
+                          {.type_id = target_.type_id, .value_id = expr_id}),
+                      .category = SemIR::ExprCategory::Value};
+
+    case SemIR::ExprCategory::Value:
+      if (target_.kind == ConversionTarget::DurableRef) {
+        if (target_.diagnose) {
+          CARBON_DIAGNOSTIC(ConversionFailureNonRefToRef, Error,
+                            "cannot bind durable reference to non-reference "
+                            "value of type {0}",
+                            SemIR::TypeId);
+          context_.emitter().Emit(loc_id_, ConversionFailureNonRefToRef,
+                                  target_.type_id);
+        }
+        return Done{SemIR::ErrorInst::InstId};
+      }
+      if (target_.kind == ConversionTarget::RefParam) {
+        // Don't diagnose a non-reference scrutinee if it has a user-written
+        // `ref` tag, because that's diagnosed in `CheckRefTag`.
+        if (target_.diagnose) {
+          if (auto lookup_result = context_.ref_tags().Lookup(expr_id);
+              !lookup_result ||
+              lookup_result.value() != Context::RefTag::Present) {
+            CARBON_DIAGNOSTIC(ValueForRefParam, Error,
+                              "value expression passed to reference parameter");
+            context_.emitter().Emit(loc_id_, ValueForRefParam);
+          }
+        }
+        return Done{SemIR::ErrorInst::InstId};
+      }
+
+      // When initializing from a value, perform a copy.
+      if (target_.is_initializer()) {
+        return Done{PerformCopy(context_, expr_id, target_)};
+      }
+
+      // When initializing a C++ thunk parameter, form a reference, creating a
+      // temporary if needed.
+      if (target_.kind == ConversionTarget::CppThunkRef) {
+        return Done{ConvertValueForCppThunkRef(context_, expr_id)};
+      }
+
+      return Done{expr_id};
+  }
+}
+
 auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
              ConversionTarget target, SemIR::ClassType* vtable_class_type)
     -> SemIR::InstId {
@@ -1621,108 +1793,9 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
   }
 
   // Now perform any necessary value category conversions.
-  // This uses fallthrough to implement a very simple state machine over the
-  // category of expr_id, which is tracked by current_category.
-  switch (auto current_category = SemIR::GetExprCategory(sem_ir, expr_id);
-          current_category) {
-    case SemIR::ExprCategory::NotExpr:
-    case SemIR::ExprCategory::Mixed:
-    case SemIR::ExprCategory::Pattern:
-      CARBON_FATAL("Unexpected expression {0} after builtin conversions",
-                   sem_ir.insts().Get(expr_id));
-
-    case SemIR::ExprCategory::Error:
-      return SemIR::ErrorInst::InstId;
-
-    case SemIR::ExprCategory::Initializing:
-      if (target.is_initializer()) {
-        if (!performed_builtin_conversion) {
-          // Don't fill in the return slot if we created the expression through
-          // a builtin conversion. In that case, we will have created it with
-          // the target already set.
-          // TODO: Find a better way to track whether we need to do this.
-          MarkInitializerFor(sem_ir, expr_id, target);
-        }
-        break;
-      }
-
-      // Commit to using a temporary for this initializing expression.
-      // TODO: Don't create a temporary if the initializing representation
-      // is already a value representation.
-      // TODO: If the target is DurableRef, materialize a VarStorage instead of
-      // a TemporaryStorage to lifetime-extend.
-      expr_id = FinalizeTemporary(context, expr_id,
-                                  target.kind == ConversionTarget::Discarded);
-      // We now have an ephemeral reference.
-      current_category = SemIR::ExprCategory::EphemeralRef;
-      [[fallthrough]];
-
-    case SemIR::ExprCategory::DurableRef:
-    case SemIR::ExprCategory::EphemeralRef:
-      if (current_category == SemIR::ExprCategory::DurableRef &&
-          target.kind == ConversionTarget::DurableRef) {
-        break;
-      }
-
-      // If a reference expression is an acceptable result, we're done.
-      if (target.kind == ConversionTarget::ValueOrRef ||
-          target.kind == ConversionTarget::Discarded ||
-          target.kind == ConversionTarget::CppThunkRef ||
-          target.kind == ConversionTarget::RefParam) {
-        break;
-      }
-
-      // If we have a reference and don't want one, form a value binding.
-      // TODO: Support types with custom value representations.
-      expr_id = AddInst<SemIR::AcquireValue>(
-          context, SemIR::LocId(expr_id),
-          {.type_id = target.type_id, .value_id = expr_id});
-      // We now have a value expression.
-      current_category = SemIR::ExprCategory::Value;
-      [[fallthrough]];
-
-    case SemIR::ExprCategory::Value:
-      if (target.kind == ConversionTarget::DurableRef) {
-        if (target.diagnose) {
-          CARBON_DIAGNOSTIC(ConversionFailureNonRefToRef, Error,
-                            "cannot bind durable reference to non-reference "
-                            "value of type {0}",
-                            SemIR::TypeId);
-          context.emitter().Emit(loc_id, ConversionFailureNonRefToRef,
-                                 target.type_id);
-        }
-        return SemIR::ErrorInst::InstId;
-      }
-      if (target.kind == ConversionTarget::RefParam) {
-        // Don't diagnose a non-reference scrutinee if it has a user-written
-        // `ref` tag, because that's diagnosed in `CheckRefTag`.
-        if (target.diagnose) {
-          if (auto lookup_result = context.ref_tags().Lookup(expr_id);
-              !lookup_result ||
-              lookup_result.value() != Context::RefTag::Present) {
-            CARBON_DIAGNOSTIC(ValueForRefParam, Error,
-                              "value expression passed to reference parameter");
-            context.emitter().Emit(loc_id, ValueForRefParam);
-          }
-        }
-        return SemIR::ErrorInst::InstId;
-      }
-
-      // When initializing from a value, perform a copy.
-      if (target.is_initializer()) {
-        expr_id = PerformCopy(context, expr_id, target);
-        current_category = SemIR::ExprCategory::Initializing;
-      }
-
-      // When initializing a C++ thunk parameter, form a reference, creating a
-      // temporary if needed.
-      if (target.kind == ConversionTarget::CppThunkRef) {
-        expr_id = ConvertValueForCppThunkRef(context, expr_id);
-        current_category = SemIR::ExprCategory::EphemeralRef;
-      }
-
-      break;
-  }
+  expr_id =
+      CategoryConverter(context, loc_id, target, performed_builtin_conversion)
+          .Convert(expr_id);
 
   // Perform a final destination store, if necessary.
   if (target.kind == ConversionTarget::FullInitializer) {

toolchain/check/testdata/tuple/class_tuples.carbon

@@ -21,10 +21,17 @@ class D {}
 
 var c: C;
 var d: D;
-// CHECK:STDERR: fail_element_type_mismatch.carbon:[[@LINE+7]]:19: error: cannot implicitly convert expression of type `C` to `C*` [ConversionFailure]
+// CHECK:STDERR: fail_element_type_mismatch.carbon:[[@LINE+14]]:19: error: cannot implicitly convert expression of type `C` to `C*` [ConversionFailure]
 // CHECK:STDERR: var x: (C*, C*) = (c, d);
 // CHECK:STDERR:                   ^~~~~~
-// CHECK:STDERR: fail_element_type_mismatch.carbon:[[@LINE+4]]:19: note: type `C` does not implement interface `Core.ImplicitAs(C*)` [MissingImplInMemberAccessNote]
+// CHECK:STDERR: fail_element_type_mismatch.carbon:[[@LINE+11]]:19: note: type `C` does not implement interface `Core.ImplicitAs(C*)` [MissingImplInMemberAccessNote]
+// CHECK:STDERR: var x: (C*, C*) = (c, d);
+// CHECK:STDERR:                   ^~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_element_type_mismatch.carbon:[[@LINE+7]]:19: error: cannot implicitly convert expression of type `D` to `C*` [ConversionFailure]
+// CHECK:STDERR: var x: (C*, C*) = (c, d);
+// CHECK:STDERR:                   ^~~~~~
+// CHECK:STDERR: fail_element_type_mismatch.carbon:[[@LINE+4]]:19: note: type `D` does not implement interface `Core.ImplicitAs(C*)` [MissingImplInMemberAccessNote]
 // CHECK:STDERR: var x: (C*, C*) = (c, d);
 // CHECK:STDERR:                   ^~~~~~
 // CHECK:STDERR: