Rubocop: Lint/ShadowedException gets confused by Timeout::Error

[DavidS]

Expected behavior

The Lint/ShadowedException cop should ignore multiple exceptions on a single rescue clause, as they are not shadowing each other.

Actual behavior

begin
  puts "foo"
rescue StandardError, Timeout::Error => e
  puts e.inspect
end

causes

$ bundle exec rubocop -D tmp/shadow.rb 
Inspecting 1 file
W

Offenses:

tmp/shadow.rb:3:3: W: Lint/ShadowedException: Do not shadow rescued Exceptions
  puts "foo" ...
  ^^^^^^^^^^

1 file inspected, 1 offense detected
$

regardless of the order of StandardError, and Timeout::Error on the rescue clause.

RuboCop version

$ bundle exec rubocop -V
0.41.1 (using Parser 2.3.1.2, running on ruby 2.3.1 x86_64-linux-gnu)
$

[DavidS]

ping @rrosenblum

[rrosenblum]

In the general sense, it is not shadowed since they fall into the same rescue group. The rescue of Timeout:: Error is unnecessary though since it is inherited from StandardError. The wording of this offense could be improved. Maybe this cop should be split into two cops.

[DavidS]

Ah, I missed that relationship. A improved wording in the description would probably go a long way.

[rrosenblum]

Sorry for the confusion.

I'm currently on vacation and went have time to tweak the wording for the next week or so. If anyone else is interested in tackling this, please go ahead and do so.

[vinagrito]

First allow me to apologize me if I should be opening a different issue...
@rrosenblum I had this exact same behavior but then I noticed something else:
This works fine:

class TestClass
  def do_something
    # some work here
  rescue StandardError, TimeoutError => exception
    puts exception.message
  end
end

Now this:

class TestClass
  def do_something
    # some work here
  rescue StandardError, TimeoutError => exception
    puts exception.message
  ensure
    puts "At least I'll do this!"
  end
end

Returns
```W: Lint/ShadowedException: Do not shadow rescued Exceptions.
rescue StandardError, TimeoutError => exception

---

bundle exec rubocop -V
0.46.0 (using Parser 2.3.3.1, running on ruby 2.2.5 x86_64-darwin15)
```

[rrosenblum]

@vinagrito, I am not sure that I follow what you are referring to. If I am understanding correctly, the first example that you provided does not register an offense in RuboCop and the second example does. When I tried running RuboCop on the examples, both of them registered an offense. The messaging here is a little confusing since it is the same rescue "group". We do consider this an offense because StandardError is a more generic exception type than TimeoutError. Since TimeoutError (deprecated in favor of Timeout::Error) is a descendant of StandardError there is no need to rescue from the TimeoutError.

[2] pry(main)> TimeoutError.ancestors
=> [Timeout::Error, RuntimeError, StandardError, Exception, Object, PP::ObjectMixin, Kernel, BasicObject]

[vinagrito]

@rrosenblum darn, bad copy/paste case - I'm editing it. My bad!.
I totally get the point the reason why we shouldn't be rescuing TimeoutError together with StandardError (or at all since, as you mention, we have now Timeout::Error). But how come then the first example passes in my build? (I don't have the cop disabled). So, to state the obvious, the ensure clause changes the behavior for me.

[rrosenblum]

I have run RuboCop on the updated example, and it did not register an offense. This is a bug with that cop. The issue is that, aside from the RescueModifier cop, we don't have a great way of knowing if a rescue is in its modifier form or not. ShadowedException and ParallelAssignment both implement a basic check to see if the rescue is in modifier form or not. I believe that in order to fix this, we will have to move some of the logic from RescueModifier into a helper, and modify all the cops to use a consistent reliable mechanism for checking if the rescue is a modifier or not.

The check that ShadowedException is using to see if it is a rescue modifier is

def rescue_modifier?(node)
  node && node.rescue_type? &&
    (node.parent.nil? || !(node.parent.kwbegin_type? ||
    node.parent.ensure_type?))
end

I don't think that it will be too difficult to modify the cops to use a common check. If you could open a new issue for this, that would be helpful.

[rrosenblum]

@vinagrito, I started some of the work to use a common check for if a node is a rescue modifier. Unfortunately, I may have just come across a bug in RescueModifier.

[Drenmi]

@rrosenblum As you have now added the common method in #4067, would the fix for this bug be relatively simple? Is it something you could open a PR for? 🙇

[rrosenblum]

@Drenmi it looks like this was already fixed by https://github.com/rubocop-hq/rubocop/pull/4067. For some reason, I created a separate issue around this, and I guess this issue was never closed after the fix was implemented.

[Drenmi]

Thanks @rrosenblum! 🙇 Closing.

[vinagrito]

@rrosenblum better late than never. Thx for addressing this 🙏 . I somehow lost track of this and only now saw it.