Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`Lint/EnsureReturn` not necessarily safe for autocorrect #8385

Closed
OmriSama opened this issue Jul 23, 2020 · 2 comments
Closed

`Lint/EnsureReturn` not necessarily safe for autocorrect #8385

OmriSama opened this issue Jul 23, 2020 · 2 comments
Assignees

Comments

@OmriSama
Copy link

@OmriSama OmriSama commented Jul 23, 2020

We had an issue in production today that was caused by an autocorrect on a Lint/EnsureReturn case.

Basically, here's what it looked like:

module MyModule
  module_function

  def call(some_arg:)
    # Lots of code here...
    
    # Here we have code that potentially defines a variable `some_var`, 
    # depending on where it is located.
    case some_string
    when 'string_one'
      if some_condition
        some_var = function_A
      else
        # More code with no reference to `some_var`
      end
    when 'string_two'
      some_var = function_B
    end
  rescue ExceptionType1 => e
    # ...
  rescue ExceptionType2 => e
    # ... 2 more of these
  ensure
    return some_var if some_var
    
    # ...
    # Here is more code that returns something else if `some_var` is not defined.
  end
end

What Rubocop did was change return some_var if some_var to just some_var, which did not return, went unnoticed by us, and caused an issue in production.

While I now understand that using return in an ensure block is not great practice and is considered code smell (did not know this until researching this today), is it possible to consider this Cop as not necessarily safe for autocorrect?

Thanks.


RuboCop version

$ bundle exec rubocop -V
0.85.0 (using Parser 2.7.1.3, rubocop-ast 0.0.3, running on ruby 2.5.7 x86_64-darwin19)
@marcandre marcandre self-assigned this Jul 23, 2020
@marcandre
Copy link
Contributor

@marcandre marcandre commented Jul 23, 2020

I'm very sorry you had that issue. Let me see what I can do.

marcandre added a commit to marcandre/rubocop that referenced this issue Jul 23, 2020
marcandre added a commit to marcandre/rubocop that referenced this issue Jul 23, 2020
… multi-value returns
marcandre added a commit to marcandre/rubocop that referenced this issue Jul 23, 2020
…lag multi-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
marcandre added a commit to marcandre/rubocop that referenced this issue Jul 23, 2020
…lag multi-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 rubocop-hq#7934
marcandre added a commit to marcandre/rubocop that referenced this issue Jul 23, 2020
…lag multi-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 rubocop-hq#7934
@marcandre marcandre closed this in e4f2da9 Jul 23, 2020
@marcandre
Copy link
Contributor

@marcandre marcandre commented Jul 23, 2020

Right, our auto-correction was wrong and I've had to remove it. There's no valid correction that can be done. The example you provided is a good example I believe;I can't provide an auto-correction myself and I suspect it involves a bit of refactoring to be written correctly. The cop is also right that some other exceptions might be swallowed without you knowing about it so a refactoring is indeed desirable.

I'm sorry you had issues in production. Thanks for raising the issue and providing a detailed example 👍

marcandre added a commit to marcandre/rubocop that referenced this issue Jul 23, 2020
marcandre added a commit that referenced this issue Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.