diff --git a/.rubocop.yml b/.rubocop.yml index 59c5d81f4..e323635f6 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -111,6 +111,8 @@ RSpec/DescribeClass: Exclude: - spec/project/**/*.rb +RSpec/DiscardedMatcher: {Enabled: true} + RSpec/ExampleLength: CountAsOne: - heredoc diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a01452ef..acc2bae5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Fix a false positive for `RSpec/ScatteredSetup` when the hook is defined inside a class method. ([@d4rky-pl]) - Fix a false positive for `RSpec/DescribedClass` inside dynamically evaluated blocks (`class_eval`, `module_eval`, `instance_eval`, `class_exec`, `module_exec`, `instance_exec`). ([@sucicfilip]) - Add new cop `RSpec/Output`. ([@kevinrobell-st]) +- Add new cop `RSpec/DiscardedMatcher` to detect matchers in void context (e.g. missing `.and` between compound matchers). ([@ydakuka]) ## 3.8.0 (2025-11-12) @@ -1109,6 +1110,7 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features. [@yasu551]: https://github.com/yasu551 [@ybiquitous]: https://github.com/ybiquitous [@ydah]: https://github.com/ydah +[@ydakuka]: https://github.com/ydakuka [@yevhene]: https://github.com/yevhene [@ypresto]: https://github.com/ypresto [@yujideveloper]: https://github.com/yujideveloper diff --git a/config/default.yml b/config/default.yml index 351bfc735..ac5ab7cac 100644 --- a/config/default.yml +++ b/config/default.yml @@ -311,6 +311,13 @@ RSpec/Dialect: VersionAdded: '1.33' Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Dialect +RSpec/DiscardedMatcher: + Description: Checks for matchers that are used in void context. + Enabled: pending + VersionAdded: "<>" + CustomMatcherMethods: [] + Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/DiscardedMatcher + RSpec/DuplicatedMetadata: Description: Avoid duplicated metadata. Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 9c1e7746f..732b0b210 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -23,6 +23,7 @@ * xref:cops_rspec.adoc#rspecdescribedclass[RSpec/DescribedClass] * xref:cops_rspec.adoc#rspecdescribedclassmodulewrapping[RSpec/DescribedClassModuleWrapping] * xref:cops_rspec.adoc#rspecdialect[RSpec/Dialect] +* xref:cops_rspec.adoc#rspecdiscardedmatcher[RSpec/DiscardedMatcher] * xref:cops_rspec.adoc#rspecduplicatedmetadata[RSpec/DuplicatedMetadata] * xref:cops_rspec.adoc#rspecemptyexamplegroup[RSpec/EmptyExampleGroup] * xref:cops_rspec.adoc#rspecemptyhook[RSpec/EmptyHook] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 822aeee51..b1c39e771 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/docs/modules/ROOT/pages/cops_rspec.adoc @@ -1216,6 +1216,69 @@ end * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Dialect +[#rspecdiscardedmatcher] +== RSpec/DiscardedMatcher + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| No +| <> +| - +|=== + +Checks for matchers that are used in void context. + +Matcher calls like `change`, `receive`, etc. that appear as +standalone expressions have their result silently discarded. +This usually means a missing `.and` to chain compound matchers. + +The list of matcher methods can be configured +with `CustomMatcherMethods`. + +[#examples-rspecdiscardedmatcher] +=== Examples + +[source,ruby] +---- +# bad +specify do + expect { result } + .to change { obj.foo }.from(1).to(2) + change { obj.bar }.from(3).to(4) +end + +# good +specify do + expect { result } + .to change { obj.foo }.from(1).to(2) + .and change { obj.bar }.from(3).to(4) +end + +# good +specify do + expect { result }.to change { obj.foo }.from(1).to(2) +end +---- + +[#configurable-attributes-rspecdiscardedmatcher] +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| CustomMatcherMethods +| `[]` +| Array +|=== + +[#references-rspecdiscardedmatcher] +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/DiscardedMatcher + [#rspecduplicatedmetadata] == RSpec/DuplicatedMetadata diff --git a/lib/rubocop-rspec.rb b/lib/rubocop-rspec.rb index 3de03a7ae..f9999ec4a 100644 --- a/lib/rubocop-rspec.rb +++ b/lib/rubocop-rspec.rb @@ -14,6 +14,7 @@ require_relative 'rubocop/cop/rspec/mixin/file_help' require_relative 'rubocop/cop/rspec/mixin/final_end_location' +require_relative 'rubocop/cop/rspec/mixin/inside_example' require_relative 'rubocop/cop/rspec/mixin/inside_example_group' require_relative 'rubocop/cop/rspec/mixin/location_help' require_relative 'rubocop/cop/rspec/mixin/metadata' diff --git a/lib/rubocop/cop/rspec/discarded_matcher.rb b/lib/rubocop/cop/rspec/discarded_matcher.rb new file mode 100644 index 000000000..d5084e364 --- /dev/null +++ b/lib/rubocop/cop/rspec/discarded_matcher.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for matchers that are used in void context. + # + # Matcher calls like `change`, `receive`, etc. that appear as + # standalone expressions have their result silently discarded. + # This usually means a missing `.and` to chain compound matchers. + # + # The list of matcher methods can be configured + # with `CustomMatcherMethods`. + # + # @example + # # bad + # specify do + # expect { result } + # .to change { obj.foo }.from(1).to(2) + # change { obj.bar }.from(3).to(4) + # end + # + # # good + # specify do + # expect { result } + # .to change { obj.foo }.from(1).to(2) + # .and change { obj.bar }.from(3).to(4) + # end + # + # # good + # specify do + # expect { result }.to change { obj.foo }.from(1).to(2) + # end + # + class DiscardedMatcher < Base + include InsideExample + + MSG = 'The result of `%s` is not used. ' \ + 'Did you mean to chain it with `.and`?' + + MATCHER_METHODS = %i[ + change have_received output + receive receive_messages receive_message_chain + ].to_set.freeze + + def on_send(node) + check_discarded_matcher(node, node) + end + + def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler, InternalAffairs/ItblockHandler + check_discarded_matcher(node.send_node, node) + end + + private + + def check_discarded_matcher(send_node, node) + return unless matcher_call?(send_node) + return unless inside_example?(node) + return unless example_with_matcher_expectation?(node) + + target = find_outermost_chain(node) + return unless void_value?(target) + + add_offense(target, message: format(MSG, method: node.method_name)) + end + + def example_with_matcher_expectation?(node) + example_node = + node.each_ancestor(:block).find { |ancestor| example?(ancestor) } + + example_node.each_descendant(:send).any? do |send_node| + expectation_with_matcher?(send_node) + end + end + + def expectation_with_matcher?(node) + %i[to to_not not_to].include?(node.method_name) && + node.arguments.any? do |arg| + arg.each_node(:send).any? { |s| matcher_call?(s) } + end + end + + def void_value?(node) + case node.parent.type + when :block + example?(node.parent) + when :begin, :case, :when + void_value?(node.parent) + end + end + + def matcher_call?(node) + node.receiver.nil? && all_matcher_methods.include?(node.method_name) + end + + def all_matcher_methods + @all_matcher_methods ||= + (MATCHER_METHODS + custom_matcher_methods).freeze + end + + def custom_matcher_methods + cop_config.fetch('CustomMatcherMethods', []).map(&:to_sym) + end + + def find_outermost_chain(node) + current = node + current = current.parent while current.parent.receiver == current + current + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec/empty_example_group.rb b/lib/rubocop/cop/rspec/empty_example_group.rb index b3e8e86cf..8d6c36b07 100644 --- a/lib/rubocop/cop/rspec/empty_example_group.rb +++ b/lib/rubocop/cop/rspec/empty_example_group.rb @@ -38,6 +38,7 @@ module RSpec class EmptyExampleGroup < Base extend AutoCorrector + include InsideExample include RangeHelp MSG = 'Empty example group detected.' @@ -138,7 +139,7 @@ class EmptyExampleGroup < Base def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler, InternalAffairs/ItblockHandler return if node.each_ancestor(:any_def).any? - return if node.each_ancestor(:block).any? { |block| example?(block) } + return if inside_example?(node) example_group_body(node) do |body| next unless offensive?(body) diff --git a/lib/rubocop/cop/rspec/mixin/inside_example.rb b/lib/rubocop/cop/rspec/mixin/inside_example.rb new file mode 100644 index 000000000..09cc6bc22 --- /dev/null +++ b/lib/rubocop/cop/rspec/mixin/inside_example.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Helps check if a given node is within an example block. + module InsideExample + private + + def inside_example?(node) + node.each_ancestor(:block).any? { |ancestor| example?(ancestor) } + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec/skip_block_inside_example.rb b/lib/rubocop/cop/rspec/skip_block_inside_example.rb index 95e0afa1f..8bce85150 100644 --- a/lib/rubocop/cop/rspec/skip_block_inside_example.rb +++ b/lib/rubocop/cop/rspec/skip_block_inside_example.rb @@ -24,6 +24,8 @@ module RSpec # end # class SkipBlockInsideExample < Base + include InsideExample + MSG = "Don't pass a block to `skip` inside examples." def on_block(node) @@ -35,12 +37,6 @@ def on_block(node) alias on_numblock on_block alias on_itblock on_block - - private - - def inside_example?(node) - node.each_ancestor(:block).any? { |ancestor| example?(ancestor) } - end end end end diff --git a/lib/rubocop/cop/rspec/void_expect.rb b/lib/rubocop/cop/rspec/void_expect.rb index 9e151984d..51bbac097 100644 --- a/lib/rubocop/cop/rspec/void_expect.rb +++ b/lib/rubocop/cop/rspec/void_expect.rb @@ -13,6 +13,8 @@ module RSpec # expect(something).to be(1) # class VoidExpect < Base + include InsideExample + MSG = 'Do not use `expect()` without `.to` or `.not_to`. ' \ 'Chain the methods or remove it.' RESTRICT_ON_SEND = %i[expect].freeze @@ -55,10 +57,6 @@ def void?(expect) parent.block_type? && parent.body == expect end - - def inside_example?(node) - node.each_ancestor(:block).any? { |ancestor| example?(ancestor) } - end end end end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 877d466f2..f6f844913 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -21,6 +21,7 @@ require_relative 'rspec/described_class' require_relative 'rspec/described_class_module_wrapping' require_relative 'rspec/dialect' +require_relative 'rspec/discarded_matcher' require_relative 'rspec/duplicated_metadata' require_relative 'rspec/empty_example_group' require_relative 'rspec/empty_hook' diff --git a/spec/rubocop/cop/rspec/discarded_matcher_spec.rb b/spec/rubocop/cop/rspec/discarded_matcher_spec.rb new file mode 100644 index 000000000..9f12fbaa0 --- /dev/null +++ b/spec/rubocop/cop/rspec/discarded_matcher_spec.rb @@ -0,0 +1,321 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::DiscardedMatcher do + it 'registers an offense for standalone `change` with block in example' do + expect_offense(<<~RUBY) + specify do + expect { result }.to \\ + change { obj.foo }.from(1).to(2) + change { obj.bar }.from(3).to(4) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The result of `change` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'registers an offense for standalone `change` with arguments' do + expect_offense(<<~RUBY) + specify do + expect { result }.to change(Foo, :bar).by(1) + change(Foo, :baz).by(2) + ^^^^^^^^^^^^^^^^^^^^^^^ The result of `change` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'registers an offense for standalone `receive`' do + expect_offense(<<~RUBY) + specify do + expect(foo).to receive(:bar) + receive(:baz).and_return(1) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ The result of `receive` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'does not register an offense for `receive_messages`' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to receive_messages(foo: 1, bar: 2) + end + RUBY + end + + it 'registers an offense for standalone `receive_message_chain`' do + expect_offense(<<~RUBY) + specify do + expect(foo).to receive(:bar) + receive_message_chain(:baz, :qux) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The result of `receive_message_chain` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'registers an offense for standalone `output`' do + expect_offense(<<~RUBY) + specify do + expect { result }.to output('foo').to_stdout + output('bar').to_stderr + ^^^^^^^^^^^^^^^^^^^^^^^ The result of `output` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'does not register for `output` unrelated to matcher' do + expect_no_offenses(<<~RUBY) + specify do + output.rewind + parsed = JSON.parse(output.read.chomp) + expect(parsed["message"]).to eq("error message") + end + RUBY + end + + it 'registers an offense for standalone `have_received`' do + expect_offense(<<~RUBY) + specify do + expect(foo).to have_received(:bar) + have_received(:baz) + ^^^^^^^^^^^^^^^^^^^ The result of `have_received` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'does not register an offense for `output`' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to output('foo').to_stdout + end + RUBY + end + + it 'does not register an offense for `have_received`' do + expect_no_offenses(<<~RUBY) + specify do + expect(foo).to have_received(:bar) + end + RUBY + end + + it 'does not register an offense for `change`' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to change { obj.foo }.from(1).to(2) + end + RUBY + end + + it 'does not register an offense when `change` is chained with `.and`' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to \\ + change { obj.foo }.from(1).to(2) + .and change { obj.bar }.from(3).to(4) + end + RUBY + end + + it 'does not register an offense for `change` in parenthesized arg' do + expect_no_offenses(<<~RUBY) + expect { result }.to (change { obj.bar }) + RUBY + end + + it 'does not register an offense when `receive` is used with `allow`' do + expect_no_offenses(<<~RUBY) + specify do + allow(foo).to receive(:bar).and_return(1) + end + RUBY + end + + it 'does not register an offense for `receive`' do + expect_no_offenses(<<~RUBY) + specify do + expect(foo).to receive(:bar) + end + RUBY + end + + it 'does not register an offense outside of example context' do + expect_no_offenses(<<~RUBY) + expect { result }.to change { obj.bar } + RUBY + end + + it 'does not register an offense for multiple matchers outside example' do + expect_no_offenses(<<~RUBY) + expect { result }.to \\ + change { obj.foo } + change { obj.bar } + RUBY + end + + it 'does not register an offense for multiple matchers with args outside' do + expect_no_offenses(<<~RUBY) + expect { result }.to \\ + change(Foo, :foo) + change(Foo, :bar) + RUBY + end + + it 'does not register for `change` in modifier `if`' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to change { obj.bar } if condition + end + RUBY + end + + it 'does not register for `change` in modifier `unless`' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to change { obj.bar } unless condition + end + RUBY + end + + it 'does not register for `change` in `if` and `else` branches' do + expect_no_offenses(<<~RUBY) + specify do + change_temp = + if condition + change { obj.bar }.from(3).to(4) + else + change { obj.baz }.from(5).to(6) + end + + expect { result }.to change_temp + end + RUBY + end + + it 'does not register an offense for `change` in non-void `if` branch' do + expect_no_offenses(<<~RUBY) + specify do + result = if condition + expect { result }.to change { obj.foo }.from(1).to(2) + end + end + RUBY + end + + it 'does not register an offense ' \ + 'for `change` in non-void `if` and `else` branches' do + expect_no_offenses(<<~RUBY) + specify do + result = if condition + expect { result }.to change { obj.foo }.from(1).to(2) + else + expect { result }.to change { obj.foo }.from(3).to(4) + end + end + RUBY + end + + it 'registers an offense for `change` in a `case` else branch' do + expect_offense(<<~RUBY) + specify do + case condition + when :update then expect { result }.to change { obj.bar }.from(3).to(4) + else change { obj.baz }.from(5).to(6) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The result of `change` is not used. Did you mean to chain it with `.and`? + end + end + RUBY + end + + it 'does not register an offense for `change` in `when` with `expect`' do + expect_no_offenses(<<~RUBY) + specify do + case condition + when :update then expect { result }.to change { obj.bar }.from(3).to(4) + end + end + RUBY + end + + it 'does not register an offense for `change` in non-void `when` branch' do + expect_no_offenses(<<~RUBY) + specify do + change_temp = case season + when :summer then change { temp }.from(0).to(1) + when :winter then change { temp }.from(0).to(2) + end + + expect { result }.to change_temp + end + RUBY + end + + it 'does not register an offense for `change` in non-void `case` else' do + expect_no_offenses(<<~RUBY) + specify do + change_temp = case season + when :summer then change { temp }.from(0).to(1) + else change { temp }.from(0).to(2) + end + + expect { result }.to change_temp + end + RUBY + end + + it 'does not register an offense for `change` used with `&` operator' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to \\ + change { obj.foo }.from(1).to(2) & + change { obj.bar }.from(3).to(4) + end + RUBY + end + + it 'registers an offense for `change` after custom expect helper' do + expect_offense(<<~RUBY) + specify do + def expect_action + expect { action!(performer: performer, context: context) } + end + + expect_action + .to change { obj.foo }.from(1).to(2) + change { obj.bar }.from(3).to(4) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The result of `change` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + + it 'does not register an offense when `change` is called on a receiver' do + expect_no_offenses(<<~RUBY) + specify do + expect { result }.to change { obj.bar }.from(1).to(2) + foo.change { obj.foo } + end + RUBY + end + + it 'does not register when `change` on a receiver has no expectation' do + expect_no_offenses(<<~RUBY) + specify do + foo.change { obj.foo } + end + RUBY + end + + context 'with custom CustomMatcherMethods' do + let(:cop_config) do + { 'CustomMatcherMethods' => %w[custom_matcher] } + end + + it 'registers an offense for configured custom matcher' do + expect_offense(<<~RUBY) + specify do + expect(foo).to \\ + receive(:bar) + custom_matcher(:foo) + ^^^^^^^^^^^^^^^^^^^^ The result of `custom_matcher` is not used. Did you mean to chain it with `.and`? + end + RUBY + end + end +end