diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index d08b312aa4e..ef131da4af3 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -1035,21 +1035,23 @@ 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 ---- @@ -1057,10 +1059,24 @@ end ---- # 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 ---- diff --git a/lib/rubocop/cop/lint/ensure_return.rb b/lib/rubocop/cop/lint/ensure_return.rb index 424d5b791fd..1bc6d60e0d8 100644 --- a/lib/rubocop/cop/lint/ensure_return.rb +++ b/lib/rubocop/cop/lint/ensure_return.rb @@ -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 diff --git a/spec/rubocop/cop/lint/ensure_return_spec.rb b/spec/rubocop/cop/lint/ensure_return_spec.rb index 8bf78f85e94..c24ef4a1d20 100644 --- a/spec/rubocop/cop/lint/ensure_return_spec.rb +++ b/spec/rubocop/cop/lint/ensure_return_spec.rb @@ -14,13 +14,7 @@ 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 @@ -28,39 +22,35 @@ 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