Mark RSpec/RedundantPredicateMatcher as Safe: false

[koic]

RSpec/RedundantPredicateMatcher cop is marked as unsafe because false positives occur when be_start_with is used as a legitimate predicate matcher for start_with? method.

e.g., rubocop/rubocop-ast@f9e6682

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[pirj]

We’ve had this discussion recently with @bquorning and I still think that we should not mark cops as unsafe for any period of time for a reason of a bug causing incorrect inspection or autocorrection. See #1687. I don’t think we’ll ever fix those issues by muting them.

[bquorning]

Maybe I misunderstood the issue. But we have a spec for correcting be_start_with to start_with.

If you don’t want be_start_with detected or corrected, perhaps disable the cop.

[pirj]

The problem as I understand it is that rspec-expectations’ start_with/end_with matcher won’t work for ProcessedSource custom implementation of start_with?/end_with?
I suggest to still flag those as potential offences, but not autocorrect those two matchers.
However, let’s stretch our imagination as far as overriding respond_to? in a way that the respond_to matcher won’t work. Should we consider this?

Why doesn’t the ProcessedSource’s implementation work with the regular matcher?

[bquorning]

The regular matcher seems to not call #start_with? but rather #[] with two arguments (start and length). Here’s the source for those methods in ProcessedSource:

      def [](*args)
        lines[*args]
      end

      def start_with?(string)
        return false if self[0].nil?

        self[0].start_with?(string)
      end

It seems that #[] will return lines, and #start_with? will compare characters. This is not compatible with the String implementation of those two methods that rspec-expectations seem to expect.

And yes, we should probably consider if other of the RedundantPredicateMatcher corrections are unsafe too.

I would suggest changing the specs to e.g. expect(processed_source.start_with?('start')).to be(false), I don’t think the predicate matchers does anything good for readability in this case.

[pirj]

Speaking in defence of the RSpec matchers in wuestion, they support types beyond just strings.

The decision to (manually) delegate the start_with? to the first line is justified. Even though it may be surprising if used with a multi-line string/regexp.

There’s yet another thing that makes me suspicious regarding starts_with?, and it’s this processing. Is this a potential performance optimization?

[tdeo]

I was coming across another unsafe autocorrection of this cop, typically in the following case:

class BusinessRule < ApplicationRecord
  def match?(business_object)
    # check if the rule is applicable to the object
  end
end

describe BusinessRule do
  describe '#matches?' do
    context 'when the object is relevant' do
      it 'returns true' do
        expect(business_rule).to be_match(business_object)
      end
    end
  end
end

which gets wrongly autocorrected.

Since people may define any kind of method like def all?, this cop will never be a safe autocorrection

[tdeo]

@pirj could you please elaborate a bit on the decision to close this while there seem to be known unsafe cases? This one seems definitely unsafe by design and not because of a bug or improper handling of edge-cases.

[pirj]

We can trick cops into suggesting contextually undafe corrections by using the near-infinite flexibility of Ruby.
I disagree that we should defensively mark cops as unsafe for all such situations.

I admit that I am biased against the example in question, and the code looks sub-par. Also marking cops unsafe is my pet peeve.
You may convince me in this specific situation with more examples, but at the moment I have a gut feeling that the current state of affairs is how it should remain.

[pirj]

I admit I have better things to apply my time than to start a RuboCop-wide discussion on the unsafeness topic and to refactor rubocop-ast. I wish someone did that for me. In the meanwhile I’ll try to defend the RSpec department from being disabled one by one. I hope noone will think I’m challenging them 😄

[tdeo]

We can trick cops into suggesting contextually undafe corrections by using the near-infinite flexibility of Ruby.

Agreed that you can probably make every cop unsafe if you try to figure out the specific counter-example that will break things.

I disagree that we should defensively mark cops as unsafe for all such situations.

But we're several people having ran into that without trying at all

I admit that I am biased against the example in question, and the code looks sub-par. Also marking cops unsafe is my pet peeve. You may convince me in this specific situation with more examples, but at the moment I have a gut feeling that the current state of affairs is how it should remain.
I admit I have better things to apply my time than to start a RuboCop-wide discussion on the unsafeness topic and to refactor rubocop-ast. I wish someone did that for me. In the meanwhile I’ll try to defend the RSpec department from being disabled one by one. I hope noone will think I’m challenging them 😄

I gladly admit that the advantage with invalid autocorrections of RSpec cops is that the tests break instead of production code 😅

[pirj]

ran into that without trying at all

This happens with existing code. I would prefer fixing the implementation of ProcessedSource.
For the code created in the future, the cop will cue towards an implementation that is more compatible to String/Hash etc.

tests break instead of production code

Good point. Sometime tests may become pass-all deceptive, though.