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

Remove Ruby 1.9 code from Lint/ShadowedException cop #6527

Merged
merged 1 commit into from Nov 30, 2018

Conversation

bquorning
Copy link
Contributor

The "non-inline" rescue nil was allowed in Ruby 1.9, but disallowed since 2.0: #3249 (comment)

A bit of the code handling rescue nil was removed from this cop in 7ef0cf4 (#4846), but there was still a bit left behind, plus a few specs.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

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.
@bquorning bquorning requested a review from koic November 27, 2018 21:25
@bquorning
Copy link
Contributor Author

/cc @rrosenblum

Copy link
Collaborator

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 💎

Copy link
Contributor

@rrosenblum rrosenblum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself looks fine to me. However, supporting rescue nil was included in here to prevent possible errors or exceptions in our own code. rescue nil does not work in newer versions of Ruby, but it results in a TypeError during execution. Nothing prevents a user from writing rescue nil. This is the reason that the Lint/RescueType cop exists. Handling rescue nil in this cop is an edge case.

@bquorning
Copy link
Contributor Author

Do you mean it’s possible to get a TypeError during a RuboCop run?

@rrosenblum
Copy link
Contributor

No, you won't get a TypeError during a RuboCop run. The TypeError happens at run time of the user's code. If I remember correctly, there was an issue comparing nil with an exception class in RuboCop.

I assume that the tests that were removed will fail with this code change. Handling rescue nil is really here for the edge case of if a user happens to be rescuing nil. This cop will still function properly. Since it is an edge case, if we think the chances of someone still having rescue nil are slim, then we are good to merge this in. If we want to ensure this cop works with bad code, then we should leave it in.

If a user has Lint/RescueType disabled, it is possible for occurrences of rescue nil to be in someone's code base. In that situation, this cop will run into issues.

@bquorning
Copy link
Contributor Author

bquorning commented Nov 28, 2018

Ah, gotcha. I just checked, and the cop still works with rescue nil, and even with rescue 1, 'a', "#{42}", 0.0, [], {}, nil (inspired by a Lint/RescueType spec).

@rrosenblum
Copy link
Contributor

Awesome, that was my main concern.

Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the discussion and investigation. I fully agree ❤️

@koic koic merged commit edf51f0 into rubocop:master Nov 30, 2018
@bquorning bquorning deleted the rescue-nil-was-ruby-1.9-only branch November 30, 2018 05:42
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 this pull request may close these issues.

None yet

4 participants