From 9b194a31c9234cf4638c194f295569c9f39731eb Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 6 Dec 2023 14:45:19 -0800 Subject: [PATCH] Support modifiers on namespace, in theory. (#3462) In theory because none are allowed. This is to improve consistency in handle_decl_name_scope's modifier handling, removing the namespace special-case. I noticed there's a crash bug on `impl ` which I'll address separately. This builds on #3461. --- toolchain/check/decl_state.h | 22 +++++++-- toolchain/check/handle_namespace.cpp | 16 +++++-- .../testdata/namespace/fail_modifiers.carbon | 45 +++++++++++++++++++ toolchain/diagnostics/diagnostic_kind.def | 1 - toolchain/parse/handle_decl_scope_loop.cpp | 10 +---- toolchain/parse/node_kind.def | 7 +-- .../testdata/namespace/fail_modifiers.carbon | 30 ------------- .../parse/testdata/namespace/modifiers.carbon | 27 +++++++++++ 8 files changed, 108 insertions(+), 50 deletions(-) create mode 100644 toolchain/check/testdata/namespace/fail_modifiers.carbon delete mode 100644 toolchain/parse/testdata/namespace/fail_modifiers.carbon create mode 100644 toolchain/parse/testdata/namespace/modifiers.carbon diff --git a/toolchain/check/decl_state.h b/toolchain/check/decl_state.h index 695d9795ff48..9f68ba10e121 100644 --- a/toolchain/check/decl_state.h +++ b/toolchain/check/decl_state.h @@ -6,13 +6,17 @@ #define CARBON_TOOLCHAIN_CHECK_DECL_STATE_H_ #include "llvm/ADT/BitmaskEnum.h" +#include "toolchain/parse/tree.h" namespace Carbon::Check { LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); // Represents a set of keyword modifiers, using a separate bit per modifier. -enum class KeywordModifierSet { +// +// We expect this to grow, so are using a bigger size than needed. +// NOLINTNEXTLINE(performance-enum-size) +enum class KeywordModifierSet : uint32_t { // At most one of these access modifiers allowed for a given declaration, // and if present it must be first: Private = 1 << 0, @@ -35,7 +39,7 @@ enum class KeywordModifierSet { Interface = Default | Final, None = 0, - LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ Virtual) + LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Virtual) }; inline auto operator!(KeywordModifierSet k) -> bool { @@ -45,8 +49,18 @@ inline auto operator!(KeywordModifierSet k) -> bool { // State stored for each declaration we are currently in: the kind of // declaration and the keyword modifiers that apply to that declaration. struct DeclState { - // What kind of declaration - enum DeclKind { FileScope, Class, Base, Constraint, Fn, Interface, Let, Var }; + // The kind of declaration. + enum DeclKind : int8_t { + FileScope, + Base, + Class, + Constraint, + Fn, + Interface, + Let, + Namespace, + Var + }; explicit DeclState(DeclKind decl_kind, Parse::NodeId parse_node) : kind(decl_kind), first_node(parse_node) {} diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp index 39d5229edf0a..70ad397847f3 100644 --- a/toolchain/check/handle_namespace.cpp +++ b/toolchain/check/handle_namespace.cpp @@ -3,23 +3,31 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #include "toolchain/check/context.h" +#include "toolchain/check/decl_state.h" +#include "toolchain/check/modifiers.h" #include "toolchain/sem_ir/inst.h" namespace Carbon::Check { -auto HandleNamespaceStart(Context& context, Parse::NodeId /*parse_node*/) - -> bool { +auto HandleNamespaceStart(Context& context, Parse::NodeId parse_node) -> bool { + // Optional modifiers and the name follow. + context.decl_state_stack().Push(DeclState::Namespace, parse_node); context.decl_name_stack().PushScopeAndStartName(); return true; } -auto HandleNamespace(Context& context, Parse::NodeId parse_node) -> bool { +auto HandleNamespace(Context& context, Parse::NodeId /*parse_node*/) -> bool { auto name_context = context.decl_name_stack().FinishName(); + auto first_node = context.decl_state_stack().innermost().first_node; + LimitModifiersOnDecl(context, KeywordModifierSet::None, + Lex::TokenKind::Namespace); auto namespace_id = context.AddInst(SemIR::Namespace{ - parse_node, context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType), + first_node, context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType), context.name_scopes().Add()}); context.decl_name_stack().AddNameToLookup(name_context, namespace_id); + context.decl_name_stack().PopScope(); + context.decl_state_stack().Pop(DeclState::Namespace); return true; } diff --git a/toolchain/check/testdata/namespace/fail_modifiers.carbon b/toolchain/check/testdata/namespace/fail_modifiers.carbon new file mode 100644 index 000000000000..d62a4a7e567e --- /dev/null +++ b/toolchain/check/testdata/namespace/fail_modifiers.carbon @@ -0,0 +1,45 @@ +// 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 + +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+24]]:1: ERROR: `private` not allowed on `namespace` declaration. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+21]]:9: ERROR: `abstract` not allowed on `namespace` declaration. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:18: ERROR: `base` not allowed on declaration with `abstract`. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:9: `abstract` previously appeared here. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+12]]:23: ERROR: `default` not allowed on declaration with `abstract`. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+9]]:9: `abstract` previously appeared here. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+6]]:31: ERROR: `final` not allowed on declaration with `abstract`. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~ +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:9: `abstract` previously appeared here. +// CHECK:STDERR: private abstract base default final namespace Foo; +// CHECK:STDERR: ^~~~~~~~ +private abstract base default final namespace Foo; + +// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: ERROR: `impl` not allowed on `namespace` declaration. +// CHECK:STDERR: impl namespace Bar; +// CHECK:STDERR: ^~~~ +impl namespace Bar; + +// CHECK:STDOUT: --- fail_modifiers.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: file { +// CHECK:STDOUT: package: = namespace {.Foo = %.loc31, .Bar = %.loc36} +// CHECK:STDOUT: %.loc31: = namespace {} +// CHECK:STDOUT: %.loc36: = namespace {} +// CHECK:STDOUT: } +// CHECK:STDOUT: diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 2f739beda9b4..970bc36d3449 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -110,7 +110,6 @@ CARBON_DIAGNOSTIC_KIND(MethodImplNotAllowed) CARBON_DIAGNOSTIC_KIND(ParamsRequiredAfterImplicit) CARBON_DIAGNOSTIC_KIND(ParamsRequiredByIntroducer) CARBON_DIAGNOSTIC_KIND(ExpectedAfterBase) -CARBON_DIAGNOSTIC_KIND(NamespaceAfterModifiers) // ============================================================================ // Semantics diagnostics diff --git a/toolchain/parse/handle_decl_scope_loop.cpp b/toolchain/parse/handle_decl_scope_loop.cpp index 7f026160c14f..c3acf04c8719 100644 --- a/toolchain/parse/handle_decl_scope_loop.cpp +++ b/toolchain/parse/handle_decl_scope_loop.cpp @@ -47,6 +47,7 @@ static auto TokenIsModifierOrIntroducer(Lex::TokenKind token_kind) -> bool { case Lex::TokenKind::Impl: case Lex::TokenKind::Interface: case Lex::TokenKind::Let: + case Lex::TokenKind::Namespace: case Lex::TokenKind::Private: case Lex::TokenKind::Protected: case Lex::TokenKind::Var: @@ -248,14 +249,7 @@ auto HandleDeclScopeLoop(Context& context) -> void { // they can't have modifiers and don't use bracketing parse nodes that // would allow a variable number of modifier nodes. case Lex::TokenKind::Namespace: { - if (saw_modifier) { - CARBON_DIAGNOSTIC(NamespaceAfterModifiers, Error, - "`namespace` unexpected after modifiers."); - context.emitter().Emit(*context.position(), NamespaceAfterModifiers); - OutputInvalidParseSubtree(context, state.subtree_start); - } else { - introducer(NodeKind::NamespaceStart, State::Namespace); - } + introducer(NodeKind::NamespaceStart, State::Namespace); return; } case Lex::TokenKind::Semi: { diff --git a/toolchain/parse/node_kind.def b/toolchain/parse/node_kind.def index 7f1dbd363698..88c4cd5cdf68 100644 --- a/toolchain/parse/node_kind.def +++ b/toolchain/parse/node_kind.def @@ -175,12 +175,13 @@ CARBON_PARSE_NODE_KIND_CHILD_COUNT(LibrarySpecifier, 1, CARBON_TOKEN(Library)) // `namespace`: // NamespaceStart +// _repeated_ _external_: modifier // _external_: Name or QualifiedDecl // Namespace CARBON_PARSE_NODE_KIND_CHILD_COUNT(NamespaceStart, 0, CARBON_TOKEN(Namespace)) -CARBON_PARSE_NODE_KIND_CHILD_COUNT(Namespace, 2, - CARBON_TOKEN(Semi) - CARBON_IF_ERROR(CARBON_TOKEN(Namespace))) +CARBON_PARSE_NODE_KIND_BRACKET(Namespace, NamespaceStart, + CARBON_TOKEN(Semi) + CARBON_IF_ERROR(CARBON_TOKEN(Namespace))) // A code block: // CodeBlockStart diff --git a/toolchain/parse/testdata/namespace/fail_modifiers.carbon b/toolchain/parse/testdata/namespace/fail_modifiers.carbon deleted file mode 100644 index a904082560c7..000000000000 --- a/toolchain/parse/testdata/namespace/fail_modifiers.carbon +++ /dev/null @@ -1,30 +0,0 @@ -// 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 - -// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:37: ERROR: `namespace` unexpected after modifiers. -// CHECK:STDERR: private abstract base default final namespace Foo; -// CHECK:STDERR: ^~~~~~~~~ -private abstract base default final namespace Foo; - -// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: ERROR: Unrecognized declaration introducer. -// CHECK:STDERR: impl namespace -// CHECK:STDERR: ^~~~ -impl namespace - -// CHECK:STDOUT: - filename: fail_modifiers.carbon -// CHECK:STDOUT: parse_tree: [ -// CHECK:STDOUT: {kind: 'FileStart', text: ''}, -// CHECK:STDOUT: {kind: 'InvalidParseStart', text: 'namespace', has_error: yes}, -// CHECK:STDOUT: {kind: 'PrivateModifier', text: 'private'}, -// CHECK:STDOUT: {kind: 'AbstractModifier', text: 'abstract'}, -// CHECK:STDOUT: {kind: 'BaseModifier', text: 'base'}, -// CHECK:STDOUT: {kind: 'DefaultModifier', text: 'default'}, -// CHECK:STDOUT: {kind: 'FinalModifier', text: 'final'}, -// CHECK:STDOUT: {kind: 'InvalidParseSubtree', text: ';', has_error: yes, subtree_size: 7}, -// CHECK:STDOUT: {kind: 'InvalidParseStart', text: 'impl', has_error: yes}, -// CHECK:STDOUT: {kind: 'InvalidParseSubtree', text: 'namespace', has_error: yes, subtree_size: 2}, -// CHECK:STDOUT: {kind: 'FileEnd', text: ''}, -// CHECK:STDOUT: ] diff --git a/toolchain/parse/testdata/namespace/modifiers.carbon b/toolchain/parse/testdata/namespace/modifiers.carbon new file mode 100644 index 000000000000..753395dfe4db --- /dev/null +++ b/toolchain/parse/testdata/namespace/modifiers.carbon @@ -0,0 +1,27 @@ +// 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 + +private abstract base default final namespace Foo; + +impl namespace Foo; + +// CHECK:STDOUT: - filename: modifiers.carbon +// CHECK:STDOUT: parse_tree: [ +// CHECK:STDOUT: {kind: 'FileStart', text: ''}, +// CHECK:STDOUT: {kind: 'NamespaceStart', text: 'namespace'}, +// CHECK:STDOUT: {kind: 'PrivateModifier', text: 'private'}, +// CHECK:STDOUT: {kind: 'AbstractModifier', text: 'abstract'}, +// CHECK:STDOUT: {kind: 'BaseModifier', text: 'base'}, +// CHECK:STDOUT: {kind: 'DefaultModifier', text: 'default'}, +// CHECK:STDOUT: {kind: 'FinalModifier', text: 'final'}, +// CHECK:STDOUT: {kind: 'Name', text: 'Foo'}, +// CHECK:STDOUT: {kind: 'Namespace', text: ';', subtree_size: 8}, +// CHECK:STDOUT: {kind: 'NamespaceStart', text: 'namespace'}, +// CHECK:STDOUT: {kind: 'ImplModifier', text: 'impl'}, +// CHECK:STDOUT: {kind: 'Name', text: 'Foo'}, +// CHECK:STDOUT: {kind: 'Namespace', text: ';', subtree_size: 4}, +// CHECK:STDOUT: {kind: 'FileEnd', text: ''}, +// CHECK:STDOUT: ]