Late response to comments on #4698 (#4758)

toolchain/check/context.cpp

@@ -867,7 +867,8 @@ auto Context::EndSubpatternAsExpr(SemIR::InstId result_id)
 
 auto Context::EndSubpatternAsEmpty() -> void {
   auto block_id = inst_block_stack().Pop();
-  CARBON_CHECK(block_id == region_stack_.PeekArray().front());
+  CARBON_CHECK(block_id == region_stack_.PeekArray().back());
+  CARBON_CHECK(region_stack_.PeekArray().size() == 1);
   CARBON_CHECK(inst_blocks().Get(block_id).empty());
   region_stack_.PopArray();
 }

toolchain/check/context.h

@@ -638,7 +638,10 @@ class Context {
   auto global_init() -> GlobalInit& { return global_init_; }
 
   // Marks the start of a region of insts in a pattern context that might
-  // represent an expression or a pattern.
+  // represent an expression or a pattern. Typically this is called when
+  // handling a parse node that can immediately precede a subpattern (such
+  // as `let` or a `,` in a pattern list), and the handler for the subpattern
+  // node makes the matching `EndSubpatternAs*` call.
   auto BeginSubpattern() -> void;
 
   // Ends a region started by BeginSubpattern (in stack order), treating it as
@@ -673,7 +676,7 @@ class Context {
     // The corresponding AnyBindName inst.
     SemIR::InstId bind_name_id;
     // The region of insts that computes the type of the binding.
-    SemIR::ExprRegionId type_expr_id;
+    SemIR::ExprRegionId type_expr_region_id;
   };
   auto bind_name_map() -> Map<SemIR::InstId, BindingPatternInfo>& {
     return bind_name_map_;

toolchain/check/handle_binding_pattern.cpp

@@ -226,11 +226,11 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
               name_node,
               {.type_id = cast_type_id, .entity_name_id = entity_name_id});
         }
-        bool inserted =
-            context.bind_name_map()
-                .Insert(pattern_inst_id, {.bind_name_id = bind_id,
-                                          .type_expr_id = type_expr_region_id})
-                .is_inserted();
+        bool inserted = context.bind_name_map()
+                            .Insert(pattern_inst_id, {.bind_name_id = bind_id,
+                                                      .type_expr_region_id =
+                                                          type_expr_region_id})
+                            .is_inserted();
         CARBON_CHECK(inserted);
         param_pattern_id = context.AddPatternInst<SemIR::ValueParamPattern>(
             node_id,

toolchain/check/handle_operator.cpp

@@ -404,7 +404,7 @@ static auto HandleShortCircuitOperator(Context& context, Parse::NodeId node_id)
   context.AddInst<SemIR::BranchWithArg>(
       node_id, {.target_id = resume_block_id, .arg_id = rhs_id});
   context.inst_block_stack().Pop();
-  context.AddToRegion(context.inst_block_stack().PeekOrAdd(), node_id);
+  context.AddToRegion(resume_block_id, node_id);
 
   // Collect the result from either the first or second operand.
   auto result_id = context.AddInst<SemIR::BlockArg>(

toolchain/check/handle_pattern_list.cpp

@@ -32,7 +32,7 @@ auto HandleParseNode(Context& context, Parse::ImplicitParamListId node_id)
   return true;
 }
 
-static auto HandleTuplePatternStart(Context& context, Parse::NodeId node_id)
+auto HandleParseNode(Context& context, Parse::TuplePatternStartId node_id)
     -> bool {
   context.node_stack().Push(node_id);
   context.param_and_arg_refs_stack().Push();
@@ -40,11 +40,6 @@ static auto HandleTuplePatternStart(Context& context, Parse::NodeId node_id)
   return true;
 }
 
-auto HandleParseNode(Context& context, Parse::TuplePatternStartId node_id)
-    -> bool {
-  return HandleTuplePatternStart(context, node_id);
-}
-
 auto HandleParseNode(Context& context, Parse::PatternListCommaId /*node_id*/)
     -> bool {
   context.param_and_arg_refs_stack().ApplyComma();

toolchain/check/pattern_match.cpp

@@ -141,9 +141,13 @@ auto MatchContext::EmitPatternMatch(Context& context,
     case SemIR::BindingPattern::Kind:
     case SemIR::SymbolicBindingPattern::Kind: {
       CARBON_CHECK(kind_ == MatchKind::Callee);
-      auto [bind_name_id, type_expr_id] =
-          context.bind_name_map().Lookup(entry.pattern_id).value();
-      context.InsertHere(type_expr_id);
+      // 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::Invalid,
+           .type_expr_region_id = SemIR::ExprRegionId::Invalid});
+      context.InsertHere(type_expr_region_id);
       auto bind_name = context.insts().GetAs<SemIR::AnyBindName>(bind_name_id);
       CARBON_CHECK(!bind_name.value_id.is_valid());
       bind_name.value_id = entry.scrutinee_id;