Refactor BindName to support tracking the enclosing scope. (#3566)

This is a step towards adding enclosing scopes for imports. It creates
an indirection for all bind names.

We discussed specializing for bindings that are in function scope (i.e.,
not a useful enclosing scope for imports or diagnostics). However, the
thought is to go ahead with this singular approach for now, and only
change structure if it's a performance issues so that we have
incrementally fewer instructions to handle.

toolchain/check/check.cpp

@@ -84,7 +84,7 @@ static auto GetImportNameId(Parse::NodeId parse_node, Context& context,
   switch (import_inst.kind()) {
     case SemIR::InstKind::BindName: {
       auto bind_name = import_inst.As<SemIR::BindName>();
-      return bind_name.name_id;
+      return import_sem_ir.bind_names().Get(bind_name.bind_name_id).name_id;
     }
 
     case SemIR::InstKind::FunctionDecl: {

toolchain/check/context.cpp

@@ -144,9 +144,11 @@ auto Context::AddPackageImports(Parse::NodeId import_node,
   // Add a name for formatted output. This isn't used in name lookup in order
   // to reduce indirection, but it's separate from the Import because it
   // otherwise fits in an Inst.
+  auto bind_name_id = bind_names().Add(
+      {.name_id = name_id, .enclosing_scope_id = SemIR::NameScopeId::Package});
   AddInst(SemIR::BindName{.parse_node = import_node,
                           .type_id = type_id,
-                          .name_id = name_id,
+                          .bind_name_id = bind_name_id,
                           .value_id = inst_id});
 }
 

toolchain/check/context.h

@@ -351,6 +351,9 @@ class Context {
   auto string_literal_values() -> StringStoreWrapper<StringLiteralValueId>& {
     return sem_ir().string_literal_values();
   }
+  auto bind_names() -> ValueStore<SemIR::BindNameId>& {
+    return sem_ir().bind_names();
+  }
   auto functions() -> ValueStore<SemIR::FunctionId>& {
     return sem_ir().functions();
   }

toolchain/check/handle_binding_pattern.cpp

@@ -13,7 +13,9 @@ auto HandleAddress(Context& context, Parse::AddressId parse_node) -> bool {
   auto self_param_id =
       context.node_stack().Pop<Parse::NodeKind::BindingPattern>();
   auto self_param = context.insts().TryGetAs<SemIR::BindName>(self_param_id);
-  if (self_param && self_param->name_id == SemIR::NameId::SelfValue) {
+  if (self_param &&
+      context.bind_names().Get(self_param->bind_name_id).name_id ==
+          SemIR::NameId::SelfValue) {
     // TODO: The type of an `addr_pattern` should probably be the non-pointer
     // type, because that's the type that the pattern matches.
     context.AddInstAndPush(
@@ -49,10 +51,14 @@ auto HandleBindingPattern(Context& context, Parse::BindingPatternId parse_node)
   // Create the appropriate kind of binding for this pattern.
   //
   // TODO: Update this to create a generic or template binding as needed.
-  auto make_bind_name = [name_node = name_node, name_id = name_id](
+  auto make_bind_name = [&, name_node = name_node, name_id = name_id](
                             SemIR::TypeId type_id,
                             SemIR::InstId value_id) -> SemIR::Inst {
-    return SemIR::BindName{name_node, type_id, name_id, value_id};
+    // TODO: Set the correct enclosing_scope_id.
+    auto bind_name_id = context.bind_names().Add(
+        {.name_id = name_id,
+         .enclosing_scope_id = SemIR::NameScopeId::Invalid});
+    return SemIR::BindName{name_node, type_id, bind_name_id, value_id};
   };
 
   // A `self` binding can only appear in an implicit parameter list.

toolchain/check/handle_function.cpp

@@ -262,8 +262,9 @@ auto HandleFunctionDefinitionStart(Context& context,
     });
 
     if (auto fn_param = param.TryAs<SemIR::BindName>()) {
-      context.AddNameToLookup(fn_param->parse_node, fn_param->name_id,
-                              param_id);
+      context.AddNameToLookup(
+          fn_param->parse_node,
+          context.bind_names().Get(fn_param->bind_name_id).name_id, param_id);
     } else {
       CARBON_FATAL() << "Unexpected kind of parameter in function definition "
                      << param;

toolchain/check/handle_let.cpp

@@ -52,7 +52,8 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId parse_node) -> bool {
   context.inst_block_stack().AddInstId(pattern_id);
 
   // Add the name of the binding to the current scope.
-  context.AddNameToLookup(pattern.parse_node(), bind_name.name_id, pattern_id);
+  auto name_id = context.bind_names().Get(bind_name.bind_name_id).name_id;
+  context.AddNameToLookup(pattern.parse_node(), name_id, pattern_id);
   return true;
 }
 

toolchain/check/handle_variable.cpp

@@ -57,10 +57,10 @@ auto HandleVariableDecl(Context& context, Parse::VariableDeclId parse_node)
   if (auto bind_name = context.insts().Get(value_id).TryAs<SemIR::BindName>()) {
     // Form a corresponding name in the current context, and bind the name to
     // the variable.
-    context.decl_name_stack().AddNameToLookup(
-        context.decl_name_stack().MakeUnqualifiedName(bind_name->parse_node,
-                                                      bind_name->name_id),
-        value_id);
+    auto name_context = context.decl_name_stack().MakeUnqualifiedName(
+        bind_name->parse_node,
+        context.bind_names().Get(bind_name->bind_name_id).name_id);
+    context.decl_name_stack().AddNameToLookup(name_context, value_id);
     value_id = bind_name->value_id;
   }
 

toolchain/check/testdata/basics/builtin_insts.carbon

@@ -10,6 +10,7 @@
 // CHECK:STDOUT: filename:        builtin_insts.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:      {}
 // CHECK:STDOUT:   functions:       {}
 // CHECK:STDOUT:   classes:         {}
 // CHECK:STDOUT:   types:

toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon

@@ -22,6 +22,7 @@ fn B() {}
 // CHECK:STDOUT: filename:        a.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:      {}
 // CHECK:STDOUT:   functions:
 // CHECK:STDOUT:     function0:       {name: name0, param_refs: empty, body: [block2]}
 // CHECK:STDOUT:   classes:         {}
@@ -60,6 +61,7 @@ fn B() {}
 // CHECK:STDOUT: filename:        b.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:      {}
 // CHECK:STDOUT:   functions:
 // CHECK:STDOUT:     function0:       {name: name0, param_refs: empty, body: [block2]}
 // CHECK:STDOUT:   classes:         {}

toolchain/check/testdata/basics/multifile_raw_ir.carbon

@@ -22,6 +22,7 @@ fn B() {}
 // CHECK:STDOUT: filename:        a.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:      {}
 // CHECK:STDOUT:   functions:
 // CHECK:STDOUT:     function0:       {name: name0, param_refs: empty, body: [block2]}
 // CHECK:STDOUT:   classes:         {}
@@ -47,6 +48,7 @@ fn B() {}
 // CHECK:STDOUT: filename:        b.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:      {}
 // CHECK:STDOUT:   functions:
 // CHECK:STDOUT:     function0:       {name: name0, param_refs: empty, body: [block2]}
 // CHECK:STDOUT:   classes:         {}

toolchain/check/testdata/basics/raw_and_textual_ir.carbon

@@ -16,6 +16,8 @@ fn Foo(n: i32) -> (i32, i32, f64) {
 // CHECK:STDOUT: filename:        raw_and_textual_ir.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:
+// CHECK:STDOUT:     bindName0:       {name: name1, enclosing_scope: name_scope<invalid>}
 // CHECK:STDOUT:   functions:
 // CHECK:STDOUT:     function0:       {name: name0, param_refs: block2, return_type: type4, return_slot: inst+7, body: [block5]}
 // CHECK:STDOUT:   classes:         {}
@@ -39,7 +41,7 @@ fn Foo(n: i32) -> (i32, i32, f64) {
 // CHECK:STDOUT:   insts:
 // CHECK:STDOUT:     inst+0:          {kind: Namespace, arg0: name_scope0, type: type0}
 // CHECK:STDOUT:     inst+1:          {kind: Param, arg0: name1, type: type1}
-// CHECK:STDOUT:     inst+2:          {kind: BindName, arg0: name1, arg1: inst+1, type: type1}
+// CHECK:STDOUT:     inst+2:          {kind: BindName, arg0: bindName0, arg1: inst+1, type: type1}
 // CHECK:STDOUT:     inst+3:          {kind: TupleType, arg0: typeBlock0, type: typeTypeType}
 // CHECK:STDOUT:     inst+4:          {kind: TupleLiteral, arg0: block3, type: type2}
 // CHECK:STDOUT:     inst+5:          {kind: TupleType, arg0: typeBlock1, type: typeTypeType}

toolchain/check/testdata/basics/raw_ir.carbon

@@ -16,6 +16,8 @@ fn Foo(n: i32) -> (i32, i32, f64) {
 // CHECK:STDOUT: filename:        raw_ir.carbon
 // CHECK:STDOUT: sem_ir:
 // CHECK:STDOUT:   cross_ref_irs_size: 1
+// CHECK:STDOUT:   bind_names:
+// CHECK:STDOUT:     bindName0:       {name: name1, enclosing_scope: name_scope<invalid>}
 // CHECK:STDOUT:   functions:
 // CHECK:STDOUT:     function0:       {name: name0, param_refs: block2, return_type: type4, return_slot: inst+7, body: [block5]}
 // CHECK:STDOUT:   classes:         {}
@@ -39,7 +41,7 @@ fn Foo(n: i32) -> (i32, i32, f64) {
 // CHECK:STDOUT:   insts:
 // CHECK:STDOUT:     inst+0:          {kind: Namespace, arg0: name_scope0, type: type0}
 // CHECK:STDOUT:     inst+1:          {kind: Param, arg0: name1, type: type1}
-// CHECK:STDOUT:     inst+2:          {kind: BindName, arg0: name1, arg1: inst+1, type: type1}
+// CHECK:STDOUT:     inst+2:          {kind: BindName, arg0: bindName0, arg1: inst+1, type: type1}
 // CHECK:STDOUT:     inst+3:          {kind: TupleType, arg0: typeBlock0, type: typeTypeType}
 // CHECK:STDOUT:     inst+4:          {kind: TupleLiteral, arg0: block3, type: type2}
 // CHECK:STDOUT:     inst+5:          {kind: TupleType, arg0: typeBlock1, type: typeTypeType}

toolchain/sem_ir/file.cpp

@@ -150,6 +150,7 @@ auto File::OutputYaml(bool include_builtins) const -> Yaml::OutputMapping {
     map.Add("sem_ir", Yaml::OutputMapping([&](Yaml::OutputMapping::Map map) {
               map.Add("cross_ref_irs_size",
                       Yaml::OutputScalar(cross_ref_irs_.size()));
+              map.Add("bind_names", bind_names_.OutputYaml());
               map.Add("functions", functions_.OutputYaml());
               map.Add("classes", classes_.OutputYaml());
               map.Add("types", types_.OutputYaml());

toolchain/sem_ir/file.h

@@ -17,6 +17,16 @@
 
 namespace Carbon::SemIR {
 
+struct BindNameInfo : public Printable<BindNameInfo> {
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "{name: " << name_id << ", enclosing_scope: " << enclosing_scope_id
+        << "}";
+  }
+
+  NameId name_id;
+  NameScopeId enclosing_scope_id;
+};
+
 // A function.
 struct Function : public Printable<Function> {
   auto Print(llvm::raw_ostream& out) const -> void {
@@ -239,6 +249,10 @@ class File : public Printable<File> {
     return value_stores_->string_literal_values();
   }
 
+  auto bind_names() -> ValueStore<BindNameId>& { return bind_names_; }
+  auto bind_names() const -> const ValueStore<BindNameId>& {
+    return bind_names_;
+  }
   auto functions() -> ValueStore<FunctionId>& { return functions_; }
   auto functions() const -> const ValueStore<FunctionId>& { return functions_; }
   auto classes() -> ValueStore<ClassId>& { return classes_; }
@@ -300,6 +314,9 @@ class File : public Printable<File> {
   // TODO: If SemIR starts linking back to tokens, reuse its filename.
   std::string filename_;
 
+  // Storage for bind names.
+  ValueStore<BindNameId> bind_names_;
+
   // Storage for callable objects.
   ValueStore<FunctionId> functions_;
 

toolchain/sem_ir/formatter.cpp

@@ -463,7 +463,9 @@ class InstNamer {
           break;
         }
         case BindName::Kind: {
-          add_inst_name_id(inst.As<BindName>().name_id);
+          add_inst_name_id(sem_ir_.bind_names()
+                               .Get(inst.As<BindName>().bind_name_id)
+                               .name_id);
           continue;
         }
         case FunctionDecl::Kind: {
@@ -973,6 +975,10 @@ class Formatter {
 
   auto FormatArg(BuiltinKind kind) -> void { out_ << kind.label(); }
 
+  auto FormatArg(BindNameId id) -> void {
+    FormatName(sem_ir_.bind_names().Get(id).name_id);
+  }
+
   auto FormatArg(FunctionId id) -> void { FormatFunctionName(id); }
 
   auto FormatArg(ClassId id) -> void { FormatClassName(id); }

toolchain/sem_ir/ids.h

@@ -16,6 +16,7 @@ namespace Carbon::SemIR {
 // Forward declare indexed types, for integration with ValueStore.
 class File;
 class Inst;
+struct BindNameInfo;
 struct Class;
 struct Function;
 struct Interface;
@@ -67,6 +68,22 @@ constexpr InstId InstId::Invalid = InstId(InstId::InvalidIndex);
 // The package namespace will be the instruction after builtins.
 constexpr InstId InstId::PackageNamespace = InstId(BuiltinKind::ValidCount);
 
+// The ID of a bind name.
+struct BindNameId : public IdBase, public Printable<BindNameId> {
+  using ValueType = BindNameInfo;
+
+  // An explicitly invalid function ID.
+  static const BindNameId Invalid;
+
+  using IdBase::IdBase;
+  auto Print(llvm::raw_ostream& out) const -> void {
+    out << "bindName";
+    IdBase::Print(out);
+  }
+};
+
+constexpr BindNameId BindNameId::Invalid = BindNameId(BindNameId::InvalidIndex);
+
 // The ID of a function.
 struct FunctionId : public IdBase, public Printable<FunctionId> {
   using ValueType = Function;

toolchain/sem_ir/typed_insts.h

@@ -131,7 +131,9 @@ struct BindName {
   // TODO: Make this more specific.
   Parse::NodeId parse_node;
   TypeId type_id;
-  NameId name_id;
+  BindNameId bind_name_id;
+  // The value is inline in the inst so that value access doesn't require an
+  // indirection.
   InstId value_id;
 };
 

toolchain/sem_ir/yaml_test.cpp

@@ -50,6 +50,7 @@ TEST(SemIRTest, YAML) {
 
   auto file = Yaml::Mapping(ElementsAre(
       Pair("cross_ref_irs_size", "1"),
+      Pair("bind_names", Yaml::Mapping(SizeIs(1))),
       Pair("functions", Yaml::Mapping(SizeIs(1))),
       Pair("classes", Yaml::Mapping(SizeIs(0))),
       Pair("types", Yaml::Mapping(Each(type_builtin))),