Build VarStorage insts more efficiently. (#7422)

Instead of traversing the entire pattern block looking for
`VarPattern`s, we keep track of them on creation, and then build
`VarStorage`s directly from that list.

This also gets rid of the global `var_storage_map`, and instead keep
that information narrowly scoped to each full-pattern, and consume it in
a single linear traversal instead of with random-access lookups. To
enable that, this fixes a parse bug where nested `var` patterns were
getting diagnosed but not marked as errors.

toolchain/check/BUILD

@@ -43,6 +43,7 @@ cc_library(
         "eval.cpp",
         "eval_inst.cpp",
         "facet_type.cpp",
+        "full_pattern_stack.cpp",
         "function.cpp",
         "generic.cpp",
         "global_init.cpp",

toolchain/check/context.h

@@ -206,10 +206,6 @@ class Context {
     return bind_name_map_;
   }
 
-  auto var_storage_map() -> Map<SemIR::InstId, SemIR::InstId>& {
-    return var_storage_map_;
-  }
-
   // During Choice typechecking, each alternative turns into a name binding on
   // the Choice type, but this can't be done until the full Choice type is
   // known. This represents each binding to be done at the end of checking the
@@ -531,11 +527,6 @@ class Context {
   // pattern-match SemIR for it.
   Map<SemIR::InstId, BindingPatternInfo> bind_name_map_;
 
-  // Map from VarPattern insts to the corresponding VarStorage insts. The
-  // VarStorage insts are allocated, emitted, and stored in the map after
-  // processing the enclosing full-pattern.
-  Map<SemIR::InstId, SemIR::InstId> var_storage_map_;
-
   // Each alternative in a Choice gets an entry here, they are stored in
   // declaration order. The vector is consumed and emptied at the end of the
   // Choice definition.

toolchain/check/full_pattern_stack.cpp

@@ -0,0 +1,58 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "toolchain/check/full_pattern_stack.h"
+
+#include "toolchain/check/context.h"
+#include "toolchain/check/pattern.h"
+
+namespace Carbon::Check {
+
+auto FullPatternStack::StartPatternInitializer() -> void {
+  CARBON_CHECK(kind_stack_.back() == Kind::ClassScopeVarDecl ||
+               kind_stack_.back() == Kind::NameBindingDecl);
+  for (auto& [name_id, inst_id] : bind_name_stack_.PeekArray()) {
+    CARBON_CHECK(
+        inst_id == SemIR::InstId::InitTombstone,
+        "stashing the lookup result would overwrite an existing stash {0}",
+        inst_id);
+    auto& lookup_result = lookup_->Get(name_id);
+    if (!lookup_result.empty()) {
+      // Temporarily overwrite the result of name lookup for this binding to be
+      // `InitTombstone`, so that references to it in the initializer are
+      // diagnosed as errors. The original result of name lookup is stashed in
+      // `bind_name_stack_` so we can restore it later.
+      //
+      // TODO: find a way to preserve location information, so that we can
+      // provide good diagnostics for a redeclaration of `name_id` in
+      // the initializer, if that becomes possible.
+      std::swap(lookup_result.back().inst_id, inst_id);
+    }
+  }
+}
+
+auto FullPatternStack::EndPatternInitializer() -> void {
+  for (auto& [name_id, inst_id] : bind_name_stack_.PeekArray()) {
+    auto& lookup_result = lookup_->Get(name_id);
+    if (!lookup_result.empty()) {
+      // Restore the original result of name lookup for this binding.
+      std::swap(lookup_result.back().inst_id, inst_id);
+    }
+    CARBON_CHECK(inst_id == SemIR::InstId::InitTombstone,
+                 "name_id resolved to a non-tombstone value {0} during "
+                 "initializer handling",
+                 inst_id);
+  }
+}
+
+auto FullPatternStack::BuildLocalVarStorage(Context& context,
+                                            bool is_returned_var) -> void {
+  for (auto& var_info : var_pattern_stack_.PeekArray()) {
+    var_info.storage_id =
+        GetOrAddVarStorage(context, var_info.pattern_id, is_returned_var);
+  }
+  next_var_index_stack_.back() = 0;
+}
+
+}  // namespace Carbon::Check

toolchain/check/full_pattern_stack.h

@@ -12,6 +12,8 @@
 
 namespace Carbon::Check {
 
+class Context;
+
 // Stack of full-patterns currently being checked (a full-pattern is a pattern
 // that is not part of an enclosing pattern). It is structured as a stack to
 // handle situations like a pattern that contains an initializer, or a pattern
@@ -76,18 +78,24 @@ class FullPatternStack {
   auto PushParameterizedDecl() -> void {
     kind_stack_.push_back(Kind::NotInEitherParamList);
     bind_name_stack_.PushArray();
+    var_pattern_stack_.PushArray();
+    next_var_index_stack_.push_back(-1);
   }
 
   // Marks the start of a new full-pattern for a name binding declaration.
   auto PushNameBindingDecl() -> void {
     kind_stack_.push_back(Kind::NameBindingDecl);
     bind_name_stack_.PushArray();
+    var_pattern_stack_.PushArray();
+    next_var_index_stack_.push_back(-1);
   }
 
   // Marks the start of a new full-pattern for a class `var` declaration.
   auto PushClassScopeVarDecl() -> void {
     kind_stack_.push_back(Kind::ClassScopeVarDecl);
     bind_name_stack_.PushArray();
+    var_pattern_stack_.PushArray();
+    next_var_index_stack_.push_back(-1);
   }
 
   // Marks the start of the current parameterized entity's implicit parameter
@@ -123,37 +131,21 @@ class FullPatternStack {
   }
 
   // Marks the start of the initializer for the current name binding decl.
-  auto StartPatternInitializer() -> void {
-    CARBON_CHECK(kind_stack_.back() == Kind::ClassScopeVarDecl ||
-                 kind_stack_.back() == Kind::NameBindingDecl);
-    for (auto& [name_id, inst_id] : bind_name_stack_.PeekArray()) {
-      CARBON_CHECK(inst_id == SemIR::InstId::InitTombstone);
-      auto& lookup_result = lookup_->Get(name_id);
-      if (!lookup_result.empty()) {
-        // TODO: find a way to preserve location information, so that we can
-        // provide good diagnostics for a redeclaration of `name_id` in
-        // the initializer, if that becomes possible.
-        std::swap(lookup_result.back().inst_id, inst_id);
-      }
-    }
-  }
+  auto StartPatternInitializer() -> void;
 
   // Marks the end of the initializer for the current name-binding decl.
-  auto EndPatternInitializer() -> void {
-    for (auto& [name_id, inst_id] : bind_name_stack_.PeekArray()) {
-      auto& lookup_result = lookup_->Get(name_id);
-      if (!lookup_result.empty()) {
-        std::swap(lookup_result.back().inst_id, inst_id);
-      }
-      CARBON_CHECK(inst_id == SemIR::InstId::InitTombstone);
-    }
-  }
+  auto EndPatternInitializer() -> void;
 
-  // Marks the end of checking for the current full-pattern. This cannot be
-  // called while processing an initializer for the top pattern.
+  // Marks the end of checking and pattern matching for the current
+  // full-pattern.
   auto PopFullPattern() -> void {
     kind_stack_.pop_back();
     bind_name_stack_.PopArray();
+    int index = next_var_index_stack_.pop_back_val();
+    CARBON_CHECK(index < 0 || static_cast<size_t>(index) ==
+                                  var_pattern_stack_.PeekArray().size(),
+                 "`GetLocalVarStorage` not called for all var patterns");
+    var_pattern_stack_.PopArray();
   }
 
   // Records that `name_id` was introduced by the current full-pattern.
@@ -162,6 +154,44 @@ class FullPatternStack {
         {.name_id = name_id, .inst_id = SemIR::InstId::InitTombstone});
   }
 
+  // Records a `VarPattern` inst as part of the current full pattern, so that
+  // its `VarStorage` can be allocated and tracked. This should only be called
+  // when the current full-pattern is a kind that can have an initializer;
+  // otherwise the `VarStorage` should be allocated on demand during pattern
+  // matching.
+  auto AddLocalVarPattern(SemIR::InstId var_pattern_id) -> void {
+    CARBON_CHECK(kind_stack_.back() == Kind::ClassScopeVarDecl ||
+                 kind_stack_.back() == Kind::NameBindingDecl);
+    CARBON_CHECK(next_var_index_stack_.back() < 0);
+    var_pattern_stack_.AppendToTop(
+        {.pattern_id = var_pattern_id, .storage_id = SemIR::InstId::None});
+  }
+
+  // Creates `VarStorage` insts for all `VarPattern` insts recorded by
+  // `AddLocalVarPattern` for the current full-pattern. This must typically
+  // be called before handling the initializer (if any) for the current full-
+  // pattern, in order to preserve the dominance ordering (see the comments
+  // on `Check::Initialize` for details).
+  auto BuildLocalVarStorage(Context& context, bool is_returned_var) -> void;
+
+  // Returns the `VarStorage` inst that was allocated for `pattern_id` by
+  // `BuildLocalVarStorage`.
+  //
+  // As an optimization, this assumes (and enforces) that it will be called
+  // exactly once for each inst passed to `AddLocalVarPattern`, and in the same
+  // order.
+  auto GetLocalVarStorage(SemIR::InstId var_pattern_id) -> SemIR::InstId {
+    CARBON_CHECK(kind_stack_.back() == Kind::ClassScopeVarDecl ||
+                 kind_stack_.back() == Kind::NameBindingDecl);
+    auto& index = next_var_index_stack_.back();
+    CARBON_CHECK(index >= 0);
+    auto var_info = var_pattern_stack_.PeekArray()[index];
+    CARBON_CHECK(var_info.pattern_id == var_pattern_id,
+                 "var patterns visited in unexpected order");
+    ++index;
+    return var_info.storage_id;
+  }
+
   // Runs verification that the processing cleanly finished.
   auto VerifyOnFinish() const -> void {
     CARBON_CHECK(kind_stack_.empty(),
@@ -172,13 +202,39 @@ class FullPatternStack {
  private:
   LexicalLookup* lookup_;
 
+  // The stack of pending full-patterns is organized as a struct of arrays, with
+  // separate stacks for separate properties of a full-pattern.
+
+  // The kinds of the currently pending full patterns.
   llvm::SmallVector<Kind> kind_stack_;
 
-  struct LookupEntry {
+  // Locally stashed name-lookup information about a binding.
+  struct BindingInfo {
+    // The name of the binding.
     SemIR::NameId name_id;
+    // While handling the initializer, name lookup for `name_id` in `lookup_`
+    // temporarily resolves to `InitTombstone`. During that time, this records
+    // the inst that it resolved to before the initializer, so that it can
+    // be restored afterward. This is `InitTombstone` while not handling the
+    // initializer, or if `name_id` doesn't resolve in `lookup_`.
     SemIR::InstId inst_id;
   };
-  ArrayStack<LookupEntry> bind_name_stack_;
+
+  // The name bindings introduced by the currently pending full-patterns.
+  ArrayStack<BindingInfo> bind_name_stack_;
+
+  struct VarInfo {
+    SemIR::InstId pattern_id;
+    SemIR::InstId storage_id;
+  };
+
+  // The `var` patterns introduced by the currently pending full-patterns.
+  ArrayStack<VarInfo> var_pattern_stack_;
+
+  // For each full pattern, the index of the first un-consumed `VarInfo` in
+  // the corresponding frame of `var_pattern_stack_`, or -1 if the contents
+  // of that frame are not ready for consumption.
+  llvm::SmallVector<int> next_var_index_stack_;
 };
 
 }  // namespace Carbon::Check

toolchain/check/handle_let_and_var.cpp

@@ -119,6 +119,7 @@ auto HandleParseNode(Context& context, Parse::VariablePatternId node_id)
       pattern_id = AddInst<SemIR::VarPattern>(
           context, node_id,
           {.type_id = type_id, .subpattern_id = subpattern_id});
+      context.full_pattern_stack().AddLocalVarPattern(pattern_id);
       break;
     case FullPatternStack::Kind::ClassScopeVarDecl:
       if (InStaticClassScopeVar(context)) {
@@ -126,6 +127,7 @@ auto HandleParseNode(Context& context, Parse::VariablePatternId node_id)
         pattern_id = AddInst<SemIR::VarPattern>(
             context, node_id,
             {.type_id = type_id, .subpattern_id = subpattern_id});
+        context.full_pattern_stack().AddLocalVarPattern(pattern_id);
       } else {
         // For non-static class fields, a `FieldDecl` was created in
         // `AddBindingPattern`. Use that as the `pattern_id` so that
@@ -167,7 +169,7 @@ static auto EndFullPattern(Context& context) -> void {
   bool returned =
       context.decl_introducer_state_stack().innermost().modifier_set.HasAnyOf(
           KeywordModifierSet::Returned);
-  AddPatternVarStorage(context, pattern_block_id, returned);
+  context.full_pattern_stack().BuildLocalVarStorage(context, returned);
 }
 
 static auto StartPatternInitializer(Context& context) -> bool {
@@ -315,7 +317,6 @@ auto HandleParseNode(Context& context, Parse::LetDeclId node_id) -> bool {
   auto decl_info =
       HandleDecl<Lex::TokenKind::Let, Parse::NodeKind::LetIntroducer,
                  Parse::NodeKind::LetInitializer>(context, node_id);
-  context.full_pattern_stack().PopFullPattern();
   context.decl_introducer_state_stack().Pop<Lex::TokenKind::Let>();
 
   LimitModifiersOnDecl(
@@ -337,6 +338,7 @@ auto HandleParseNode(Context& context, Parse::LetDeclId node_id) -> bool {
     context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
                            ExpectedInitializerAfterLet);
   }
+  context.full_pattern_stack().PopFullPattern();
   return true;
 }
 

toolchain/check/handle_loop_statement.cpp

@@ -130,7 +130,6 @@ auto HandleParseNode(Context& context, Parse::ForInId node_id) -> bool {
                                   {.pattern_block_id = pattern_block_id});
   context.decl_introducer_state_stack().Pop<Lex::TokenKind::Let>();
   context.full_pattern_stack().StartPatternInitializer();
-  context.node_stack().Push(node_id, pattern_block_id);
   return true;
 }
 
@@ -147,7 +146,6 @@ static auto CallOptionalAccessor(Context& context, Parse::NodeId node_id,
 
 auto HandleParseNode(Context& context, Parse::ForHeaderId node_id) -> bool {
   auto range_id = context.node_stack().PopExpr();
-  auto pattern_block_id = context.node_stack().Pop<Parse::NodeKind::ForIn>();
   auto pattern_id = context.node_stack().PopPattern();
   auto start_node_id =
       context.node_stack().PopForSoloNodeId<Parse::NodeKind::ForHeaderStart>();
@@ -212,15 +210,16 @@ auto HandleParseNode(Context& context, Parse::ForHeaderId node_id) -> bool {
   // The loop pattern's initializer is now complete, and any bindings in it
   // should be in scope.
   context.full_pattern_stack().EndPatternInitializer();
-  context.full_pattern_stack().PopFullPattern();
 
   // Create storage for var patterns now.
-  AddPatternVarStorage(context, pattern_block_id, /*is_returned_var=*/false);
+  context.full_pattern_stack().BuildLocalVarStorage(context,
+                                                    /*is_returned_var=*/false);
 
   // Initialize the pattern from `<element>.Get()`.
   auto element_value_id =
       CallOptionalAccessor(context, node_id, element_id, CoreIdentifier::Get);
   LocalPatternMatch(context, pattern_id, element_value_id);
+  context.full_pattern_stack().PopFullPattern();
   return true;
 }
 

toolchain/check/node_stack.h

@@ -421,7 +421,6 @@ class NodeStack {
       case Parse::NodeKind::StructLiteralField:
         return Id::KindFor<SemIR::InstId>();
       case Parse::NodeKind::ExplicitParamList:
-      case Parse::NodeKind::ForIn:
       case Parse::NodeKind::IfCondition:
       case Parse::NodeKind::IfExprIf:
       case Parse::NodeKind::ImplicitParamList:
@@ -497,6 +496,7 @@ class NodeStack {
       case Parse::NodeKind::FileEnd:
       case Parse::NodeKind::FileStart:
       case Parse::NodeKind::ForHeader:
+      case Parse::NodeKind::ForIn:
       case Parse::NodeKind::Forall:
       case Parse::NodeKind::FormLiteralKeyword:
       case Parse::NodeKind::FormLiteralOpenParen:

toolchain/check/pattern.cpp

@@ -206,11 +206,8 @@ auto AddBindingPattern(Context& context, SemIR::LocId name_loc,
   return {.pattern_id = binding_pattern_id, .bind_id = bind_id};
 }
 
-// Returns a VarStorage inst for the given `var` pattern. If the pattern
-// is the body of a returned var, this reuses the return parameter, and
-// otherwise it adds a new inst.
-static auto GetOrAddVarStorage(Context& context, SemIR::InstId var_pattern_id,
-                               bool is_returned_var) -> SemIR::InstId {
+auto GetOrAddVarStorage(Context& context, SemIR::InstId var_pattern_id,
+                        bool is_returned_var) -> SemIR::InstId {
   if (is_returned_var) {
     if (auto return_param_id =
             GetReturnedVarParam(context, GetCurrentFunctionForReturn(context));
@@ -227,21 +224,6 @@ static auto GetOrAddVarStorage(Context& context, SemIR::InstId var_pattern_id,
                         .pattern_id = var_pattern_id});
 }
 
-auto AddPatternVarStorage(Context& context, SemIR::InstBlockId pattern_block_id,
-                          bool is_returned_var) -> void {
-  // We need to emit the VarStorage insts early, because they may be output
-  // arguments for the initializer. However, we can't emit them when we emit
-  // the corresponding `AnyVarPattern`s because they're part of the pattern
-  // match, not part of the pattern.
-  // TODO: Find a way to do this without walking the whole pattern block.
-  for (auto inst_id : context.inst_blocks().Get(pattern_block_id)) {
-    if (context.insts().Is<SemIR::AnyVarPattern>(inst_id)) {
-      context.var_storage_map().Insert(
-          inst_id, GetOrAddVarStorage(context, inst_id, is_returned_var));
-    }
-  }
-}
-
 auto GetParamPatternKind(Context& context, SemIR::InstId param_inst_id)
     -> ParamPatternKind {
   auto param = context.insts().Get(

toolchain/check/pattern.h

@@ -68,11 +68,11 @@ auto AddBindingForPattern(Context& context, SemIR::LocId name_loc,
                           SemIR::TypeId binding_type_id, SemIR::InstId value_id)
     -> SemIR::InstId;
 
-// Creates storage for `var` patterns nested within the given pattern at the
-// current location in the output SemIR. For a `returned var`, this
-// reuses the function's return slot when present.
-auto AddPatternVarStorage(Context& context, SemIR::InstBlockId pattern_block_id,
-                          bool is_returned_var) -> void;
+// Returns a VarStorage inst for the given `var` pattern. `is_returned_var`
+// indicates whether the pattern is the `var` part of a `returned var`; if so,
+// this reuses the return parameter, and otherwise it adds a new inst.
+auto GetOrAddVarStorage(Context& context, SemIR::InstId var_pattern_id,
+                        bool is_returned_var) -> SemIR::InstId;
 
 // Kinds of parameters that can be added by `AddParamPattern`.
 enum class ParamPatternKind {

toolchain/check/pattern_match.cpp

@@ -674,9 +674,8 @@ auto MatchContext::DoVarPreWorkImpl(State state,
 
       // In a `var`/`let` declaration, the `VarStorage` inst is created before
       // we start pattern matching.
-      auto lookup_result = context_.var_storage_map().Lookup(entry.pattern_id);
-      CARBON_CHECK(lookup_result);
-      auto storage_id = lookup_result.value();
+      auto storage_id =
+          context_.full_pattern_stack().GetLocalVarStorage(entry.pattern_id);
       if (scrutinee_id.has_value()) {
         auto init_id =
             InitializeExisting(context_, SemIR::LocId(entry.pattern_id),

toolchain/check/testdata/var/var_pattern.carbon

@@ -82,10 +82,14 @@ fn G() {
 library "[[@TEST_NAME]]";
 
 fn F() {
-  // CHECK:STDERR: fail_nested.carbon:[[@LINE+4]]:41: error: `var` nested within another `var` [NestedVar]
+  // CHECK:STDERR: fail_nested.carbon:[[@LINE+8]]:41: error: `var` nested within another `var` [NestedVar]
   // CHECK:STDERR:   let (unused x: (), var (unused y: (), var unused z: ())) = ((), ((), ()));
   // CHECK:STDERR:                                         ^~~
   // CHECK:STDERR:
+  // CHECK:STDERR: fail_nested.carbon:[[@LINE+4]]:41: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
+  // CHECK:STDERR:   let (unused x: (), var (unused y: (), var unused z: ())) = ((), ((), ()));
+  // CHECK:STDERR:                                         ^~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
   let (unused x: (), var (unused y: (), var unused z: ())) = ((), ((), ()));
 }
 
@@ -755,114 +759,7 @@ fn Call() {
 // CHECK:STDOUT:
 // CHECK:STDOUT: --- fail_nested.carbon
 // CHECK:STDOUT:
-// CHECK:STDOUT: constants {
-// CHECK:STDOUT:   %F.type: type = fn_type @F [concrete]
-// CHECK:STDOUT:   %empty_tuple.type: type = tuple_type () [concrete]
-// CHECK:STDOUT:   %F: %F.type = struct_value () [concrete]
-// CHECK:STDOUT:   %empty_tuple: %empty_tuple.type = tuple_value () [concrete]
-// CHECK:STDOUT:   %pattern_type.cb1: type = pattern_type %empty_tuple.type [concrete]
-// CHECK:STDOUT:   %x.patt: %pattern_type.cb1 = value_binding_pattern x [concrete]
-// CHECK:STDOUT:   %y.patt: %pattern_type.cb1 = ref_binding_pattern y [concrete]
-// CHECK:STDOUT:   %z.patt: %pattern_type.cb1 = ref_binding_pattern z [concrete]
-// CHECK:STDOUT:   %z.var_patt: %pattern_type.cb1 = var_pattern %z.patt [concrete]
-// CHECK:STDOUT:   %tuple.type.bcd: type = tuple_type (%empty_tuple.type, %empty_tuple.type) [concrete]
-// CHECK:STDOUT:   %pattern_type.5b8: type = pattern_type %tuple.type.bcd [concrete]
-// CHECK:STDOUT:   %.e9e: %pattern_type.5b8 = tuple_pattern (%y.patt, %z.var_patt) [concrete]
-// CHECK:STDOUT:   %.var_patt: %pattern_type.5b8 = var_pattern %.e9e [concrete]
-// CHECK:STDOUT:   %tuple.type.a21: type = tuple_type (%empty_tuple.type, %tuple.type.bcd) [concrete]
-// CHECK:STDOUT:   %pattern_type.c6c: type = pattern_type %tuple.type.a21 [concrete]
-// CHECK:STDOUT:   %.441: %pattern_type.c6c = tuple_pattern (%x.patt, %.var_patt) [concrete]
-// CHECK:STDOUT:   %tuple.d8f: %tuple.type.bcd = tuple_value (%empty_tuple, %empty_tuple) [concrete]
-// CHECK:STDOUT:   %tuple.c9e: %tuple.type.a21 = tuple_value (%empty_tuple, %tuple.d8f) [concrete]
-// CHECK:STDOUT:   %Destroy.type: type = facet_type <@Destroy> [concrete]
-// CHECK:STDOUT:   %Destroy.Op.type.1d8f74.1: type = fn_type @Destroy.Op.loc9_22.1 [concrete]
-// CHECK:STDOUT:   %Destroy.Op.1a2547.1: %Destroy.Op.type.1d8f74.1 = struct_value () [concrete]
-// CHECK:STDOUT:   %Destroy.Op.type.1d8f74.2: type = fn_type @Destroy.Op.loc9_22.2 [concrete]
-// CHECK:STDOUT:   %Destroy.Op.1a2547.2: %Destroy.Op.type.1d8f74.2 = struct_value () [concrete]
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: imports {
-// CHECK:STDOUT:   %Core: <namespace> = namespace file.%Core.import, [concrete] {
-// CHECK:STDOUT:     .Destroy = %Core.Destroy
-// CHECK:STDOUT:     import Core//prelude
-// CHECK:STDOUT:     import Core//prelude/...
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Core.Destroy: type = import_ref Core//prelude/parts/destroy, Destroy, loaded [concrete = constants.%Destroy.type]
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: file {
-// CHECK:STDOUT:   package: <namespace> = namespace [concrete] {
-// CHECK:STDOUT:     .Core = imports.%Core
-// CHECK:STDOUT:     .F = %F.decl
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Core.import = import Core
-// CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {} {}
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: fn @F() {
-// CHECK:STDOUT: !entry:
-// CHECK:STDOUT:   name_binding_decl {
-// CHECK:STDOUT:     %x.patt: %pattern_type.cb1 = value_binding_pattern x [concrete = constants.%x.patt]
-// CHECK:STDOUT:     %y.patt: %pattern_type.cb1 = ref_binding_pattern y [concrete = constants.%y.patt]
-// CHECK:STDOUT:     %z.patt: %pattern_type.cb1 = ref_binding_pattern z [concrete = constants.%z.patt]
-// CHECK:STDOUT:     %z.var_patt: %pattern_type.cb1 = var_pattern %z.patt [concrete = constants.%z.var_patt]
-// CHECK:STDOUT:     %.loc9_57: %pattern_type.5b8 = tuple_pattern (%y.patt, %z.var_patt) [concrete = constants.%.e9e]
-// CHECK:STDOUT:     %.var_patt: %pattern_type.5b8 = var_pattern %.loc9_57 [concrete = constants.%.var_patt]
-// CHECK:STDOUT:     %.loc9_58: %pattern_type.c6c = tuple_pattern (%x.patt, %.var_patt) [concrete = constants.%.441]
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %z.var: ref %empty_tuple.type = var %z.var_patt
-// CHECK:STDOUT:   %.var: ref %tuple.type.bcd = var %.var_patt
-// CHECK:STDOUT:   %.loc9_64.1: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_69.1: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_73.1: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_74.1: %tuple.type.bcd = tuple_literal (%.loc9_69.1, %.loc9_73.1) [concrete = constants.%tuple.d8f]
-// CHECK:STDOUT:   %.loc9_75: %tuple.type.a21 = tuple_literal (%.loc9_64.1, %.loc9_74.1) [concrete = constants.%tuple.c9e]
-// CHECK:STDOUT:   %empty_tuple: %empty_tuple.type = tuple_value () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_64.2: %empty_tuple.type = converted %.loc9_64.1, %empty_tuple [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_19.1: type = splice_block %.loc9_19.3 [concrete = constants.%empty_tuple.type] {
-// CHECK:STDOUT:     %.loc9_19.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:     %.loc9_19.3: type = converted %.loc9_19.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %x: %empty_tuple.type = value_binding x, %.loc9_64.2
-// CHECK:STDOUT:   %tuple.elem0.loc9_74: ref %empty_tuple.type = tuple_access %.var, element0
-// CHECK:STDOUT:   %.loc9_69.2: init %empty_tuple.type to %tuple.elem0.loc9_74 = tuple_init () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_74.2: init %empty_tuple.type = converted %.loc9_69.1, %.loc9_69.2 [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_74.3: init %empty_tuple.type to %tuple.elem0.loc9_74 = in_place_init %.loc9_74.2 [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %tuple.elem1.loc9_74: ref %empty_tuple.type = tuple_access %.var, element1
-// CHECK:STDOUT:   %.loc9_73.2: init %empty_tuple.type to %tuple.elem1.loc9_74 = tuple_init () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_74.4: init %empty_tuple.type = converted %.loc9_73.1, %.loc9_73.2 [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_74.5: init %empty_tuple.type to %tuple.elem1.loc9_74 = in_place_init %.loc9_74.4 [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_74.6: init %tuple.type.bcd to %.var = tuple_init (%.loc9_74.3, %.loc9_74.5) [concrete = constants.%tuple.d8f]
-// CHECK:STDOUT:   %.loc9_22.1: init %tuple.type.bcd = converted %.loc9_74.1, %.loc9_74.6 [concrete = constants.%tuple.d8f]
-// CHECK:STDOUT:   assign %.var, %.loc9_22.1
-// CHECK:STDOUT:   %tuple.elem0.loc9_22: ref %empty_tuple.type = tuple_access %.var, element0
-// CHECK:STDOUT:   %tuple.elem1.loc9_22: ref %empty_tuple.type = tuple_access %.var, element1
-// CHECK:STDOUT:   %.loc9_38.1: type = splice_block %.loc9_38.3 [concrete = constants.%empty_tuple.type] {
-// CHECK:STDOUT:     %.loc9_38.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:     %.loc9_38.3: type = converted %.loc9_38.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %y: ref %empty_tuple.type = ref_binding y, %tuple.elem0.loc9_22
-// CHECK:STDOUT:   %.loc9_22.2: init %empty_tuple.type = tuple_init () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   %.loc9_41: init %empty_tuple.type = converted %tuple.elem1.loc9_22, %.loc9_22.2 [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:   assign %z.var, %.loc9_41
-// CHECK:STDOUT:   %.loc9_56.1: type = splice_block %.loc9_56.3 [concrete = constants.%empty_tuple.type] {
-// CHECK:STDOUT:     %.loc9_56.2: %empty_tuple.type = tuple_literal () [concrete = constants.%empty_tuple]
-// CHECK:STDOUT:     %.loc9_56.3: type = converted %.loc9_56.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %z: ref %empty_tuple.type = ref_binding z, %z.var
-// CHECK:STDOUT:   %Destroy.Op.bound.loc9_22: <bound method> = bound_method %.var, constants.%Destroy.Op.1a2547.2
-// CHECK:STDOUT:   %Destroy.Op.call.loc9_22: init %empty_tuple.type = call %Destroy.Op.bound.loc9_22(%.var)
-// CHECK:STDOUT:   %Destroy.Op.bound.loc9_41: <bound method> = bound_method %z.var, constants.%Destroy.Op.1a2547.1
-// CHECK:STDOUT:   %Destroy.Op.call.loc9_41: init %empty_tuple.type = call %Destroy.Op.bound.loc9_41(%z.var)
-// CHECK:STDOUT:   return
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: fn @Destroy.Op.loc9_22.1(%self.param: ref %empty_tuple.type) = "no_op";
-// CHECK:STDOUT:
-// CHECK:STDOUT: fn @Destroy.Op.loc9_22.2(%self.param: ref %tuple.type.bcd) {
-// CHECK:STDOUT: !entry:
-// CHECK:STDOUT:   return
-// CHECK:STDOUT: }
+// CHECK:STDOUT: fn @F();
 // CHECK:STDOUT:
 // CHECK:STDOUT: --- fail_compile_time.carbon
 // CHECK:STDOUT:

toolchain/docs/check/pattern_matching.md

@@ -112,11 +112,13 @@ instruction IDs:
     `Context::bind_name_map` stores these `ValueBinding`s, keyed by the
     corresponding `ValueBindingPattern` instruction.
 -   A `var` pattern allocates storage during matching, which is represented by a
-    `VarStorage` instruction. This instruction must be allocated during the
-    pattern step, so that it can be used as the output parameter of scrutinee
-    expression evaluation during the scrutinee step. `Context::var_storage_map`
-    stores these `VarStorage` instructions, keyed by the corresponding
-    `VarPattern` instruction.
+    `VarStorage` instruction. For local and class-scope `var` patterns, this
+    instruction must be allocated at the end of the pattern step, so that it can
+    be used as the output parameter of scrutinee expression evaluation during
+    the scrutinee step, but doesn't get added to the instruction block that's
+    meant to capture sub-expressions (see below). `FullPatternStack` is
+    responsible for the mapping from `VarPattern` insts to the corresponding
+    `VarStorage` insts.
 
 As noted earlier, the pattern step can also emit non-pattern instructions to
 evaluate expressions that are embedded in the pattern, such as the type

toolchain/parse/handle_var.cpp

@@ -93,7 +93,8 @@ auto HandleVariablePattern(Context& context) -> void {
     context.emitter().Emit(*context.position(), NestedVar);
     state.has_error = true;
   }
-  context.PushState(StateKind::FinishVariablePattern);
+  state.kind = StateKind::FinishVariablePattern;
+  context.PushState(state);
   context.ConsumeChecked(Lex::TokenKind::Var);
 
   context.PushStateForPattern(StateKind::Pattern, /*in_var_pattern=*/true,

toolchain/parse/testdata/var/var_pattern.carbon

@@ -110,8 +110,8 @@ let (x: (), var (y: (), var z: ())) = ((), ((), ()));
 // CHECK:STDOUT:                   {kind: 'TupleLiteralStart', text: '('},
 // CHECK:STDOUT:                 {kind: 'TupleLiteral', text: ')', subtree_size: 2},
 // CHECK:STDOUT:               {kind: 'VarBindingPattern', text: ':', subtree_size: 4},
-// CHECK:STDOUT:             {kind: 'VariablePattern', text: 'var', subtree_size: 5},
-// CHECK:STDOUT:           {kind: 'TuplePattern', text: ')', subtree_size: 12},
+// CHECK:STDOUT:             {kind: 'VariablePattern', text: 'var', has_error: yes, subtree_size: 5},
+// CHECK:STDOUT:           {kind: 'TuplePattern', text: ')', has_error: yes, subtree_size: 12},
 // CHECK:STDOUT:         {kind: 'VariablePattern', text: 'var', subtree_size: 13},
 // CHECK:STDOUT:       {kind: 'TuplePattern', text: ')', subtree_size: 20},
 // CHECK:STDOUT:       {kind: 'LetInitializer', text: '='},