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 enough global variables recognised #4589

Closed
sj26 opened this issue Jul 10, 2017 · 4 comments
Closed

Performance/RegexpMatch: not enough global variables recognised #4589

sj26 opened this issue Jul 10, 2017 · 4 comments
Labels

Comments

@sj26
Copy link

sj26 commented Jul 10, 2017

Rubocop falsely reports an offense for the RegexpMatch cop on the following code:

if commit_info =~ /^commit \s* ([a-f0-9]+)$/xi
  info[:commit] = $1
end
code.rb:40:8: C: Use match? instead of =~ when MatchData is not used.
if commit_info =~ /^commit \s* ([a-f0-9]+)$/xi
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The match data is being used by $1.

To my eye this looks like the numbered global match variables ($1, $2, ...) need to be added to RegexpMatch#match_gvar?.


Expected behavior

No offense reported.

Actual behavior

Offense reported.

Steps to reproduce the problem

Use a Regexp match and global match variables like the example above.

RuboCop version

$ rubocop -V
0.49.1 (using Parser 2.4.0.0, running on ruby 2.4.1 x86_64-darwin16)
@pocke pocke added the bug label Jul 10, 2017
@pocke
Copy link
Collaborator

pocke commented Jul 10, 2017

Thanks for your report.

I can't reproduce this problem.

test.rb

if commit_info =~ /^commit \s* ([a-f0-9]+)$/xi
  info[:commit] = $1
end

.rubocop.yml

AllCops:
  TargetRubyVersion: 2.4
$ rubocop --only RegexpMatch --cache false
Inspecting 1 file
.

1 file inspected, no offenses detected

To my eye this looks like the numbered global match variables ($1, $2, ...) need to be added to RegexpMatch#match_gvar?.

This gvar is not parsed as gvar, it is parsed as nth-ref.
For example:

$ ruby-parse test.rb
(if
  (send
    (send nil :commit_info) :=~
    (regexp
      (str "^commit \\s* ([a-f0-9]+)$")
      (regopt :i :x)))
  (send
    (send nil :info) :[]=
    (sym :commit)
    (nth-ref 1)) nil)

And rubocop handle nth-ref. https://github.com/bbatsov/rubocop/blob/d87cea2cbaef494df11375900503784b5bc34859/lib/rubocop/cop/performance/regexp_match.rb#L106

Could you give me ruby-parse command output? This command is installed if rubocop is installed.

@sj26
Copy link
Author

sj26 commented Jul 10, 2017

Apologies, that was a snippet from a larger file. When I reduced the file to only the three lines I supplied it doesn't report an offense. But if I add a little more of the context up to a class method it seems to trigger the problem:

# frozen_string_literal: true
class MyClass
  def self.parse(commit_info)
    if commit_info =~ /^commit \s* ([a-f0-9]+)$/xi
      return $1
    end
  end
end
$ rubocop --only RegexpMatch --cache false app/models/my_class.rb
Inspecting 1 file
C

Offenses:

app/models/my_class.rb:4:8: C: Use match? instead of =~ when MatchData is not used.
    if commit_info =~ /^commit \s* ([a-f0-9]+)$/xi
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected
$ ruby-parse app/models/my_class.rb
warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
(class
  (const nil :MyClass) nil
  (defs
    (self) :parse
    (args
      (arg :commit_info))
    (if
      (send
        (lvar :commit_info) :=~
        (regexp
          (str "^commit \\s* ([a-f0-9]+)$")
          (regopt :i :x)))
      (return
        (nth-ref 1)) nil)))

@pocke
Copy link
Collaborator

pocke commented Jul 10, 2017

Thanks. I can reproduce it. 👍
I'll work on it.

@sj26
Copy link
Author

sj26 commented Jul 10, 2017

Thank you! 🙇🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants