[Bug #21669] Thoroughly implement void value expression check (prism)

Merged: https://github.com/ruby/ruby/pull/15999

bootstraptest/test_flow.rb

@@ -376,7 +376,7 @@ assert_equal %q{[1, 4, 7, 5, 8, 9]}, %q{$a = []; begin; ; $a << 1
     ; $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 @@ assert_equal %q{[1, 3, 11, 4, 5, 6, 7, 12, 13]}, %q{$a = []; begin; ; $a << 1
     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

bootstraptest/test_syntax.rb

@@ -571,7 +571,7 @@ assert_equal 'ok', %q{
 
 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 @@ assert_equal '3', %q{
   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 @@ assert_equal 'ok', %q{
   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 @@ assert_equal '3', %q{
   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{

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

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
+

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
+

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
+

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
+

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

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
 

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

test/prism/fixtures/4.1/void_value.txt

@@ -0,0 +1,7 @@
+x = begin
+  return
+rescue
+  "OK"
+else
+  return
+end

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

test/prism/fixtures_test.rb

@@ -24,7 +24,10 @@ module Prism
       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) }

test/prism/locals_test.rb

@@ -28,6 +28,9 @@ module Prism
 
       # 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|

test/prism/result/warnings_test.rb

@@ -230,6 +230,8 @@ module Prism
       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 @@ module Prism
       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")

test/prism/ruby/ripper_test.rb

@@ -37,6 +37,9 @@ module Prism
       ]
     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",

test/prism/ruby/ruby_parser_test.rb

@@ -83,6 +83,8 @@ module Prism
       "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",

test/ruby/test_syntax.rb

@@ -1539,10 +1539,10 @@ eom
       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}