Add the form ID to `FormParamPattern` (#6928)

This enables some nice simplifications, and it's also a step toward a
broader restructuring of binding and parameter patterns.

Assisted-by: Gemini 3.1 Pro via Antigravity

toolchain/check/handle_binding_pattern.cpp

@@ -177,12 +177,13 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
   const DeclIntroducerState& introducer =
       context.decl_introducer_state_stack().innermost();
 
+  auto form_id = pattern_inst_kind == SemIR::FormBindingPattern::Kind
+                     ? context.constant_values().Get(type_expr.inst_id)
+                     : SemIR::ConstantId::None;
+
   auto make_binding_pattern = [&]() -> SemIR::InstId {
     // TODO: Eventually the name will need to support associations with other
     // scopes, but right now we don't support qualified names here.
-    auto form_id = pattern_inst_kind == SemIR::FormBindingPattern::Kind
-                       ? context.constant_values().Get(type_expr.inst_id)
-                       : SemIR::ConstantId::None;
     auto phase = BindingPhase::Runtime;
     if (pattern_inst_kind == SemIR::SymbolicBindingPattern::Kind) {
       phase = is_template ? BindingPhase::Template : BindingPhase::Symbolic;
@@ -296,7 +297,9 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
         } else if (node_kind == Parse::NodeKind::FormBindingPattern) {
           result_inst_id = AddPatternInst<SemIR::FormParamPattern>(
               context, node_id,
-              {.type_id = type_id, .subpattern_id = result_inst_id});
+              {.type_id = type_id,
+               .subpattern_id = result_inst_id,
+               .form_id = form_id});
         } else {
           result_inst_id = AddPatternInst<SemIR::ValueParamPattern>(
               context, node_id,

toolchain/check/pattern.cpp

@@ -171,7 +171,8 @@ auto AddParamPattern(Context& context, SemIR::LocId loc_id,
           loc_id, SemIR::AnyParamPattern{
                       .kind = param_pattern_kind,
                       .type_id = context.insts().Get(pattern_id).type_id(),
-                      .subpattern_id = pattern_id}));
+                      .subpattern_id = pattern_id,
+                      .form_id = SemIR::ConstantId::None}));
 
   return pattern_id;
 }

toolchain/check/pattern_match.cpp

@@ -231,12 +231,9 @@ static auto InsertHere(Context& context, SemIR::ExprRegionId region_id)
 }
 
 // Returns the kind of conversion to perform on the scrutinee when matching the
-// given pattern. `form_kind` is the form of the pattern, if known; it only
-// affects the behavior of `FormBindingPattern` and `FormParamPattern`,
-// and it must be set in the `FormParamPattern` case.
-static auto ConversionKindFor(
-    Context& context, SemIR::Inst pattern, MatchContext::WorkItem entry,
-    std::optional<SemIR::InstKind> form_kind = std::nullopt)
+// given pattern.
+static auto ConversionKindFor(Context& context, SemIR::Inst pattern,
+                              MatchContext::WorkItem entry)
     -> ConversionTarget::Kind {
   CARBON_KIND_SWITCH(pattern) {
     case SemIR::OutParamPattern::Kind:
@@ -252,15 +249,13 @@ static auto ConversionKindFor(
     case SemIR::ValueParamPattern::Kind:
       return ConversionTarget::Value;
     case CARBON_KIND(SemIR::FormBindingPattern form_binding_pattern): {
-      if (!form_kind) {
-        auto form_id = context.entity_names()
-                           .Get(form_binding_pattern.entity_name_id)
-                           .form_id;
-        auto form_inst_id = context.constant_values().GetInstId(form_id);
-        form_kind = context.insts().Get(form_inst_id).kind();
-      }
+      auto form_id = context.entity_names()
+                         .Get(form_binding_pattern.entity_name_id)
+                         .form_id;
+      auto form_inst_id = context.constant_values().GetInstId(form_id);
+      auto form_inst = context.insts().Get(form_inst_id);
 
-      switch (*form_kind) {
+      switch (form_inst.kind()) {
         case SemIR::InitForm::Kind:
           context.TODO(entry.pattern_id, "Support local initializing forms");
           [[fallthrough]];
@@ -272,12 +267,15 @@ static auto ConversionKindFor(
         case SemIR::ValueForm::Kind:
           return ConversionTarget::Value;
         default:
-          CARBON_FATAL("Unexpected form kind {0}", form_kind);
+          CARBON_FATAL("Unexpected form {0}", form_inst);
       }
     }
-    case SemIR::FormParamPattern::Kind: {
-      CARBON_CHECK(form_kind);
-      switch (*form_kind) {
+    case CARBON_KIND(SemIR::FormParamPattern form_param_pattern): {
+      auto form_inst_id =
+          context.constant_values().GetInstId(form_param_pattern.form_id);
+      auto form_inst = context.insts().Get(form_inst_id);
+
+      switch (form_inst.kind()) {
         case SemIR::InitForm::Kind:
           return ConversionTarget::NoOp;
         case SemIR::RefForm::Kind:
@@ -291,7 +289,7 @@ static auto ConversionKindFor(
         case SemIR::ValueForm::Kind:
           return ConversionTarget::Value;
         default:
-          CARBON_FATAL("Unexpected form kind {0}", form_kind);
+          CARBON_FATAL("Unexpected form {0}", form_inst);
       }
     }
     default:
@@ -346,13 +344,10 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
 }
 
 // Returns the inst kind to use for the parameter corresponding to the given
-// parameter pattern. If the pattern is a `FormParamPattern`, `form_kind`
-// must be the pattern's form; otherwise it is ignored.
-static auto ParamKindFor(
-    Context& context, SemIR::Inst param_pattern, MatchContext::WorkItem entry,
-    std::optional<SemIR::InstKind> form_kind = std::nullopt)
-    -> SemIR::InstKind {
-  switch (param_pattern.kind()) {
+// parameter pattern.
+static auto ParamKindFor(Context& context, SemIR::Inst param_pattern,
+                         MatchContext::WorkItem entry) -> SemIR::InstKind {
+  CARBON_KIND_SWITCH(param_pattern) {
     case SemIR::OutParamPattern::Kind:
       return SemIR::OutParam::Kind;
     case SemIR::RefParamPattern::Kind:
@@ -360,9 +355,11 @@ static auto ParamKindFor(
       return SemIR::RefParam::Kind;
     case SemIR::ValueParamPattern::Kind:
       return SemIR::ValueParam::Kind;
-    case SemIR::FormParamPattern::Kind:
-      CARBON_CHECK(form_kind);
-      switch (*form_kind) {
+    case CARBON_KIND(SemIR::FormParamPattern form_param_pattern): {
+      auto form_inst_id =
+          context.constant_values().GetInstId(form_param_pattern.form_id);
+      auto form_inst = context.insts().Get(form_inst_id);
+      switch (form_inst.kind()) {
         case SemIR::InitForm::Kind:
         case SemIR::RefForm::Kind:
           return SemIR::RefParam::Kind;
@@ -373,8 +370,9 @@ static auto ParamKindFor(
         case SemIR::ValueForm::Kind:
           return SemIR::ValueParam::Kind;
         default:
-          CARBON_FATAL("Unexpected form kind {0}", form_kind);
+          CARBON_FATAL("Unexpected form {0}", form_inst);
       }
+    }
     default:
       CARBON_FATAL("Unexpected param pattern kind: {0}", param_pattern);
   }
@@ -383,26 +381,15 @@ static auto ParamKindFor(
 auto MatchContext::DoEmitPatternMatch(Context& context,
                                       SemIR::AnyParamPattern param_pattern,
                                       WorkItem entry) -> void {
-  // If this is a FormParamPattern, determine its form.
-  std::optional<SemIR::InstKind> form_kind;
+  // If the form is initializing, match this as a `VarPattern` before matching
+  // it as a parameter pattern.
   if (param_pattern.kind == SemIR::FormParamPattern::Kind) {
-    if (param_pattern.subpattern_id == SemIR::ErrorInst::InstId) {
-      form_kind = SemIR::ErrorInst::Kind;
-    } else {
-      auto binding_pattern = context.insts().GetAs<SemIR::FormBindingPattern>(
-          param_pattern.subpattern_id);
-      auto form_id =
-          context.entity_names().Get(binding_pattern.entity_name_id).form_id;
-      auto form_inst_id = context.constant_values().GetInstId(form_id);
-      form_kind = context.insts().Get(form_inst_id).kind();
-
-      // If the form is initializing, match this as a `VarPattern` before
-      // matching it as a parameter pattern.
-      if (form_kind == SemIR::InitForm::Kind) {
-        auto new_scrutinee_id =
-            DoEmitVarPatternMatchImpl(context, param_pattern.type_id, entry);
-        entry.scrutinee_id = new_scrutinee_id;
-      }
+    auto form_inst_id =
+        context.constant_values().GetInstId(param_pattern.form_id);
+    if (context.insts().Get(form_inst_id).kind() == SemIR::InitForm::Kind) {
+      auto new_scrutinee_id =
+          DoEmitVarPatternMatchImpl(context, param_pattern.type_id, entry);
+      entry.scrutinee_id = new_scrutinee_id;
     }
   }
 
@@ -418,8 +405,7 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
                 context.sem_ir(), callee_specific_id_, entry.pattern_id));
         call_args_.push_back(Convert(
             context, SemIR::LocId(entry.scrutinee_id), entry.scrutinee_id,
-            {.kind =
-                 ConversionKindFor(context, param_pattern, entry, form_kind),
+            {.kind = ConversionKindFor(context, param_pattern, entry),
              .type_id = scrutinee_type_id}));
       }
       // Do not traverse farther, because the caller side of the pattern
@@ -428,7 +414,7 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
     }
     case MatchKind::Callee: {
       SemIR::AnyParam param = {
-          .kind = ParamKindFor(context, param_pattern, entry, form_kind),
+          .kind = ParamKindFor(context, param_pattern, entry),
           .type_id =
               ExtractScrutineeType(context.sem_ir(), param_pattern.type_id),
           .index = SemIR::CallParamIndex(call_params_.size()),

toolchain/check/testdata/function/call/form.carbon

@@ -244,7 +244,7 @@ fn F(Form:! Core.Form()) ->? Form;
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {
 // CHECK:STDOUT:     %x.patt: %pattern_type.7ce = form_binding_pattern x [concrete]
-// CHECK:STDOUT:     %.loc4_7: %pattern_type.7ce = form_param_pattern %x.patt [concrete]
+// CHECK:STDOUT:     %.loc4_7: %pattern_type.7ce = form_param_pattern %x.patt, constants.%.1da [concrete]
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %x.param: ref %i32 = ref_param call_param0
 // CHECK:STDOUT:     %.loc4_15.1: Core.Form = splice_block %.loc4_15.2 [concrete = constants.%.1da] {
@@ -304,7 +304,7 @@ fn F(Form:! Core.Form()) ->? Form;
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {
 // CHECK:STDOUT:     %x.patt: %pattern_type.7ce = form_binding_pattern x [concrete]
-// CHECK:STDOUT:     %.loc4_7: %pattern_type.7ce = form_param_pattern %x.patt [concrete]
+// CHECK:STDOUT:     %.loc4_7: %pattern_type.7ce = form_param_pattern %x.patt, constants.%.ff5 [concrete]
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %x.param: ref %i32 = ref_param call_param0
 // CHECK:STDOUT:     %.loc4_15.1: Core.Form = splice_block %.loc4_15.2 [concrete = constants.%.ff5] {
@@ -372,7 +372,7 @@ fn F(Form:! Core.Form()) ->? Form;
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {
 // CHECK:STDOUT:     %x.patt: %pattern_type.7ce = form_binding_pattern x [concrete]
-// CHECK:STDOUT:     %.loc4_7: %pattern_type.7ce = form_param_pattern %x.patt [concrete]
+// CHECK:STDOUT:     %.loc4_7: %pattern_type.7ce = form_param_pattern %x.patt, constants.%.754 [concrete]
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %x.param: %i32 = value_param call_param0
 // CHECK:STDOUT:     %.loc4_15.1: Core.Form = splice_block %.loc4_15.2 [concrete = constants.%.754] {
@@ -414,7 +414,7 @@ fn F(Form:! Core.Form()) ->? Form;
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {
 // CHECK:STDOUT:     %x.patt: %pattern_type = form_binding_pattern x [concrete]
-// CHECK:STDOUT:     %.loc4_7: %pattern_type = form_param_pattern %x.patt [concrete]
+// CHECK:STDOUT:     %.loc4_7: %pattern_type = form_param_pattern %x.patt, constants.%.9f9 [concrete]
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %x.param: ref %empty_tuple.type = ref_param call_param0
 // CHECK:STDOUT:     %.loc4_15.1: Core.Form = splice_block %.loc4_15.2 [concrete = constants.%.9f9] {

toolchain/check/testdata/impl/fail_todo_form_thunk.carbon

@@ -12,25 +12,15 @@
 
 interface I {
   let T:! type;
-  // CHECK:STDERR: fail_todo_form_thunk.carbon:[[@LINE+7]]:8: error: semantics TODO: `Support for cloning form bindings` [SemanticsTodo]
+  // CHECK:STDERR: fail_todo_form_thunk.carbon:[[@LINE+4]]:8: error: semantics TODO: `Support for cloning form bindings` [SemanticsTodo]
   // CHECK:STDERR:   fn F(x:? form(ref ()), y: T);
   // CHECK:STDERR:        ^
   // CHECK:STDERR:
-  // CHECK:STDERR: fail_todo_form_thunk.carbon:[[@LINE+3]]:8: error: value expression passed to reference parameter [ValueForRefParam]
-  // CHECK:STDERR:   fn F(x:? form(ref ()), y: T);
-  // CHECK:STDERR:        ^~~~~~~~~~~~~~~~
   fn F(x:? form(ref ()), y: T);
 }
 
 class C {
   impl as I where .T = () {
-    // CHECK:STDERR: fail_todo_form_thunk.carbon:[[@LINE+7]]:10: note: initializing function parameter [InCallToFunctionParam]
-    // CHECK:STDERR:     fn F(x:? form(ref ()), ());
-    // CHECK:STDERR:          ^~~~~~~~~~~~~~~~
-    // CHECK:STDERR: fail_todo_form_thunk.carbon:[[@LINE-8]]:3: note: while building thunk to match the signature of this function [ThunkSignature]
-    // CHECK:STDERR:   fn F(x:? form(ref ()), y: T);
-    // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-    // CHECK:STDERR:
     fn F(x:? form(ref ()), ());
   }
 }

toolchain/check/testdata/impl/use_assoc_entity.carbon

@@ -3367,11 +3367,9 @@ fn F() {
 // CHECK:STDOUT:   %empty_tuple.type.as.J2.impl.F.decl.loc23_40.2: %empty_tuple.type.as.J2.impl.F.type.56bdaf.2 = fn_decl @empty_tuple.type.as.J2.impl.F.loc23_40.2 [concrete = constants.%empty_tuple.type.as.J2.impl.F.7b8679.2] {
 // CHECK:STDOUT:     %self.patt: %pattern_type.cb1 = value_binding_pattern self [concrete]
 // CHECK:STDOUT:     %self.param_patt: %pattern_type.cb1 = value_param_pattern %self.patt [concrete]
-// CHECK:STDOUT:     %.param_patt: <error> = value_param_pattern <error> [concrete]
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %self.param: %empty_tuple.type = value_param call_param0
 // CHECK:STDOUT:     %self: %empty_tuple.type = value_binding self, %self.param
-// CHECK:STDOUT:     %.param: <error> = value_param call_param1
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %J2.impl_witness_table = impl_witness_table (%impl_witness_assoc_constant, %empty_tuple.type.as.J2.impl.F.decl.loc23_40.2), @empty_tuple.type.as.J2.impl [concrete]
 // CHECK:STDOUT:   %J2.impl_witness: <witness> = impl_witness %J2.impl_witness_table [concrete = constants.%J2.impl_witness.941]
@@ -3407,11 +3405,9 @@ fn F() {
 // CHECK:STDOUT:   %C2.as.J2.impl.F.decl.loc32_40.2: %C2.as.J2.impl.F.type.d2a210.2 = fn_decl @C2.as.J2.impl.F.loc32_40.2 [concrete = constants.%C2.as.J2.impl.F.720bb2.2] {
 // CHECK:STDOUT:     %self.patt: %pattern_type.8df = value_binding_pattern self [concrete]
 // CHECK:STDOUT:     %self.param_patt: %pattern_type.8df = value_param_pattern %self.patt [concrete]
-// CHECK:STDOUT:     %.param_patt: <error> = value_param_pattern <error> [concrete]
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %self.param: %C2 = value_param call_param0
 // CHECK:STDOUT:     %self: %C2 = value_binding self, %self.param
-// CHECK:STDOUT:     %.param: <error> = value_param call_param1
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %J2.impl_witness_table = impl_witness_table (%impl_witness_assoc_constant, %C2.as.J2.impl.F.decl.loc32_40.2), @C2.as.J2.impl [concrete]
 // CHECK:STDOUT:   %J2.impl_witness: <witness> = impl_witness %J2.impl_witness_table [concrete = constants.%J2.impl_witness.30f]
@@ -3450,11 +3446,10 @@ fn F() {
 // CHECK:STDOUT:   return %.loc23_50
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: fn @empty_tuple.type.as.J2.impl.F.loc23_40.2(%self.param: %empty_tuple.type, %.param: <error>) -> <error> [thunk @empty_tuple.type.as.J2.impl.%empty_tuple.type.as.J2.impl.F.decl.loc23_40.1] {
+// CHECK:STDOUT: fn @empty_tuple.type.as.J2.impl.F.loc23_40.2(%self.param: %empty_tuple.type) -> <error> [thunk @empty_tuple.type.as.J2.impl.%empty_tuple.type.as.J2.impl.F.decl.loc23_40.1] {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   %F.ref: %empty_tuple.type.as.J2.impl.F.type.56bdaf.1 = name_ref F, @empty_tuple.type.as.J2.impl.%empty_tuple.type.as.J2.impl.F.decl.loc23_40.1 [concrete = constants.%empty_tuple.type.as.J2.impl.F.7b8679.1]
 // CHECK:STDOUT:   %self.ref: %empty_tuple.type = name_ref self, %self.param
-// CHECK:STDOUT:   %.ref: <error> = name_ref <none>, %.param
 // CHECK:STDOUT:   %empty_tuple.type.as.J2.impl.F.bound: <bound method> = bound_method %self.ref, %F.ref
 // CHECK:STDOUT:   %empty_tuple.type.as.J2.impl.F.call: init %empty_struct_type = call %empty_tuple.type.as.J2.impl.F.bound(%self.ref, <error>)
 // CHECK:STDOUT:   return <error>
@@ -3470,11 +3465,10 @@ fn F() {
 // CHECK:STDOUT:   return %.loc32_53.4
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: fn @C2.as.J2.impl.F.loc32_40.2(%self.param: %C2, %.param: <error>) -> <error> [thunk @C2.as.J2.impl.%C2.as.J2.impl.F.decl.loc32_40.1] {
+// CHECK:STDOUT: fn @C2.as.J2.impl.F.loc32_40.2(%self.param: %C2) -> <error> [thunk @C2.as.J2.impl.%C2.as.J2.impl.F.decl.loc32_40.1] {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   %F.ref: %C2.as.J2.impl.F.type.d2a210.1 = name_ref F, @C2.as.J2.impl.%C2.as.J2.impl.F.decl.loc32_40.1 [concrete = constants.%C2.as.J2.impl.F.720bb2.1]
 // CHECK:STDOUT:   %self.ref: %C2 = name_ref self, %self.param
-// CHECK:STDOUT:   %.ref: <error> = name_ref <none>, %.param
 // CHECK:STDOUT:   %C2.as.J2.impl.F.bound: <bound method> = bound_method %self.ref, %F.ref
 // CHECK:STDOUT:   %C2.as.J2.impl.F.call: init %C2 = call %C2.as.J2.impl.F.bound(%self.ref, <error>)
 // CHECK:STDOUT:   return <error>

toolchain/check/thunk.cpp

@@ -127,12 +127,13 @@ static auto ClonePattern(Context& context, SemIR::SpecificId specific_id,
   }
 
   // Rebuild parameter.
-  if (param) {
+  if (param && new_pattern_id != SemIR::ErrorInst::InstId) {
     new_pattern_id = RebuildPatternInst<SemIR::AnyParamPattern>(
         context, param_id,
         {.kind = param->kind,
          .type_id = get_type(param_id),
-         .subpattern_id = new_pattern_id});
+         .subpattern_id = new_pattern_id,
+         .form_id = SemIR::ConstantId::None});
   }
 
   return new_pattern_id;

toolchain/lower/file_context.cpp

@@ -572,12 +572,9 @@ auto FileContext::FunctionTypeInfoBuilder::HandleParameter(
   // corresponds to its form.
   if (auto form_param_pattern =
           param_pattern.TryAs<SemIR::FormParamPattern>()) {
-    auto form_binding_pattern = sem_ir.insts().GetAs<SemIR::FormBindingPattern>(
-        form_param_pattern->subpattern_id);
-    auto form_id =
-        sem_ir.entity_names().Get(form_binding_pattern.entity_name_id).form_id;
-    CARBON_CHECK(!form_id.is_symbolic(), "TODO");
-    auto form_inst_id = sem_ir.constant_values().GetInstId(form_id);
+    CARBON_CHECK(!form_param_pattern->form_id.is_symbolic(), "TODO");
+    auto form_inst_id =
+        sem_ir.constant_values().GetInstId(form_param_pattern->form_id);
     auto form_kind = sem_ir.insts().Get(form_inst_id).kind();
     switch (form_kind) {
       case SemIR::InitForm::Kind:

toolchain/sem_ir/formatter.h

@@ -276,6 +276,7 @@ class Formatter {
   auto FormatArg(AbsoluteInstBlockId id) -> void;
   auto FormatArg(RealId id) -> void;
   auto FormatArg(StringLiteralValueId id) -> void;
+  auto FormatArg(ConstantId id) -> void { FormatConstant(id); }
 
   // A `FormatArg` wrapper for `FormatInstArgAndKind`.
   using FormatArgFnT = auto(Formatter& formatter, int32_t arg) -> void;

toolchain/sem_ir/inst_categories.h

@@ -291,6 +291,9 @@ struct AnyParamPattern {
   // `subpattern_id`.
   TypeId type_id;
   InstId subpattern_id;
+
+  // None unless this is a FormParamPattern.
+  ConstantId form_id;
 };
 
 // clang-format off

toolchain/sem_ir/typed_insts.h

@@ -788,6 +788,7 @@ struct FormParamPattern {
 
   TypeId type_id;
   InstId subpattern_id;
+  ConstantId form_id;
 };
 
 // The type `Core.Form`.