Fix checks for void value expression

* Fixes #2821

CHANGELOG.md

@@ -56,6 +56,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:
 

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

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,

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"