Merge pull request #2802 from ruby/parameter-forwarding

Enhance parameter forwarding error messages

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:

src/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;
             }
         }

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

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:

test/prism/errors_test.rb

@@ -99,7 +99,7 @@ def test_pre_execution_context
       )
 
       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 @@ def test_missing_terminator_in_parentheses
     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 @@ def test_double_arguments_forwarding
         :foo,
         Location(),
         nil,
-        ParametersNode([], [], nil, [], [], ForwardingParameterNode(), nil),
+        ParametersNode([], [], nil, [ForwardingParameterNode()], [], ForwardingParameterNode(), nil),
         nil,
         [],
         Location(),
@@ -1466,7 +1466,7 @@ def test_loop_conditional_is_closed
     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 @@ def test_binary_range_with_left_unary_range
       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 @@ def test_block_arg_and_block
     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