From eaa08bea783370a5363bd8f2a9c57c390ee6d11e Mon Sep 17 00:00:00 2001
From: Benoit Daloze <benoit.daloze@oracle.com>
Date: Mon, 9 Jan 2023 13:14:44 +0100
Subject: [PATCH] Fix checks for void value expression

* Fixes https://github.com/oracle/truffleruby/issues/2821
---
 CHANGELOG.md                                  |  1 +
 spec/ruby/language/if_spec.rb                 | 43 +++++++++++++++++++
 .../parser/parser/ParserSupport.java          | 25 ++++++-----
 test/mri/excludes/TestSyntax.rb               |  1 -
 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ad11c6b61b00..8ca8dcdcb5f9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -55,6 +55,7 @@ Compatibility:
 * Implement `Coverage.running?` method (@andrykonchin).
 * Fix arguments implicit type conversion for `Enumerable#zip` and `Array#zip` (#2788, @andrykonchin).
 * Fix `Array#unshift` to not depend on `Array#[]=` and allow overriding `#[]=` in a subclass (#2772, @andrykonchin).
+* Fix syntactic check for `void value expression` (#2821, @eregon).
 
 Performance:
 
diff --git a/spec/ruby/language/if_spec.rb b/spec/ruby/language/if_spec.rb
index d1d95c160791..a5da69600049 100644
--- a/spec/ruby/language/if_spec.rb
+++ b/spec/ruby/language/if_spec.rb
@@ -306,6 +306,49 @@
       ScratchPad.recorded.should == [4, 5, 4, 5]
     end
   end
+
+  describe "when a branch syntactically does not return a value" do
+    it "raises SyntaxError if both do not return a value" do
+      -> {
+        eval <<~RUBY
+        def m
+          a = if rand
+            return
+          else
+            return
+          end
+          a
+        end
+        RUBY
+      }.should raise_error(SyntaxError, /void value expression/)
+    end
+
+    it "does not raise SyntaxError if one branch returns a value" do
+      eval(<<~RUBY).should == 1
+      def m
+        a = if false # using false to make it clear that's not checked for
+          42
+        else
+          return 1
+        end
+        a
+      end
+      m
+      RUBY
+
+      eval(<<~RUBY).should == 1
+      def m
+        a = if true # using true to make it clear that's not checked for
+          return 1
+        else
+          42
+        end
+        a
+      end
+      m
+      RUBY
+    end
+  end
 end
 
 describe "The postfix if form" do
diff --git a/src/main/java/org/truffleruby/parser/parser/ParserSupport.java b/src/main/java/org/truffleruby/parser/parser/ParserSupport.java
index 64aa768138f5..62a725a6fec5 100644
--- a/src/main/java/org/truffleruby/parser/parser/ParserSupport.java
+++ b/src/main/java/org/truffleruby/parser/parser/ParserSupport.java
@@ -592,9 +592,17 @@ public void warnUnlessEOption(ParseNode node, String message) {
         }
     }
 
-    public static boolean value_expr(RubyLexer lexer, ParseNode node) {
-        boolean conditional = false;
 
+    public void checkExpression(ParseNode node) {
+        value_expr(lexer, node);
+    }
+
+    public static void value_expr(RubyLexer lexer, ParseNode node) {
+        value_expr(lexer, node, false);
+    }
+
+    // Returns true if it returns a value and is not considered "void" because it "exits" via control flow
+    private static boolean value_expr(RubyLexer lexer, ParseNode node, boolean conditional) {
         while (node != null) {
             switch (node.getNodeType()) {
                 case RETURNNODE:
@@ -614,15 +622,16 @@ public static boolean value_expr(RubyLexer lexer, ParseNode node) {
                     node = ((BeginParseNode) node).getBodyNode();
                     break;
                 case IFNODE:
-                    if (!value_expr(lexer, ((IfParseNode) node).getThenBody())) {
-                        return false;
+                    if (value_expr(lexer, ((IfParseNode) node).getThenBody(), true)) {
+                        // If one branch returns a value it's enough
+                        return true;
                     }
                     node = ((IfParseNode) node).getElseBody();
                     break;
                 case ANDNODE:
                 case ORNODE:
-                    conditional = true;
-                    node = ((BinaryOperatorParseNode) node).getSecondNode();
+                    // If the LHS is void then SyntaxError
+                    node = ((BinaryOperatorParseNode) node).getFirstNode();
                     break;
                 default: // ParseNode
                     return true;
@@ -632,10 +641,6 @@ public static boolean value_expr(RubyLexer lexer, ParseNode node) {
         return true;
     }
 
-    public boolean checkExpression(ParseNode node) {
-        return value_expr(lexer, node);
-    }
-
     private void handleUselessWarn(ParseNode node, String useless) {
         warnings.warning(
                 file,
diff --git a/test/mri/excludes/TestSyntax.rb b/test/mri/excludes/TestSyntax.rb
index 93e17cd8b9ae..a9aa3f0afd4b 100644
--- a/test/mri/excludes/TestSyntax.rb
+++ b/test/mri/excludes/TestSyntax.rb
@@ -43,7 +43,6 @@
 exclude :test_undef_symbol, "needs investigation"
 exclude :test_unterminated_heredoc, "needs investigation"
 exclude :test_unterminated_heredoc_cr, "needs investigation"
-exclude :test_value_expr_in_condition, "needs investigation"
 exclude :test_warn_balanced, "needs investigation"
 exclude :test_warn_unreachable, "needs investigation"
 exclude :test_warning_literal_in_condition, "needs investigation"