From 83f481ebf0ab8987bd2bc15767e7a1fac18f05b5 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Fri, 10 May 2024 11:39:51 -0400 Subject: [PATCH] [ruby/prism] Enhance parameter forwarding error messages https://github.com/ruby/prism/commit/826657232e --- prism/config.yml | 3 ++ prism/prism.c | 69 ++++++++++++++-------------- prism/templates/src/diagnostic.c.erb | 4 +- prism/templates/src/token_type.c.erb | 4 +- test/prism/errors_test.rb | 18 ++++---- 5 files changed, 50 insertions(+), 48 deletions(-) diff --git a/prism/config.yml b/prism/config.yml index eb22d57301..c48e5f28dc 100644 --- a/prism/config.yml +++ b/prism/config.yml @@ -193,6 +193,7 @@ errors: - PARAMETER_ASSOC_SPLAT_MULTI - PARAMETER_BLOCK_MULTI - PARAMETER_CIRCULAR + - PARAMETER_FORWARDING_AFTER_REST - PARAMETER_METHOD_NAME - PARAMETER_NAME_DUPLICATED - PARAMETER_NO_DEFAULT @@ -3084,6 +3085,8 @@ nodes: # On parsing error of `f(**kwargs, ...)` or `f(**nil, ...)`, the keyword_rest value is moved here: - KeywordRestParameterNode - NoKeywordsParameterNode + # On parsing error of `f(..., ...)`, the first forwarding parameter is moved here: + - ForwardingParameterNode - name: keywords type: node[] kind: diff --git a/prism/prism.c b/prism/prism.c index b517aa959e..4c8b42f802 100644 --- a/prism/prism.c +++ b/prism/prism.c @@ -14118,31 +14118,37 @@ static pm_parameters_order_t parameters_ordering[PM_TOKEN_MAXIMUM] = { * Check if current parameter follows valid parameters ordering. If not it adds * an error to the list without stopping the parsing, otherwise sets the * parameters state to the one corresponding to the current parameter. + * + * It returns true if it was successful, and false otherwise. */ -static void +static bool update_parameter_state(pm_parser_t *parser, pm_token_t *token, pm_parameters_order_t *current) { pm_parameters_order_t state = parameters_ordering[token->type]; - if (state == PM_PARAMETERS_NO_CHANGE) return; + if (state == PM_PARAMETERS_NO_CHANGE) return true; // If we see another ordered argument after a optional argument // we only continue parsing ordered arguments until we stop seeing ordered arguments. if (*current == PM_PARAMETERS_ORDER_OPTIONAL && state == PM_PARAMETERS_ORDER_NAMED) { *current = PM_PARAMETERS_ORDER_AFTER_OPTIONAL; - return; + return true; } else if (*current == PM_PARAMETERS_ORDER_AFTER_OPTIONAL && state == PM_PARAMETERS_ORDER_NAMED) { - return; + return true; } if (token->type == PM_TOKEN_USTAR && *current == PM_PARAMETERS_ORDER_AFTER_OPTIONAL) { pm_parser_err_token(parser, token, PM_ERR_PARAMETER_STAR); - } - - if (*current == PM_PARAMETERS_ORDER_NOTHING_AFTER || state > *current) { + return false; + } else if (token->type == PM_TOKEN_UDOT_DOT_DOT && (*current >= PM_PARAMETERS_ORDER_KEYWORDS_REST && *current <= PM_PARAMETERS_ORDER_AFTER_OPTIONAL)) { + pm_parser_err_token(parser, token, *current == PM_PARAMETERS_ORDER_AFTER_OPTIONAL ? PM_ERR_PARAMETER_FORWARDING_AFTER_REST : PM_ERR_PARAMETER_ORDER); + return false; + } else if (*current == PM_PARAMETERS_ORDER_NOTHING_AFTER || state > *current) { // We know what transition we failed on, so we can provide a better error here. pm_parser_err_token(parser, token, PM_ERR_PARAMETER_ORDER); - } else if (state < *current) { - *current = state; + return false; } + + if (state < *current) *current = state; + return true; } /** @@ -14211,27 +14217,22 @@ parse_parameters( pm_parser_err_current(parser, PM_ERR_ARGUMENT_NO_FORWARDING_ELLIPSES); } - if (order > PM_PARAMETERS_ORDER_NOTHING_AFTER) { - update_parameter_state(parser, &parser->current, &order); - parser_lex(parser); + bool succeeded = update_parameter_state(parser, &parser->current, &order); + parser_lex(parser); - parser->current_scope->parameters |= PM_SCOPE_PARAMETERS_FORWARDING_ALL; + parser->current_scope->parameters |= PM_SCOPE_PARAMETERS_FORWARDING_ALL; + pm_forwarding_parameter_node_t *param = pm_forwarding_parameter_node_create(parser, &parser->previous); - pm_forwarding_parameter_node_t *param = pm_forwarding_parameter_node_create(parser, &parser->previous); - if (params->keyword_rest != NULL) { - // If we already have a keyword rest parameter, then we replace it with the - // forwarding parameter and move the keyword rest parameter to the posts list. - pm_node_t *keyword_rest = params->keyword_rest; - pm_parameters_node_posts_append(params, keyword_rest); - pm_parser_err_previous(parser, PM_ERR_PARAMETER_UNEXPECTED_FWD); - params->keyword_rest = NULL; - } - pm_parameters_node_keyword_rest_set(params, (pm_node_t *)param); - } else { - update_parameter_state(parser, &parser->current, &order); - parser_lex(parser); + if (params->keyword_rest != NULL) { + // If we already have a keyword rest parameter, then we replace it with the + // forwarding parameter and move the keyword rest parameter to the posts list. + pm_node_t *keyword_rest = params->keyword_rest; + pm_parameters_node_posts_append(params, keyword_rest); + if (succeeded) pm_parser_err_previous(parser, PM_ERR_PARAMETER_UNEXPECTED_FWD); + params->keyword_rest = NULL; } + pm_parameters_node_keyword_rest_set(params, (pm_node_t *) param); break; } case PM_TOKEN_CLASS_VARIABLE: @@ -14896,7 +14897,7 @@ parse_arguments_list(pm_parser_t *parser, pm_arguments_t *arguments, bool accept arguments->closing_loc = PM_LOCATION_TOKEN_VALUE(&parser->previous); } else { pm_accepts_block_stack_push(parser, true); - parse_arguments(parser, arguments, true, PM_TOKEN_PARENTHESIS_RIGHT); + parse_arguments(parser, arguments, accepts_block, PM_TOKEN_PARENTHESIS_RIGHT); if (!accept1(parser, PM_TOKEN_PARENTHESIS_RIGHT)) { PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->current, PM_ERR_ARGUMENT_TERM_PAREN, pm_token_type_human(parser->current.type)); @@ -14914,7 +14915,7 @@ parse_arguments_list(pm_parser_t *parser, pm_arguments_t *arguments, bool accept // If we get here, then the subsequent token cannot be used as an infix // operator. In this case we assume the subsequent token is part of an // argument to this method call. - parse_arguments(parser, arguments, true, PM_TOKEN_EOF); + parse_arguments(parser, arguments, accepts_block, PM_TOKEN_EOF); // If we have done with the arguments and still not consumed the comma, // then we have a trailing comma where we need to check whether it is @@ -14945,11 +14946,8 @@ parse_arguments_list(pm_parser_t *parser, pm_arguments_t *arguments, bool accept if (arguments->block == NULL && !arguments->has_forwarding) { arguments->block = (pm_node_t *) block; } else { - if (arguments->has_forwarding) { - pm_parser_err_node(parser, (pm_node_t *) block, PM_ERR_ARGUMENT_BLOCK_FORWARDING); - } else { - pm_parser_err_node(parser, (pm_node_t *) block, PM_ERR_ARGUMENT_BLOCK_MULTI); - } + pm_parser_err_node(parser, (pm_node_t *) block, PM_ERR_ARGUMENT_BLOCK_MULTI); + if (arguments->block != NULL) { if (arguments->arguments == NULL) { arguments->arguments = pm_arguments_node_create(parser); @@ -17048,7 +17046,8 @@ pm_parser_err_prefix(pm_parser_t *parser, pm_diagnostic_id_t diag_id) { PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->previous, diag_id, pm_token_type_human(parser->previous.type)); break; } - case PM_ERR_HASH_VALUE: { + case PM_ERR_HASH_VALUE: + case PM_ERR_EXPECT_EXPRESSION_AFTER_OPERATOR: { PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->current, diag_id, pm_token_type_human(parser->current.type)); break; } @@ -20329,7 +20328,7 @@ parse_expression_infix(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t // In this case we have an operator but we don't know what it's for. // We need to treat it as an error. For now, we'll mark it as an error // and just skip right past it. - pm_parser_err_previous(parser, PM_ERR_EXPECT_EXPRESSION_AFTER_OPERATOR); + PM_PARSER_ERR_TOKEN_FORMAT(parser, parser->previous, PM_ERR_EXPECT_EXPRESSION_AFTER_OPERATOR, pm_token_type_human(parser->current.type)); return node; } } diff --git a/prism/templates/src/diagnostic.c.erb b/prism/templates/src/diagnostic.c.erb index 75a00958fc..451f9c3f7c 100644 --- a/prism/templates/src/diagnostic.c.erb +++ b/prism/templates/src/diagnostic.c.erb @@ -91,7 +91,6 @@ static const pm_diagnostic_data_t diagnostic_messages[PM_DIAGNOSTIC_ID_MAX] = { [PM_ERR_ARGUMENT_AFTER_BLOCK] = { "unexpected argument after a block argument", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_ARGUMENT_AFTER_FORWARDING_ELLIPSES] = { "unexpected argument after `...`", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_ARGUMENT_BARE_HASH] = { "unexpected bare hash argument", PM_ERROR_LEVEL_SYNTAX }, - [PM_ERR_ARGUMENT_BLOCK_FORWARDING] = { "both a block argument and a forwarding argument; only one block is allowed", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_ARGUMENT_BLOCK_MULTI] = { "both block arg and actual block given; only one block is allowed", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_ARGUMENT_CONFLICT_AMPERSAND] = { "unexpected `&`; anonymous block parameter is also used within block", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_ARGUMENT_CONFLICT_STAR] = { "unexpected `*`; anonymous rest parameter is also used within block", PM_ERROR_LEVEL_SYNTAX }, @@ -177,7 +176,7 @@ static const pm_diagnostic_data_t diagnostic_messages[PM_DIAGNOSTIC_ID_MAX] = { [PM_ERR_EXPECT_EXPRESSION_AFTER_EQUAL] = { "expected an expression after `=`", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_EXPECT_EXPRESSION_AFTER_LESS_LESS] = { "expected an expression after `<<`", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_EXPECT_EXPRESSION_AFTER_LPAREN] = { "expected an expression after `(`", PM_ERROR_LEVEL_SYNTAX }, - [PM_ERR_EXPECT_EXPRESSION_AFTER_OPERATOR] = { "expected an expression after the operator", PM_ERROR_LEVEL_SYNTAX }, + [PM_ERR_EXPECT_EXPRESSION_AFTER_OPERATOR] = { "unexpected %s; expected an expression after the operator", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_EXPECT_EXPRESSION_AFTER_SPLAT] = { "expected an expression after `*` splat in an argument", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_EXPECT_EXPRESSION_AFTER_SPLAT_HASH] = { "expected an expression after `**` in a hash", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_EXPECT_EXPRESSION_AFTER_STAR] = { "expected an expression after `*`", PM_ERROR_LEVEL_SYNTAX }, @@ -276,6 +275,7 @@ static const pm_diagnostic_data_t diagnostic_messages[PM_DIAGNOSTIC_ID_MAX] = { [PM_ERR_PARAMETER_ASSOC_SPLAT_MULTI] = { "unexpected multiple `**` splat parameters", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_PARAMETER_BLOCK_MULTI] = { "multiple block parameters; only one block is allowed", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_PARAMETER_CIRCULAR] = { "circular argument reference - %.*s", PM_ERROR_LEVEL_SYNTAX }, + [PM_ERR_PARAMETER_FORWARDING_AFTER_REST] = { "... after rest argument", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_PARAMETER_METHOD_NAME] = { "unexpected name for a parameter", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_PARAMETER_NAME_DUPLICATED] = { "duplicated argument name", PM_ERROR_LEVEL_SYNTAX }, [PM_ERR_PARAMETER_NO_DEFAULT] = { "expected a default value for the parameter", PM_ERROR_LEVEL_SYNTAX }, diff --git a/prism/templates/src/token_type.c.erb b/prism/templates/src/token_type.c.erb index 0587fc6cdf..a3095eeab8 100644 --- a/prism/templates/src/token_type.c.erb +++ b/prism/templates/src/token_type.c.erb @@ -90,9 +90,9 @@ pm_token_type_human(pm_token_type_t token_type) { case PM_TOKEN_DOT: return "'.'"; case PM_TOKEN_DOT_DOT: - return "'..'"; + return ".."; case PM_TOKEN_DOT_DOT_DOT: - return "'...'"; + return "..."; case PM_TOKEN_EMBDOC_BEGIN: return "'=begin'"; case PM_TOKEN_EMBDOC_END: diff --git a/test/prism/errors_test.rb b/test/prism/errors_test.rb index db4d2af5e1..5d18780ddd 100644 --- a/test/prism/errors_test.rb +++ b/test/prism/errors_test.rb @@ -99,7 +99,7 @@ module Prism ) assert_errors expected, "BEGIN { 1 + }", [ - ["expected an expression after the operator", 10..11], + ["unexpected '}'; expected an expression after the operator", 12..13], ["unexpected '}', assuming it is closing the parent 'BEGIN' block", 12..13] ] end @@ -210,7 +210,7 @@ module Prism def test_unterminated_argument_expression assert_errors expression('a %'), 'a %', [ ["invalid `%` token", 2..3], - ["expected an expression after the operator", 2..3], + ["unexpected end of file; expected an expression after the operator", 3..3], ["unexpected end of file, assuming it is closing the parent top level context", 3..3] ] end @@ -864,7 +864,7 @@ module Prism :foo, Location(), nil, - ParametersNode([], [], nil, [], [], ForwardingParameterNode(), nil), + ParametersNode([], [], nil, [ForwardingParameterNode()], [], ForwardingParameterNode(), nil), nil, [], Location(), @@ -1466,7 +1466,7 @@ module Prism def test_forwarding_arg_after_keyword_rest source = "def f(**,...);end" assert_errors expression(source), source, [ - ["unexpected `...` in parameters", 9..12], + ["unexpected parameter order", 9..12] ] end @@ -1942,10 +1942,10 @@ module Prism RUBY assert_errors expression(source), source, [ - ["unexpected '..', expecting end-of-input", 3..5], - ["unexpected '..', ignoring it", 3..5], - ["unexpected '..', expecting end-of-input", 10..12], - ["unexpected '..', ignoring it", 10..12] + ["unexpected .., expecting end-of-input", 3..5], + ["unexpected .., ignoring it", 3..5], + ["unexpected .., expecting end-of-input", 10..12], + ["unexpected .., ignoring it", 10..12] ] end @@ -2082,7 +2082,7 @@ module Prism def test_forwarding_arg_and_block source = 'def foo(...) = foo(...) { }' assert_errors expression(source), source, [ - ['both a block argument and a forwarding argument; only one block is allowed', 24..27] + ['both block arg and actual block given; only one block is allowed', 24..27] ] end