Prevent block matchers use with value expectations

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
https://rspec.rubystyle.guide/#implicit-block-expectations

Spec changes are due to:

    matcher.matches?(invalid_value)

doesn't work with block-only matchers, as no block is passed in, and it
fails with a message:

    1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message`
       Failure/Error: expect(message).to include("detailed inspect")
         expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect"

The redundant (due to existing check in ExpectationTarget) `Proc ===
@event_proc` checks could not be removed safely as well, since
@actual_after is not initialized yet when we haven't executed the block:

     RuntimeError:
       Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized

Ruby 1.8-specific workarounds:

    multiple values for a block parameter (0 for 1)

If a block expects an argument, it ought to be provided an argument in 1.8

Changelog.md

@@ -5,6 +5,10 @@ Enhancements:
 
 * Return `true` from `aggregate_failures` when no exception occurs. (Jon Rowe, #1225)
 
+Deprecations:
+* Print a deprecation message when using the implicit block expectation syntax.
+  (Phil Pirozhkov, #1139)
+
 ### 3.10.0 / 2020-10-30
 [Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.9.3...v3.10.0)
 
@@ -112,7 +116,7 @@ Bug Fixes:
 * Prevent composed `all` matchers from leaking into their siblings leading to duplicate
   failures. (Jamie English, #1086)
 * Prevent objects which change their hash on comparison from failing change checks.
-  (Phil Pirozhkov, #1110)
+  (Phil Pirozhkov, #1100)
 * Issue an `ArgumentError` rather than a `NoMethodError` when `be_an_instance_of` and
   `be_kind_of` matchers encounter objects not supporting those methods.
   (Taichi Ishitani, #1107)

lib/rspec/expectations/expectation_target.rb

@@ -42,7 +42,7 @@ def self.for(value, block)
         elsif block
           raise ArgumentError, "You cannot pass both an argument and a block to `expect`."
         else
-          new(value)
+          ValueExpectationTarget.new(value)
         end
       end
 
@@ -90,6 +90,44 @@ def prevent_operator_matchers(verb)
       include InstanceMethods
     end
 
+    # @private
+    # Validates the provided matcher to ensure it supports block
+    # expectations, in order to avoid user confusion when they
+    # use a block thinking the expectation will be on the return
+    # value of the block rather than the block itself.
+    class ValueExpectationTarget < ExpectationTarget
+      def to(matcher=nil, message=nil, &block)
+        enforce_value_expectation(matcher)
+        super
+      end
+
+      def not_to(matcher=nil, message=nil, &block)
+        enforce_value_expectation(matcher)
+        super
+      end
+
+    private
+
+      def enforce_value_expectation(matcher)
+        return if supports_value_expectations?(matcher)
+
+        RSpec.deprecate(
+          "expect(value).to #{RSpec::Support::ObjectFormatter.format(matcher)}",
+          :message =>
+            "The implicit block expectation syntax is deprecated, you should pass " \
+            "a block rather than an argument to `expect` to use the provided " \
+            "block expectation matcher (#{RSpec::Support::ObjectFormatter.format(matcher)}), " \
+            "or the matcher must implement `supports_value_expectations?`."
+        )
+      end
+
+      def supports_value_expectations?(matcher)
+        matcher.supports_value_expectations?
+      rescue NoMethodError
+        true
+      end
+    end
+
     # @private
     # Validates the provided matcher to ensure it supports block
     # expectations, in order to avoid user confusion when they

lib/rspec/matchers/built_in/base_matcher.rb

@@ -78,6 +78,11 @@ def supports_block_expectations?
           false
         end
 
+        # @private
+        def supports_value_expectations?
+          true
+        end
+
         # @api private
         def expects_call_stack_jump?
           false

lib/rspec/matchers/built_in/change.rb

@@ -77,6 +77,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def initialize(receiver=nil, message=nil, &block)
@@ -158,6 +163,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def failure_reason
@@ -201,6 +211,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def perform_change(event_proc)
@@ -337,6 +352,8 @@ def change_description
       class ChangeDetails
         attr_reader :actual_after
 
+        UNDEFINED = Module.new.freeze
+
         def initialize(matcher_name, receiver=nil, message=nil, &block)
           if receiver && !message
             raise(
@@ -351,6 +368,11 @@ def initialize(matcher_name, receiver=nil, message=nil, &block)
           @receiver = receiver
           @message = message
           @value_proc = block
+          # TODO: temporary measure to mute warning of access to an initialized
+          # instance variable when a deprecated implicit block expectation
+          # syntax is used. This may be removed once `fail` is used, and the
+          # matcher never issues this warning.
+          @actual_after = UNDEFINED
         end
 
         def value_representation

lib/rspec/matchers/built_in/compound.rb

@@ -26,11 +26,19 @@ def description
           "#{matcher_1.description} #{conjunction} #{matcher_2.description}"
         end
 
+        # @api private
         def supports_block_expectations?
           matcher_supports_block_expectations?(matcher_1) &&
           matcher_supports_block_expectations?(matcher_2)
         end
 
+        # @api private
+        def supports_value_expectations?
+          matcher_supports_value_expectations?(matcher_1) &&
+          matcher_supports_value_expectations?(matcher_2)
+        end
+
+        # @api private
         def expects_call_stack_jump?
           NestedEvaluator.matcher_expects_call_stack_jump?(matcher_1) ||
           NestedEvaluator.matcher_expects_call_stack_jump?(matcher_2)
@@ -102,6 +110,12 @@ def matcher_supports_block_expectations?(matcher)
           false
         end
 
+        def matcher_supports_value_expectations?(matcher)
+          matcher.supports_value_expectations?
+        rescue NoMethodError
+          true
+        end
+
         def matcher_is_diffable?(matcher)
           matcher.diffable?
         rescue NoMethodError

lib/rspec/matchers/built_in/output.rb

@@ -94,6 +94,13 @@ def supports_block_expectations?
           true
         end
 
+        # @api private
+        # Indicates this matcher matches against a block only.
+        # @return [False]
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def captured?

lib/rspec/matchers/built_in/raise_error.rb

@@ -86,6 +86,12 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
+        # @private
         def expects_call_stack_jump?
           true
         end

lib/rspec/matchers/built_in/throw_symbol.rb

@@ -88,12 +88,16 @@ def description
         end
 
         # @api private
-        # Indicates this matcher matches against a block.
-        # @return [True]
         def supports_block_expectations?
           true
         end
 
+        # @api private
+        def supports_value_expectations?
+          false
+        end
+
+        # @api private
         def expects_call_stack_jump?
           true
         end

lib/rspec/matchers/built_in/yield.rb

@@ -129,6 +129,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def failure_reason
@@ -169,6 +174,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def positive_failure_reason
@@ -231,6 +241,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def positive_failure_reason
@@ -328,6 +343,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def expected_arg_description

lib/rspec/matchers/dsl.rb

@@ -403,6 +403,10 @@ def supports_block_expectations?
           false
         end
 
+        def supports_value_expectations?
+          true
+        end
+
         # Most matchers do not expect call stack jumps.
         def expects_call_stack_jump?
           false

lib/rspec/matchers/matcher_protocol.rb

@@ -60,6 +60,12 @@ class MatcherProtocol
       #   @return [Boolean] true if this matcher can be used in block expressions.
       #   @note If not defined, RSpec assumes a value of `false` for this method.
 
+      # @!method supports_value_expectations?
+      #   Indicates that this matcher can be used in a value expectation expression,
+      #   such as `expect(foo).to eq(bar)`.
+      #   @return [Boolean] true if this matcher can be used in value expressions.
+      #   @note If not defined, RSpec assumes a value of `true` for this method.
+
       # @!method expects_call_stack_jump?
       #   Indicates that when this matcher is used in a block expectation
       #   expression, it expects the block to use a ruby construct that causes

spec/rspec/expectations/extensions/kernel_spec.rb

@@ -1,7 +1,7 @@
 RSpec.describe Object, "#should" do
   before(:example) do
     @target = "target"
-    @matcher = double("matcher")
+    @matcher = double("matcher", :supports_value_expectations? => true)
     allow(@matcher).to receive(:matches?).and_return(true)
     allow(@matcher).to receive(:failure_message)
   end
@@ -59,7 +59,7 @@ def method_missing(name, *args)
 RSpec.describe Object, "#should_not" do
   before(:example) do
     @target = "target"
-    @matcher = double("matcher")
+    @matcher = double("matcher", :supports_value_expectations? => true)
   end
 
   it "accepts and interacts with a matcher" do

spec/rspec/matchers/aliased_matcher_spec.rb

@@ -14,7 +14,7 @@ def description
       end
       RSpec::Matchers.alias_matcher :alias_of_my_base_matcher, :my_base_matcher
 
-      it_behaves_like "an RSpec matcher", :valid_value => 13, :invalid_value => nil do
+      it_behaves_like "an RSpec value matcher", :valid_value => 13, :invalid_value => nil do
         let(:matcher) { alias_of_my_base_matcher }
       end
 

spec/rspec/matchers/built_in/all_spec.rb

@@ -1,7 +1,7 @@
 module RSpec::Matchers::BuiltIn
   RSpec.describe All do
 
-    it_behaves_like 'an RSpec matcher', :valid_value => ['A', 'A', 'A'], :invalid_value => ['A', 'A', 'B'], :disallows_negation => true do
+    it_behaves_like 'an RSpec value matcher', :valid_value => ['A', 'A', 'A'], :invalid_value => ['A', 'A', 'B'], :disallows_negation => true do
       let(:matcher) { all( eq('A') ) }
     end
 

spec/rspec/matchers/built_in/be_between_spec.rb

@@ -80,7 +80,7 @@ def inspect
       end
     end
 
-    it_behaves_like "an RSpec matcher", :valid_value => (10), :invalid_value => (11) do
+    it_behaves_like "an RSpec value matcher", :valid_value => (10), :invalid_value => (11) do
       let(:matcher) { be_between(1, 10) }
     end
 

spec/rspec/matchers/built_in/be_instance_of_spec.rb

@@ -2,7 +2,7 @@ module RSpec
   module Matchers
     [:be_an_instance_of, :be_instance_of].each do |method|
       RSpec.describe "expect(actual).to #{method}(expected)" do
-        it_behaves_like "an RSpec matcher", :valid_value => "a", :invalid_value => 5 do
+        it_behaves_like "an RSpec value matcher", :valid_value => "a", :invalid_value => 5 do
           let(:matcher) { send(method, String) }
         end
 

spec/rspec/matchers/built_in/be_kind_of_spec.rb

@@ -2,7 +2,7 @@ module RSpec
   module Matchers
     [:be_a_kind_of, :be_kind_of].each do |method|
       RSpec.describe "expect(actual).to #{method}(expected)" do
-        it_behaves_like "an RSpec matcher", :valid_value => 5, :invalid_value => "a" do
+        it_behaves_like "an RSpec value matcher", :valid_value => 5, :invalid_value => "a" do
           let(:matcher) { send(method, Integer) }
         end
 

spec/rspec/matchers/built_in/be_within_spec.rb

@@ -1,7 +1,7 @@
 module RSpec
   module Matchers
     RSpec.describe "expect(actual).to be_within(delta).of(expected)" do
-      it_behaves_like "an RSpec matcher", :valid_value => 5, :invalid_value => -5 do
+      it_behaves_like "an RSpec value matcher", :valid_value => 5, :invalid_value => -5 do
         let(:matcher) { be_within(2).of(4.0) }
       end