Merge pull request #1139 from pirj/warn-on-block-matchers-being-used-…

…with-value-expectation-target

Print deprecation message when implicit block expectation syntax is used

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,46 @@ 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 or the matcher must implement " \
+            "`supports_value_expectations?`. e.g  `expect { value }.to " \
+            "#{RSpec::Support::ObjectFormatter.format(matcher)}` not " \
+            "`expect(value).to #{RSpec::Support::ObjectFormatter.format(matcher)}`"
+        )
+      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

@@ -85,12 +85,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

@@ -116,6 +116,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def failure_reason
@@ -156,6 +161,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def positive_failure_reason
@@ -218,6 +228,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def positive_failure_reason
@@ -315,6 +330,11 @@ def supports_block_expectations?
           true
         end
 
+        # @private
+        def supports_value_expectations?
+          false
+        end
+
       private
 
         def expected_arg_description

lib/rspec/matchers/dsl.rb

@@ -361,6 +361,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/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