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/ShadowedException fails for rescue nil #3249

Closed
janraasch opened this issue Jun 27, 2016 · 5 comments · Fixed by #3262
Closed

Lint/ShadowedException fails for rescue nil #3249

janraasch opened this issue Jun 27, 2016 · 5 comments · Fixed by #3262

Comments

@janraasch
Copy link
Contributor

Test code

begin
  # something
rescue nil
end

Actual behavior

An error occurred while Lint/ShadowedException cop was inspecting /test.rb

Scanning /test.rb
An error occurred while Lint/ShadowedException cop was inspecting /vagrant/sellery/app/test.rb.
undefined method `<' for nil:NilClass
/home/vagrant/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/rubocop-0.41.1/lib/rubocop/cop/lint/shadowed_exception.rb:63:in `block (2 levels) in contains_multiple_levels_of_exceptions?'

RuboCop version

$ rubocop -V
0.41.1 (using Parser 2.3.1.2, running on ruby 2.3.0 x86_64-linux)
@rrosenblum
Copy link
Contributor

I can update the code to safeguard against this, but when I try out an example for rescuing nil, it does not appear to be valid Ruby. @bbatsov and @jonas054 what are your thoughts on this?

[2] pry(main)> begin
[2] pry(main)*   1/0
[2] pry(main)* rescue nil  
[2] pry(main)*   puts 'rescued'
[2] pry(main)* end  
TypeError: class or module required for rescue clause

@janraasch
Copy link
Contributor Author

What version of ruby are you using?

Have you tried putting that code into a *.rb file instead of pry?

@rrosenblum
Copy link
Contributor

The example that I posted was run against Ruby 2.3.0. I tried it in a .rb file and had the same result. I repeated the same test for multiple versions of Ruby. It appears that rescue nil is valid in Ruby 1.9.3.

# test.rb
begin
  1/0
rescue nil
  puts 'rescued'
end
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ rbenv local 1.9.3-p484 
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ ruby test.rb 
test.rb:2:in `/': divided by 0 (ZeroDivisionError)
        from test.rb:2:in `<main>'
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ rbenv local 2.0.0-p598 
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ ruby test.rb          
test.rb:3:in `rescue in <main>': class or module required for rescue clause (TypeError)
        from test.rb:1:in `<main>'
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ rbenv local 2.1.6
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ ruby test.rb     
test.rb:3:in `rescue in <main>': class or module required for rescue clause (TypeError)
        from test.rb:1:in `<main>'
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ rbenv local 2.2.4 
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ ruby test.rb 
test.rb:3:in `rescue in <main>': class or module required for rescue clause (TypeError)
        from test.rb:1:in `<main>'
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ rbenv local 2.3.0 
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ ruby test.rb 
test.rb:3:in `rescue in <main>': class or module required for rescue clause (TypeError)
        from test.rb:1:in `<main>'

According to the output of rubocop -V that you posted, it looks like you are running Ruby 2.3.0. Can you verify what version you are running against?

I can update this cop to account for rescue nil. Looking at the output running of against Ruby 1.9.3, it appears that rescue nil does not actually rescue anything. I would like to do a bit more research on this, and it seems like we should have a cop that recommends against rescue nil. This may prove to be a bit tricky because rescue nil is valid in the modifier form.

@janraasch
Copy link
Contributor Author

Sorry, I think I misunderstood what you meant by

it does not appear to be valid Ruby.

To clarify, writing

begin
  1/0
rescue nil
  puts 'rescued'
end

is perfectly correct from a syntax perspective.

However, it does raise a TypeError, when the rescue clause is called.

So you are probably right, it is an edge-case. We just ran into this because of a code-smell. Essentially we had something like

begin
  puts('This never fails')
  puts('Thus we never run into the TypeError`)
rescue nil
  puts('rescued')
end

in our code. So we never ran into the TypeError and I only noticed this, when running your Cop against our codebase. So maybe we should just have a cop to guard against rescue nil here. Not sure if something like this is in scope for rubocop.

@rrosenblum
Copy link
Contributor

Thank you for the clarification, I believe that we are on the same page now. Since this does not produce a TypeError in Ruby 1.9.3, we will need to add a safeguard to this cop.

RuboCop can check for rescue nil and recommend against using it. We will need to do some cleanup around how we check for a rescue modifier in cops other than Style/RescueModifier.

rrosenblum added a commit to rrosenblum/rubocop that referenced this issue Jun 29, 2016
Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016
bquorning added a commit to bquorning/rubocop that referenced this issue Nov 27, 2018
The "non-inline" `rescue nil` was allowed in Ruby 1.9, but disallowed since 2.0:
rubocop#3249 (comment)

A bit of the code handling `rescue nil` was removed from this cop in
7ef0cf4 (rubocop#4846),
but there was still a bit left behind, plus a few specs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants