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

Fix Style/CaseLikeIf not properly handling overriden equality methods #8624

Merged
merged 1 commit into from Aug 31, 2020

Conversation

Skipants
Copy link

@Skipants Skipants commented Aug 31, 2020

If a class overrides one of #==, #eql?, or equal? and the overridden method has no arguments, then CaseLikeIf#find_target_in_equality_node would error as it assumes there is at least one argument.

Without the fix, the new spec returns an error:

➜  rubocop (master) ✔ bundle exec rspec spec/rubocop/cop/style/case_like_if_spec.rb:340
Run options: include {:focus=>true, :locations=>{"./spec/rubocop/cop/style/case_like_if_spec.rb"=>[340]}}

Randomized with seed 42434
F

Failures:

  1) RuboCop::Cop::Style::CaseLikeIf does not register an offense when an object overrides `equal?` with no arity
     Failure/Error: if argument.literal? || const_reference?(argument)

     NoMethodError:
       undefined method `literal?' for nil:NilClass
     # ./lib/rubocop/cop/style/case_like_if.rb:121:in `find_target_in_equality_node'
     # ./lib/rubocop/cop/style/case_like_if.rb:105:in `find_target_in_send_node'
     # ./lib/rubocop/cop/style/case_like_if.rb:95:in `find_target'
     # ./lib/rubocop/cop/style/case_like_if.rb:39:in `on_if'
     # ./lib/rubocop/cop/commissioner.rb:99:in `block (2 levels) in trigger_responding_cops'
     # ./lib/rubocop/cop/commissioner.rb:152:in `with_cop_error_handling'
     # ./lib/rubocop/cop/commissioner.rb:98:in `block in trigger_responding_cops'
     # ./lib/rubocop/cop/commissioner.rb:97:in `each'
     # ./lib/rubocop/cop/commissioner.rb:97:in `trigger_responding_cops'
     # ./lib/rubocop/cop/commissioner.rb:70:in `on_if'
     # ./lib/rubocop/cop/commissioner.rb:85:in `investigate'
     # ./lib/rubocop/cop/team.rb:152:in `investigate_partial'
     # ./lib/rubocop/cop/team.rb:83:in `investigate'
     # ./lib/rubocop/rspec/cop_helper.rb:54:in `_investigate'
     # ./lib/rubocop/rspec/cop_helper.rb:25:in `inspect_source'
     # ./lib/rubocop/rspec/expect_offense.rb:187:in `expect_no_offenses'
     # ./spec/rubocop/cop/style/case_like_if_spec.rb:341:in `block (2 levels) in <top (required)>'

Finished in 0.03323 seconds (files took 1.35 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/rubocop/cop/style/case_like_if_spec.rb:340 # RuboCop::Cop::Style::CaseLikeIf does not register an offense when an object overrides `equal?` with no arity

Randomized with seed 42434

I doubt you'll see much of == being overridden in the wild, but this particular case was found with equal?. An object in my work's app takes in a bunch of data in its constructor and equal? is overridden and used to verify some criterion on it.


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.

@Skipants Skipants force-pushed the master branch 3 times, most recently from 47b35ed to a5fef3f Compare August 31, 2020 18:35
If a class overrides one of `#==`, `#eql?`, or `equal?` and the
overridden method has no arguments, then
CaseLikeIf#find_target_in_equality_node would error as it assumes there
is at least one argument.
@koic koic merged commit f69a7a8 into rubocop:master Aug 31, 2020
@koic
Copy link
Member

koic commented Aug 31, 2020

Nice patch! Thank you!

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

2 participants