From ebef4dbae6cbbe636499c93d7ff0bf31dab88d18 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 29 Jan 2026 20:48:08 +0100 Subject: [PATCH 1/3] [ruby/prism] RB_GC_GUARD() the VALUE for RSTRING_PTR() to ensure the char* does not move * See https://github.com/ruby/prism/pull/3886#discussion_r2741579984. https://github.com/ruby/prism/commit/98603ce8cc --- prism/extension.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/prism/extension.c b/prism/extension.c index cde10bf360df2a..ecfc960d619145 100644 --- a/prism/extension.c +++ b/prism/extension.c @@ -201,12 +201,16 @@ build_options_i(VALUE key, VALUE value, VALUE argument) { const char *version = check_string(value); if (RSTRING_LEN(value) == 7 && strncmp(version, "current", 7) == 0) { - const char *ruby_version = RSTRING_PTR(rb_const_get(rb_cObject, rb_intern("RUBY_VERSION"))); + VALUE ruby_version_string = rb_const_get(rb_cObject, rb_intern("RUBY_VERSION")); + const char *ruby_version = RSTRING_PTR(ruby_version_string); if (!pm_options_version_set(options, ruby_version, 3)) { - rb_exc_raise(rb_exc_new_cstr(rb_cPrismCurrentVersionError, ruby_version)); + rb_exc_raise(rb_exc_new_str(rb_cPrismCurrentVersionError, ruby_version_string)); } + + RB_GC_GUARD(ruby_version_string); // Do not move ruby_version while running the code above } else if (RSTRING_LEN(value) == 7 && strncmp(version, "nearest", 7) == 0) { - const char *ruby_version = RSTRING_PTR(rb_const_get(rb_cObject, rb_intern("RUBY_VERSION"))); + VALUE ruby_version_string = rb_const_get(rb_cObject, rb_intern("RUBY_VERSION")); + const char *ruby_version = RSTRING_PTR(ruby_version_string); const char *nearest_version; if (ruby_version[0] < '3' || (ruby_version[0] == '3' && ruby_version[2] < '3')) { @@ -220,6 +224,8 @@ build_options_i(VALUE key, VALUE value, VALUE argument) { if (!pm_options_version_set(options, nearest_version, 3)) { rb_raise(rb_eArgError, "invalid nearest version: %s", nearest_version); } + + RB_GC_GUARD(ruby_version_string); // Do not move ruby_version while running the code above } else if (!pm_options_version_set(options, version, RSTRING_LEN(value))) { rb_raise(rb_eArgError, "invalid version: %" PRIsVALUE, value); } From 6a4e53f94ddc946b6116b250a2b20b78bef3f0ef Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Tue, 3 Feb 2026 14:44:30 +0100 Subject: [PATCH 2/3] [ruby/prism] Directly use `ruby_version` from `ruby/version.h` When I initially wrote this, I used `RUBY_VERSION` so it's easier to stub in tests But turns out that doesn't work in CRuby tests https://github.com/ruby/prism/commit/e30e2591de --- prism/extension.c | 10 +--------- prism/extension.h | 1 + 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/prism/extension.c b/prism/extension.c index ecfc960d619145..363144970c8c97 100644 --- a/prism/extension.c +++ b/prism/extension.c @@ -201,16 +201,10 @@ build_options_i(VALUE key, VALUE value, VALUE argument) { const char *version = check_string(value); if (RSTRING_LEN(value) == 7 && strncmp(version, "current", 7) == 0) { - VALUE ruby_version_string = rb_const_get(rb_cObject, rb_intern("RUBY_VERSION")); - const char *ruby_version = RSTRING_PTR(ruby_version_string); if (!pm_options_version_set(options, ruby_version, 3)) { - rb_exc_raise(rb_exc_new_str(rb_cPrismCurrentVersionError, ruby_version_string)); + rb_exc_raise(rb_exc_new_cstr(rb_cPrismCurrentVersionError, ruby_version)); } - - RB_GC_GUARD(ruby_version_string); // Do not move ruby_version while running the code above } else if (RSTRING_LEN(value) == 7 && strncmp(version, "nearest", 7) == 0) { - VALUE ruby_version_string = rb_const_get(rb_cObject, rb_intern("RUBY_VERSION")); - const char *ruby_version = RSTRING_PTR(ruby_version_string); const char *nearest_version; if (ruby_version[0] < '3' || (ruby_version[0] == '3' && ruby_version[2] < '3')) { @@ -224,8 +218,6 @@ build_options_i(VALUE key, VALUE value, VALUE argument) { if (!pm_options_version_set(options, nearest_version, 3)) { rb_raise(rb_eArgError, "invalid nearest version: %s", nearest_version); } - - RB_GC_GUARD(ruby_version_string); // Do not move ruby_version while running the code above } else if (!pm_options_version_set(options, version, RSTRING_LEN(value))) { rb_raise(rb_eArgError, "invalid version: %" PRIsVALUE, value); } diff --git a/prism/extension.h b/prism/extension.h index 4ddc3a7b8617d0..a2aaa6388fdd66 100644 --- a/prism/extension.h +++ b/prism/extension.h @@ -5,6 +5,7 @@ #include #include +#include #include "prism.h" VALUE pm_source_new(const pm_parser_t *parser, rb_encoding *encoding, bool freeze); From 3237be163c313af0b6626b209e55bdb0b33c9f0f Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Fri, 30 Jan 2026 09:30:21 +0100 Subject: [PATCH 3/3] [Bug #21669] Thoroughly implement void value expression check (prism) --- bootstraptest/test_flow.rb | 4 +- bootstraptest/test_syntax.rb | 8 +- prism/prism.c | 99 ++++++++++++++++--- .../singleton_method_with_void_value.txt | 3 + test/prism/errors/3.4-4.0/void_value.txt | 18 ++++ .../4.1/singleton_method_with_void_value.txt | 4 + test/prism/errors/4.1/void_value.txt | 44 +++++++++ .../errors/singleton_method_for_literals.txt | 2 - ...id_value_expression_in_begin_statement.txt | 2 - test/prism/fixtures/3.3-4.0/void_value.txt | 29 ++++++ test/prism/fixtures/4.1/void_value.txt | 7 ++ test/prism/fixtures/non_void_value.txt | 31 ++++++ test/prism/fixtures_test.rb | 3 + test/prism/locals_test.rb | 3 + test/prism/result/warnings_test.rb | 19 ++++ test/prism/ruby/ripper_test.rb | 3 + test/prism/ruby/ruby_parser_test.rb | 2 + test/ruby/test_syntax.rb | 6 +- 18 files changed, 261 insertions(+), 26 deletions(-) create mode 100644 test/prism/errors/3.3-4.0/singleton_method_with_void_value.txt create mode 100644 test/prism/errors/3.4-4.0/void_value.txt create mode 100644 test/prism/errors/4.1/singleton_method_with_void_value.txt create mode 100644 test/prism/errors/4.1/void_value.txt create mode 100644 test/prism/fixtures/3.3-4.0/void_value.txt create mode 100644 test/prism/fixtures/4.1/void_value.txt create mode 100644 test/prism/fixtures/non_void_value.txt diff --git a/bootstraptest/test_flow.rb b/bootstraptest/test_flow.rb index 15528a42130fc1..7a95def1e6fb15 100644 --- a/bootstraptest/test_flow.rb +++ b/bootstraptest/test_flow.rb @@ -376,7 +376,7 @@ def m1 *args; $a << 2 ; $a << 3 end; $a << 4 def m2; $a << 5 - m1(:a, :b, (return 1; :c)); $a << 6 + m1(:a, :b, (return 1 if true; :c)); $a << 6 end; $a << 7 m2; $a << 8 ; $a << 9 @@ -399,7 +399,7 @@ def m(); $a << 4 m2(begin; $a << 5 2; $a << 6 ensure; $a << 7 - return 3; $a << 8 + return 3 if true; $a << 8 end); $a << 9 4; $a << 10 end; $a << 11 diff --git a/bootstraptest/test_syntax.rb b/bootstraptest/test_syntax.rb index fbc9c6f62e377a..29bf93cb8f6957 100644 --- a/bootstraptest/test_syntax.rb +++ b/bootstraptest/test_syntax.rb @@ -571,7 +571,7 @@ class << (ary=[]); def []; 0; end; def []=(x); super(0,x);end;end; ary[]+=1 assert_equal 'ok', %q{ 1.times{ - p(1, (next; 2)) + p(1, (next if true; 2)) }; :ok } assert_equal '3', %q{ @@ -585,7 +585,7 @@ class << (ary=[]); def []; 0; end; def []=(x); super(0,x);end;end; ary[]+=1 i = 0 1 + (while true break 2 if (i+=1) > 1 - p(1, (next; 2)) + p(1, (next if true; 2)) end) } # redo @@ -594,7 +594,7 @@ class << (ary=[]); def []; 0; end; def []=(x); super(0,x);end;end; ary[]+=1 1.times{ break if i>1 i+=1 - p(1, (redo; 2)) + p(1, (redo if true; 2)) }; :ok } assert_equal '3', %q{ @@ -608,7 +608,7 @@ class << (ary=[]); def []; 0; end; def []=(x); super(0,x);end;end; ary[]+=1 i = 0 1 + (while true break 2 if (i+=1) > 1 - p(1, (redo; 2)) + p(1, (redo if true; 2)) end) } assert_equal '1', %q{ diff --git a/prism/prism.c b/prism/prism.c index 2c039612850e92..8f3617adce9b44 100644 --- a/prism/prism.c +++ b/prism/prism.c @@ -1054,6 +1054,13 @@ pm_parser_constant_id_token(pm_parser_t *parser, const pm_token_t *token) { return pm_parser_constant_id_raw(parser, token->start, token->end); } +/** + * This macro allows you to define a case statement for all of the nodes that + * may result in a void value. + */ +#define PM_CASE_VOID_VALUE PM_RETURN_NODE: case PM_BREAK_NODE: case PM_NEXT_NODE: \ + case PM_REDO_NODE: case PM_RETRY_NODE: case PM_MATCH_REQUIRED_NODE + /** * Check whether or not the given node is value expression. * If the node is value node, it returns NULL. @@ -1065,12 +1072,7 @@ pm_check_value_expression(pm_parser_t *parser, pm_node_t *node) { while (node != NULL) { switch (PM_NODE_TYPE(node)) { - case PM_RETURN_NODE: - case PM_BREAK_NODE: - case PM_NEXT_NODE: - case PM_REDO_NODE: - case PM_RETRY_NODE: - case PM_MATCH_REQUIRED_NODE: + case PM_CASE_VOID_VALUE: return void_node != NULL ? void_node : node; case PM_MATCH_PREDICATE_NODE: return NULL; @@ -1090,25 +1092,36 @@ pm_check_value_expression(pm_parser_t *parser, pm_node_t *node) { node = UP(cast->ensure_clause); } else if (cast->rescue_clause != NULL) { - if (cast->statements == NULL) return NULL; + // https://bugs.ruby-lang.org/issues/21669 + if (cast->else_clause == NULL || parser->version < PM_OPTIONS_VERSION_CRUBY_4_1) { + if (cast->statements == NULL) return NULL; - pm_node_t *vn = pm_check_value_expression(parser, UP(cast->statements)); - if (vn == NULL) return NULL; - if (void_node == NULL) void_node = vn; + pm_node_t *vn = pm_check_value_expression(parser, UP(cast->statements)); + if (vn == NULL) return NULL; + if (void_node == NULL) void_node = vn; + } for (pm_rescue_node_t *rescue_clause = cast->rescue_clause; rescue_clause != NULL; rescue_clause = rescue_clause->subsequent) { pm_node_t *vn = pm_check_value_expression(parser, UP(rescue_clause->statements)); + if (vn == NULL) { + // https://bugs.ruby-lang.org/issues/21669 + if (parser->version >= PM_OPTIONS_VERSION_CRUBY_4_1) { + return NULL; + } void_node = NULL; break; } - if (void_node == NULL) { - void_node = vn; - } } if (cast->else_clause != NULL) { node = UP(cast->else_clause); + + // https://bugs.ruby-lang.org/issues/21669 + if (parser->version >= PM_OPTIONS_VERSION_CRUBY_4_1) { + pm_node_t *vn = pm_check_value_expression(parser, node); + if (vn != NULL) return vn; + } } else { return void_node; } @@ -1118,6 +1131,50 @@ pm_check_value_expression(pm_parser_t *parser, pm_node_t *node) { break; } + case PM_CASE_NODE: { + // https://bugs.ruby-lang.org/issues/21669 + if (parser->version < PM_OPTIONS_VERSION_CRUBY_4_1) { + return NULL; + } + + pm_case_node_t *cast = (pm_case_node_t *) node; + if (cast->else_clause == NULL) return NULL; + + pm_node_t *condition; + PM_NODE_LIST_FOREACH(&cast->conditions, index, condition) { + assert(PM_NODE_TYPE_P(condition, PM_WHEN_NODE)); + + pm_when_node_t *cast = (pm_when_node_t *) condition; + pm_node_t *vn = pm_check_value_expression(parser, UP(cast->statements)); + if (vn == NULL) return NULL; + if (void_node == NULL) void_node = vn; + } + + node = UP(cast->else_clause); + break; + } + case PM_CASE_MATCH_NODE: { + // https://bugs.ruby-lang.org/issues/21669 + if (parser->version < PM_OPTIONS_VERSION_CRUBY_4_1) { + return NULL; + } + + pm_case_match_node_t *cast = (pm_case_match_node_t *) node; + if (cast->else_clause == NULL) return NULL; + + pm_node_t *condition; + PM_NODE_LIST_FOREACH(&cast->conditions, index, condition) { + assert(PM_NODE_TYPE_P(condition, PM_IN_NODE)); + + pm_in_node_t *cast = (pm_in_node_t *) condition; + pm_node_t *vn = pm_check_value_expression(parser, UP(cast->statements)); + if (vn == NULL) return NULL; + if (void_node == NULL) void_node = vn; + } + + node = UP(cast->else_clause); + break; + } case PM_ENSURE_NODE: { pm_ensure_node_t *cast = (pm_ensure_node_t *) node; node = UP(cast->statements); @@ -1130,6 +1187,22 @@ pm_check_value_expression(pm_parser_t *parser, pm_node_t *node) { } case PM_STATEMENTS_NODE: { pm_statements_node_t *cast = (pm_statements_node_t *) node; + + // https://bugs.ruby-lang.org/issues/21669 + if (parser->version >= PM_OPTIONS_VERSION_CRUBY_4_1) { + pm_node_t *body_part; + PM_NODE_LIST_FOREACH(&cast->body, index, body_part) { + switch (PM_NODE_TYPE(body_part)) { + case PM_CASE_VOID_VALUE: + if (void_node == NULL) { + void_node = body_part; + } + return void_node; + default: break; + } + } + } + node = cast->body.nodes[cast->body.size - 1]; break; } diff --git a/test/prism/errors/3.3-4.0/singleton_method_with_void_value.txt b/test/prism/errors/3.3-4.0/singleton_method_with_void_value.txt new file mode 100644 index 00000000000000..2954f7ea4895f2 --- /dev/null +++ b/test/prism/errors/3.3-4.0/singleton_method_with_void_value.txt @@ -0,0 +1,3 @@ +def ((return; 1)).bar; end + ^ cannot define singleton method for literals + diff --git a/test/prism/errors/3.4-4.0/void_value.txt b/test/prism/errors/3.4-4.0/void_value.txt new file mode 100644 index 00000000000000..c03139bb052be4 --- /dev/null +++ b/test/prism/errors/3.4-4.0/void_value.txt @@ -0,0 +1,18 @@ +x = begin + return + ^~~~~~ unexpected void value expression +rescue + return +else + return +end + +x = begin + return +rescue + "OK" +else + return + ^~~~~~ unexpected void value expression +end + diff --git a/test/prism/errors/4.1/singleton_method_with_void_value.txt b/test/prism/errors/4.1/singleton_method_with_void_value.txt new file mode 100644 index 00000000000000..bc6cf9c602de4f --- /dev/null +++ b/test/prism/errors/4.1/singleton_method_with_void_value.txt @@ -0,0 +1,4 @@ +def ((return; 1)).bar; end + ^~~~~~ unexpected void value expression + ^ cannot define singleton method for literals + diff --git a/test/prism/errors/4.1/void_value.txt b/test/prism/errors/4.1/void_value.txt new file mode 100644 index 00000000000000..a27ffd763aa237 --- /dev/null +++ b/test/prism/errors/4.1/void_value.txt @@ -0,0 +1,44 @@ +x = begin + return +rescue + return +else + return + ^~~~~~ unexpected void value expression +end + +x = begin + ignored_because_else_branch +rescue + return +else + return + ^~~~~~ unexpected void value expression +end + +x = case + when 1 then return + ^~~~~~ unexpected void value expression + else return +end + +x = case 1 + in 2 then return + ^~~~~~ unexpected void value expression + else return +end + +x = begin + return + ^~~~~~ unexpected void value expression + "NG" +end + +x = if rand < 0.5 + return + ^~~~~~ unexpected void value expression + "NG" +else + return +end + diff --git a/test/prism/errors/singleton_method_for_literals.txt b/test/prism/errors/singleton_method_for_literals.txt index 6247b4f025523f..ae850fca29d885 100644 --- a/test/prism/errors/singleton_method_for_literals.txt +++ b/test/prism/errors/singleton_method_for_literals.txt @@ -2,8 +2,6 @@ def (1).g; end ^ cannot define singleton method for literals def ((a; 1)).foo; end ^ cannot define singleton method for literals -def ((return; 1)).bar; end - ^ cannot define singleton method for literals def (((1))).foo; end ^ cannot define singleton method for literals def (__FILE__).foo; end diff --git a/test/prism/errors/void_value_expression_in_begin_statement.txt b/test/prism/errors/void_value_expression_in_begin_statement.txt index aa8f1ded964c66..fb968a12e117f8 100644 --- a/test/prism/errors/void_value_expression_in_begin_statement.txt +++ b/test/prism/errors/void_value_expression_in_begin_statement.txt @@ -14,8 +14,6 @@ x = begin return ensure return end ^~~~~~ unexpected void value expression x = begin return; rescue; return end ^~~~~~ unexpected void value expression -x = begin return; rescue; return; else return end - ^~~~~~ unexpected void value expression x = begin; return; rescue; retry; end ^~~~~~ unexpected void value expression diff --git a/test/prism/fixtures/3.3-4.0/void_value.txt b/test/prism/fixtures/3.3-4.0/void_value.txt new file mode 100644 index 00000000000000..bfb8eff09c9577 --- /dev/null +++ b/test/prism/fixtures/3.3-4.0/void_value.txt @@ -0,0 +1,29 @@ +x = begin + foo +rescue + return +else + return +end + +x = case + when 1 then return + else return +end + +x = case 1 + in 2 then return + else return +end + +x = begin + return + "NG" +end + +x = if rand < 0.5 + return + "NG" +else + return +end diff --git a/test/prism/fixtures/4.1/void_value.txt b/test/prism/fixtures/4.1/void_value.txt new file mode 100644 index 00000000000000..915112d62346a0 --- /dev/null +++ b/test/prism/fixtures/4.1/void_value.txt @@ -0,0 +1,7 @@ +x = begin + return +rescue + "OK" +else + return +end diff --git a/test/prism/fixtures/non_void_value.txt b/test/prism/fixtures/non_void_value.txt new file mode 100644 index 00000000000000..388e9f2574bcf6 --- /dev/null +++ b/test/prism/fixtures/non_void_value.txt @@ -0,0 +1,31 @@ +x = begin + return if true + "conditional" +end + +x = if rand < 0.5 + return + "else is nil" +end + +x = if true + return if true + "conditional" +else + return +end + +x = if true + return if true +else + return if true + "conditional" +end + +x = case + when 1 + return if true + "conditional" + else + return +end diff --git a/test/prism/fixtures_test.rb b/test/prism/fixtures_test.rb index 7df97029d3aa52..3a704b83895054 100644 --- a/test/prism/fixtures_test.rb +++ b/test/prism/fixtures_test.rb @@ -24,7 +24,10 @@ class FixturesTest < TestCase except << "whitequark/ruby_bug_19281.txt" end + # https://bugs.ruby-lang.org/issues/21168#note-5 except << "command_method_call_2.txt" + # https://bugs.ruby-lang.org/issues/21669 + except << "4.1/void_value.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 4f8d6080e8075d..48449018041dcf 100644 --- a/test/prism/locals_test.rb +++ b/test/prism/locals_test.rb @@ -28,6 +28,9 @@ class LocalsTest < TestCase # https://bugs.ruby-lang.org/issues/21168#note-5 "command_method_call_2.txt", + + # https://bugs.ruby-lang.org/issues/21669 + "4.1/void_value.txt" ] Fixture.each_for_current_ruby(except: except) do |fixture| diff --git a/test/prism/result/warnings_test.rb b/test/prism/result/warnings_test.rb index 4643fb134fe759..27f1119b98333c 100644 --- a/test/prism/result/warnings_test.rb +++ b/test/prism/result/warnings_test.rb @@ -230,6 +230,8 @@ def test_unused_local_variables refute_warning("foo = 1", compare: false, command_line: "e") refute_warning("foo = 1", compare: false, scopes: [[]]) + refute_warning("foo(bar = 1)") + assert_warning("def foo; bar = 1; end", "unused") assert_warning("def foo; bar, = 1; end", "unused") @@ -263,6 +265,23 @@ def test_unused_local_variables refute_warning("def foo; bar = 1; end", line: -2, compare: false) end + def test_unused_local_variable_or_assign_with_begin_node + assert_warning(<<~RUBY, "assigned but unused variable - foo", compare: false) + var ||= begin + foo = bar + baz + end + RUBY + + assert_warning(<<~RUBY, "assigned but unused variable - foo", compare: false) + foo = false + var ||= begin + foo = true + bar + end + RUBY + end + def test_void_statements assert_warning("foo = 1; foo", "a variable in void") assert_warning("@foo", "a variable in void") diff --git a/test/prism/ruby/ripper_test.rb b/test/prism/ruby/ripper_test.rb index 758505ac2ab96d..15f535f3d6a157 100644 --- a/test/prism/ruby/ripper_test.rb +++ b/test/prism/ruby/ripper_test.rb @@ -37,6 +37,9 @@ class RipperTest < TestCase ] end + # https://bugs.ruby-lang.org/issues/21669 + incorrect << "4.1/void_value.txt" + # Skip these tests that we haven't implemented yet. omitted_sexp_raw = [ "bom_leading_space.txt", diff --git a/test/prism/ruby/ruby_parser_test.rb b/test/prism/ruby/ruby_parser_test.rb index 4b7e9c93eddedc..6e4ba8ce16f25f 100644 --- a/test/prism/ruby/ruby_parser_test.rb +++ b/test/prism/ruby/ruby_parser_test.rb @@ -83,6 +83,8 @@ class RubyParserTest < TestCase "3.3-3.3/keyword_args_in_array_assignment.txt", "3.3-3.3/return_in_sclass.txt", + "3.3-4.0/void_value.txt", + "3.4/circular_parameters.txt", "4.0/endless_methods_command_call.txt", diff --git a/test/ruby/test_syntax.rb b/test/ruby/test_syntax.rb index b355128a7344a6..5065a1db332a54 100644 --- a/test/ruby/test_syntax.rb +++ b/test/ruby/test_syntax.rb @@ -1539,10 +1539,10 @@ def test_return_toplevel begin raise; rescue; return; end return false; raise return 1; raise - "#{return}" - raise((return; "should not raise")) + "#{return if true}" + raise((return if true; "should not raise")) begin raise; ensure return; end; self - nil&defined?0--begin e=no_method_error(); return; 0;end + nil&defined?0--begin e=no_method_error(); return if true; 0;end return puts('ignored') #=> ignored BEGIN {return} END {return if false}