Skip to content

Commit

Permalink
[fixes #8385] Remove Lint/EnsureReturn autocorrection and flag multi-…
Browse files Browse the repository at this point in the history
…value returns

It is not possible to autocorrect a `return` in an `ensure`, as not only does it
modify control flow without the ensure block, it also overrides the normal return
value

This reverts #7934
  • Loading branch information
marcandre committed Jul 23, 2020
1 parent a39a226 commit e4f2da9
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 59 deletions.
30 changes: 23 additions & 7 deletions docs/modules/ROOT/pages/cops_lint.adoc
Expand Up @@ -1035,32 +1035,48 @@ end
|===

This cop checks for `return` from an `ensure` block.
Explicit return from an ensure block alters the control flow
as the return will take precedence over any exception being raised,
`return` from an ensure block is a dangerous code smell as it
will take precedence over any exception being raised,
and the exception will be silently thrown away as if it were rescued.

If you want to rescue some (or all) exceptions, best to do it explicitly

=== Examples

[source,ruby]
----
# bad
begin
def foo
do_something
ensure
do_something_else
return
cleanup
return self
end
----

[source,ruby]
----
# good
begin
def foo
do_something
self
ensure
do_something_else
cleanup
end
# also good
def foo
begin
do_something
rescue SomeException
# Let's ignore this exception
end
self
ensure
cleanup
end
----

Expand Down
56 changes: 27 additions & 29 deletions lib/rubocop/cop/lint/ensure_return.rb
Expand Up @@ -4,57 +4,55 @@ module RuboCop
module Cop
module Lint
# This cop checks for `return` from an `ensure` block.
# Explicit return from an ensure block alters the control flow
# as the return will take precedence over any exception being raised,
# `return` from an ensure block is a dangerous code smell as it
# will take precedence over any exception being raised,
# and the exception will be silently thrown away as if it were rescued.
#
# If you want to rescue some (or all) exceptions, best to do it explicitly
#
# @example
#
# # bad
#
# begin
# def foo
# do_something
# ensure
# do_something_else
# return
# cleanup
# return self
# end
#
# @example
#
# # good
#
# begin
# def foo
# do_something
# self
# ensure
# cleanup
# end
#
# # also good
#
# def foo
# begin
# do_something
# rescue SomeException
# # Let's ignore this exception
# end
# self
# ensure
# do_something_else
# cleanup
# end
class EnsureReturn < Cop
class EnsureReturn < Base
extend AutoCorrector
include RangeHelp

MSG = 'Do not return from an `ensure` block.'

def on_ensure(node)
ensure_body = node.body

return unless ensure_body

ensure_body.each_node(:return) do |return_node|
next if return_node.arguments.size >= 2

add_offense(return_node, location: :keyword)
end
end

def autocorrect(node)
lambda do |corrector|
if node.arguments?
corrector.replace(node, node.source.gsub(/return\s*/, ''))
else
range = range_by_whole_lines(
node.loc.expression, include_final_newline: true
)
corrector.remove(range)
end
node.body&.each_node(:return) do |return_node|
add_offense(return_node)
end
end
end
Expand Down
36 changes: 13 additions & 23 deletions spec/rubocop/cop/lint/ensure_return_spec.rb
Expand Up @@ -14,53 +14,43 @@
end
RUBY

expect_correction(<<~RUBY)
begin
something
ensure
file.close
end
RUBY
expect_no_corrections
end

it 'registers an offense and corrects for return with argument in ensure' do
expect_offense(<<~RUBY)
begin
foo
ensure
bar
return baz
^^^^^^ Do not return from an `ensure` block.
^^^^^^^^^^ Do not return from an `ensure` block.
end
RUBY

expect_correction(<<~RUBY)
begin
foo
ensure
bar
baz
end
RUBY
expect_no_corrections
end

it 'does not register an offense for return outside ensure' do
expect_no_offenses(<<~RUBY)
it 'registers an offense when returning multiple values in `ensure`' do
expect_offense(<<~RUBY)
begin
something
return
ensure
file.close
do_something
return foo, bar
^^^^^^^^^^^^^^^ Do not return from an `ensure` block.
end
RUBY

expect_no_corrections
end

it "doesn't register an offense when returning multiple values in `ensure`" do
it 'does not register an offense for return outside ensure' do
expect_no_offenses(<<~RUBY)
begin
something
return
ensure
return foo, bar
file.close
end
RUBY
end
Expand Down

0 comments on commit e4f2da9

Please sign in to comment.