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

Performance/RegexpMatch not safe for variables that may be numeric #355

Open
formigarafa opened this issue May 4, 2023 · 2 comments
Open

Comments

@formigarafa
Copy link

Numerics can be used with =~ operator but not be used as argument for #match?
The code transformation applied can break the code.


For example if it takes a file with this content:

def has_an_o?(arg)
  if arg =~ /o/i
    true
  else
    false
  end
end

has_an_o?("one") # => true
has_an_o?(1) # => false

The file contents is turned into this

def has_an_o?(arg)
  if /o/i.match?(arg)
    true
  else
    false
  end
end

has_an_o?("one") # => true
has_an_o?(1) # => :2:in `match?': no implicit conversion of Integer into String (TypeError)

Which breaks as seen above.

RuboCop version

1.50.2 (using Parser 3.2.2.1, rubocop-ast 1.28.1, running on ruby 3.0.6) [x86_64-darwin22]

  • rubocop-performance 1.14.3
  • rubocop-rails 2.15.2
@koic
Copy link
Member

koic commented May 6, 2023

Note, the behavior of Object#=~ has changed in Ruby 3.2:

$ ruby -ve '42 =~ /regexp/'
ruby 3.1.4p223 (2023-03-30 revision 957bb7cb81) [x86_64-darwin19]
-e:1: warning: deprecated Object#=~ is called on Integer; it always returns nil

$ ruby -ve '42 =~ /regexp/'
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-darwin19]
-e:1:in `<main>': undefined method `=~' for 42:Integer (NoMethodError)

@formigarafa
Copy link
Author

Ah, that's really good to know.
I will get to adjust that before get to version 3.2.

My config is set to 3.0 for now, though. I am using:

AllCops:
  TargetRubyVersion: 3.0

Would there be a way to mark a cop conditionally safe by TargetRubyVersion or is this a bad idea somehow?
I do understand it will be better getting the code shape to be ready for future versions, but I believe as me most developers may be disappointed by the tool when we are trying to get things right and the tool works against it.

BTW, I am asking if it is a good or bad idea so I dig further and help making a PR. I took a quick look at the code and it will take me a while to get there. I am just trying to save my time if it may be a silly idea.

Earlopain added a commit to Earlopain/rubocop-performance that referenced this issue Feb 29, 2024
This is related to but not the same as rubocop#355

```rb
x = 'foo'
puts 'OK' if x =~ /foo/

x = nil
puts 'BOOM' if x.match?(/foo/)
# => undefined method `match?' for nil (NoMethodError)
```
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

No branches or pull requests

2 participants