From ed6d2baa99e50204f0111b188a1e92c2ff54e7f5 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 30 May 2024 15:14:43 -0700 Subject: [PATCH] Refactor application of name qualifiers in decl name stack. (#4008) Split apart the handling of name qualifiers and the final name a little, in preparation for also handling parameters when checking name qualifiers. Slightly improve diagnostic for non-scope qualifier. --- toolchain/check/decl_name_stack.cpp | 98 ++++++++++--------- toolchain/check/decl_name_stack.h | 14 +-- .../no_prelude/fail_local_in_namespace.carbon | 2 +- ..._conflict_imported_namespace_second.carbon | 11 ++- .../namespace/fail_decl_in_alias.carbon | 8 +- .../packages/no_prelude/export_name.carbon | 11 ++- .../no_prelude/fail_export_name_member.carbon | 11 ++- 7 files changed, 87 insertions(+), 68 deletions(-) diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index aa101613aa21..fe5c713b0fa3 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/toolchain/check/decl_name_stack.cpp @@ -44,7 +44,7 @@ auto DeclNameStack::MakeEmptyNameContext() -> NameContext { auto DeclNameStack::MakeUnqualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id) -> NameContext { NameContext context = MakeEmptyNameContext(); - ApplyNameQualifierTo(context, loc_id, name_id, /*is_unqualified=*/true); + ApplyAndLookupName(context, loc_id, name_id, /*is_unqualified=*/true); return context; } @@ -59,8 +59,8 @@ auto DeclNameStack::FinishName(const NameComponent& name) -> NameContext { CARBON_CHECK(decl_name_stack_.back().state != NameContext::State::Finished) << "Finished name twice"; - ApplyNameQualifierTo(decl_name_stack_.back(), name.name_loc_id, name.name_id, - /*is_unqualified=*/false); + ApplyAndLookupName(decl_name_stack_.back(), name.name_loc_id, name.name_id, + /*is_unqualified=*/false); NameContext result = decl_name_stack_.back(); decl_name_stack_.back().state = NameContext::State::Finished; @@ -182,31 +182,47 @@ auto DeclNameStack::ApplyNameQualifier(const NameComponent& name) -> void { if (name.implicit_params_id.is_valid() || name.params_id.is_valid()) { context_->TODO(name.params_loc_id, "name qualifier with parameters"); } - ApplyNameQualifierTo(decl_name_stack_.back(), name.name_loc_id, name.name_id, - /*is_unqualified=*/false); + + auto& name_context = decl_name_stack_.back(); + ApplyAndLookupName(name_context, name.name_loc_id, name.name_id, + /*is_unqualified=*/false); + name_context.has_qualifiers = true; + if (!CheckValidAsQualifier(name_context)) { + name_context.state = NameContext::State::Error; + } } -auto DeclNameStack::ApplyNameQualifierTo(NameContext& name_context, - SemIR::LocId loc_id, - SemIR::NameId name_id, - bool is_unqualified) -> void { - if (TryResolveQualifier(name_context, loc_id)) { - // For identifier nodes, we need to perform a lookup on the identifier. - auto resolved_inst_id = context_->LookupNameInDecl( - name_context.loc_id, name_id, name_context.enclosing_scope_id); - if (!resolved_inst_id.is_valid()) { - // Invalid indicates an unresolved name. Store it and return. - name_context.unresolved_name_id = name_id; - name_context.state = NameContext::State::Unresolved; - return; - } else { - // Store the resolved instruction and continue for the target scope - // update. - name_context.resolved_inst_id = resolved_inst_id; - } +auto DeclNameStack::ApplyAndLookupName(NameContext& name_context, + SemIR::LocId loc_id, + SemIR::NameId name_id, + bool is_unqualified) -> void { + // The location of the name is the location of the last name token we've + // processed so far. + name_context.loc_id = loc_id; - UpdateScopeIfNeeded(name_context, is_unqualified); + // Don't perform any more lookups after we hit an error. We still track the + // final name, though. + if (name_context.state == NameContext::State::Error) { + name_context.unresolved_name_id = name_id; + return; } + + // For identifier nodes, we need to perform a lookup on the identifier. + auto resolved_inst_id = context_->LookupNameInDecl( + name_context.loc_id, name_id, name_context.enclosing_scope_id); + if (!resolved_inst_id.is_valid()) { + // Invalid indicates an unresolved name. Store it and return. + name_context.unresolved_name_id = name_id; + name_context.state = NameContext::State::Unresolved; + return; + } else { + // Store the resolved instruction and continue for the target scope + // update. + name_context.resolved_inst_id = resolved_inst_id; + } + + // Enter the scope of the existing entity. + UpdateScopeIfNeeded(name_context, is_unqualified); } // Push a scope corresponding to a name qualifier. For example, for @@ -293,21 +309,25 @@ auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context, } } -auto DeclNameStack::TryResolveQualifier(NameContext& name_context, - SemIR::LocId loc_id) -> bool { - // Update has_qualifiers based on the state before any possible changes. If - // this is the first qualifier, it may just be the name. - name_context.has_qualifiers = name_context.state != NameContext::State::Empty; - +auto DeclNameStack::CheckValidAsQualifier(const NameContext& name_context) + -> bool { switch (name_context.state) { case NameContext::State::Error: // Already in an error state, so return without examining. return false; + case NameContext::State::Resolved: + return true; + + case NameContext::State::Empty: + CARBON_FATAL() << "No qualifier to resolve"; + + case NameContext::State::Finished: + CARBON_FATAL() << "Added a qualifier after calling FinishName"; + case NameContext::State::Unresolved: // Because more qualifiers were found, we diagnose that the earlier // qualifier failed to resolve. - name_context.state = NameContext::State::Error; context_->DiagnoseNameNotFound(name_context.loc_id, name_context.unresolved_name_id); return false; @@ -346,24 +366,14 @@ auto DeclNameStack::TryResolveQualifier(NameContext& name_context, "Name qualifiers are only allowed for entities that " "provide a scope."); CARBON_DIAGNOSTIC(QualifiedNameNonScopeEntity, Note, - "Non-scope entity referenced here."); + "Referenced non-scope entity declared here."); context_->emitter() - .Build(loc_id, QualifiedNameInNonScope) - .Note(name_context.loc_id, QualifiedNameNonScopeEntity) + .Build(name_context.loc_id, QualifiedNameInNonScope) + .Note(name_context.resolved_inst_id, QualifiedNameNonScopeEntity) .Emit(); } - name_context.state = NameContext::State::Error; return false; } - - case NameContext::State::Empty: - case NameContext::State::Resolved: { - name_context.loc_id = loc_id; - return true; - } - - case NameContext::State::Finished: - CARBON_FATAL() << "Added a qualifier after calling FinishName"; } } diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h index 1b4d445351bb..5d5002c7c662 100644 --- a/toolchain/check/decl_name_stack.h +++ b/toolchain/check/decl_name_stack.h @@ -228,14 +228,14 @@ class DeclNameStack { // Returns a name context corresponding to an empty name. auto MakeEmptyNameContext() -> NameContext; - // Applies a Name from the name stack to given name context. - auto ApplyNameQualifierTo(NameContext& name_context, SemIR::LocId loc_id, - SemIR::NameId name_id, bool is_unqualified) -> void; + // Appends a name to the given name context, and performs a lookup to find + // what, if anything, the name refers to. + auto ApplyAndLookupName(NameContext& name_context, SemIR::LocId loc_id, + 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. - auto TryResolveQualifier(NameContext& name_context, SemIR::LocId loc_id) - -> bool; + // Checks and returns whether the given name context can be used as a + // qualifier. A suitable diagnostic is issued if not. + auto CheckValidAsQualifier(const NameContext& name_context) -> bool; // Updates the scope on name_context as needed. This is called after // resolution is complete, whether for Name or expression. When updating for diff --git a/toolchain/check/testdata/alias/no_prelude/fail_local_in_namespace.carbon b/toolchain/check/testdata/alias/no_prelude/fail_local_in_namespace.carbon index 35f64d1a7509..411ca104789f 100644 --- a/toolchain/check/testdata/alias/no_prelude/fail_local_in_namespace.carbon +++ b/toolchain/check/testdata/alias/no_prelude/fail_local_in_namespace.carbon @@ -47,7 +47,7 @@ fn F() -> {} { // CHECK:STDOUT: fn @F() -> {} { // CHECK:STDOUT: !entry: // CHECK:STDOUT: %.loc18_17: {} = struct_literal () -// CHECK:STDOUT: %.loc18_9: = bind_alias , [template = ] +// CHECK:STDOUT: %.loc18_12: = bind_alias , [template = ] // CHECK:STDOUT: %NS.ref: = name_ref NS, file.%NS [template = file.%NS] // CHECK:STDOUT: %a.ref: = name_ref a, [template = ] // CHECK:STDOUT: return diff --git a/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon b/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon index b374f303b182..2a98e6337d6b 100644 --- a/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon +++ b/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon @@ -28,12 +28,15 @@ import library "fn"; // CHECK:STDERR: namespace NS; -// CHECK:STDERR: fail_conflict.carbon:[[@LINE+6]]:7: ERROR: Name qualifiers are only allowed for entities that provide a scope. -// CHECK:STDERR: fn NS.Foo(); -// CHECK:STDERR: ^~~ -// CHECK:STDERR: fail_conflict.carbon:[[@LINE+3]]:4: Non-scope entity referenced here. +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+9]]:4: ERROR: Name qualifiers are only allowed for entities that provide a scope. // CHECK:STDERR: fn NS.Foo(); // CHECK:STDERR: ^~ +// CHECK:STDERR: fail_conflict.carbon:[[@LINE-17]]:1: In import. +// CHECK:STDERR: import library "fn"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: fn.carbon:4:1: Referenced non-scope entity declared here. +// CHECK:STDERR: fn NS(); +// CHECK:STDERR: ^~~~~~~~ fn NS.Foo(); // CHECK:STDOUT: --- fn.carbon diff --git a/toolchain/check/testdata/namespace/fail_decl_in_alias.carbon b/toolchain/check/testdata/namespace/fail_decl_in_alias.carbon index af3b3fc330cb..cd678ea985c4 100644 --- a/toolchain/check/testdata/namespace/fail_decl_in_alias.carbon +++ b/toolchain/check/testdata/namespace/fail_decl_in_alias.carbon @@ -9,12 +9,12 @@ namespace NS; alias ns = NS; // Aliases can't be used when declaring names. -// CHECK:STDERR: fail_decl_in_alias.carbon:[[@LINE+6]]:7: ERROR: Name qualifiers are only allowed for entities that provide a scope. -// CHECK:STDERR: fn ns.A() -> i32 { return 0; } -// CHECK:STDERR: ^ -// CHECK:STDERR: fail_decl_in_alias.carbon:[[@LINE+3]]:4: Non-scope entity referenced here. +// CHECK:STDERR: fail_decl_in_alias.carbon:[[@LINE+6]]:4: ERROR: Name qualifiers are only allowed for entities that provide a scope. // CHECK:STDERR: fn ns.A() -> i32 { return 0; } // CHECK:STDERR: ^~ +// CHECK:STDERR: fail_decl_in_alias.carbon:[[@LINE-6]]:7: Referenced non-scope entity declared here. +// CHECK:STDERR: alias ns = NS; +// CHECK:STDERR: ^~ fn ns.A() -> i32 { return 0; } // CHECK:STDOUT: --- fail_decl_in_alias.carbon diff --git a/toolchain/check/testdata/packages/no_prelude/export_name.carbon b/toolchain/check/testdata/packages/no_prelude/export_name.carbon index 7f32a4d69427..7ac893fc132e 100644 --- a/toolchain/check/testdata/packages/no_prelude/export_name.carbon +++ b/toolchain/check/testdata/packages/no_prelude/export_name.carbon @@ -116,12 +116,15 @@ library "fail_export_member"; import library "base"; -// CHECK:STDERR: fail_export_member.carbon:[[@LINE+7]]:10: ERROR: Name qualifiers are only allowed for entities that provide a scope. -// CHECK:STDERR: export C.x; -// CHECK:STDERR: ^ -// CHECK:STDERR: fail_export_member.carbon:[[@LINE+4]]:8: Non-scope entity referenced here. +// CHECK:STDERR: fail_export_member.carbon:[[@LINE+10]]:8: ERROR: Name qualifiers are only allowed for entities that provide a scope. // CHECK:STDERR: export C.x; // CHECK:STDERR: ^ +// CHECK:STDERR: fail_export_member.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import library "base"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: base.carbon:4:1: Referenced non-scope entity declared here. +// CHECK:STDERR: class C { +// CHECK:STDERR: ^~~~~~~~~ // CHECK:STDERR: export C.x; diff --git a/toolchain/check/testdata/packages/no_prelude/fail_export_name_member.carbon b/toolchain/check/testdata/packages/no_prelude/fail_export_name_member.carbon index f146da097712..be3f144dc704 100644 --- a/toolchain/check/testdata/packages/no_prelude/fail_export_name_member.carbon +++ b/toolchain/check/testdata/packages/no_prelude/fail_export_name_member.carbon @@ -20,12 +20,15 @@ import library "a"; // TODO: This diagnostic doesn't clearly explain the problem. We should instead // say something like: Only namespace-scope names can be exported. -// CHECK:STDERR: fail_b.carbon:[[@LINE+6]]:10: ERROR: Name qualifiers are only allowed for entities that provide a scope. -// CHECK:STDERR: export C.n; -// CHECK:STDERR: ^ -// CHECK:STDERR: fail_b.carbon:[[@LINE+3]]:8: Non-scope entity referenced here. +// CHECK:STDERR: fail_b.carbon:[[@LINE+9]]:8: ERROR: Name qualifiers are only allowed for entities that provide a scope. // CHECK:STDERR: export C.n; // CHECK:STDERR: ^ +// CHECK:STDERR: fail_b.carbon:[[@LINE-7]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: a.carbon:4:1: Referenced non-scope entity declared here. +// CHECK:STDERR: class C { +// CHECK:STDERR: ^~~~~~~~~ export C.n; // CHECK:STDOUT: --- a.carbon