Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Performance/StringInclude autocorrect is unsafe #145

Closed
beauraF opened this issue Jul 7, 2020 · 2 comments · Fixed by #153
Closed

Performance/StringInclude autocorrect is unsafe #145

beauraF opened this issue Jul 7, 2020 · 2 comments · Fixed by #153
Labels
bug Something isn't working

Comments

@beauraF
Copy link

beauraF commented Jul 7, 2020

Hi,

New cop Performance/StringInclude introduced by #117 is unsafe when use with --auto-correct.

  1. It currently auto-corrects the usage of match and =~. But the return values between match or =~ and include? are different. If the value is used, the behavior is altered.
  2. =~ doesn't raise an error in case of nil =~ /foobar/. Same, behavior is altered.

Two examples that broke in our codebase:

search_by_street_address = params[:types] =~ /street_address/
@agenda_requested_in_config ||= method.match(/agenda/)

I think it should only fix match? and reserve match and =~ in the case of --auto-correct-all. I don't know if you can make that distinction?

If not, maybe move this cop as unsafe? What do you think?

@koic koic added the bug Something isn't working label Jul 8, 2020
@viraptor
Copy link

viraptor commented Jul 9, 2020

Adding another edge case - =~ works on symbols, but .include? doesn't:

irb(main):002:0> :foo =~ /o/
=> 1
irb(main):003:0> :foo.include?('o')
Traceback (most recent call last):
...

@eugeneius
Copy link
Contributor

Note that Performance/StartWith and Performance/EndWith, which Performance/StringInclude was based on, have these problems too. The issue with nil receivers was previously reported in #50.

koic added a commit to koic/rubocop-performance that referenced this issue Jul 16, 2020
Fixes rubocop#145.

This PR marks `Performance/StringInclude` as `SafeAutocorrect: false`
and disable autocorrect by default.

The cop's offenses are not safe to auto-correct if a receiver is nil.
@koic koic closed this as completed in #153 Jul 17, 2020
koic added a commit that referenced this issue Jul 17, 2020
…orrect

[Fix #145] Mark `Performance/StringInclude` as unsafe
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants