From 526a39fd67a04d356c1cf47590493639ecd01e4f Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Thu, 12 Feb 2026 13:10:40 +0100 Subject: [PATCH] [Feature #19107] Allow trailing comma in method signature --- .../trailing_comma_after_method_arguments.txt | 207 ++++++++++++++++++ src/prism.c | 56 +++-- ...w_trailing_commas_in_method_parameters.txt | 0 ...ing_commas_after_terminating_arguments.txt | 6 + .../trailing_comma_after_method_arguments.txt | 15 ++ test/prism/fixtures_test.rb | 2 + test/prism/locals_test.rb | 5 +- test/prism/ruby/ripper_test.rb | 2 + 8 files changed, 277 insertions(+), 16 deletions(-) create mode 100644 snapshots/4.1/trailing_comma_after_method_arguments.txt rename test/prism/errors/{ => 3.3-4.0}/do_not_allow_trailing_commas_in_method_parameters.txt (100%) create mode 100644 test/prism/errors/4.1/do_not_allow_trailing_commas_after_terminating_arguments.txt create mode 100644 test/prism/fixtures/4.1/trailing_comma_after_method_arguments.txt diff --git a/snapshots/4.1/trailing_comma_after_method_arguments.txt b/snapshots/4.1/trailing_comma_after_method_arguments.txt new file mode 100644 index 0000000000..b71459ff54 --- /dev/null +++ b/snapshots/4.1/trailing_comma_after_method_arguments.txt @@ -0,0 +1,207 @@ +@ ProgramNode (location: (1,0)-(15,5)) +├── flags: ∅ +├── locals: [] +└── statements: + @ StatementsNode (location: (1,0)-(15,5)) + ├── flags: ∅ + └── body: (length: 6) + ├── @ DefNode (location: (1,0)-(1,19)) + │ ├── flags: newline + │ ├── name: :foo + │ ├── name_loc: (1,4)-(1,7) = "foo" + │ ├── receiver: ∅ + │ ├── parameters: + │ │ @ ParametersNode (location: (1,8)-(1,13)) + │ │ ├── flags: ∅ + │ │ ├── requireds: (length: 3) + │ │ │ ├── @ RequiredParameterNode (location: (1,8)-(1,9)) + │ │ │ │ ├── flags: ∅ + │ │ │ │ └── name: :a + │ │ │ ├── @ RequiredParameterNode (location: (1,10)-(1,11)) + │ │ │ │ ├── flags: ∅ + │ │ │ │ └── name: :b + │ │ │ └── @ RequiredParameterNode (location: (1,12)-(1,13)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :c + │ │ ├── optionals: (length: 0) + │ │ ├── rest: ∅ + │ │ ├── posts: (length: 0) + │ │ ├── keywords: (length: 0) + │ │ ├── keyword_rest: ∅ + │ │ └── block: ∅ + │ ├── body: ∅ + │ ├── locals: [:a, :b, :c] + │ ├── def_keyword_loc: (1,0)-(1,3) = "def" + │ ├── operator_loc: ∅ + │ ├── lparen_loc: (1,7)-(1,8) = "(" + │ ├── rparen_loc: (1,14)-(1,15) = ")" + │ ├── equal_loc: ∅ + │ └── end_keyword_loc: (1,16)-(1,19) = "end" + ├── @ DefNode (location: (3,0)-(3,20)) + │ ├── flags: newline + │ ├── name: :foo + │ ├── name_loc: (3,4)-(3,7) = "foo" + │ ├── receiver: ∅ + │ ├── parameters: + │ │ @ ParametersNode (location: (3,8)-(3,14)) + │ │ ├── flags: ∅ + │ │ ├── requireds: (length: 2) + │ │ │ ├── @ RequiredParameterNode (location: (3,8)-(3,9)) + │ │ │ │ ├── flags: ∅ + │ │ │ │ └── name: :a + │ │ │ └── @ RequiredParameterNode (location: (3,10)-(3,11)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :b + │ │ ├── optionals: (length: 0) + │ │ ├── rest: + │ │ │ @ RestParameterNode (location: (3,12)-(3,14)) + │ │ │ ├── flags: ∅ + │ │ │ ├── name: :c + │ │ │ ├── name_loc: (3,13)-(3,14) = "c" + │ │ │ └── operator_loc: (3,12)-(3,13) = "*" + │ │ ├── posts: (length: 0) + │ │ ├── keywords: (length: 0) + │ │ ├── keyword_rest: ∅ + │ │ └── block: ∅ + │ ├── body: ∅ + │ ├── locals: [:a, :b, :c] + │ ├── def_keyword_loc: (3,0)-(3,3) = "def" + │ ├── operator_loc: ∅ + │ ├── lparen_loc: (3,7)-(3,8) = "(" + │ ├── rparen_loc: (3,15)-(3,16) = ")" + │ ├── equal_loc: ∅ + │ └── end_keyword_loc: (3,17)-(3,20) = "end" + ├── @ DefNode (location: (5,0)-(5,19)) + │ ├── flags: newline + │ ├── name: :foo + │ ├── name_loc: (5,4)-(5,7) = "foo" + │ ├── receiver: ∅ + │ ├── parameters: + │ │ @ ParametersNode (location: (5,8)-(5,13)) + │ │ ├── flags: ∅ + │ │ ├── requireds: (length: 2) + │ │ │ ├── @ RequiredParameterNode (location: (5,8)-(5,9)) + │ │ │ │ ├── flags: ∅ + │ │ │ │ └── name: :a + │ │ │ └── @ RequiredParameterNode (location: (5,10)-(5,11)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :b + │ │ ├── optionals: (length: 0) + │ │ ├── rest: + │ │ │ @ RestParameterNode (location: (5,12)-(5,13)) + │ │ │ ├── flags: ∅ + │ │ │ ├── name: ∅ + │ │ │ ├── name_loc: ∅ + │ │ │ └── operator_loc: (5,12)-(5,13) = "*" + │ │ ├── posts: (length: 0) + │ │ ├── keywords: (length: 0) + │ │ ├── keyword_rest: ∅ + │ │ └── block: ∅ + │ ├── body: ∅ + │ ├── locals: [:a, :b] + │ ├── def_keyword_loc: (5,0)-(5,3) = "def" + │ ├── operator_loc: ∅ + │ ├── lparen_loc: (5,7)-(5,8) = "(" + │ ├── rparen_loc: (5,14)-(5,15) = ")" + │ ├── equal_loc: ∅ + │ └── end_keyword_loc: (5,16)-(5,19) = "end" + ├── @ DefNode (location: (7,0)-(7,21)) + │ ├── flags: newline + │ ├── name: :foo + │ ├── name_loc: (7,4)-(7,7) = "foo" + │ ├── receiver: ∅ + │ ├── parameters: + │ │ @ ParametersNode (location: (7,8)-(7,15)) + │ │ ├── flags: ∅ + │ │ ├── requireds: (length: 2) + │ │ │ ├── @ RequiredParameterNode (location: (7,8)-(7,9)) + │ │ │ │ ├── flags: ∅ + │ │ │ │ └── name: :a + │ │ │ └── @ RequiredParameterNode (location: (7,10)-(7,11)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :b + │ │ ├── optionals: (length: 0) + │ │ ├── rest: ∅ + │ │ ├── posts: (length: 0) + │ │ ├── keywords: (length: 0) + │ │ ├── keyword_rest: + │ │ │ @ KeywordRestParameterNode (location: (7,12)-(7,15)) + │ │ │ ├── flags: ∅ + │ │ │ ├── name: :c + │ │ │ ├── name_loc: (7,14)-(7,15) = "c" + │ │ │ └── operator_loc: (7,12)-(7,14) = "**" + │ │ └── block: ∅ + │ ├── body: ∅ + │ ├── locals: [:a, :b, :c] + │ ├── def_keyword_loc: (7,0)-(7,3) = "def" + │ ├── operator_loc: ∅ + │ ├── lparen_loc: (7,7)-(7,8) = "(" + │ ├── rparen_loc: (7,16)-(7,17) = ")" + │ ├── equal_loc: ∅ + │ └── end_keyword_loc: (7,18)-(7,21) = "end" + ├── @ DefNode (location: (9,0)-(9,20)) + │ ├── flags: newline + │ ├── name: :foo + │ ├── name_loc: (9,4)-(9,7) = "foo" + │ ├── receiver: ∅ + │ ├── parameters: + │ │ @ ParametersNode (location: (9,8)-(9,14)) + │ │ ├── flags: ∅ + │ │ ├── requireds: (length: 2) + │ │ │ ├── @ RequiredParameterNode (location: (9,8)-(9,9)) + │ │ │ │ ├── flags: ∅ + │ │ │ │ └── name: :a + │ │ │ └── @ RequiredParameterNode (location: (9,10)-(9,11)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :b + │ │ ├── optionals: (length: 0) + │ │ ├── rest: ∅ + │ │ ├── posts: (length: 0) + │ │ ├── keywords: (length: 0) + │ │ ├── keyword_rest: + │ │ │ @ KeywordRestParameterNode (location: (9,12)-(9,14)) + │ │ │ ├── flags: ∅ + │ │ │ ├── name: ∅ + │ │ │ ├── name_loc: ∅ + │ │ │ └── operator_loc: (9,12)-(9,14) = "**" + │ │ └── block: ∅ + │ ├── body: ∅ + │ ├── locals: [:a, :b] + │ ├── def_keyword_loc: (9,0)-(9,3) = "def" + │ ├── operator_loc: ∅ + │ ├── lparen_loc: (9,7)-(9,8) = "(" + │ ├── rparen_loc: (9,15)-(9,16) = ")" + │ ├── equal_loc: ∅ + │ └── end_keyword_loc: (9,17)-(9,20) = "end" + └── @ DefNode (location: (11,0)-(15,5)) + ├── flags: newline + ├── name: :foo + ├── name_loc: (11,4)-(11,7) = "foo" + ├── receiver: ∅ + ├── parameters: + │ @ ParametersNode (location: (12,2)-(14,3)) + │ ├── flags: ∅ + │ ├── requireds: (length: 3) + │ │ ├── @ RequiredParameterNode (location: (12,2)-(12,3)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :a + │ │ ├── @ RequiredParameterNode (location: (13,2)-(13,3)) + │ │ │ ├── flags: ∅ + │ │ │ └── name: :b + │ │ └── @ RequiredParameterNode (location: (14,2)-(14,3)) + │ │ ├── flags: ∅ + │ │ └── name: :c + │ ├── optionals: (length: 0) + │ ├── rest: ∅ + │ ├── posts: (length: 0) + │ ├── keywords: (length: 0) + │ ├── keyword_rest: ∅ + │ └── block: ∅ + ├── body: ∅ + ├── locals: [:a, :b, :c] + ├── def_keyword_loc: (11,0)-(11,3) = "def" + ├── operator_loc: ∅ + ├── lparen_loc: (11,7)-(11,8) = "(" + ├── rparen_loc: (15,0)-(15,1) = ")" + ├── equal_loc: ∅ + └── end_keyword_loc: (15,2)-(15,5) = "end" diff --git a/src/prism.c b/src/prism.c index 34e5d38b0a..c63e42b8d1 100644 --- a/src/prism.c +++ b/src/prism.c @@ -13875,6 +13875,43 @@ update_parameter_state(pm_parser_t *parser, pm_token_t *token, pm_parameters_ord return true; } +static inline void +parse_parameters_handle_trailing_comma( + pm_parser_t *parser, + pm_parameters_node_t *params, + pm_parameters_order_t order, + bool in_block, + bool allows_trailing_comma +) { + if (!allows_trailing_comma) { + pm_parser_err_previous(parser, PM_ERR_PARAMETER_WILD_LOOSE_COMMA); + return; + } + + if (in_block) { + if (order >= PM_PARAMETERS_ORDER_NAMED) { + // foo do |bar,|; end + pm_node_t *param = UP(pm_implicit_rest_node_create(parser, &parser->previous)); + + if (params->rest == NULL) { + pm_parameters_node_rest_set(params, param); + } else { + pm_parser_err_node(parser, UP(param), PM_ERR_PARAMETER_SPLAT_MULTI); + pm_parameters_node_posts_append(params, UP(param)); + } + } else { + // foo do |*bar,|; end + pm_parser_err_previous(parser, PM_ERR_PARAMETER_WILD_LOOSE_COMMA); + } + } else { + // https://bugs.ruby-lang.org/issues/19107 + // Allow `def foo(bar,); end`, `def foo(*bar,); end`, etc. but not `def foo(...,); end` + if (parser->version < PM_OPTIONS_VERSION_CRUBY_4_1 || order == PM_PARAMETERS_ORDER_NOTHING_AFTER) { + pm_parser_err_previous(parser, PM_ERR_PARAMETER_WILD_LOOSE_COMMA); + } + } +} + /** * Parse a list of parameters on a method definition. */ @@ -14216,20 +14253,7 @@ parse_parameters( } default: if (parser->previous.type == PM_TOKEN_COMMA) { - if (allows_trailing_comma && order >= PM_PARAMETERS_ORDER_NAMED) { - // If we get here, then we have a trailing comma in a - // block parameter list. - pm_node_t *param = UP(pm_implicit_rest_node_create(parser, &parser->previous)); - - if (params->rest == NULL) { - pm_parameters_node_rest_set(params, param); - } else { - pm_parser_err_node(parser, UP(param), PM_ERR_PARAMETER_SPLAT_MULTI); - pm_parameters_node_posts_append(params, UP(param)); - } - } else { - pm_parser_err_previous(parser, PM_ERR_PARAMETER_WILD_LOOSE_COMMA); - } + parse_parameters_handle_trailing_comma(parser, params, order, in_block, allows_trailing_comma); } parsing = false; @@ -18826,7 +18850,9 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, b if (match1(parser, PM_TOKEN_PARENTHESIS_RIGHT)) { params = NULL; } else { - params = parse_parameters(parser, PM_BINDING_POWER_DEFINED, true, false, true, true, false, (uint16_t) (depth + 1)); + // https://bugs.ruby-lang.org/issues/19107 + bool allow_trailing_comma = parser->version >= PM_OPTIONS_VERSION_CRUBY_4_1; + params = parse_parameters(parser, PM_BINDING_POWER_DEFINED, true, allow_trailing_comma, true, true, false, (uint16_t) (depth + 1)); } lex_state_set(parser, PM_LEX_STATE_BEG); diff --git a/test/prism/errors/do_not_allow_trailing_commas_in_method_parameters.txt b/test/prism/errors/3.3-4.0/do_not_allow_trailing_commas_in_method_parameters.txt similarity index 100% rename from test/prism/errors/do_not_allow_trailing_commas_in_method_parameters.txt rename to test/prism/errors/3.3-4.0/do_not_allow_trailing_commas_in_method_parameters.txt diff --git a/test/prism/errors/4.1/do_not_allow_trailing_commas_after_terminating_arguments.txt b/test/prism/errors/4.1/do_not_allow_trailing_commas_after_terminating_arguments.txt new file mode 100644 index 0000000000..b3e06f4154 --- /dev/null +++ b/test/prism/errors/4.1/do_not_allow_trailing_commas_after_terminating_arguments.txt @@ -0,0 +1,6 @@ +def foo(a,b,...,);end + ^ unexpected `,` in parameters + +def foo(a,b,&block,);end + ^ unexpected `,` in parameters + diff --git a/test/prism/fixtures/4.1/trailing_comma_after_method_arguments.txt b/test/prism/fixtures/4.1/trailing_comma_after_method_arguments.txt new file mode 100644 index 0000000000..ef1385d973 --- /dev/null +++ b/test/prism/fixtures/4.1/trailing_comma_after_method_arguments.txt @@ -0,0 +1,15 @@ +def foo(a,b,c,);end + +def foo(a,b,*c,);end + +def foo(a,b,*,);end + +def foo(a,b,**c,);end + +def foo(a,b,**,);end + +def foo( + a, + b, + c, +);end diff --git a/test/prism/fixtures_test.rb b/test/prism/fixtures_test.rb index 3a704b8389..dcbcb7c117 100644 --- a/test/prism/fixtures_test.rb +++ b/test/prism/fixtures_test.rb @@ -28,6 +28,8 @@ class FixturesTest < TestCase except << "command_method_call_2.txt" # https://bugs.ruby-lang.org/issues/21669 except << "4.1/void_value.txt" + # https://bugs.ruby-lang.org/issues/19107 + except << "4.1/trailing_comma_after_method_arguments.txt" Fixture.each_for_current_ruby(except: except) do |fixture| define_method(fixture.test_name) { assert_valid_syntax(fixture.read) } diff --git a/test/prism/locals_test.rb b/test/prism/locals_test.rb index 4844901804..42f6648bc2 100644 --- a/test/prism/locals_test.rb +++ b/test/prism/locals_test.rb @@ -30,7 +30,10 @@ class LocalsTest < TestCase "command_method_call_2.txt", # https://bugs.ruby-lang.org/issues/21669 - "4.1/void_value.txt" + "4.1/void_value.txt", + + # https://bugs.ruby-lang.org/issues/19107 + "4.1/trailing_comma_after_method_arguments.txt", ] Fixture.each_for_current_ruby(except: except) do |fixture| diff --git a/test/prism/ruby/ripper_test.rb b/test/prism/ruby/ripper_test.rb index 15f535f3d6..3544825943 100644 --- a/test/prism/ruby/ripper_test.rb +++ b/test/prism/ruby/ripper_test.rb @@ -39,6 +39,8 @@ class RipperTest < TestCase # https://bugs.ruby-lang.org/issues/21669 incorrect << "4.1/void_value.txt" + # https://bugs.ruby-lang.org/issues/19107 + incorrect << "4.1/trailing_comma_after_method_arguments.txt" # Skip these tests that we haven't implemented yet. omitted_sexp_raw = [