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: Does not consider ::Regexp.last_match(1) #314

Closed
DannyBen opened this issue Nov 19, 2022 · 5 comments · Fixed by #320
Closed

Performance/RegexpMatch: Does not consider ::Regexp.last_match(1) #314

DannyBen opened this issue Nov 19, 2022 · 5 comments · Fixed by #320

Comments

@DannyBen
Copy link

I noticed a couple of issues with this cop.

  1. It does not recognize ::Regexp.last_match(1) as use of MatchData
  2. It gets confused and assumes use of MatchData if a following unrelated line mentions MatchData-like statements.

Expected behavior

Expectation 1: ::Regexp.last_match(1) should be detected as MatchData use

Expecting the cop detect ::Regexp.last_match(1) as use of MatchData and not trigger an offense (in the same way it detects Regexp.last_match(1) or $1).

# Expected not to fail
::Regexp.last_match(1) if string =~ /hello (.*)/

Expectation 2: Offense should be reported

The first line below does not use MatchData, the cop is expected to report an offense. Removing the second (unrelated) line makes it work as expected.

p 'hello' if string =~ /hello (.*)/
p $1 if string =~ /hello (.*)/

Actual behavior

This code triggers an offense where it shouldn't:

p ::Regexp.last_match(1) if string =~ /hello (.*)/

This code does not trigger an offense, where it should:

p 'hello' if string =~ /hello (.*)/
p $1 if string =~ /hello (.*)/

Steps to reproduce the problem

# .rubocop.yml

require: rubocop-performance

AllCops:
  NewCops: enable
  TargetRubyVersion: 3.0
# test.rb

string = 'hello bob'

# FAIL: Offense is reported on the last line. The first two lines pass as
#       expected
p $1 if string =~ /hello (.*)/
p Regexp.last_match(1) if string =~ /hello (.*)/
p ::Regexp.last_match(1) if string =~ /hello (.*)/

# FAIL: Offense is NOT reported on first line since we use $1 in the
#       unrelated next line
p 'hello' if string =~ /hello (.*)/
p $1 if string =~ /hello (.*)/

then run rubocop --only Performance and get this output (trimmed for readability):

➜ rubocop --only Performance
test.rb:7:29: C: [Correctable] Performance/RegexpMatch: Use match? instead of =~ when MatchData is not used.
p ::Regexp.last_match(1) if string =~ /hello (.*)/
                            ^^^^^^^^^^^^^^^^^^^^^^

Additional consideration

Note that normally I am using $1 and not Regexp.last_match (and definitely not ::Regexp.last_match), but the native Style/PerlBackrefs cop autocorrected my code to ::Regexp.last_match.

The point being - if one cop is saying "use ::Regexp.last_match", then the other cop should be aware of it.

RuboCop version

$ rubocop -V
1.39.0 (using Parser 3.1.2.1, rubocop-ast 1.23.0, running on ruby 3.1.0) [x86_64-linux]
  - rubocop-performance 1.15.1
@fatkodima
Copy link
Contributor

This code does not trigger an offense, where it should:

p 'hello' if string =~ /hello (.)/
p $1 if string =~ /hello (.
)/

For the second case it should not trigger an offense, because of $1.

@DannyBen
Copy link
Author

DannyBen commented Dec 1, 2022

For the second case it should not trigger an offense, because of $1.

Consider the two lines together. The first should trigger an offense, but it does not since there is an unrelated $1 beneath it. Switching the order of the lines does trigger. Or is it by design that any subsequent line with $1 is considered as part of the "block"?

@fatkodima
Copy link
Contributor

Or is it by design that any subsequent line with $1 is considered as part of the "block"?

Seems like yes.

@DannyBen
Copy link
Author

DannyBen commented Dec 1, 2022

Seems like yes.

I understand the reasoning.
But the first issue is a bug, yes? the ::Regexp.last_match(1).

@fatkodima
Copy link
Contributor

Yes, I opened a PR with the fix.

@koic koic closed this as completed in #320 Dec 1, 2022
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