diff --git a/toolchain/check/declaration_name_stack.cpp b/toolchain/check/declaration_name_stack.cpp index 896097432ac1..52ef3a76016c 100644 --- a/toolchain/check/declaration_name_stack.cpp +++ b/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; diff --git a/toolchain/check/declaration_name_stack.h b/toolchain/check/declaration_name_stack.h index b4eb9b2e04b3..0813772b4d9d 100644 --- a/toolchain/check/declaration_name_stack.h +++ b/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) diff --git a/toolchain/check/handle_variable.cpp b/toolchain/check/handle_variable.cpp index 1f885eb345ef..c2ea5a07207b 100644 --- a/toolchain/check/handle_variable.cpp +++ b/toolchain/check/handle_variable.cpp @@ -21,12 +21,22 @@ auto HandleVariableDeclaration(Context& context, Parse::Node parse_node) .PopAndDiscardSoloParseNode(); } - // Get the storage and add it to name lookup. + // Extract the name binding. SemIR::NodeId var_id = context.node_stack().Pop(); auto var = context.semantics_ir().GetNodeAs(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 diff --git a/toolchain/check/testdata/class/basic.carbon b/toolchain/check/testdata/class/basic.carbon index 4c12bf65b671..bc9ab297d035 100644 --- a/toolchain/check/testdata/class/basic.carbon +++ b/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 { diff --git a/toolchain/check/testdata/ir/duplicate_name_same_line.carbon b/toolchain/check/testdata/ir/duplicate_name_same_line.carbon index a632f8ed09a9..6f4b28d0266e 100644 --- a/toolchain/check/testdata/ir/duplicate_name_same_line.carbon +++ b/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: = 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: } diff --git a/toolchain/check/testdata/var/fail_shadowing.carbon b/toolchain/check/testdata/var/fail_shadowing.carbon new file mode 100644 index 000000000000..03ccdb854718 --- /dev/null +++ b/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: = 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: }