archive/ruby
1
0
Fork 0
mirror of https://github.com/ruby/ruby.git synced 2025-08-24 05:25:34 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

[ruby/prism] Enhance parameter forwarding error messages

Browse source 
826657232e
This commit is contained in:
Kevin Newton 2024-05-10 11:39:51 -04:00 • committed by git
parent 955f13238d
commit 83f481ebf0
5 changed files with 50 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

3
prism/config.yml
View file

@ -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:

69
prism/prism.c
View file

@ -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;
}
}

4
prism/templates/src/diagnostic.c.erb
View file

@ -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 },

4
prism/templates/src/token_type.c.erb
View file

@ -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:

18
test/prism/errors_test.rb
View file

@ -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