Use the full name declaration logic when adding `var` names to scope. (#3312)

This causes the names of class members to get added to the class scope.

toolchain/check/declaration_name_stack.cpp

@@ -8,11 +8,20 @@
 
 namespace Carbon::Check {
 
+auto DeclarationNameStack::MakeEmptyNameContext() -> NameContext {
+  return NameContext{.target_scope_id = context_->current_scope_id()};
+}
+
+auto DeclarationNameStack::MakeUnqualifiedName(Parse::Node parse_node,
+                                               SemIR::StringId name_id)
+    -> NameContext {
+  NameContext context = MakeEmptyNameContext();
+  ApplyNameQualifierTo(context, parse_node, name_id);
+  return context;
+}
+
 auto DeclarationNameStack::Push() -> void {
-  declaration_name_stack_.push_back(
-      {.state = NameContext::State::New,
-       .target_scope_id = context_->current_scope_id(),
-       .resolved_node_id = SemIR::NodeId::Invalid});
+  declaration_name_stack_.push_back(MakeEmptyNameContext());
 }
 
 auto DeclarationNameStack::Pop() -> NameContext {
@@ -41,7 +50,7 @@ auto DeclarationNameStack::LookupOrAddName(NameContext name_context,
       // The name is invalid and a diagnostic has already been emitted.
       return SemIR::NodeId::Invalid;
 
-    case NameContext::State::New:
+    case NameContext::State::Empty:
       CARBON_FATAL() << "Name is missing, not expected to call AddNameToLookup "
                         "(but that may change based on error handling).";
 
@@ -98,10 +107,20 @@ auto DeclarationNameStack::ApplyExpressionQualifier(Parse::Node parse_node,
 
 auto DeclarationNameStack::ApplyNameQualifier(Parse::Node parse_node,
                                               SemIR::StringId name_id) -> void {
-  auto& name_context = declaration_name_stack_.back();
+  ApplyNameQualifierTo(declaration_name_stack_.back(), parse_node, name_id);
+}
+
+auto DeclarationNameStack::ApplyNameQualifierTo(NameContext& name_context,
+                                                Parse::Node parse_node,
+                                                SemIR::StringId name_id)
+    -> void {
   if (CanResolveQualifier(name_context, parse_node)) {
     // For identifier nodes, we need to perform a lookup on the identifier.
     // This means the input node_id is actually a string ID.
+    //
+    // TODO: This doesn't perform the right kind of lookup. We will find names
+    // from enclosing lexical scopes here, in the case where `target_scope_id`
+    // is invalid.
     auto resolved_node_id = context_->LookupName(
         name_context.parse_node, name_id, name_context.target_scope_id,
         /*print_diagnostics=*/false);
@@ -195,7 +214,7 @@ auto DeclarationNameStack::CanResolveQualifier(NameContext& name_context,
       return false;
     }
 
-    case NameContext::State::New:
+    case NameContext::State::Empty:
     case NameContext::State::Resolved: {
       name_context.parse_node = parse_node;
       return true;

toolchain/check/declaration_name_stack.h

@@ -32,19 +32,19 @@ class Context;
 // Example state transitions:
 //
 // ```
-// // New -> Unresolved, because `MyNamespace` is newly declared.
+// // Empty -> Unresolved, because `MyNamespace` is newly declared.
 // namespace MyNamespace;
 //
-// // New -> Resolved -> Unresolved, because `MyType` is newly declared.
+// // Empty -> Resolved -> Unresolved, because `MyType` is newly declared.
 // class MyNamespace.MyType;
 //
-// // New -> Resolved -> Resolved, because `MyType` was forward declared.
+// // Empty -> Resolved -> Resolved, because `MyType` was forward declared.
 // class MyNamespace.MyType {
-//   // New -> Unresolved, because `DoSomething` is newly declared.
+//   // Empty -> Unresolved, because `DoSomething` is newly declared.
 //   fn DoSomething();
 // }
 //
-// // New -> Resolved -> Resolved -> ResolvedNonScope, because `DoSomething`
+// // Empty -> Resolved -> Resolved -> ResolvedNonScope, because `DoSomething`
 // // is forward declared in `MyType`, but is not a scope itself.
 // fn MyNamespace.MyType.DoSomething() { ... }
 // ```
@@ -53,8 +53,8 @@ class DeclarationNameStack {
   // Context for declaration name construction.
   struct NameContext {
     enum class State : int8_t {
-      // A new context which has not processed any parts of the qualifier.
-      New,
+      // A context that has not processed any parts of the qualifier.
+      Empty,
 
       // A node ID has been resolved, whether through an identifier or
       // expression. This provided a new scope, such as a type.
@@ -73,12 +73,12 @@ class DeclarationNameStack {
       Error,
     };
 
-    State state = State::New;
+    State state = State::Empty;
 
     // The scope which qualified names are added to. For unqualified names in
     // an unnamed scope, this will be Invalid to indicate the current scope
     // should be used.
-    SemIR::NameScopeId target_scope_id = SemIR::NameScopeId::Invalid;
+    SemIR::NameScopeId target_scope_id;
 
     // The last parse node used.
     Parse::Node parse_node = Parse::Node::Invalid;
@@ -104,6 +104,13 @@ class DeclarationNameStack {
   // node stack, which will be applied to the declaration name if appropriate.
   auto Pop() -> NameContext;
 
+  // Creates and returns a name context corresponding to declaring an
+  // unqualified name in the current context. This is suitable for adding to
+  // name lookup in situations where a qualified name is not permitted, such as
+  // a pattern binding.
+  auto MakeUnqualifiedName(Parse::Node parse_node, SemIR::StringId name_id)
+      -> NameContext;
+
   // Applies an expression from the node stack to the top of the declaration
   // name stack.
   auto ApplyExpressionQualifier(Parse::Node parse_node, SemIR::NodeId node_id)
@@ -124,6 +131,13 @@ class DeclarationNameStack {
       -> SemIR::NodeId;
 
  private:
+  // Returns a name context corresponding to an empty name.
+  auto MakeEmptyNameContext() -> NameContext;
+
+  // Applies a Name from the node stack to given name context.
+  auto ApplyNameQualifierTo(NameContext& name_context, Parse::Node parse_node,
+                            SemIR::StringId name_id) -> void;
+
   // Returns true if the context is in a state where it can resolve qualifiers.
   // Updates name_context as needed.
   auto CanResolveQualifier(NameContext& name_context, Parse::Node parse_node)

toolchain/check/handle_variable.cpp

@@ -21,12 +21,22 @@ auto HandleVariableDeclaration(Context& context, Parse::Node parse_node)
         .PopAndDiscardSoloParseNode<Parse::NodeKind::VariableInitializer>();
   }
 
-  // Get the storage and add it to name lookup.
+  // Extract the name binding.
   SemIR::NodeId var_id =
       context.node_stack().Pop<Parse::NodeKind::PatternBinding>();
   auto var = context.semantics_ir().GetNodeAs<SemIR::VarStorage>(var_id);
-  context.AddNameToLookup(var.parse_node, var.name_id, var_id);
-  // If there was an initializer, assign it to storage.
+
+  // Form a corresponding name in the current context, and bind the name to the
+  // variable.
+  context.declaration_name_stack().AddNameToLookup(
+      context.declaration_name_stack().MakeUnqualifiedName(var.parse_node,
+                                                           var.name_id),
+      var_id);
+
+  // If there was an initializer, assign it to the storage.
+  //
+  // TODO: In a class scope, we should instead save the initializer somewhere
+  // so that we can use it as a default.
   if (has_init) {
     init_id = Initialize(context, parse_node, var_id, init_id);
     // TODO: Consider using different node kinds for assignment versus

toolchain/check/testdata/class/basic.carbon

@@ -36,6 +36,7 @@ fn Run() -> i32 {
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   .F = %F
 // CHECK:STDOUT:   .G = %G
+// CHECK:STDOUT:   .k = %k
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: fn @F(%n: i32) -> i32 {

toolchain/check/testdata/ir/duplicate_name_same_line.carbon

@@ -4,7 +4,7 @@
 //
 // AUTOUPDATE
 
-fn A() { var n: i32 = 1; if (true) { var n: i32 = 2; } }
+fn A() { if (true) { var n: i32 = 1; } if (true) { var n: i32 = 2; } }
 
 // CHECK:STDOUT: file "duplicate_name_same_line.carbon" {
 // CHECK:STDOUT:   %A: <function> = fn_decl @A
@@ -12,18 +12,25 @@ fn A() { var n: i32 = 1; if (true) { var n: i32 = 2; } }
 // CHECK:STDOUT:
 // CHECK:STDOUT: fn @A() {
 // CHECK:STDOUT: !entry:
-// CHECK:STDOUT:   %n.loc7_14: ref i32 = var "n"
-// CHECK:STDOUT:   %.loc7_23: i32 = int_literal 1
-// CHECK:STDOUT:   assign %n.loc7_14, %.loc7_23
-// CHECK:STDOUT:   %.loc7_30: bool = bool_literal true
-// CHECK:STDOUT:   if %.loc7_30 br !if.then else br !if.else
+// CHECK:STDOUT:   %.loc7_14: bool = bool_literal true
+// CHECK:STDOUT:   if %.loc7_14 br !if.then.loc7_18 else br !if.else.loc7_18
 // CHECK:STDOUT:
-// CHECK:STDOUT: !if.then:
-// CHECK:STDOUT:   %n.loc7_42: ref i32 = var "n"
-// CHECK:STDOUT:   %.loc7_51: i32 = int_literal 2
-// CHECK:STDOUT:   assign %n.loc7_42, %.loc7_51
-// CHECK:STDOUT:   br !if.else
+// CHECK:STDOUT: !if.then.loc7_18:
+// CHECK:STDOUT:   %n.loc7_26: ref i32 = var "n"
+// CHECK:STDOUT:   %.loc7_35: i32 = int_literal 1
+// CHECK:STDOUT:   assign %n.loc7_26, %.loc7_35
+// CHECK:STDOUT:   br !if.else.loc7_18
 // CHECK:STDOUT:
-// CHECK:STDOUT: !if.else:
+// CHECK:STDOUT: !if.else.loc7_18:
+// CHECK:STDOUT:   %.loc7_44: bool = bool_literal true
+// CHECK:STDOUT:   if %.loc7_44 br !if.then.loc7_48 else br !if.else.loc7_48
+// CHECK:STDOUT:
+// CHECK:STDOUT: !if.then.loc7_48:
+// CHECK:STDOUT:   %n.loc7_56: ref i32 = var "n"
+// CHECK:STDOUT:   %.loc7_65: i32 = int_literal 2
+// CHECK:STDOUT:   assign %n.loc7_56, %.loc7_65
+// CHECK:STDOUT:   br !if.else.loc7_48
+// CHECK:STDOUT:
+// CHECK:STDOUT: !if.else.loc7_48:
 // CHECK:STDOUT:   return
 // CHECK:STDOUT: }

toolchain/check/testdata/var/fail_shadowing.carbon

@@ -0,0 +1,47 @@
+// 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
+//
+// AUTOUPDATE
+
+fn Main() {
+  var x: i32 = 0;
+  if (true) {
+    // TODO: We should reject the use of the shadowed variable `x`, not this
+    // shadowing declaration. Also the diagnostic text is wrong ("same scope").
+    // CHECK:STDERR: fail_shadowing.carbon:[[@LINE+6]]:9: ERROR: Duplicate name being declared in the same scope.
+    // CHECK:STDERR:     var x: i32 = 0;
+    // CHECK:STDERR:         ^
+    // CHECK:STDERR: fail_shadowing.carbon:[[@LINE-7]]:7: Name is previously declared here.
+    // CHECK:STDERR:   var x: i32 = 0;
+    // CHECK:STDERR:       ^
+    var x: i32 = 0;
+
+    x = 1;
+  }
+}
+
+// CHECK:STDOUT: file "fail_shadowing.carbon" {
+// CHECK:STDOUT:   %Main: <function> = fn_decl @Main
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @Main() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   %x.loc8: ref i32 = var "x"
+// CHECK:STDOUT:   %.loc8: i32 = int_literal 0
+// CHECK:STDOUT:   assign %x.loc8, %.loc8
+// CHECK:STDOUT:   %.loc9: bool = bool_literal true
+// CHECK:STDOUT:   if %.loc9 br !if.then else br !if.else
+// CHECK:STDOUT:
+// CHECK:STDOUT: !if.then:
+// CHECK:STDOUT:   %x.loc18: ref i32 = var "x"
+// CHECK:STDOUT:   %.loc18: i32 = int_literal 0
+// CHECK:STDOUT:   assign %x.loc18, %.loc18
+// CHECK:STDOUT:   %x.ref: ref i32 = name_reference "x", %x.loc8
+// CHECK:STDOUT:   %.loc20: i32 = int_literal 1
+// CHECK:STDOUT:   assign %x.ref, %.loc20
+// CHECK:STDOUT:   br !if.else
+// CHECK:STDOUT:
+// CHECK:STDOUT: !if.else:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT: }