Lint/EnsureReturn not necessarily safe for autocorrect

[OmriSama]

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]

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

[marcandre]

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 👍