Add new `RSpec/DiscardedMatcher` cop by ydakuka · Pull Request #2163 · rubocop/rubocop-rspec

ydakuka · 2026-02-21T15:33:11Z

Fixes rubocop/rubocop#14772

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 · 2026-02-21T21:02:38Z

lib/rubocop/cop/rspec/discarded_matcher.rb

+      #     expect { result }.to \
+      #       change { obj.foo }.from(1).to(2)


expect { ... } .to ...

is more common?
But it does make the discarded matcher problem more obvious.

expect { result } .to change { obj.foo }.from(1).to(2) change { obj.bar }.from(3).to(4)

quite easy to spot.

pirj · 2026-02-21T21:04:48Z

lib/rubocop/cop/rspec/discarded_matcher.rb

+          change
+          receive
+          receive_messages
+          receive_message_chain


Is it correct to say that any block matcher is prone to this?
Can we externalize to our language conf this and make it user-configurable, too to allow adding custom matchers?

pirj · 2026-02-21T21:07:55Z

lib/rubocop/cop/rspec/discarded_matcher.rb

+        def inside_example?(node)
+          node.each_ancestor(:block).any? { |ancestor| example?(ancestor) }
+        end


Now when it's used in three cops, does it make sense to extract to a module?

pirj · 2026-02-21T21:11:14Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+
+  it 'registers an offense for standalone `change` in a one-liner example' do
+    expect_offense(<<~RUBY)
+      specify { change { obj.bar }.from(1).to(2) }


I've seen this a couple of times with value matchers, too:

it { validate_precense_of(:email) }

But it should be detected already by https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecnoexpectationexample

pirj · 2026-02-21T21:13:51Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+  it 'does not register an offense for `change` at end of non-example block' do
+    expect_no_offenses(<<~RUBY)
+      specify do
+        [1, 2, 3].each do |n|
+          change { obj.foo }.from(n).to(n + 1)
+        end
+      end
+    RUBY
+  end


Are there legitimate cases for which we want to ignore this?

pirj · 2026-02-21T21:16:24Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+      specify do
+        if change { obj.foo }.from(1).to(2)
+          something
+        end


Why would anyone do this?

pirj · 2026-02-21T21:18:36Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+      specify do
+        case change { obj.foo }.from(1).to(2)
+        when 1 then something
+        end
+      end


I can't imagine a real spec using this

pirj · 2026-02-21T21:21:30Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+        result = case condition
+                 when :update then something
+                 else change { obj.baz }.from(5).to(6)
+                 end


This seems like a probable scenario e.g.

change_temp = case season when :summer then change { temp }.from(0).to(20) when :winter then change { temp }.from(20).to(0) end expect { advance_clock }.to change_temp

pirj · 2026-02-21T21:23:18Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+      specify do
+        case condition
+        when change { obj.foo }.from(1).to(2) then something
+        end
+      end


Does it make sense to simplify the cop to only detect cases when a matcher follows the expect?

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

pirj · 2026-02-21T21:31:49Z

Looks very good and userful overall. My comments here are mostly to suggest narrowing down the scope to detecting just the basic case. I have no objections to merging as is as well if you think weird use cases can cause false offences.

ydakuka · 2026-02-21T21:38:42Z

Thank you for your comments! I’ll take them into account and make the necessary updates as soon as possible.

pirj · 2026-02-21T21:51:04Z

rubocop --only RSpec/DiscardedMatcher ../real-world-rspec

Legit offence (source):

/Users/pirj/source/real-world-rspec/gitlabhq/spec/services/event_create_service_spec.rb:186:9: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
        change { Event.approved_action.where(target: merge_request).count }.by(1)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

False positive? (source)

/Users/pirj/source/real-world-rspec/cartodb/spec/gears/carto_gears_api/users_service_spec.rb:59:17: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
          }.to (change { @user.reload.crypted_password })
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/Users/pirj/source/real-world-rspec/mastodon/spec/models/admin/account_action_spec.rb:86:14: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
        .to (change(Admin::ActionLog.where(action: type), :count).by 1)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/Users/pirj/source/real-world-rspec/cartodb/spec/gears/carto_gears_api/users_service_spec.rb:65:17: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
          }.to (change { @user.reload.last_password_change_date })
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I believe it's similar to the last year's story from RubyWeekly about a whitespace betweeen the method name and parenthesis. The AST is different. But for RSpec, there's no difference, is there?

Those look like false positives (source):

/Users/pirj/source/real-world-rspec/gitlabhq/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb:75:7: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
      change.non_housekeeper_changes = [:code]
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb:80:7: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
      change.non_housekeeper_changes = [:title]
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb:87:7: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
      change.non_housekeeper_changes = [:code, :approvals]
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb:92:7: C: RSpec/DiscardedMatcher: The result of change is not used. Did you mean to chain it with .and?
      change.non_housekeeper_changes = [:code, :title]
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

ydakuka · 2026-02-28T21:24:10Z

I’ve addressed all comments, added a few follow-up commits (kept them unsquashed for now), and validated the cop against real-world-rspec/ and real-world-rails/. Could you please take another look when you have time?

pirj

I've run the cop against real-world-rspec (I've updated its submodules with the latest), and the cop detected zero offences. Is that right?

The gitlabhq/spec/services/event_create_service_spec.rb still has the offence.

pirj · 2026-03-01T14:25:47Z

spec/rubocop/cop/rspec/discarded_matcher_spec.rb

+      specify do
+        def expect_action
+          expect { action!(performer: performer, context: context) }
+        end
+


Suggested change

specify do

def expect_action

expect { action!(performer: performer, context: context) }

end

def expect_action

expect { action!(performer: performer, context: context) }

end

specify do

ydakuka · 2026-03-01T14:39:41Z

I've run the cop against real-world-rspec (I've updated its submodules with the latest), and the cop detected zero offences. Is that right?

The gitlabhq/spec/services/event_create_service_spec.rb still has the offence.

As you mentioned above, detecting that offense is not the responsibility of this cop. Therefore, I only take into account code that already contains expect / is_expected with to/ not_to/ to_not. Probably I need to add more test examples to demonstrate variety.

$ RUBOCOP_TARGET_RUBY_VERSION=3.4 rubocop ../real-world-rspec/gitlabhq/spec/services/event_create_service_spec.rb  -c /dev/null --plugin rubocop-rspec --only RSpec/NoExpectationExample

Offenses:

/home/ydakuka/Work/real-world-rspec/gitlabhq/spec/services/event_create_service_spec.rb:183:7: C: RSpec/NoExpectationExample: No expectation found in this example.
      it 'creates new event' do ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

I also checked the cop against the examples from the issue description, and it works for me.

pirj

Thank you!

ydakuka requested a review from a team as a code owner February 21, 2026 15:33

ydakuka force-pushed the add-new-rspec-discarded-matcher branch from 46f7545 to 680ff12 Compare February 21, 2026 15:41

pirj reviewed Feb 21, 2026

View reviewed changes

spec/rubocop/cop/rspec/discarded_matcher_spec.rb Show resolved Hide resolved

ydakuka added 10 commits February 28, 2026 13:52

Add new RSpec/DiscardedMatcher cop

f37ec94

Make the example more readable

1bc283e

Make block matchers configurable via MatcherMethods

e8ee426

Add InsideExample mixin

e0a11c4

Remove an unreal example

e81684b

Add example

23e091a

Add #check_discarded_matcher

357f8ee

Narrow DiscardedMatcher to examples with expectations

fff745e

Remove unused conditions

3dd584c

Fix false negatives/positives

9c86416

ydakuka force-pushed the add-new-rspec-discarded-matcher branch from 680ff12 to 5d7a24f Compare February 28, 2026 21:12

Update docs

68bfe98

ydakuka force-pushed the add-new-rspec-discarded-matcher branch from 5d7a24f to 68bfe98 Compare February 28, 2026 21:19

pirj reviewed Mar 1, 2026

View reviewed changes

pirj approved these changes Mar 2, 2026

View reviewed changes

Uh oh!

Conversation

ydakuka commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pirj commented Feb 21, 2026

Uh oh!

ydakuka commented Feb 21, 2026

Uh oh!

pirj commented Feb 21, 2026

Uh oh!

ydakuka commented Feb 28, 2026

Uh oh!

pirj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydakuka commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pirj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ydakuka commented Feb 21, 2026 •

edited

Loading

ydakuka commented Mar 1, 2026 •

edited

Loading