From 3d661c96f3bfb6f226deee5f8446fbaf5d9e4749 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 21 Dec 2023 10:36:33 -0800 Subject: [PATCH] Handle out-of-line declarations. (#3536) By adding an InstId to the NameScope, we can determine whether the declaration is being added to a scoped entity (versus a namespace). The choice of InstId on NameScope is chosen versus other solutions because, for imports, we want to just have a list of InstIds to import and, from those, get the containing namespaces for addition. Similar may also be desirable for printing fully qualified names given a singular InstId. That means an InstId must have a path to find enclosing name scopes. What we're looking at here is: - NameScopeId knows its InstId. (done here) - Inst knows the enclosing NameScopeId. (future work) - To walk up enclosing scopes for an Inst: 1. Fetch the Inst. 2. Find its enclosing NameScopeId (which will be per-declaration due to Function etc complexity). 3. Fetch the NameScope if not Package scope. (if Package scope, done) 4. Use the InstId on the NameScope to go back to step 1. --- toolchain/check/check.cpp | 3 +- toolchain/check/decl_name_stack.cpp | 29 +++++++++++---- toolchain/check/decl_name_stack.h | 5 ++- toolchain/check/handle_class.cpp | 2 +- toolchain/check/handle_interface.cpp | 2 +- toolchain/check/handle_namespace.cpp | 8 +++- .../class/fail_base_method_define.carbon | 6 ++- .../class/fail_out_of_line_decl.carbon | 37 +++++++++++++++++++ toolchain/diagnostics/diagnostic_kind.def | 1 + toolchain/sem_ir/value_stores.h | 9 ++++- 10 files changed, 85 insertions(+), 17 deletions(-) create mode 100644 toolchain/check/testdata/class/fail_out_of_line_decl.carbon diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index 786bd8cafdec..5c2e2f2ed253 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -73,7 +73,8 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info) -> void { // Define the package scope, with an instruction for `package` expressions to // reference. - auto package_scope_id = context.name_scopes().Add(); + auto package_scope_id = + context.name_scopes().Add(SemIR::InstId::PackageNamespace); CARBON_CHECK(package_scope_id == SemIR::NameScopeId::Package); auto package_inst = context.AddInst(SemIR::Namespace{ diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index 669c446217c4..61113601ccca 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/toolchain/check/decl_name_stack.cpp @@ -69,11 +69,22 @@ auto DeclNameStack::LookupOrAddName(NameContext name_context, context_->AddNameToLookup(name_context.parse_node, name_context.unresolved_name_id, target_id); } else { - // TODO: Reject unless the scope is a namespace scope or the name is - // unqualified. - bool success = context_->name_scopes().AddEntry( - name_context.target_scope_id, name_context.unresolved_name_id, - target_id); + auto& name_scope = + context_->name_scopes().Get(name_context.target_scope_id); + if (name_context.has_qualifiers) { + auto inst = context_->insts().Get(name_scope.inst_id); + if (!inst.Is()) { + // TODO: Point at the declaration for the scoped entity. + CARBON_DIAGNOSTIC( + QualifiedDeclOutsideScopeEntity, Error, + "Out-of-line declaration requires a declaration in " + "scoped entity."); + context_->emitter().Emit(name_context.parse_node, + QualifiedDeclOutsideScopeEntity); + } + } + auto [_, success] = name_scope.names.insert( + {name_context.unresolved_name_id, target_id}); CARBON_CHECK(success) << "Duplicate names should have been resolved previously: " << name_context.unresolved_name_id << " in " @@ -102,7 +113,7 @@ auto DeclNameStack::ApplyNameQualifier(Parse::NodeId parse_node, auto DeclNameStack::ApplyNameQualifierTo(NameContext& name_context, Parse::NodeId parse_node, SemIR::NameId name_id) -> void { - if (CanResolveQualifier(name_context, parse_node)) { + if (TryResolveQualifier(name_context, parse_node)) { // For identifier nodes, we need to perform a lookup on the identifier. auto resolved_inst_id = context_->LookupNameInDecl( name_context.parse_node, name_id, name_context.target_scope_id); @@ -152,8 +163,12 @@ auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context) -> void { } } -auto DeclNameStack::CanResolveQualifier(NameContext& name_context, +auto DeclNameStack::TryResolveQualifier(NameContext& name_context, Parse::NodeId parse_node) -> 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; + switch (name_context.state) { case NameContext::State::Error: // Already in an error state, so return without examining. diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h index d98b200e3776..a56a4f039f58 100644 --- a/toolchain/check/decl_name_stack.h +++ b/toolchain/check/decl_name_stack.h @@ -110,6 +110,9 @@ class DeclNameStack { State state = State::Empty; + // Whether there have been qualifiers in the name. + bool has_qualifiers = false; + // 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. @@ -182,7 +185,7 @@ class DeclNameStack { // 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::NodeId parse_node) + auto TryResolveQualifier(NameContext& name_context, Parse::NodeId parse_node) -> bool; // Updates the scope on name_context as needed. This is called after diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index eedec4710560..948ea3190827 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -136,7 +136,7 @@ auto HandleClassDefinitionStart(Context& context, Parse::NodeId parse_node) .Emit(); } else { class_info.definition_id = class_decl_id; - class_info.scope_id = context.name_scopes().Add(); + class_info.scope_id = context.name_scopes().Add(class_decl_id); } // Enter the class scope. diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index 9dd1b7256847..e0feeb794045 100644 --- a/toolchain/check/handle_interface.cpp +++ b/toolchain/check/handle_interface.cpp @@ -109,7 +109,7 @@ auto HandleInterfaceDefinitionStart(Context& context, Parse::NodeId parse_node) .Emit(); } else { interface_info.definition_id = interface_decl_id; - interface_info.scope_id = context.name_scopes().Add(); + interface_info.scope_id = context.name_scopes().Add(interface_decl_id); } // Enter the interface scope. diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp index 4445dd27e41e..ebeced48b506 100644 --- a/toolchain/check/handle_namespace.cpp +++ b/toolchain/check/handle_namespace.cpp @@ -5,6 +5,7 @@ #include "toolchain/check/context.h" #include "toolchain/check/decl_state.h" #include "toolchain/check/modifiers.h" +#include "toolchain/sem_ir/ids.h" #include "toolchain/sem_ir/inst.h" namespace Carbon::Check { @@ -21,9 +22,12 @@ auto HandleNamespace(Context& context, Parse::NodeId parse_node) -> bool { auto name_context = context.decl_name_stack().FinishName(); LimitModifiersOnDecl(context, KeywordModifierSet::None, Lex::TokenKind::Namespace); - auto namespace_id = context.AddInst(SemIR::Namespace{ + auto namespace_inst = SemIR::Namespace{ parse_node, context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType), - context.name_scopes().Add()}); + SemIR::NameScopeId::Invalid}; + auto namespace_id = context.AddInst(namespace_inst); + namespace_inst.name_scope_id = context.name_scopes().Add(namespace_id); + context.insts().Set(namespace_id, namespace_inst); context.decl_name_stack().AddNameToLookup(name_context, namespace_id); context.decl_name_stack().PopScope(); diff --git a/toolchain/check/testdata/class/fail_base_method_define.carbon b/toolchain/check/testdata/class/fail_base_method_define.carbon index db6fb70a196d..d186fe7aa990 100644 --- a/toolchain/check/testdata/class/fail_base_method_define.carbon +++ b/toolchain/check/testdata/class/fail_base_method_define.carbon @@ -16,7 +16,9 @@ class D { extend base: B; } -// TODO: This should be rejected. +// CHECK:STDERR: fail_base_method_define.carbon:[[@LINE+3]]:6: ERROR: Out-of-line declaration requires a declaration in scoped entity. +// CHECK:STDERR: fn D.F() {} +// CHECK:STDERR: ^ fn D.F() {} // CHECK:STDERR: fail_base_method_define.carbon:[[@LINE+3]]:6: ERROR: Name `C` not found. @@ -40,7 +42,7 @@ fn D.C.F() {} // CHECK:STDOUT: %D.decl = class_decl @D, () // CHECK:STDOUT: %D: type = class_type @D // CHECK:STDOUT: %F: = fn_decl @F.3 -// CHECK:STDOUT: %.loc25: = fn_decl @.1 +// CHECK:STDOUT: %.loc27: = fn_decl @.1 // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: class @B { diff --git a/toolchain/check/testdata/class/fail_out_of_line_decl.carbon b/toolchain/check/testdata/class/fail_out_of_line_decl.carbon new file mode 100644 index 000000000000..627409e0eca1 --- /dev/null +++ b/toolchain/check/testdata/class/fail_out_of_line_decl.carbon @@ -0,0 +1,37 @@ +// 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 + +class C {} + +// CHECK:STDERR: fail_out_of_line_decl.carbon:[[@LINE+3]]:6: ERROR: Out-of-line declaration requires a declaration in scoped entity. +// CHECK:STDERR: fn C.F() {} +// CHECK:STDERR: ^ +fn C.F() {} + +// CHECK:STDOUT: --- fail_out_of_line_decl.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.loc7: type = struct_type {} +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: package: = namespace {.C = %C.decl} +// CHECK:STDOUT: %C.decl = class_decl @C, () +// CHECK:STDOUT: %C: type = class_type @C +// CHECK:STDOUT: %F: = fn_decl @F +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @C { +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .F = file.%F +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: return +// CHECK:STDOUT: } +// CHECK:STDOUT: diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 52cce33e09f8..0828219bf201 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -214,6 +214,7 @@ CARBON_DIAGNOSTIC_KIND(ReturnStatementMissingExpr) CARBON_DIAGNOSTIC_KIND(ImplicitAsConversionFailure) CARBON_DIAGNOSTIC_KIND(ExplicitAsConversionFailure) CARBON_DIAGNOSTIC_KIND(TypeExprEvaluationFailure) +CARBON_DIAGNOSTIC_KIND(QualifiedDeclOutsideScopeEntity) CARBON_DIAGNOSTIC_KIND(QualifiedDeclInIncompleteClassScope) CARBON_DIAGNOSTIC_KIND(QualifiedDeclInNonScope) CARBON_DIAGNOSTIC_KIND(QualifiedDeclNonScopeEntity) diff --git a/toolchain/sem_ir/value_stores.h b/toolchain/sem_ir/value_stores.h index aaef8b951e4b..d1d353305ff5 100644 --- a/toolchain/sem_ir/value_stores.h +++ b/toolchain/sem_ir/value_stores.h @@ -173,7 +173,7 @@ class NameStoreWrapper { struct NameScope { // Names in the scope. - llvm::DenseMap names; + llvm::DenseMap names = llvm::DenseMap(); // Scopes extended by this scope. // @@ -193,6 +193,9 @@ struct NameScope { // TODO: Consider using something like `TinyPtrVector` for this. llvm::SmallVector extended_scopes; + // The instructioning which owns the scope. + InstId inst_id; + // Whether we have diagnosed an error in a construct that would have added // names to this scope. For example, this can happen if an `import` failed or // an `extend` declaration was ill-formed. If true, the `names` map is @@ -205,7 +208,9 @@ struct NameScope { class NameScopeStore { public: // Adds a name scope, returning an ID to reference it. - auto Add() -> NameScopeId { return values_.AddDefaultValue(); } + auto Add(InstId inst_id) -> NameScopeId { + return values_.Add({.inst_id = inst_id}); + } // Adds an entry to a name scope. Returns true on success, false on // duplicates.