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 #5038] Fix false positive for `Performance/RegexpMatch` cop #5569

Conversation

Projects
None yet
3 participants
@koic
Copy link
Member

commented Feb 13, 2018

Fixes #5038.

This PR fixes a false positive for Performance::RegexpMatch when using MatchData before guard clause.

Perhaps other false positives may be considered, but this PR will first solve the problem reported in the above issue.


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 rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

@koic koic force-pushed the koic:fix_false_positive_for_performance_regexp_match_cop branch from e665d53 to 1ed74e7 Feb 13, 2018

@pocke pocke self-requested a review Feb 14, 2018

@@ -199,6 +202,10 @@ def scope_root(node)
end
end

def modifier_if_or_unless?(source, condition)
source =~ Regexp.new("(if|unless) +#{Regexp.escape(condition)}\\z")
end

This comment has been minimized.

This comment has been minimized.

Copy link
@koic

koic Feb 16, 2018

Author Member

Thanks for the advice. I fixed it.

@@ -154,6 +154,9 @@ def message(node)
def last_match_used?(match_node)
scope_root = scope_root(match_node)
body = scope_root ? scope_body(scope_root) : match_node.ancestors.last

return true if modifier_if_or_unless?(body.source, match_node.source)

This comment has been minimized.

Copy link
@pocke

pocke Feb 16, 2018

Member

I guess the implementation has a false positive. The cop will ignore the following code.

def foo
  return if /re/ =~ str
  do_something Regexp.last_match[1]
end

This comment has been minimized.

Copy link
@koic

koic Feb 16, 2018

Author Member

I'm not sure I understand this problem.
Is it this problem even at the time of RuboCop 0.52.1?
I don't understand the expected behavior and the actual behavior 💦

@koic koic force-pushed the koic:fix_false_positive_for_performance_regexp_match_cop branch from 1ed74e7 to 9fa4a74 Feb 16, 2018

[Fix #5038] Fix false positive for `Performance/RegexpMatch` cop
Fixes #5038.

This PR fixes a false positive for `Performance::RegexpMatch` when
using `MatchData` before guard clause.

Perhaps other false positives may be considered, but this PR will first
solve the problem reported in the above issue.

@koic koic force-pushed the koic:fix_false_positive_for_performance_regexp_match_cop branch from 9fa4a74 to d49652e Feb 16, 2018

@bbatsov bbatsov merged commit 4f4a241 into rubocop-hq:master Feb 18, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2018

👍

@koic koic deleted the koic:fix_false_positive_for_performance_regexp_match_cop branch Feb 18, 2018

satyap pushed a commit to satyap/rubocop that referenced this pull request Feb 24, 2018

[Fix rubocop-hq#5038] Fix false positive for `Performance/RegexpMatch…
…` cop (rubocop-hq#5569)

Fixes rubocop-hq#5038.

This PR fixes a false positive for `Performance::RegexpMatch` when
using `MatchData` before guard clause.

Perhaps other false positives may be considered, but this PR will first
solve the problem reported in the above issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.