From 21687e8cb109cca6523b6d8ac5a55bab63d2f799 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 14 Mar 2025 13:16:01 -0700 Subject: [PATCH] Fix use of keyword names in qualifiers with params (#5130) This is a crash bug, since NameQualifierWithParams needs a specific open kind. I'd missed that this wasn't actually tested. --- toolchain/check/handle_name.cpp | 8 ++++- toolchain/check/node_stack.h | 3 +- .../parse/handle_decl_name_and_params.cpp | 7 ++-- toolchain/parse/node_kind.def | 3 +- .../testdata/function/declaration.carbon | 32 ++++++++++++++++--- .../generics/params/name_qualifier.carbon | 10 +++--- toolchain/parse/typed_nodes.h | 22 +++++++++---- toolchain/parse/typed_nodes_test.cpp | 4 +-- 8 files changed, 67 insertions(+), 22 deletions(-) diff --git a/toolchain/check/handle_name.cpp b/toolchain/check/handle_name.cpp index 398d8f7f3aca..8fbe1bee64dd 100644 --- a/toolchain/check/handle_name.cpp +++ b/toolchain/check/handle_name.cpp @@ -192,7 +192,8 @@ auto HandleParseNode(Context& context, Parse::SelfValueNameExprId node_id) } auto HandleParseNode(Context& context, - Parse::NameQualifierWithParamsId /*node_id*/) -> bool { + Parse::IdentifierNameQualifierWithParamsId /*node_id*/) + -> bool { context.decl_name_stack().ApplyNameQualifier(PopNameComponent(context)); return true; } @@ -240,6 +241,11 @@ auto HandleParseNode(Context& context, Parse::DesignatorExprId node_id) return true; } +auto HandleParseNode(Context& context, + Parse::KeywordNameQualifierWithParamsId node_id) -> bool { + return context.TODO(node_id, "KeywordNameQualifierWithParamsId"); +} + auto HandleParseNode(Context& context, Parse::KeywordNameQualifierWithoutParamsId node_id) -> bool { diff --git a/toolchain/check/node_stack.h b/toolchain/check/node_stack.h index d4666aa889d7..89b056389a63 100644 --- a/toolchain/check/node_stack.h +++ b/toolchain/check/node_stack.h @@ -472,12 +472,14 @@ class NodeStack { case Parse::NodeKind::ForHeader: case Parse::NodeKind::ForHeaderStart: case Parse::NodeKind::ForIn: + case Parse::NodeKind::IdentifierNameQualifierWithParams: case Parse::NodeKind::IdentifierNameQualifierWithoutParams: case Parse::NodeKind::IdentifierPackageName: case Parse::NodeKind::IfConditionStart: case Parse::NodeKind::ImportIntroducer: case Parse::NodeKind::IndexExprStart: case Parse::NodeKind::InvalidParseStart: + case Parse::NodeKind::KeywordNameQualifierWithParams: case Parse::NodeKind::KeywordNameQualifierWithoutParams: case Parse::NodeKind::LibraryIntroducer: case Parse::NodeKind::LibrarySpecifier: @@ -498,7 +500,6 @@ class NodeStack { case Parse::NodeKind::MatchStatementStart: case Parse::NodeKind::NamedConstraintDefinitionStart: case Parse::NodeKind::NamedConstraintIntroducer: - case Parse::NodeKind::NameQualifierWithParams: case Parse::NodeKind::NamespaceStart: case Parse::NodeKind::PackageIntroducer: case Parse::NodeKind::ParenExprStart: diff --git a/toolchain/parse/handle_decl_name_and_params.cpp b/toolchain/parse/handle_decl_name_and_params.cpp index 4fde242e4f01..6b5f191ad6ab 100644 --- a/toolchain/parse/handle_decl_name_and_params.cpp +++ b/toolchain/parse/handle_decl_name_and_params.cpp @@ -104,8 +104,11 @@ auto HandleDeclNameAndParamsAfterParams(Context& context) -> void { auto state = context.PopState(); if (auto period = context.ConsumeIf(Lex::TokenKind::Period)) { - context.AddNode(NodeKind::NameQualifierWithParams, *period, - state.has_error); + auto start_kind = context.tree().node_kind(NodeId(state.subtree_start)); + auto node_kind = start_kind == NodeKind::IdentifierNameBeforeParams + ? NodeKind::IdentifierNameQualifierWithParams + : NodeKind::KeywordNameQualifierWithParams; + context.AddNode(node_kind, *period, state.has_error); context.PushState(State::DeclNameAndParams); } } diff --git a/toolchain/parse/node_kind.def b/toolchain/parse/node_kind.def index 00e5d7cf9e1d..9032155a21f1 100644 --- a/toolchain/parse/node_kind.def +++ b/toolchain/parse/node_kind.def @@ -126,8 +126,9 @@ CARBON_PARSE_NODE_KIND(LibraryDecl) CARBON_PARSE_NODE_KIND(LibrarySpecifier) -CARBON_PARSE_NODE_KIND(NameQualifierWithParams) +CARBON_PARSE_NODE_KIND(IdentifierNameQualifierWithParams) CARBON_PARSE_NODE_KIND(IdentifierNameQualifierWithoutParams) +CARBON_PARSE_NODE_KIND(KeywordNameQualifierWithParams) CARBON_PARSE_NODE_KIND(KeywordNameQualifierWithoutParams) CARBON_PARSE_NODE_KIND(ExportIntroducer) diff --git a/toolchain/parse/testdata/function/declaration.carbon b/toolchain/parse/testdata/function/declaration.carbon index 3a8306458f98..0b1438ea4a00 100644 --- a/toolchain/parse/testdata/function/declaration.carbon +++ b/toolchain/parse/testdata/function/declaration.carbon @@ -48,9 +48,13 @@ fn destroy {} fn MyClass.destroy() {} -// --- keyword_decl.carbon +// --- keyword_decl_qualified_no_params.carbon -fn destroy.destroy() {} +fn destroy.Foo() {} + +// --- keyword_decl_qualified_with_params.carbon + +fn destroy[self: Self]().Foo() {} // --- impl_fn.carbon @@ -305,19 +309,39 @@ fn (a tokens c d e f g h i j k l m n o p q r s t u v w x y z); // CHECK:STDOUT: {kind: 'FunctionDefinition', text: '}', subtree_size: 8}, // CHECK:STDOUT: {kind: 'FileEnd', text: ''}, // CHECK:STDOUT: ] -// CHECK:STDOUT: - filename: keyword_decl.carbon +// CHECK:STDOUT: - filename: keyword_decl_qualified_no_params.carbon // CHECK:STDOUT: parse_tree: [ // CHECK:STDOUT: {kind: 'FileStart', text: ''}, // CHECK:STDOUT: {kind: 'FunctionIntroducer', text: 'fn'}, // CHECK:STDOUT: {kind: 'KeywordNameNotBeforeParams', text: 'destroy'}, // CHECK:STDOUT: {kind: 'KeywordNameQualifierWithoutParams', text: '.', subtree_size: 2}, -// CHECK:STDOUT: {kind: 'KeywordNameBeforeParams', text: 'destroy'}, +// CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'Foo'}, // CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 2}, // CHECK:STDOUT: {kind: 'FunctionDefinitionStart', text: '{', subtree_size: 7}, // CHECK:STDOUT: {kind: 'FunctionDefinition', text: '}', subtree_size: 8}, // CHECK:STDOUT: {kind: 'FileEnd', text: ''}, // CHECK:STDOUT: ] +// CHECK:STDOUT: - filename: keyword_decl_qualified_with_params.carbon +// CHECK:STDOUT: parse_tree: [ +// CHECK:STDOUT: {kind: 'FileStart', text: ''}, +// CHECK:STDOUT: {kind: 'FunctionIntroducer', text: 'fn'}, +// CHECK:STDOUT: {kind: 'KeywordNameBeforeParams', text: 'destroy'}, +// CHECK:STDOUT: {kind: 'ImplicitParamListStart', text: '['}, +// CHECK:STDOUT: {kind: 'SelfValueName', text: 'self'}, +// CHECK:STDOUT: {kind: 'SelfTypeNameExpr', text: 'Self'}, +// CHECK:STDOUT: {kind: 'LetBindingPattern', text: ':', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ImplicitParamList', text: ']', subtree_size: 5}, +// CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, +// CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 2}, +// CHECK:STDOUT: {kind: 'KeywordNameQualifierWithParams', text: '.', subtree_size: 9}, +// CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'Foo'}, +// CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, +// CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 2}, +// CHECK:STDOUT: {kind: 'FunctionDefinitionStart', text: '{', subtree_size: 14}, +// CHECK:STDOUT: {kind: 'FunctionDefinition', text: '}', subtree_size: 15}, +// CHECK:STDOUT: {kind: 'FileEnd', text: ''}, +// CHECK:STDOUT: ] // CHECK:STDOUT: - filename: impl_fn.carbon // CHECK:STDOUT: parse_tree: [ // CHECK:STDOUT: {kind: 'FileStart', text: ''}, diff --git a/toolchain/parse/testdata/generics/params/name_qualifier.carbon b/toolchain/parse/testdata/generics/params/name_qualifier.carbon index e49af9b0c717..1d31b287ec6b 100644 --- a/toolchain/parse/testdata/generics/params/name_qualifier.carbon +++ b/toolchain/parse/testdata/generics/params/name_qualifier.carbon @@ -54,7 +54,7 @@ fn OuterGeneric(T:! type).InnerGeneric(U:! type).F(x: T, y: U) {} // CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, // CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'NameQualifierWithParams', text: '.', subtree_size: 7}, +// CHECK:STDOUT: {kind: 'IdentifierNameQualifierWithParams', text: '.', subtree_size: 7}, // CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'F'}, // CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 2}, @@ -91,7 +91,7 @@ fn OuterGeneric(T:! type).InnerGeneric(U:! type).F(x: T, y: U) {} // CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'T'}, // CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'NameQualifierWithParams', text: '.', subtree_size: 12}, +// CHECK:STDOUT: {kind: 'IdentifierNameQualifierWithParams', text: '.', subtree_size: 12}, // CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'F'}, // CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 2}, @@ -121,7 +121,7 @@ fn OuterGeneric(T:! type).InnerGeneric(U:! type).F(x: T, y: U) {} // CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, // CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'NameQualifierWithParams', text: '.', subtree_size: 7}, +// CHECK:STDOUT: {kind: 'IdentifierNameQualifierWithParams', text: '.', subtree_size: 7}, // CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'InnerGeneric'}, // CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, // CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'U'}, @@ -149,14 +149,14 @@ fn OuterGeneric(T:! type).InnerGeneric(U:! type).F(x: T, y: U) {} // CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, // CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'NameQualifierWithParams', text: '.', subtree_size: 7}, +// CHECK:STDOUT: {kind: 'IdentifierNameQualifierWithParams', text: '.', subtree_size: 7}, // CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'InnerGeneric'}, // CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, // CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'U'}, // CHECK:STDOUT: {kind: 'TypeTypeLiteral', text: 'type'}, // CHECK:STDOUT: {kind: 'CompileTimeBindingPattern', text: ':!', subtree_size: 3}, // CHECK:STDOUT: {kind: 'ExplicitParamList', text: ')', subtree_size: 5}, -// CHECK:STDOUT: {kind: 'NameQualifierWithParams', text: '.', subtree_size: 7}, +// CHECK:STDOUT: {kind: 'IdentifierNameQualifierWithParams', text: '.', subtree_size: 7}, // CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'F'}, // CHECK:STDOUT: {kind: 'ExplicitParamListStart', text: '('}, // CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'x'}, diff --git a/toolchain/parse/typed_nodes.h b/toolchain/parse/typed_nodes.h index 9ceddcfd327e..70bb941f1294 100644 --- a/toolchain/parse/typed_nodes.h +++ b/toolchain/parse/typed_nodes.h @@ -169,15 +169,25 @@ using BaseName = // A name qualifier with parameters, such as `A(T:! type).` or `A[T:! type](N:! // T).`. -struct NameQualifierWithParams { - static constexpr auto Kind = NodeKind::NameQualifierWithParams.Define( - {.bracketed_by = IdentifierNameBeforeParams::Kind}); +struct IdentifierNameQualifierWithParams { + static constexpr auto Kind = + NodeKind::IdentifierNameQualifierWithParams.Define( + {.bracketed_by = IdentifierNameBeforeParams::Kind}); IdentifierNameBeforeParamsId name; std::optional implicit_params; std::optional params; Lex::PeriodTokenIndex token; }; +struct KeywordNameQualifierWithParams { + static constexpr auto Kind = NodeKind::KeywordNameQualifierWithParams.Define( + {.bracketed_by = KeywordNameBeforeParams::Kind}); + + KeywordNameBeforeParamsId name; + std::optional implicit_params; + std::optional params; + Lex::PeriodTokenIndex token; +}; // A name qualifier without parameters, such as `A.`. struct IdentifierNameQualifierWithoutParams { @@ -200,9 +210,9 @@ struct KeywordNameQualifierWithoutParams { // A complete name in a declaration: `A.C(T:! type).F(n: i32)`. // Note that this includes the parameters of the entity itself. struct DeclName { - llvm::SmallVector< - NodeIdOneOf> + llvm::SmallVector> qualifiers; AnyNonExprNameId name; std::optional implicit_params; diff --git a/toolchain/parse/typed_nodes_test.cpp b/toolchain/parse/typed_nodes_test.cpp index e9f3445d2103..f6a1c0045a52 100644 --- a/toolchain/parse/typed_nodes_test.cpp +++ b/toolchain/parse/typed_nodes_test.cpp @@ -292,8 +292,8 @@ NodeIdForKind error: wrong kind IdentifierNameBeforeParams, expected ImplicitPar Optional [^:]*: missing NodeIdInCategory NonExprName: kind IdentifierNameBeforeParams consumed Vector: begin -NodeIdOneOf NameQualifierWithParams or IdentifierNameQualifierWithoutParams or KeywordNameQualifierWithoutParams: IdentifierNameQualifierWithoutParams consumed -NodeIdOneOf error: wrong kind AbstractModifier, expected NameQualifierWithParams or IdentifierNameQualifierWithoutParams or KeywordNameQualifierWithoutParams +NodeIdOneOf IdentifierNameQualifierWithParams or IdentifierNameQualifierWithoutParams or KeywordNameQualifierWithParams or KeywordNameQualifierWithoutParams: IdentifierNameQualifierWithoutParams consumed +NodeIdOneOf error: wrong kind AbstractModifier, expected IdentifierNameQualifierWithParams or IdentifierNameQualifierWithoutParams or KeywordNameQualifierWithParams or KeywordNameQualifierWithoutParams Vector: end Aggregate [^:]*: success Vector: begin