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

Add {Assert,Refute}Match OnlyRegexpLiteral config #323

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Oct 4, 2024

AssertMatch & RefuteMatch enforce

-assert matcher.match?(object)
+assert_match(matcher, object)

and

-refute matcher.match?(object)
+refute_match(matcher, object)

respectively, which use =~ under the hood. This makes the cops highly susceptible to false positives, in cases where the matcher object responds to match?, but not to =~

class SuitColorMatcher
  BLACK = [:clubs,    :spades]
  RED   = [:diamonds, :hearts]
  SUITS = BLACK + RED

  def initialize(suit)
    raise ArgumentError, suit.inspect unless SUITS.include?(suit)
    @suit = suit
  end

  def match?(other)
    case @suit
    when *BLACK then BLACK.include?(other)
    when *RED   then RED  .include?(other)
    end
  end
end

SuitColorMatcher.new(:clubs).match?(:spades) # => true
SuitColorMatcher.new(:clubs) =~ :spades      # NoMethodError

or cases where the matcher object does also respond to =~, but in an incompatible way

"".match? "" # => true
"" =~ ""     # TypeError

Given RuboCop doesn't have runtime type information, the only case we can reliably identify is the case where a Regexp literal is used.

Therefore, this adds an OnlyRegexpLiteral config (false by default), which consumers can enable if their codebase has many false positives.

This provides a way for consumers to address #310, without changing the cop's behavior for everyone.

As part of making this change, a common module is extracted for the implementations of AssertMatch and RefuteMatch, which are largely identical.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
    I wasn't sure about this, since this PR isn't changing/fixing the default behavior, it's just adding an option which allows consumers to fix the behavior.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
    Likewise, I tagged the change as "new", since it's an new option, and consumers have to opt-in to the new behavior. We could eventually make it the default though.

These two cops have near identical implementations. Extracting them
allows us to make further changes in a single place.
`AssertMatch` & `RefuteMatch` enforce that

    assert matcher.match?(object)
    refute matcher.match?(object)

be replaced with

    assert_match(matcher, object)
    refute_match(matcher, object)

respectively, which use `=~` under the hood. This makes the cops highly
succeptible to false positives, in cases where the matcher object
responds to `match?`, but not to `=~`

   class SuitColorMatcher
     BLACK = [:clubs,    :spades]
     RED   = [:diamonds, :hearts]
     SUITS = BLACK + RED

     def initialize(suit)
       raise ArgumentError, suit.inspect unless SUITS.include?(suit)
       @suit = suit
     end

     def match?(other)
       case @suit
       when *BLACK then BLACK.include?(other)
       when *RED   then RED  .include?(other)
       end
     end
   end

   SuitColorMatcher.new(:clubs).match?(:spades) # => true
   SuitColorMatcher.new(:clubs) =~ :spades      # NoMethodError

or cases where the matcher object does also respond to `=~`, but in an
incompatible way

    "".match? "" # => true
    "" =~ ""     # TypeError

Given RuboCop doesn't have runtime type information, the only case we
can reliably identify is the case where a `Regexp` literal is used.

Therefore, this adds an `OnlyRegexpLiteral` config (false by default),
which consumers can enable if their codebase has many false positives.
@sambostock sambostock force-pushed the assert-refute-match-only-regexp-literal branch from d79f892 to 909b1e9 Compare October 4, 2024 20:00
@sambostock sambostock marked this pull request as ready for review October 4, 2024 20:02
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 this pull request may close these issues.

1 participant