From 4ab42c0712ad102bb7c689411dd552d8ac5311e4 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 20 Feb 2024 10:43:57 -0800 Subject: [PATCH] Fix for MakeUnqualifiedName conflicting with a namespace (#3707) Unqualified names don't handle scopes the way that typical names do, so a name conflict with a namespace needs to be handled specially. I'm still favoring keeping code close as much as possible, particularly since long-term this syntax will probably shift to be more consistent. For now I'm just flagging when we shouldn't push scopes, so that MakeUnqualifiedName doesn't need to clean up. Note, a different approach would basically be: ``` PushScopeAndStartName ApplyNameQualifierTo result = decl_name_stack_.back(); decl_name_stack_.back().state = NameContext::State::Finished; PopScope return result; ``` But that approach feels worse to me, due to the additional stack manipulations and the need to duplicate some of the FinishName logic just to be able to pop the scope that didn't really need to be added. --- toolchain/check/decl_name_stack.cpp | 26 +++++++---- toolchain/check/decl_name_stack.h | 9 ++-- .../var/fail_namespace_conflict.carbon | 46 +++++++++++++++++++ toolchain/check/testdata/var/shadowing.carbon | 45 +++++++++++------- 4 files changed, 98 insertions(+), 28 deletions(-) create mode 100644 toolchain/check/testdata/var/fail_namespace_conflict.carbon diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index 359462d8d4d9..553fb82decac 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/toolchain/check/decl_name_stack.cpp @@ -18,7 +18,7 @@ auto DeclNameStack::MakeEmptyNameContext() -> NameContext { auto DeclNameStack::MakeUnqualifiedName(Parse::NodeId parse_node, SemIR::NameId name_id) -> NameContext { NameContext context = MakeEmptyNameContext(); - ApplyNameQualifierTo(context, parse_node, name_id); + ApplyNameQualifierTo(context, parse_node, name_id, /*is_unqualified=*/true); return context; } @@ -127,12 +127,14 @@ auto DeclNameStack::AddNameToLookup(NameContext name_context, auto DeclNameStack::ApplyNameQualifier(Parse::NodeId parse_node, SemIR::NameId name_id) -> void { - ApplyNameQualifierTo(decl_name_stack_.back(), parse_node, name_id); + ApplyNameQualifierTo(decl_name_stack_.back(), parse_node, name_id, + /*is_unqualified=*/false); } auto DeclNameStack::ApplyNameQualifierTo(NameContext& name_context, Parse::NodeId parse_node, - SemIR::NameId name_id) -> void { + SemIR::NameId name_id, + bool is_unqualified) -> void { if (TryResolveQualifier(name_context, parse_node)) { // For identifier nodes, we need to perform a lookup on the identifier. auto resolved_inst_id = context_->LookupNameInDecl( @@ -148,7 +150,7 @@ auto DeclNameStack::ApplyNameQualifierTo(NameContext& name_context, name_context.resolved_inst_id = resolved_inst_id; } - UpdateScopeIfNeeded(name_context); + UpdateScopeIfNeeded(name_context, is_unqualified); } } @@ -172,7 +174,8 @@ static auto PushNameQualifierScope(Context& context, context.scope_stack().Push(); } -auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context) -> void { +auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context, + bool is_unqualified) -> void { // This will only be reached for resolved instructions. We update the target // scope based on the resolved type. auto resolved_inst = context_->insts().Get(name_context.resolved_inst_id); @@ -183,8 +186,10 @@ auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context) -> void { if (class_info.is_defined()) { name_context.state = NameContext::State::Resolved; name_context.target_scope_id = class_info.scope_id; - PushNameQualifierScope(*context_, name_context.resolved_inst_id, - class_info.scope_id); + if (!is_unqualified) { + PushNameQualifierScope(*context_, name_context.resolved_inst_id, + class_info.scope_id); + } } else { name_context.state = NameContext::State::ResolvedNonScope; } @@ -194,8 +199,11 @@ auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context) -> void { auto scope_id = resolved_inst.As().name_scope_id; name_context.state = NameContext::State::Resolved; name_context.target_scope_id = scope_id; - PushNameQualifierScope(*context_, name_context.resolved_inst_id, scope_id, - context_->name_scopes().Get(scope_id).has_error); + if (!is_unqualified) { + PushNameQualifierScope(*context_, name_context.resolved_inst_id, + scope_id, + context_->name_scopes().Get(scope_id).has_error); + } break; } default: diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h index a7ca1600434b..4033a60210f3 100644 --- a/toolchain/check/decl_name_stack.h +++ b/toolchain/check/decl_name_stack.h @@ -201,7 +201,7 @@ class DeclNameStack { // Applies a Name from the name stack to given name context. auto ApplyNameQualifierTo(NameContext& name_context, Parse::NodeId parse_node, - SemIR::NameId name_id) -> void; + SemIR::NameId name_id, bool is_unqualified) -> void; // Returns true if the context is in a state where it can resolve qualifiers. // Updates name_context as needed. @@ -209,8 +209,11 @@ class DeclNameStack { -> bool; // Updates the scope on name_context as needed. This is called after - // resolution is complete, whether for Name or expression. - auto UpdateScopeIfNeeded(NameContext& name_context) -> void; + // resolution is complete, whether for Name or expression. When updating for + // an unqualified name, the resolution is noted without pushing scopes; it's + // instead expected this will become a name conflict. + auto UpdateScopeIfNeeded(NameContext& name_context, bool is_unqualified) + -> void; // The linked context. Context* context_; diff --git a/toolchain/check/testdata/var/fail_namespace_conflict.carbon b/toolchain/check/testdata/var/fail_namespace_conflict.carbon new file mode 100644 index 000000000000..cbf210397502 --- /dev/null +++ b/toolchain/check/testdata/var/fail_namespace_conflict.carbon @@ -0,0 +1,46 @@ +// 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 + +namespace A; + +// CHECK:STDERR: fail_namespace_conflict.carbon:[[@LINE+6]]:5: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: var A: i32; +// CHECK:STDERR: ^ +// CHECK:STDERR: fail_namespace_conflict.carbon:[[@LINE-5]]:1: Name is previously declared here. +// CHECK:STDERR: namespace A; +// CHECK:STDERR: ^~~~~~~~~~~~ +var A: i32; + +// CHECK:STDERR: fail_namespace_conflict.carbon:[[@LINE+6]]:5: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: var A: i32 = 1; +// CHECK:STDERR: ^ +// CHECK:STDERR: fail_namespace_conflict.carbon:[[@LINE-13]]:1: Name is previously declared here. +// CHECK:STDERR: namespace A; +// CHECK:STDERR: ^~~~~~~~~~~~ +var A: i32 = 1; + +// CHECK:STDOUT: --- fail_namespace_conflict.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: i32 = int_literal 1 [template] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: package: = namespace {.A = %.loc7} [template] +// CHECK:STDOUT: %.loc7: = namespace {} [template] +// CHECK:STDOUT: %A.var.loc15: ref i32 = var A +// CHECK:STDOUT: %A.loc15: ref i32 = bind_name A, %A.var.loc15 +// CHECK:STDOUT: %A.var.loc23: ref i32 = var A +// CHECK:STDOUT: %A.loc23: ref i32 = bind_name A, %A.var.loc23 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @__global_init() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %.loc23: i32 = int_literal 1 [template = constants.%.1] +// CHECK:STDOUT: assign file.%A.var.loc23, %.loc23 +// CHECK:STDOUT: return +// CHECK:STDOUT: } +// CHECK:STDOUT: diff --git a/toolchain/check/testdata/var/shadowing.carbon b/toolchain/check/testdata/var/shadowing.carbon index 6e663a8e5220..6802a017af5c 100644 --- a/toolchain/check/testdata/var/shadowing.carbon +++ b/toolchain/check/testdata/var/shadowing.carbon @@ -4,7 +4,12 @@ // // AUTOUPDATE +namespace NS; + fn Main() { + var NS: i32 = 0; + NS = 1; + var x: i32 = 0; if (true) { var x: i32 = 0; @@ -18,32 +23,40 @@ fn Main() { // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 0 [template] -// CHECK:STDOUT: %.2: bool = bool_literal true [template] -// CHECK:STDOUT: %.3: i32 = int_literal 1 [template] +// CHECK:STDOUT: %.2: i32 = int_literal 1 [template] +// CHECK:STDOUT: %.3: bool = bool_literal true [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { -// CHECK:STDOUT: package: = namespace {.Main = %Main} [template] +// CHECK:STDOUT: package: = namespace {.NS = %.loc7, .Main = %Main} [template] +// CHECK:STDOUT: %.loc7: = namespace {} [template] // CHECK:STDOUT: %Main: = fn_decl @Main [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: fn @Main() { // CHECK:STDOUT: !entry: -// CHECK:STDOUT: %x.var.loc8: ref i32 = var x -// CHECK:STDOUT: %x.loc8: ref i32 = bind_name x, %x.var.loc8 -// CHECK:STDOUT: %.loc8: i32 = int_literal 0 [template = constants.%.1] -// CHECK:STDOUT: assign %x.var.loc8, %.loc8 -// CHECK:STDOUT: %.loc9: bool = bool_literal true [template = constants.%.2] -// CHECK:STDOUT: if %.loc9 br !if.then else br !if.else +// CHECK:STDOUT: %NS.var: ref i32 = var NS +// CHECK:STDOUT: %NS: ref i32 = bind_name NS, %NS.var +// CHECK:STDOUT: %.loc10: i32 = int_literal 0 [template = constants.%.1] +// CHECK:STDOUT: assign %NS.var, %.loc10 +// CHECK:STDOUT: %NS.ref: ref i32 = name_ref NS, %NS +// CHECK:STDOUT: %.loc11: i32 = int_literal 1 [template = constants.%.2] +// CHECK:STDOUT: assign %NS.ref, %.loc11 +// CHECK:STDOUT: %x.var.loc13: ref i32 = var x +// CHECK:STDOUT: %x.loc13: ref i32 = bind_name x, %x.var.loc13 +// CHECK:STDOUT: %.loc13: i32 = int_literal 0 [template = constants.%.1] +// CHECK:STDOUT: assign %x.var.loc13, %.loc13 +// CHECK:STDOUT: %.loc14: bool = bool_literal true [template = constants.%.3] +// CHECK:STDOUT: if %.loc14 br !if.then else br !if.else // CHECK:STDOUT: // CHECK:STDOUT: !if.then: -// CHECK:STDOUT: %x.var.loc10: ref i32 = var x -// CHECK:STDOUT: %x.loc10: ref i32 = bind_name x, %x.var.loc10 -// CHECK:STDOUT: %.loc10: i32 = int_literal 0 [template = constants.%.1] -// CHECK:STDOUT: assign %x.var.loc10, %.loc10 -// CHECK:STDOUT: %x.ref: ref i32 = name_ref x, %x.loc10 -// CHECK:STDOUT: %.loc13: i32 = int_literal 1 [template = constants.%.3] -// CHECK:STDOUT: assign %x.ref, %.loc13 +// CHECK:STDOUT: %x.var.loc15: ref i32 = var x +// CHECK:STDOUT: %x.loc15: ref i32 = bind_name x, %x.var.loc15 +// CHECK:STDOUT: %.loc15: i32 = int_literal 0 [template = constants.%.1] +// CHECK:STDOUT: assign %x.var.loc15, %.loc15 +// CHECK:STDOUT: %x.ref: ref i32 = name_ref x, %x.loc15 +// CHECK:STDOUT: %.loc18: i32 = int_literal 1 [template = constants.%.2] +// CHECK:STDOUT: assign %x.ref, %.loc18 // CHECK:STDOUT: br !if.else // CHECK:STDOUT: // CHECK:STDOUT: !if.else: