Prevent block-only matchers from being used with value expectation ta…

…rget

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

@@ -10,6 +10,11 @@ Enhancements:
 * Add the ability to re-raise expectation errors when matching
   with `match_when_negated` blocks. (Jon Rowe, #1130)
 
+Bug Fixes:
+
+* Prevent unsupported implicit block expectation syntax from being used.
+  (Phil Pirozhkov, #1125)
+
 ### 3.8.4 / 2019-06-10
 [Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.8.3...v3.8.4)
 
@@ -26,7 +31,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

@@ -33,16 +33,16 @@ def initialize(value)
       end
 
       # @private
-      def self.for(value, block)
+      def self.for(value, &block)
         if UndefinedValue.equal?(value)
-          unless block
+          unless block_given?
             raise ArgumentError, "You must pass either an argument or a block to `expect`."
           end
           BlockExpectationTarget.new(block)
-        elsif block
+        elsif block_given?
           raise ArgumentError, "You cannot pass both an argument and a block to `expect`."
         else
-          new(value)
+          ValueExpectationTarget.new(value)
         end
       end
 
@@ -90,6 +90,40 @@ 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)
+
+        raise ExpectationNotMetError, "You must pass a block rather than an argument to `expect` to use the provided " \
+          "block expectation matcher (#{RSpec::Support::ObjectFormatter.format(matcher)})."
+      end
+
+      def supports_value_expectations?(matcher)
+        if matcher.respond_to?(:supports_value_expectations?)
+          matcher.supports_value_expectations?
+        else
+          true
+        end
+      end
+    end
+
     # @private
     # Validates the provided matcher to ensure it supports block
     # expectations, in order to avoid user confusion when they
@@ -118,9 +152,11 @@ def enforce_block_expectation(matcher)
       end
 
       def supports_block_expectations?(matcher)
-        matcher.supports_block_expectations?
-      rescue NoMethodError
-        false
+        if matcher.respond_to?(:supports_block_expectations?)
+          matcher.supports_block_expectations?
+        else
+          false
+        end
       end
     end
   end

lib/rspec/expectations/syntax.rb

@@ -70,7 +70,7 @@ def enable_expect(syntax_host=::RSpec::Matchers)
 
         syntax_host.module_exec do
           def expect(value=::RSpec::Expectations::ExpectationTarget::UndefinedValue, &block)
-            ::RSpec::Expectations::ExpectationTarget.for(value, block)
+            ::RSpec::Expectations::ExpectationTarget.for(value, &block)
           end
         end
       end

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)
@@ -107,12 +112,10 @@ def raise_block_syntax_error
         end
 
         def positive_failure_reason
-          return "was not given a block" unless Proc === @event_proc
           "is still #{@actual_before_description}"
         end
 
         def negative_failure_reason
-          return "was not given a block" unless Proc === @event_proc
           "did change from #{@actual_before_description} " \
           "to #{description_of change_details.actual_after}"
         end
@@ -158,10 +161,14 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def failure_reason
-          return "was not given a block" unless Proc === @event_proc
           "was changed by #{description_of @change_details.actual_delta}"
         end
       end
@@ -190,7 +197,6 @@ def description
 
         # @private
         def failure_message
-          return not_given_a_block_failure unless Proc === @event_proc
           return before_value_failure      unless @matches_before
           return did_not_change_failure    unless @change_details.changed?
           after_value_failure
@@ -201,6 +207,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def perform_change(event_proc)
@@ -242,11 +253,6 @@ def did_change_failure
           "did change from #{@actual_before_description} " \
           "to #{description_of @change_details.actual_after}"
         end
-
-        def not_given_a_block_failure
-          "expected #{@change_details.value_representation} to have changed " \
-          "#{change_description}, but was not given a block"
-        end
       end
 
       # @api private
@@ -278,7 +284,6 @@ def does_not_match?(event_proc)
 
         # @private
         def failure_message_when_negated
-          return not_given_a_block_failure unless Proc === @event_proc
           return before_value_failure unless @matches_before
           did_change_failure
         end

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,20 +94,25 @@ 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?
           @actual.length > 0
         end
 
         def positive_failure_reason
-          return "was not a block" unless Proc === @block
           return "output #{actual_output_description}" if @expected
           "did not"
         end
 
         def negative_failure_reason
-          return "was not a block" unless Proc === @block
           "output #{actual_output_description}"
         end
 

lib/rspec/matchers/built_in/raise_error.rb

@@ -76,6 +76,12 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
+        # @private
         def expects_call_stack_jump?
           true
         end
@@ -199,7 +205,6 @@ def format_backtrace(backtrace)
         end
 
         def given_error
-          return " but was not given a block" unless Proc === @given_proc
           return " but nothing was raised" unless @actual_error
 
           backtrace = format_backtrace(@actual_error.backtrace)

lib/rspec/matchers/built_in/throw_symbol.rb

@@ -88,20 +88,23 @@ 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
 
       private
 
         def actual_result
-          return "but was not a block" unless Proc === @block
           "got #{caught}"
         end