Restructure handling of expressions in patterns (#7445)

Instead of maintaining a stack of pending subpatterns which might or
might not contain expressions, we mark non-nesting regions during
pattern handling that might contain an expression. The implementation
remains largely the same; the difference is that callers are expected to
end a pending expression region as soon as possible, rather than wait
for the end of the subpattern. This makes it possible to emit
non-pattern insts during pattern handling, without the risk that they
will get caught in a pending expression region further up the stack.

toolchain/check/cpp/import.cpp

@@ -1473,7 +1473,7 @@ static auto MakeSelfParamPatternBlockId(
   const auto* method_decl = cast<clang::CXXMethodDecl>(&clang_decl);
 
   // Build a `self` parameter from the object parameter.
-  BeginSubpattern(context);
+  BeginExprRegionForPattern(context);
 
   clang::QualType param_type =
       method_decl->getFunctionObjectParameterReferenceType();
@@ -1483,9 +1483,9 @@ static auto MakeSelfParamPatternBlockId(
   auto param_info = MapParameterType(context, loc_id, param_type, passing_mode);
   auto [type_inst_id, type_id] = param_info.type;
   SemIR::ExprRegionId type_expr_region_id =
-      ConsumeSubpatternExpr(context, type_inst_id);
+      ConsumeExprRegionForPattern(context, type_inst_id);
 
-  EndEmptySubpattern(context);
+  EndEmptyExprRegionForPattern(context);
 
   if (!type_id.has_value()) {
     context.TODO(loc_id,
@@ -1538,8 +1538,8 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
         ClangGetUnqualifiedTypePreserveNonNull(context, orig_param_type);
 
     // Mark the start of a region of insts, needed for the type expression
-    // created later with the call of `ConsumeSubpatternExpr()`.
-    BeginSubpattern(context);
+    // created later with the call of `ConsumeExprRegionForPattern()`.
+    BeginExprRegionForPattern(context);
     auto param_info = MapParameterType(context, loc_id, param_type,
                                        signature.GetPassingMode(i));
     auto [type_inst_id, type_id] = param_info.type;
@@ -1547,8 +1547,8 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
     // region that allows control flow in the type expression e.g. fn F(x: if C
     // then i32 else i64).
     SemIR::ExprRegionId type_expr_region_id =
-        ConsumeSubpatternExpr(context, type_inst_id);
-    EndEmptySubpattern(context);
+        ConsumeExprRegionForPattern(context, type_inst_id);
+    EndEmptyExprRegionForPattern(context);
 
     if (!type_id.has_value()) {
       context.TODO(loc_id, llvm::formatv("Unsupported: parameter type: {0}",

toolchain/check/function.cpp

@@ -117,29 +117,23 @@ static auto MakeFunctionSignature(Context& context, SemIR::LocId loc_id,
   if (!args.self_type_id.has_value() && args.param_type_ids.empty()) {
     insts.param_patterns_id = SemIR::InstBlockId::Empty;
   } else {
-    context.inst_block_stack().Push();
+    llvm::SmallVector<SemIR::InstId> param_patterns;
     if (args.self_type_id.has_value()) {
-      BeginSubpattern(context);
-      auto self_type_region_id = ConsumeSubpatternExpr(
+      auto self_type_region_id = MakeEmptyRegion(
           context, context.types().GetTypeInstId(args.self_type_id));
-      EndEmptySubpattern(context);
-
       insts.self_param_id = AddParamPattern(
           context, loc_id, SemIR::NameId::SelfValue, self_type_region_id,
           args.self_type_id, args.self_kind);
-      context.inst_block_stack().AddInstId(insts.self_param_id);
+      param_patterns.push_back(insts.self_param_id);
     }
     for (auto param_type_id : args.param_type_ids) {
-      BeginSubpattern(context);
-      auto param_type_region_id = ConsumeSubpatternExpr(
+      auto param_type_region_id = MakeEmptyRegion(
           context, context.types().GetTypeInstId(param_type_id));
-      EndEmptySubpattern(context);
-
-      context.inst_block_stack().AddInstId(AddParamPattern(
+      param_patterns.push_back(AddParamPattern(
           context, loc_id, SemIR::NameId::Underscore, param_type_region_id,
           param_type_id, args.param_kind));
     }
-    insts.param_patterns_id = context.inst_block_stack().Pop();
+    insts.param_patterns_id = context.inst_blocks().Add(param_patterns);
   }
   context.full_pattern_stack().EndExplicitParamList();
 

toolchain/check/handle_binding_pattern.cpp

@@ -167,7 +167,7 @@ static auto HandleAnyBindingPattern(
                                                self_type_inst_id);
 
   SemIR::ExprRegionId type_expr_region_id =
-      ConsumeSubpatternExpr(context, type_expr.inst_id);
+      ConsumeExprRegionForPattern(context, type_expr.inst_id);
 
   // The name in a generic binding may be wrapped in `template`.
   bool is_generic = node_kind == Parse::NodeKind::CompileTimeBindingPattern;
@@ -502,7 +502,7 @@ auto HandleParseNode(Context& context,
   auto [cast_type_inst_id, cast_type_id] =
       ExprAsType(context, type_node, parsed_type_id);
 
-  auto region_id = ConsumeSubpatternExpr(context, cast_type_inst_id);
+  auto region_id = ConsumeExprRegionForPattern(context, cast_type_inst_id);
   // TODO: Should we be tracking this somewhere?
   (void)region_id;
 

toolchain/check/handle_let_and_var.cpp

@@ -74,7 +74,7 @@ static auto HandleIntroducer(Context& context, Parse::NodeId node_id) -> bool {
   } else {
     context.full_pattern_stack().PushNameBindingDecl();
   }
-  BeginSubpattern(context);
+  BeginExprRegionForPattern(context);
   return true;
 }
 
@@ -97,11 +97,13 @@ auto HandleParseNode(Context& context, Parse::VariableIntroducerId node_id)
 
 auto HandleParseNode(Context& context, Parse::VariablePatternId node_id)
     -> bool {
+  EndExprRegionForPattern(context, context.node_stack());
   auto subpattern_id = context.node_stack().PopPattern();
   auto type_id = context.insts().Get(subpattern_id).type_id();
 
   if (subpattern_id == SemIR::ErrorInst::InstId) {
     context.node_stack().Push(node_id, SemIR::ErrorInst::InstId);
+    BeginExprRegionForPattern(context);
     return true;
   }
 
@@ -140,13 +142,14 @@ auto HandleParseNode(Context& context, Parse::VariablePatternId node_id)
   }
 
   context.node_stack().Push(node_id, pattern_id);
+  BeginExprRegionForPattern(context);
   return true;
 }
 
 // Handle the end of the full-pattern of a let/var declaration (before the
 // start of the initializer, if any).
 static auto EndFullPattern(Context& context) -> void {
-  EndSubpattern(context, context.node_stack());
+  EndExprRegionForPattern(context, context.node_stack());
   if (context.name_scopes().InstIs<SemIR::InterfaceWithSelfDecl>(
           context.scope_stack().PeekNameScopeId())) {
     // Don't emit NameBindingDecl for an associated constant, because it will

toolchain/check/handle_loop_statement.cpp

@@ -117,14 +117,14 @@ auto HandleParseNode(Context& context, Parse::ForHeaderStartId node_id)
   context.decl_introducer_state_stack().Push<Lex::TokenKind::Let>();
   context.pattern_block_stack().Push();
   context.full_pattern_stack().PushNameBindingDecl();
-  BeginSubpattern(context);
+  BeginExprRegionForPattern(context);
 
   context.node_stack().Push(node_id);
   return true;
 }
 
 auto HandleParseNode(Context& context, Parse::ForInId node_id) -> bool {
-  EndSubpattern(context, context.node_stack());
+  EndExprRegionForPattern(context, context.node_stack());
   auto pattern_block_id = context.pattern_block_stack().Pop();
   AddInst<SemIR::NameBindingDecl>(context, node_id,
                                   {.pattern_block_id = pattern_block_id});

toolchain/check/handle_pattern_list.cpp

@@ -17,7 +17,7 @@ static auto HandlePatternListStart(Context& context, Parse::NodeId node_id)
     -> bool {
   context.node_stack().Push(node_id);
   context.param_and_arg_refs_stack().Push();
-  BeginSubpattern(context);
+  BeginExprRegionForPattern(context);
   return true;
 }
 
@@ -29,6 +29,9 @@ auto HandleParseNode(Context& context, Parse::ImplicitParamListStartId node_id)
 
 auto HandleParseNode(Context& context, Parse::TuplePatternStartId node_id)
     -> bool {
+  // End the pending `ExprRegion`, so that we can start a new one in
+  // `HandlePatternListStart`.
+  EndEmptyExprRegionForPattern(context);
   return HandlePatternListStart(context, node_id);
 }
 
@@ -43,11 +46,12 @@ auto HandleParseNode(Context& context, Parse::ExplicitParamListStartId node_id)
 static auto HandleParamListEnd(Context& context, Parse::NodeId node_id,
                                Parse::NodeKind start_kind) -> bool {
   if (context.node_stack().PeekIs(start_kind)) {
-    // End the subpattern started by a trailing comma, or the opening delimiter
-    // of an empty list.
-    EndEmptySubpattern(context);
+    // End the pending region started by a trailing comma, or the opening
+    // delimiter of an empty list.
+    EndEmptyExprRegionForPattern(context);
   } else {
-    EndSubpattern(context, context.node_stack());
+    // End the pending region for the last pattern in the list.
+    EndExprRegionForPattern(context, context.node_stack());
   }
   // Note the Start node remains on the stack, where the param list handler can
   // make use of it.
@@ -71,22 +75,26 @@ auto HandleParseNode(Context& context, Parse::ExplicitParamListId node_id)
 }
 
 auto HandleParseNode(Context& context, Parse::ParenPatternId node_id) -> bool {
-  EndSubpattern(context, context.node_stack());
+  EndExprRegionForPattern(context, context.node_stack());
   auto pattern_id = context.node_stack().PopPattern();
   context.param_and_arg_refs_stack().PopAndDiscard();
   context.node_stack()
       .PopAndDiscardSoloNodeId<Parse::NodeKind::TuplePatternStart>();
   context.node_stack().Push(node_id, pattern_id);
+  // Start a new pending `ExprRegion`, to maintain the invariant that one is
+  // pending at the end of handling for a pattern.
+  BeginExprRegionForPattern(context);
   return true;
 }
 
 auto HandleParseNode(Context& context, Parse::TuplePatternId node_id) -> bool {
   if (context.node_stack().PeekIs(Parse::NodeKind::TuplePatternStart)) {
-    // End the subpattern started by a trailing comma, or the opening delimiter
-    // of an empty list.
-    EndEmptySubpattern(context);
+    // End the pending region started by a trailing comma, or the opening
+    // delimiter of an empty list.
+    EndEmptyExprRegionForPattern(context);
   } else {
-    EndSubpattern(context, context.node_stack());
+    // End the pending region for the last pattern in the list.
+    EndExprRegionForPattern(context, context.node_stack());
   }
   auto refs_id = context.param_and_arg_refs_stack().EndAndPop(
       Parse::NodeKind::TuplePatternStart);
@@ -115,14 +123,17 @@ auto HandleParseNode(Context& context, Parse::TuplePatternId node_id) -> bool {
       node_id,
       AddInst<SemIR::TuplePattern>(
           context, node_id, {.type_id = type_id, .elements_id = refs_id}));
+  // Start a new pending `ExprRegion`, to maintain the invariant that one is
+  // pending at the end of handling for a pattern.
+  BeginExprRegionForPattern(context);
   return true;
 }
 
 auto HandleParseNode(Context& context, Parse::PatternListCommaId /*node_id*/)
     -> bool {
-  EndSubpattern(context, context.node_stack());
+  EndExprRegionForPattern(context, context.node_stack());
   context.param_and_arg_refs_stack().ApplyComma();
-  BeginSubpattern(context);
+  BeginExprRegionForPattern(context);
   return true;
 }
 

toolchain/check/pattern.cpp

@@ -17,14 +17,14 @@
 
 namespace Carbon::Check {
 
-auto BeginSubpattern(Context& context) -> void {
+auto BeginExprRegionForPattern(Context& context) -> void {
   context.inst_block_stack().Push();
   // TODO: This allocates an InstBlockId even in the case where the pattern has
   // no associated expression. Find a way to avoid this.
   context.region_stack().PushRegion(context.inst_block_stack().PeekOrAdd());
 }
 
-static auto PopSubpatternExpr(Context& context, SemIR::InstId result_id)
+static auto PopExprRegion(Context& context, SemIR::InstId result_id)
     -> SemIR::ExprRegionId {
   if (context.region_stack().PeekRegion().size() > 1) {
     // End the exit block with a branch to a successor block, whose contents
@@ -46,16 +46,16 @@ static auto PopSubpatternExpr(Context& context, SemIR::InstId result_id)
        .result_id = result_id});
 }
 
-auto ConsumeSubpatternExpr(Context& context, SemIR::InstId result_id)
+auto ConsumeExprRegionForPattern(Context& context, SemIR::InstId result_id)
     -> SemIR::ExprRegionId {
-  auto region_id = PopSubpatternExpr(context, result_id);
+  auto region_id = PopExprRegion(context, result_id);
   // Push an empty, unreachable region so that we can later detect the region
   // has been consumed.
   context.region_stack().PushUnreachableRegion();
   return region_id;
 }
 
-auto EndEmptySubpattern(Context& context) -> void {
+auto EndEmptyExprRegionForPattern(Context& context) -> void {
   if (!context.region_stack().PeekRegion().empty()) {
     CARBON_CHECK(context.inst_block_stack().PeekCurrentBlockContents().empty());
     auto block_id = context.inst_block_stack().Pop();
@@ -65,13 +65,13 @@ auto EndEmptySubpattern(Context& context) -> void {
   context.region_stack().PopAndDiscardRegion();
 }
 
-auto EndSubpattern(Context& context, NodeStack& node_stack) -> void {
+auto EndExprRegionForPattern(Context& context, NodeStack& node_stack) -> void {
   auto [node_id, maybe_expr_id] =
       node_stack.PopWithNodeIdIf<Parse::NodeCategory::Expr>();
   if (maybe_expr_id) {
     // We formed an expression, not a pattern, so convert it to an expression
     // pattern now.
-    auto expr_region_id = PopSubpatternExpr(context, *maybe_expr_id);
+    auto expr_region_id = PopExprRegion(context, *maybe_expr_id);
     auto pattern_type_id =
         GetPatternType(context, context.insts().Get(*maybe_expr_id).type_id());
     node_stack.Push(node_id, AddInst<SemIR::ExprPattern>(
@@ -81,10 +81,16 @@ auto EndSubpattern(Context& context, NodeStack& node_stack) -> void {
   } else {
     // The expression region should have been consumed when forming the pattern
     // instruction, so should now effectively be empty.
-    EndEmptySubpattern(context);
+    EndEmptyExprRegionForPattern(context);
   }
 }
 
+auto MakeEmptyRegion(Context& context, SemIR::InstId result_id)
+    -> SemIR::ExprRegionId {
+  return context.sem_ir().expr_regions().Add(
+      {.block_ids = {SemIR::InstBlockId::Empty}, .result_id = result_id});
+}
+
 auto AddBindingEntityName(Context& context, SemIR::NameId name_id,
                           SemIR::InstId form_id, bool is_unused,
                           BindingPhase phase) -> SemIR::EntityNameId {

toolchain/check/pattern.h

@@ -10,32 +10,43 @@
 
 namespace Carbon::Check {
 
-// Marks the start of a region of insts in a pattern context that might contain
-// an expression. Typically this is called when handling a parse node that can
-// immediately precede a subpattern (such as `let` or a `,` in a pattern list).
-// `End[Empty]Subpattern` should be called later by the consumer of the
-// subpattern.
-auto BeginSubpattern(Context& context) -> void;
+// The following functions are used to mark the start and end of a time interval
+// during pattern handling, in which we may build an `ExprRegion` to represent
+// an expression. During one of these intervals, we say that an `ExprRegion` is
+// _pending_. Any insts added to `inst_block_stack` while an `ExprRegion` is
+// pending are treated as part of the expression. These intervals do not nest:
+// we can't start an interval if an `ExprRegion` is already pending.
+//
+// To ensure that each start has a matching end, without nesting, callers should
+// maintain the invariant that an `ExprRegion` is pending before handling the
+// start of a pattern, and after handling the end of a pattern.
 
-// Consumes the expression in a region started by the most recent
-// BeginSubpattern, and returns the ID of the region. The region will not yet
-// have any control-flow edges into or out of it.
-auto ConsumeSubpatternExpr(Context& context, SemIR::InstId result_id)
+// Marks the start of a pending `ExprRegion` (see above).
+auto BeginExprRegionForPattern(Context& context) -> void;
+
+// Finishes building the pending `ExprRegion`, and returns its ID. It will not
+// yet have any control-flow edges into or out of it. An empty `ExprRegion` will
+// still be pending after the call, so `End[Empty]ExprRegionForPattern` must be
+// called separately after this.
+auto ConsumeExprRegionForPattern(Context& context, SemIR::InstId result_id)
     -> SemIR::ExprRegionId;
 
-// Ends a region started by BeginSubpattern (in stack order), asserting that
-// it either had no expression content or the expression has been consumed.
-auto EndEmptySubpattern(Context& context) -> void;
+// Ends the pending `ExprRegion`, and asserts that it is empty.
+auto EndEmptyExprRegionForPattern(Context& context) -> void;
 
-// Ends a region started by BeginSubpattern (in stack order). If the top of the
-// node stack is an expression, the subpattern region is consumed and converted
-// to an expression pattern, which replaces the expression on the node stack.
-// Otherwise, the top of the node stack should be a pattern, in which case this
-// asserts that the subpattern region is either empty or has been consumed.
+// Ends the pending `ExprRegion`. If the top of the node stack is an expression,
+// the `ExprRegion` is consumed and converted to an expression pattern, which
+// replaces the expression on the node stack. Otherwise, the top of the node
+// stack should be a pattern, in which case this asserts that the pending region
+// is empty, and discards it.
 //
 // The node stack is passed explicitly as a reminder that this function affects
-// the node stack, unlike the other *Subpattern functions.
-auto EndSubpattern(Context& context, NodeStack& node_stack) -> void;
+// the node stack, unlike the other `*ExprRegionForPattern` functions.
+auto EndExprRegionForPattern(Context& context, NodeStack& node_stack) -> void;
+
+// Builds and returns an empty `ExprRegion`.
+auto MakeEmptyRegion(Context& context, SemIR::InstId result_id)
+    -> SemIR::ExprRegionId;
 
 // Information about a created binding pattern.
 struct BindingPatternInfo {

toolchain/check/pattern_match.cpp

@@ -531,7 +531,9 @@ auto MatchContext::DoPreWork(State state, SemIR::AnyParamPattern param_pattern,
                  .work = PreWork{.scrutinee_id = param_id},
                  .allow_unmarked_ref = entry.allow_unmarked_ref});
       } else {
-        results_stack_.AppendToTop(param_id);
+        if (need_subpattern_results()) {
+          results_stack_.AppendToTop(param_id);
+        }
       }
 
       callee_state->PushCallParamPattern(context_, loc_id, entry.pattern_id,

toolchain/docs/check/pattern_matching.md

@@ -126,11 +126,12 @@ expressions of binding patterns, and expressions that are used as patterns
 themselves (although those have not been implemented yet). The parse tree
 doesn't mark these situations in advance: any given subpattern might turn out to
 be one that emits non-pattern instructions. To handle these situations, we
-speculatively push an instruction block onto the (non-pattern) stack whenever we
-are about to begin handling a subpattern, and then pop it at the end of the
-subpattern, with different treatment depending on whether the subpattern turned
-out to involve a subexpression. This is handled by `BeginSubpattern`,
-`ConsumeSubpatternExpr`, `EndSubpattern`, and `EndEmptySubpattern`.
+speculatively prepare to build an `ExprRegion` whenever we are about to begin
+handling a pattern that might be a binding or expression pattern, and then
+either consume or discard the region once we've passed the point where the
+expression (if any) would appear. This is handled by `BeginExprRegionForPattern`,
+`ConsumeExprRegionForPattern`, `EndExprRegionForPattern`, and
+`EndEmptyExprRegionForPattern`.
 
 One further complication here is that the type expression can contain control
 flow (such as an `if` expression). Consequently, we can't represent the type