Clean up handling of Call params (#5061)

- Explicitly document that `*Param` and `*ParamPattern` insts represent `Call` parameters. - Stop wrapping compile-time parameter patterns in `ValueParamPattern` insts (because they aren't `Call` parameters). - Document how `MatchContext::results_` relates to the `Call` parameters, and be more consistent about when it's written to. - Remove `RuntimeParamIndex::Unknown`: we no longer need to distinguish "this `Param`'s runtime index is unknown" from "this `Param` isn't a runtime param", because we no longer use `Param`s at all in the latter case. - Rename `RuntimeParamIndex` to `CallParamIndex`. As a side effect of removing the `ValueParamPattern` insts, this fixes a minor diagnostic bug where `NoteInitializingParam` didn't identify the specific parameter that led to a deduction failure, because it expects generic parameters to only be represented by `SymbolicBindingPattern`s, but before this change they could be wrapped in `ValueParamPattern`s.
carbon-language · Mar 4, 2025 · d264f14 · d264f14
1 parent 4e21c0c
commit d264f14
Show file tree

Hide file tree

Showing 148 changed files with 1,424 additions and 2,410 deletions.
diff --git a/toolchain/check/full_pattern_stack.h b/toolchain/check/full_pattern_stack.h
@@ -95,6 +95,13 @@ class FullPatternStack {
         {.name_id = name_id, .inst_id = SemIR::InstId::InitTombstone});
   }
 
+  // Runs verification that the processing cleanly finished.
+  auto VerifyOnFinish() const -> void {
+    CARBON_CHECK(kind_stack_.empty(),
+                 "full_pattern_stack still has {1} entries",
+                 kind_stack_.size());
+  }
+
  private:
   LexicalLookup* lookup_;
 

diff --git a/toolchain/check/handle_binding_pattern.cpp b/toolchain/check/handle_binding_pattern.cpp
@@ -195,7 +195,6 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
       // in a function definition. We don't know which kind we have here.
       // TODO: A tuple pattern can appear in other places than function
       // parameters.
-      auto param_pattern_id = SemIR::InstId::None;
       bool had_error = false;
       switch (introducer.kind) {
         case Lex::TokenKind::Fn: {
@@ -242,23 +241,23 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
         default:
           break;
       }
+      auto result_inst_id = SemIR::InstId::None;
       if (had_error) {
         AddNameToLookup(context, name_id, SemIR::ErrorInst::SingletonInstId);
         // Replace the parameter with `ErrorInst` so that we don't try
         // constructing a generic based on it.
-        param_pattern_id = SemIR::ErrorInst::SingletonInstId;
+        result_inst_id = SemIR::ErrorInst::SingletonInstId;
       } else {
-        auto pattern_inst_id = make_binding_pattern();
-        param_pattern_id = AddPatternInst<SemIR::ValueParamPattern>(
-            context, node_id,
-            {
-                .type_id = context.insts().Get(pattern_inst_id).type_id(),
-                .subpattern_id = pattern_inst_id,
-                .runtime_index = is_generic ? SemIR::RuntimeParamIndex::None
-                                            : SemIR::RuntimeParamIndex::Unknown,
-            });
+        result_inst_id = make_binding_pattern();
+        if (node_kind == Parse::NodeKind::LetBindingPattern) {
+          result_inst_id = AddPatternInst<SemIR::ValueParamPattern>(
+              context, node_id,
+              {.type_id = context.insts().Get(result_inst_id).type_id(),
+               .subpattern_id = result_inst_id,
+               .index = SemIR::CallParamIndex::None});
+        }
       }
-      context.node_stack().Push(node_id, param_pattern_id);
+      context.node_stack().Push(node_id, result_inst_id);
       break;
     }
 

diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp
@@ -69,7 +69,7 @@ auto HandleParseNode(Context& context, Parse::ReturnTypeId node_id) -> bool {
       context, node_id,
       {.type_id = type_id,
        .subpattern_id = return_slot_pattern_id,
-       .runtime_index = SemIR::RuntimeParamIndex::Unknown});
+       .index = SemIR::CallParamIndex::None});
   context.node_stack().Push(node_id, param_pattern_id);
   return true;
 }

diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp
@@ -228,22 +228,23 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
   Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id);
   --last_param_iter;
 
-  return {
-      .name_loc_id = Parse::NodeId::None,
-      .name_id = SemIR::NameId::None,
-      .first_param_node_id = first_param_node_id,
-      .last_param_node_id = *last_param_iter,
-      .implicit_params_loc_id = implicit_params_loc_id,
-      .implicit_param_patterns_id =
-          implicit_param_patterns_id.value_or(SemIR::InstBlockId::None),
-      .params_loc_id = Parse::NodeId::None,
-      .param_patterns_id = SemIR::InstBlockId::None,
-      .call_params_id = SemIR::InstBlockId::None,
-      .return_slot_pattern_id = SemIR::InstId::None,
-      .pattern_block_id = implicit_param_patterns_id
-                              ? context.pattern_block_stack().Pop()
-                              : SemIR::InstBlockId::None,
-  };
+  auto pattern_block_id = SemIR::InstBlockId::None;
+  if (implicit_param_patterns_id) {
+    pattern_block_id = context.pattern_block_stack().Pop();
+    context.full_pattern_stack().PopFullPattern();
+  }
+  return {.name_loc_id = Parse::NodeId::None,
+          .name_id = SemIR::NameId::None,
+          .first_param_node_id = first_param_node_id,
+          .last_param_node_id = *last_param_iter,
+          .implicit_params_loc_id = implicit_params_loc_id,
+          .implicit_param_patterns_id =
+              implicit_param_patterns_id.value_or(SemIR::InstBlockId::None),
+          .params_loc_id = Parse::NodeId::None,
+          .param_patterns_id = SemIR::InstBlockId::None,
+          .call_params_id = SemIR::InstBlockId::None,
+          .return_slot_pattern_id = SemIR::InstId::None,
+          .pattern_block_id = pattern_block_id};
 }
 
 static auto MergeImplRedecl(Context& context, SemIR::Impl& new_impl,

diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp
@@ -965,10 +965,6 @@ static auto LoadLocalPatternConstantIds(ImportRefResolver& resolver,
       pattern_inst = resolver.import_insts().Get(pattern_id);
       GetLocalConstantId(resolver, pattern_inst.type_id());
     }
-    pattern_id = resolver.import_insts()
-                     .GetAs<SemIR::ValueParamPattern>(pattern_id)
-                     .subpattern_id;
-    pattern_inst = resolver.import_insts().Get(pattern_id);
     // If the parameter is a symbolic binding, build the
     // SymbolicBindingPattern constant.
     if (pattern_inst.Is<SemIR::SymbolicBindingPattern>()) {
@@ -1016,10 +1012,14 @@ static auto GetLocalParamPatternsId(ImportContext& context,
       param_pattern_id = addr_inst->inner_id;
     }
 
-    auto param_pattern = context.import_insts().GetAs<SemIR::ValueParamPattern>(
-        param_pattern_id);
+    auto param_pattern =
+        context.import_insts().TryGetAs<SemIR::ValueParamPattern>(
+            param_pattern_id);
+    auto binding_id = addr_pattern_id;
+    if (param_pattern) {
+      binding_id = param_pattern->subpattern_id;
+    }
 
-    auto binding_id = param_pattern.subpattern_id;
     auto binding =
         context.import_insts().GetAs<SemIR::AnyBindingPattern>(binding_id);
 
@@ -1058,13 +1058,16 @@ static auto GetLocalParamPatternsId(ImportContext& context,
         CARBON_FATAL("Unexpected kind: ", binding.kind);
       }
     }
-    new_param_id = AddInstInNoBlock(
-        context.local_context(),
-        MakeImportedLocIdAndInst<SemIR::ValueParamPattern>(
-            context.local_context(), AddImportIRInst(context, param_pattern_id),
-            {.type_id = type_id,
-             .subpattern_id = new_param_id,
-             .runtime_index = param_pattern.runtime_index}));
+    if (param_pattern) {
+      new_param_id =
+          AddInstInNoBlock(context.local_context(),
+                           MakeImportedLocIdAndInst<SemIR::ValueParamPattern>(
+                               context.local_context(),
+                               AddImportIRInst(context, param_pattern_id),
+                               {.type_id = type_id,
+                                .subpattern_id = new_param_id,
+                                .index = param_pattern->index}));
+    }
     if (addr_inst) {
       type_id = context.local_context().types().GetTypeIdForTypeConstantId(
           GetLocalConstantIdChecked(context, addr_inst->type_id));
@@ -1115,7 +1118,7 @@ static auto GetLocalReturnSlotPatternId(
           AddImportIRInst(context, import_return_slot_pattern_id),
           {.type_id = type_id,
            .subpattern_id = new_return_slot_pattern_id,
-           .runtime_index = param_pattern.runtime_index}));
+           .index = param_pattern.index}));
 }
 
 // Translates a NameScopeId from the import IR to a local NameScopeId. Adds

diff --git a/toolchain/check/merge.cpp b/toolchain/check/merge.cpp
@@ -242,13 +242,15 @@ static auto CheckRedeclParam(Context& context, bool is_implicit_param,
     }
   }
 
-  new_param_pattern = context.insts().Get(
-      new_param_pattern.As<SemIR::ValueParamPattern>().subpattern_id);
-  prev_param_pattern = context.insts().Get(
-      prev_param_pattern.As<SemIR::ValueParamPattern>().subpattern_id);
-  if (new_param_pattern.kind() != prev_param_pattern.kind()) {
-    emit_diagnostic();
-    return false;
+  if (new_param_pattern.Is<SemIR::AnyParamPattern>()) {
+    new_param_pattern = context.insts().Get(
+        new_param_pattern.As<SemIR::ValueParamPattern>().subpattern_id);
+    prev_param_pattern = context.insts().Get(
+        prev_param_pattern.As<SemIR::ValueParamPattern>().subpattern_id);
+    if (new_param_pattern.kind() != prev_param_pattern.kind()) {
+      emit_diagnostic();
+      return false;
+    }
   }
 
   auto new_name_id =

diff --git a/toolchain/check/pattern_match.cpp b/toolchain/check/pattern_match.cpp
@@ -80,7 +80,7 @@ class MatchContext {
 
  private:
   // Allocates the next unallocated RuntimeParamIndex, starting from 0.
-  auto NextRuntimeIndex() -> SemIR::RuntimeParamIndex {
+  auto NextRuntimeIndex() -> SemIR::CallParamIndex {
     auto result = next_index_;
     ++next_index_.index;
     return result;
@@ -125,9 +125,13 @@ class MatchContext {
   llvm::SmallVector<WorkItem> stack_;
 
   // The next index to be allocated by `NextRuntimeIndex`.
-  SemIR::RuntimeParamIndex next_index_;
+  SemIR::CallParamIndex next_index_;
 
   // The pending results that will be returned by the current `DoWork` call.
+  // It represents the contents of the `Call` arguments block when kind_
+  // is Caller, or the `Call` parameters block when kind_ is Callee
+  // (it is empty when kind_ is Local). Consequently, it is populated
+  // only by DoEmitPatternMatch for *ParamPattern insts.
   llvm::SmallVector<SemIR::InstId> results_;
 
   // The kind of pattern match being performed.
@@ -200,31 +204,25 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
                                       SemIR::AnyBindingPattern binding_pattern,
                                       SemIR::LocId /*pattern_loc_id*/,
                                       MatchContext::WorkItem entry) -> void {
+  if (kind_ == MatchKind::Caller) {
+    CARBON_CHECK(binding_pattern.kind == SemIR::SymbolicBindingPattern::Kind,
+                 "Found runtime binding pattern during caller pattern match");
+    return;
+  }
   // We're logically consuming this map entry, so we invalidate it in order
   // to avoid accidentally consuming it twice.
   auto [bind_name_id, type_expr_region_id] =
       std::exchange(context.bind_name_map().Lookup(entry.pattern_id).value(),
                     {.bind_name_id = SemIR::InstId::None,
                      .type_expr_region_id = SemIR::ExprRegionId::None});
   InsertHere(context, type_expr_region_id);
-  auto value_id = entry.scrutinee_id;
-  switch (kind_) {
-    case MatchKind::Local: {
-      value_id = ConvertToValueOrRefOfType(
-          context, context.insts().GetLocId(entry.scrutinee_id),
-          entry.scrutinee_id, binding_pattern.type_id);
-      break;
-    }
-    case MatchKind::Callee: {
-      if (context.insts()
-              .GetAs<SemIR::AnyParam>(value_id)
-              .runtime_index.has_value()) {
-        results_.push_back(value_id);
-      }
-      break;
-    }
-    case MatchKind::Caller:
-      CARBON_FATAL("Found binding pattern during caller pattern match");
+  auto value_id = SemIR::InstId::None;
+  if (kind_ == MatchKind::Local) {
+    value_id = ConvertToValueOrRefOfType(
+        context, context.insts().GetLocId(entry.scrutinee_id),
+        entry.scrutinee_id, binding_pattern.type_id);
+  } else {
+    value_id = entry.scrutinee_id;
   }
   auto bind_name = context.insts().GetAs<SemIR::AnyBindName>(bind_name_id);
   CARBON_CHECK(!bind_name.value_id.has_value());
@@ -259,8 +257,10 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
       context.emitter().Emit(
           TokenOnly(context.insts().GetLocId(entry.scrutinee_id)),
           AddrSelfIsNonRef);
-      results_.push_back(SemIR::ErrorInst::SingletonInstId);
-      return;
+      // Add fake reference expression to preserve invariants.
+      auto scrutinee = context.insts().GetWithLocId(entry.scrutinee_id);
+      scrutinee_ref_id = AddInst<SemIR::TemporaryStorage>(
+          context, scrutinee.loc_id, {.type_id = scrutinee.inst.type_id()});
   }
   auto scrutinee_ref = context.insts().Get(scrutinee_ref_id);
   auto new_scrutinee = AddInst<SemIR::AddrOf>(
@@ -274,11 +274,11 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
                                       SemIR::ValueParamPattern param_pattern,
                                       SemIR::LocId pattern_loc_id,
                                       WorkItem entry) -> void {
-  CARBON_CHECK(param_pattern.runtime_index.index < 0 ||
-                   static_cast<size_t>(param_pattern.runtime_index.index) ==
-                       results_.size(),
-               "Parameters out of order; expecting {0} but got {1}",
-               results_.size(), param_pattern.runtime_index.index);
+  CARBON_CHECK(
+      param_pattern.index.index < 0 ||
+          static_cast<size_t>(param_pattern.index.index) == results_.size(),
+      "Parameters out of order; expecting {0} but got {1}", results_.size(),
+      param_pattern.index.index);
   switch (kind_) {
     case MatchKind::Caller: {
       CARBON_CHECK(entry.scrutinee_id.has_value());
@@ -296,16 +296,18 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
       break;
     }
     case MatchKind::Callee: {
-      if (param_pattern.runtime_index == SemIR::RuntimeParamIndex::Unknown) {
-        param_pattern.runtime_index = NextRuntimeIndex();
+      if (!param_pattern.index.has_value()) {
+        param_pattern.index = NextRuntimeIndex();
         ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
       }
+      auto param_id = AddInst<SemIR::ValueParam>(
+          context, pattern_loc_id,
+          {.type_id = param_pattern.type_id,
+           .index = param_pattern.index,
+           .pretty_name_id = GetPrettyName(context, param_pattern)});
       AddWork({.pattern_id = param_pattern.subpattern_id,
-               .scrutinee_id = AddInst<SemIR::ValueParam>(
-                   context, pattern_loc_id,
-                   {.type_id = param_pattern.type_id,
-                    .runtime_index = param_pattern.runtime_index,
-                    .pretty_name_id = GetPrettyName(context, param_pattern)})});
+               .scrutinee_id = param_id});
+      results_.push_back(param_id);
       break;
     }
     case MatchKind::Local: {
@@ -333,16 +335,18 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
     case MatchKind::Callee: {
       // TODO: Consider ways to address near-duplication with the
       // ValueParamPattern case.
-      if (param_pattern.runtime_index == SemIR::RuntimeParamIndex::Unknown) {
-        param_pattern.runtime_index = NextRuntimeIndex();
+      if (!param_pattern.index.has_value()) {
+        param_pattern.index = NextRuntimeIndex();
         ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
       }
+      auto param_id = AddInst<SemIR::OutParam>(
+          context, pattern_loc_id,
+          {.type_id = param_pattern.type_id,
+           .index = param_pattern.index,
+           .pretty_name_id = GetPrettyName(context, param_pattern)});
       AddWork({.pattern_id = param_pattern.subpattern_id,
-               .scrutinee_id = AddInst<SemIR::OutParam>(
-                   context, pattern_loc_id,
-                   {.type_id = param_pattern.type_id,
-                    .runtime_index = param_pattern.runtime_index,
-                    .pretty_name_id = GetPrettyName(context, param_pattern)})});
+               .scrutinee_id = param_id});
+      results_.push_back(param_id);
       break;
     }
     case MatchKind::Local: {
@@ -365,7 +369,6 @@ auto MatchContext::DoEmitPatternMatch(
           .LookupOrAddName(SemIR::NameId::ReturnSlot, return_slot_id)
           .has_value();
   CARBON_CHECK(!already_in_lookup);
-  results_.push_back(entry.scrutinee_id);
 }
 
 auto MatchContext::DoEmitPatternMatch(Context& context,
@@ -459,7 +462,6 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
 auto MatchContext::EmitPatternMatch(Context& context,
                                     MatchContext::WorkItem entry) -> void {
   if (entry.pattern_id == SemIR::ErrorInst::SingletonInstId) {
-    results_.push_back(SemIR::ErrorInst::SingletonInstId);
     return;
   }
   DiagnosticAnnotationScope annotate_diagnostics(
@@ -566,14 +568,6 @@ auto CallerPatternMatch(Context& context, SemIR::SpecificId specific_id,
   // Check type conversions per-element.
   for (auto [arg_id, param_pattern_id] : llvm::reverse(llvm::zip_equal(
            arg_refs, context.inst_blocks().GetOrEmpty(param_patterns_id)))) {
-    auto runtime_index = SemIR::Function::GetParamPatternInfoFromPatternId(
-                             context.sem_ir(), param_pattern_id)
-                             .inst.runtime_index;
-    if (!runtime_index.has_value()) {
-      // Not a runtime parameter: we don't pass an argument.
-      continue;
-    }
-
     match.AddWork({.pattern_id = param_pattern_id, .scrutinee_id = arg_id});
   }
 

diff --git a/toolchain/check/scope_stack.cpp b/toolchain/check/scope_stack.cpp
@@ -11,6 +11,7 @@ namespace Carbon::Check {
 
 auto ScopeStack::VerifyOnFinish() const -> void {
   CARBON_CHECK(scope_stack_.empty(), "{0}", scope_stack_.size());
+  full_pattern_stack_.VerifyOnFinish();
 }
 
 auto ScopeStack::VerifyNextCompileTimeBindIndex(llvm::StringLiteral label,

diff --git a/toolchain/check/testdata/alias/no_prelude/fail_params.carbon b/toolchain/check/testdata/alias/no_prelude/fail_params.carbon
@@ -31,8 +31,7 @@ alias A(T:! type) = T*;
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %T.ref: type = name_ref T, %T [symbolic = constants.%T]
 // CHECK:STDOUT:   %ptr: type = ptr_type %T [symbolic = constants.%ptr]
-// CHECK:STDOUT:   %T.param: type = value_param runtime_param<none>
-// CHECK:STDOUT:   %T: type = bind_symbolic_name T, 0, %T.param [symbolic = constants.%T]
+// CHECK:STDOUT:   %T: type = bind_symbolic_name T, 0 [symbolic = constants.%T]
 // CHECK:STDOUT:   %A: <error> = bind_alias A, <error> [concrete = <error>]
 // CHECK:STDOUT: }
 // CHECK:STDOUT: