Skip to content

Commit

Permalink
Add a flag on returns when they are redundant
Browse files Browse the repository at this point in the history
  • Loading branch information
kddnewton committed Apr 26, 2024
1 parent 23cf91e commit 450541d
Show file tree
Hide file tree
Showing 20 changed files with 229 additions and 2 deletions.
8 changes: 8 additions & 0 deletions config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -3330,6 +3335,9 @@ nodes:
^^^^^
- name: ReturnNode
fields:
- name: flags
type: flags
kind: ReturnNodeFlags
- name: keyword_loc
type: location
- name: arguments
Expand Down
112 changes: 112 additions & 0 deletions src/prism.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions test/prism/errors_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand All @@ -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
)
Expand Down
73 changes: 73 additions & 0 deletions test/prism/redundant_return_test.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions test/prism/snapshots/if.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/prism/snapshots/methods.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/prism/snapshots/rescue.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions test/prism/snapshots/return.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/prism/snapshots/seattlerb/block_return.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/prism/snapshots/seattlerb/parse_line_defn_complex.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/prism/snapshots/seattlerb/parse_line_return.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 450541d

Please sign in to comment.