[ruby/prism] Add a flag on returns when they are redundant

ruby/prism@450541d2c3

prism/config.yml

@@ -706,6 +706,11 @@ flags:
       - name: FORCED_US_ASCII_ENCODING
         comment: "internal bytes forced the encoding to US-ASCII"
     comment: Flags for regular expression and match last line nodes.
+  - name: ReturnNodeFlags
+    values:
+      - name: REDUNDANT
+        comment: "a return statement that is redundant because it is the last statement in a method"
+    comment: Flags for return nodes.
   - name: ShareableConstantNodeFlags
     values:
       - name: LITERAL
@@ -3199,6 +3204,9 @@ nodes:
           ^^^^^
   - name: ReturnNode
     fields:
+      - name: flags
+        type: flags
+        kind: ReturnNodeFlags
       - name: keyword_loc
         type: location
       - name: arguments

prism/prism.c

@@ -3716,6 +3716,113 @@ pm_def_node_receiver_check(pm_parser_t *parser, const pm_node_t *node) {
     }
 }
 
+/**
+ * When a method body is created, we want to check if the last statement is a
+ * return or a statement that houses a return. If it is, then we want to mark
+ * that return as being redundant so that we can compile it differently but also
+ * so that we can indicate that to the user.
+ */
+static void
+pm_def_node_body_redundant_return(pm_node_t *node) {
+    switch (PM_NODE_TYPE(node)) {
+        case PM_RETURN_NODE:
+            node->flags |= PM_RETURN_NODE_FLAGS_REDUNDANT;
+            break;
+        case PM_BEGIN_NODE: {
+            pm_begin_node_t *cast = (pm_begin_node_t *) node;
+
+            if (cast->statements != NULL && cast->else_clause == NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->statements);
+            }
+            break;
+        }
+        case PM_STATEMENTS_NODE: {
+            pm_statements_node_t *cast = (pm_statements_node_t *) node;
+
+            if (cast->body.size > 0) {
+                pm_def_node_body_redundant_return(cast->body.nodes[cast->body.size - 1]);
+            }
+            break;
+        }
+        case PM_IF_NODE: {
+            pm_if_node_t *cast = (pm_if_node_t *) node;
+
+            if (cast->statements != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->statements);
+            }
+
+            if (cast->consequent != NULL) {
+                pm_def_node_body_redundant_return(cast->consequent);
+            }
+            break;
+        }
+        case PM_UNLESS_NODE: {
+            pm_unless_node_t *cast = (pm_unless_node_t *) node;
+
+            if (cast->statements != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->statements);
+            }
+
+            if (cast->consequent != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->consequent);
+            }
+            break;
+        }
+        case PM_ELSE_NODE: {
+            pm_else_node_t *cast = (pm_else_node_t *) node;
+
+            if (cast->statements != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->statements);
+            }
+            break;
+        }
+        case PM_CASE_NODE: {
+            pm_case_node_t *cast = (pm_case_node_t *) node;
+            pm_node_t *condition;
+
+            PM_NODE_LIST_FOREACH(&cast->conditions, index, condition) {
+                pm_def_node_body_redundant_return(condition);
+            }
+
+            if (cast->consequent != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->consequent);
+            }
+            break;
+        }
+        case PM_WHEN_NODE: {
+            pm_when_node_t *cast = (pm_when_node_t *) node;
+
+            if (cast->statements != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->statements);
+            }
+            break;
+        }
+        case PM_CASE_MATCH_NODE: {
+            pm_case_match_node_t *cast = (pm_case_match_node_t *) node;
+            pm_node_t *condition;
+
+            PM_NODE_LIST_FOREACH(&cast->conditions, index, condition) {
+                pm_def_node_body_redundant_return(condition);
+            }
+
+            if (cast->consequent != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->consequent);
+            }
+            break;
+        }
+        case PM_IN_NODE: {
+            pm_in_node_t *cast = (pm_in_node_t *) node;
+
+            if (cast->statements != NULL) {
+                pm_def_node_body_redundant_return((pm_node_t *) cast->statements);
+            }
+            break;
+        }
+        default:
+            break;
+    }
+}
+
 /**
  * Allocate and initialize a new DefNode node.
  */
@@ -3748,6 +3855,10 @@ pm_def_node_create(
         pm_def_node_receiver_check(parser, receiver);
     }
 
+    if (body != NULL) {
+        pm_def_node_body_redundant_return(body);
+    }
+
     *node = (pm_def_node_t) {
         {
             .type = PM_DEF_NODE,
@@ -6397,6 +6508,7 @@ pm_return_node_create(pm_parser_t *parser, const pm_token_t *keyword, pm_argumen
     *node = (pm_return_node_t) {
         {
             .type = PM_RETURN_NODE,
+            .flags = 0,
             .location = {
                 .start = keyword->start,
                 .end = (arguments == NULL ? keyword->end : arguments->base.location.end)

test/prism/errors_test.rb

@@ -1094,7 +1094,7 @@ def test_dont_allow_return_inside_class_body
         ConstantReadNode(:A),
         nil,
         nil,
-        StatementsNode([ReturnNode(Location(), nil)]),
+        StatementsNode([ReturnNode(0, Location(), nil)]),
         Location(),
         :A
       )
@@ -1109,7 +1109,7 @@ def test_dont_allow_return_inside_module_body
         [],
         Location(),
         ConstantReadNode(:A),
-        StatementsNode([ReturnNode(Location(), nil)]),
+        StatementsNode([ReturnNode(0, Location(), nil)]),
         Location(),
         :A
       )

test/prism/redundant_return_test.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+require_relative "test_helper"
+
+module Prism
+  class RedundantReturnTest < TestCase
+    def test_statements
+      assert_redundant_return("def foo; return; end")
+      refute_redundant_return("def foo; return; 1; end")
+    end
+
+    def test_begin_implicit
+      assert_redundant_return("def foo; return; rescue; end")
+      refute_redundant_return("def foo; return; 1; rescue; end")
+      refute_redundant_return("def foo; return; rescue; else; end")
+    end
+
+    def test_begin_explicit
+      assert_redundant_return("def foo; begin; return; rescue; end; end")
+      refute_redundant_return("def foo; begin; return; 1; rescue; end; end")
+      refute_redundant_return("def foo; begin; return; rescue; else; end; end")
+    end
+
+    def test_if
+      assert_redundant_return("def foo; return if bar; end")
+    end
+
+    def test_unless
+      assert_redundant_return("def foo; return unless bar; end")
+    end
+
+    def test_else
+      assert_redundant_return("def foo; if bar; baz; else; return; end; end")
+    end
+
+    def test_case_when
+      assert_redundant_return("def foo; case bar; when baz; return; end; end")
+    end
+
+    def test_case_else
+      assert_redundant_return("def foo; case bar; when baz; else; return; end; end")
+    end
+
+    def test_case_match_in
+      assert_redundant_return("def foo; case bar; in baz; return; end; end")
+    end
+
+    def test_case_match_else
+      assert_redundant_return("def foo; case bar; in baz; else; return; end; end")
+    end
+
+    private
+
+    def assert_redundant_return(source)
+      assert find_return(source).redundant?
+    end
+
+    def refute_redundant_return(source)
+      refute find_return(source).redundant?
+    end
+
+    def find_return(source)
+      queue = [Prism.parse(source).value]
+
+      while (current = queue.shift)
+        return current if current.is_a?(ReturnNode)
+        queue.concat(current.compact_child_nodes)
+      end
+
+      flunk "Could not find return node in #{node.inspect}"
+    end
+  end
+end

test/prism/snapshots/if.txt

@@ -162,6 +162,7 @@
         │   │   @ StatementsNode (location: (14,0)-(14,6))
         │   │   └── body: (length: 1)
         │   │       └── @ ReturnNode (location: (14,0)-(14,6))
+        │   │           ├── flags: ∅
         │   │           ├── keyword_loc: (14,0)-(14,6) = "return"
         │   │           └── arguments: ∅
         │   ├── consequent: ∅

test/prism/snapshots/methods.txt

@@ -814,6 +814,7 @@
         │   │       │   │   @ StatementsNode (location: (99,0)-(99,10))
         │   │       │   │   └── body: (length: 1)
         │   │       │   │       └── @ ReturnNode (location: (99,0)-(99,10))
+        │   │       │   │           ├── flags: ∅
         │   │       │   │           ├── keyword_loc: (99,0)-(99,6) = "return"
         │   │       │   │           └── arguments:
         │   │       │   │               @ ArgumentsNode (location: (99,7)-(99,10))

test/prism/snapshots/rescue.txt

@@ -88,6 +88,7 @@
         ├── @ RescueModifierNode (location: (10,0)-(10,17))
         │   ├── expression:
         │   │   @ ReturnNode (location: (10,0)-(10,6))
+        │   │   ├── flags: ∅
         │   │   ├── keyword_loc: (10,0)-(10,6) = "return"
         │   │   └── arguments: ∅
         │   ├── keyword_loc: (10,7)-(10,13) = "rescue"

test/prism/snapshots/return.txt

@@ -4,9 +4,11 @@
     @ StatementsNode (location: (1,0)-(23,9))
     └── body: (length: 10)
         ├── @ ReturnNode (location: (1,0)-(1,6))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (1,0)-(1,6) = "return"
         │   └── arguments: ∅
         ├── @ ReturnNode (location: (3,0)-(3,20))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (3,0)-(3,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (3,7)-(3,20))
@@ -40,6 +42,7 @@
         │               ├── opening_loc: (3,17)-(3,18) = "("
         │               └── closing_loc: (3,19)-(3,20) = ")"
         ├── @ ReturnNode (location: (5,0)-(5,9))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (5,0)-(5,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (5,7)-(5,9))
@@ -52,6 +55,7 @@
         │                   ├── flags: decimal
         │                   └── value: 1
         ├── @ ReturnNode (location: (7,0)-(7,8))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (7,0)-(7,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (7,7)-(7,8))
@@ -61,6 +65,7 @@
         │               ├── flags: decimal
         │               └── value: 1
         ├── @ ReturnNode (location: (9,0)-(10,1))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (9,0)-(9,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (9,7)-(10,1))
@@ -76,6 +81,7 @@
         │               ├── flags: decimal
         │               └── value: 3
         ├── @ ReturnNode (location: (12,0)-(12,14))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (12,0)-(12,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (12,7)-(12,14))
@@ -91,6 +97,7 @@
         │               ├── flags: decimal
         │               └── value: 3
         ├── @ ReturnNode (location: (14,0)-(14,16))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (14,0)-(14,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (14,7)-(14,16))
@@ -111,6 +118,7 @@
         │               ├── opening_loc: (14,7)-(14,8) = "["
         │               └── closing_loc: (14,15)-(14,16) = "]"
         ├── @ ReturnNode (location: (16,0)-(19,1))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (16,0)-(16,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (16,6)-(19,1))
@@ -129,6 +137,7 @@
         │               ├── opening_loc: (16,6)-(16,7) = "("
         │               └── closing_loc: (19,0)-(19,1) = ")"
         ├── @ ReturnNode (location: (21,0)-(21,8))
+        │   ├── flags: ∅
         │   ├── keyword_loc: (21,0)-(21,6) = "return"
         │   └── arguments:
         │       @ ArgumentsNode (location: (21,6)-(21,8))
@@ -139,6 +148,7 @@
         │               ├── opening_loc: (21,6)-(21,7) = "("
         │               └── closing_loc: (21,7)-(21,8) = ")"
         └── @ ReturnNode (location: (23,0)-(23,9))
+            ├── flags: ∅
             ├── keyword_loc: (23,0)-(23,6) = "return"
             └── arguments:
                 @ ArgumentsNode (location: (23,6)-(23,9))

test/prism/snapshots/seattlerb/block_return.txt

@@ -4,6 +4,7 @@
     @ StatementsNode (location: (1,0)-(1,27))
     └── body: (length: 1)
         └── @ ReturnNode (location: (1,0)-(1,27))
+            ├── flags: ∅
             ├── keyword_loc: (1,0)-(1,6) = "return"
             └── arguments:
                 @ ArgumentsNode (location: (1,7)-(1,27))

test/prism/snapshots/seattlerb/parse_line_defn_complex.txt

@@ -49,6 +49,7 @@
             │       │   ├── operator: :*
             │       │   └── depth: 0
             │       └── @ ReturnNode (location: (4,2)-(4,10))
+            │           ├── flags: redundant
             │           ├── keyword_loc: (4,2)-(4,8) = "return"
             │           └── arguments:
             │               @ ArgumentsNode (location: (4,9)-(4,10))

test/prism/snapshots/seattlerb/parse_line_return.txt

@@ -20,6 +20,7 @@
             │           │   @ StatementsNode (location: (3,10)-(3,19))
             │           │   └── body: (length: 1)
             │           │       └── @ ReturnNode (location: (3,10)-(3,19))
+            │           │           ├── flags: redundant
             │           │           ├── keyword_loc: (3,10)-(3,16) = "return"
             │           │           └── arguments:
             │           │               @ ArgumentsNode (location: (3,17)-(3,19))