Merge 946d78e into 9a500bf

Changelog.md

@@ -8,6 +8,16 @@ Deprecations
 * Deprecate `be_true`/`be_false` in favour of `be_truthy`/`be_falsey`
   (for Ruby's conditional semantics) or `be true`/`be false`
   (for exact equality). (Sam Phippen)
+* Deprecate calling helper methods from a custom matcher with the wrong
+  scope. (Myron Marston)
+  * `def self.foo` / `extend Helper` can be used to add macro methods
+    (e.g. methods that call the custom matcher DSL methods), but should
+    not be used to define helper methods called from within the DSL
+    blocks.
+  * `def foo` / `include Helper` is the opposite: it's for helper methods
+    callable from within a DSL block, but not for defining macros.
+  * RSpec 2.x allowed helper methods defined either way to be used for
+    either purpose, but RSpec 3.0 will not.
 
 Bug fixes:
 

lib/rspec/matchers/differentiate_block_method_types.rb

@@ -0,0 +1,55 @@
+module RSpec
+  module Matchers
+    # Evaluates a block in order to determine what methods, if any,
+    # it defines as instance methods (using `def foo`) vs singleton
+    # methods (using `def self.foo`).
+    #
+    # @api private
+    class DifferentiateBlockMethodTypes
+      def initialize(*block_args, &block)
+        @block_args = block_args
+        @block = block
+
+        ignore_macro_methods
+
+        capture_added_methods(singletons_singleton_class, singleton_methods)
+        capture_added_methods(singleton_class, instance_methods)
+
+        singleton_class.class_exec(*block_args, &block)
+      end
+
+      def singleton_methods
+        @singleton_methods ||= []
+      end
+
+      def instance_methods
+        @instance_methods ||= []
+      end
+
+    private
+
+      def capture_added_methods(object, method_list)
+        object.__send__(:define_method, :singleton_method_added) do |method_name|
+          method_list << method_name
+        end
+
+        method_list.delete(:singleton_method_added)
+      end
+
+      unless method_defined?(:singleton_class)
+        def singleton_class
+          class << self; self; end
+        end
+      end
+
+      def singletons_singleton_class
+        class << singleton_class; self; end
+      end
+
+      def ignore_macro_methods
+        def singleton_class.method_missing(*); self; end
+      end
+    end
+  end
+end
+

lib/rspec/matchers/matcher.rb

@@ -1,4 +1,5 @@
 require 'set'
+require 'rspec/matchers/differentiate_block_method_types'
 
 module RSpec
   module Matchers
@@ -22,6 +23,9 @@ def initialize(name, &declarations)
           @expected_exception, @rescued_exception = nil, nil
           @match_for_should_not_block = nil
           @messages = {}
+          @define_block_executed = false
+          @block_method_differentiator = nil
+          @deprecated_methods = Set.new
         end
 
         PERSISTENT_INSTANCE_VARIABLES = [
@@ -38,9 +42,14 @@ def for_expected(*expected)
               instance_variable_set(ivar, nil) unless (PERSISTENT_INSTANCE_VARIABLES + [:@expected]).include?(ivar)
             end
             @messages = {}
+            @deprecated_methods = Set.new
+
+            @block_method_differentiator = DifferentiateBlockMethodTypes.new(*@expected, &@declarations)
             making_declared_methods_public do
               instance_eval_with_args(*@expected, &@declarations)
             end
+
+            @define_block_executed = true
             self
           end
         end
@@ -237,8 +246,38 @@ def method_missing(method, *args, &block)
           end
         end
 
-        def include(*args)
-          singleton_class.__send__(:include, *args)
+        def include(*modules)
+          return_value = singleton_class.__send__(:include, *modules)
+
+          modules.each do |mod|
+            mod.instance_methods.each do |name|
+              add_deprecation_warning_to(name,
+                "Calling a helper method (`#{name}`) from a module included in a custom matcher as a macro",
+                "`extend #{mod.name || "TheModule"}`",
+                "included in the custom matcher",
+                :unless
+              )
+            end
+          end
+
+          return_value
+        end
+
+        def extend(*modules)
+          return_value = super
+
+          modules.each do |mod|
+            mod.instance_methods.each do |name|
+              add_deprecation_warning_to(name,
+                "Calling a helper method (`#{name}`) from a module extended onto a custom matcher",
+                "`include #{mod.name || "TheModule"}`",
+                "extended onto the custom matcher",
+                :if
+              )
+            end
+          end unless @define_block_executed
+
+          return_value
         end
 
         def define_method(name, &block)
@@ -294,6 +333,50 @@ def singleton_class
             class << self; self; end
           end
         end
+
+        def singleton_method_added(name)
+          return unless @block_method_differentiator
+
+          if @block_method_differentiator.instance_methods.include?(name)
+            add_deprecation_warning_to(name,
+              "Calling a helper method (`#{name}`) defined as an instance method (using `def #{name}`) as a macro from a custom matcher `define` block",
+              "`def self.#{name}` (to define it as a singleton method)",
+              "defined in the custom matcher definition block",
+              :unless
+            )
+          elsif @block_method_differentiator.singleton_methods.include?(name)
+            add_deprecation_warning_to(name,
+              "Calling a helper method (`#{name}`) defined as a singleton method (using `def self.#{name}`) on a custom matcher",
+              "`def #{name}` (to define it as an instance method)",
+              "defined in the custom matcher definition block",
+              :if
+            )
+          end
+        end
+
+        def add_deprecation_warning_to(method_name, msg, replacement, extra_call_site_msg, condition)
+          return if @deprecated_methods.include?(method_name)
+          @deprecated_methods << method_name
+
+          aliased_name = aliased_name_for(method_name)
+          singleton_class.__send__(:alias_method, aliased_name, method_name)
+
+          singleton_class.class_eval(<<-EOS, __FILE__, __LINE__ + 1)
+            def #{method_name}(*a, &b)
+              ::RSpec.deprecate(#{msg.inspect},
+                :replacement => #{replacement.inspect},
+                :call_site => CallerFilter.first_non_rspec_line + " and #{extra_call_site_msg} at #{CallerFilter.first_non_rspec_line}"
+              ) #{condition} @define_block_executed
+
+              __send__(#{aliased_name.inspect}, *a, &b)
+            end
+          EOS
+        end
+
+        def aliased_name_for(method_name)
+          target, punctuation = method_name.to_s.sub(/([?!=])$/, ''), $1
+          "#{target}_without_rspec_deprecation_warning#{punctuation}"
+        end
       end
     end
   end

spec/rspec/matchers/differentiate_block_method_types_spec.rb

@@ -0,0 +1,39 @@
+require 'spec_helper'
+
+module RSpec
+  module Matchers
+    describe DifferentiateBlockMethodTypes do
+      let(:differentiator) do
+        DifferentiateBlockMethodTypes.new do
+          def some_instance_method_1; end
+          def self.some_singleton_method_1; end
+          define_method(:some_instance_method_2) { }
+
+          if RUBY_VERSION.to_f > 1.8
+            define_singleton_method(:some_singleton_method_2) { }
+          else
+            def self.some_singleton_method_2; end
+          end
+        end
+      end
+
+      it 'differentiates singleton method defs from instance method defs' do
+        expect(differentiator.instance_methods).to eq([:some_instance_method_1, :some_instance_method_2])
+        expect(differentiator.singleton_methods).to eq([:some_singleton_method_1, :some_singleton_method_2])
+      end
+
+      it 'passes the given args through to the block' do
+        expect { |b|
+          DifferentiateBlockMethodTypes.new(1, 2, &b)
+        }.to yield_with_args(1, 2)
+      end
+
+      it 'ignores unrecognized DSL methods called in the block' do
+        expect {
+          DifferentiateBlockMethodTypes.new { foo.bar; some_dsl { nested } }
+        }.not_to raise_error
+      end
+    end
+  end
+end
+

spec/rspec/matchers/matcher_spec.rb

@@ -327,6 +327,130 @@ def second_word
       expect(matcher.matches?(8)).to be_truthy
     end
 
+    shared_examples_for "accessing a singleton helper method" do
+      before { allow_deprecation }
+
+      it 'can access the helper method from `match`' do
+        expect([2, 3]).to matcher.for_expected(5)
+        expect([2, 3]).not_to matcher.for_expected(4)
+      end
+
+      it 'prints a deprecation warning when the helper method is accessed `match`' do
+        expect(RSpec).to receive(:deprecate).with(/sum_of/, an_instance_of(Hash))
+        matcher.for_expected(5).matches?([2, 3])
+      end
+
+      it 'includes the call site in the deprecation warning' do
+        expect_deprecation_with_call_site(__FILE__, line)
+        matcher.for_expected(5).matches?([2, 3])
+      end
+
+      it 'does not print a deprecation warning if the helper method is used as a macro' do
+        expect(RSpec).not_to receive(:deprecate)
+        matcher.for_expected(:use_as_macro).matches?([2, 3])
+      end
+    end
+
+    context "when a module of helper methods is extended" do
+      include_examples "accessing a singleton helper method" do
+        let(:matcher) do
+          RSpec::Matchers::DSL::Matcher.new(:sum_to) do |sum|
+            extend Module.new {
+              def sum_of(x, y) x + y end
+              def define_match() match {} end
+            }
+
+            if sum == :use_as_macro
+              define_match
+            else
+              match { |summands| sum_of(*summands) == sum }
+            end
+          end
+        end
+        let(:line) { __LINE__ - 4 }
+      end
+    end
+
+    context "when a helper method is defined using `self.`" do
+      include_examples "accessing a singleton helper method" do
+        let(:matcher) do
+          RSpec::Matchers::DSL::Matcher.new(:sum_to) do |sum|
+            def self.sum_of(x, y) x + y end
+            def self.define_match() match {} end
+
+            if sum == :use_as_macro
+              define_match
+            else
+              match { |summands| sum_of(*summands) == sum }
+            end
+          end
+        end
+        let(:line) { __LINE__ - 4 }
+      end
+    end
+
+    shared_examples_for "accessing an instance helper method" do
+      before { allow_deprecation }
+
+      it 'can access the helper method from `match`' do
+        expect([2, 3]).to matcher.for_expected(5)
+        expect([2, 3]).not_to matcher.for_expected(4)
+      end
+
+      it 'does not print a deprecation warning when the helper method is accessed from `match`' do
+        expect(RSpec).not_to receive(:deprecate)
+        matcher.for_expected(5).matches?([2, 3])
+      end
+
+      it 'prints a deprecation warning if the helper method is used as a macro' do
+        expect(RSpec).to receive(:deprecate).with(/define_match/, an_instance_of(Hash))
+        matcher.for_expected(:use_as_macro).matches?([2, 3])
+      end
+
+      it 'includes the call site in the deprecation warning' do
+        expect_deprecation_with_call_site(__FILE__, line)
+        matcher.for_expected(:use_as_macro).matches?([2, 3])
+      end
+    end
+
+    context "when a module of helper methods is included" do
+      include_examples "accessing an instance helper method" do
+        let(:matcher) do
+          RSpec::Matchers::DSL::Matcher.new(:sum_to) do |sum|
+            include Module.new {
+              def sum_of(x, y) x + y end
+              def define_match() match {} end
+            }
+
+            if sum == :use_as_macro
+              define_match
+            else
+              match { |summands| sum_of(*summands) == sum }
+            end
+          end
+        end
+        let(:line) { __LINE__ - 6 }
+      end
+    end
+
+    context "when a helper method is defined using `def foo`" do
+      include_examples "accessing an instance helper method" do
+        let(:matcher) do
+          RSpec::Matchers::DSL::Matcher.new(:sum_to) do |sum|
+            def sum_of(x, y) x + y end
+            def define_match() match {} end
+
+            if sum == :use_as_macro
+              define_match
+            else
+              match { |summands| sum_of(*summands) == sum }
+            end
+          end
+        end
+        let(:line) { __LINE__ - 6 }
+      end
+    end
+
     context 'when multiple instances of the same matcher are used in the same example' do
       RSpec::Matchers.define(:be_like_a) do |expected|
         match { |actual| actual == expected }
@@ -377,7 +501,7 @@ def assert_equal(a,b)
         let(:matcher) do
           m = mod
           RSpec::Matchers::DSL::Matcher.new :equal do |expected|
-            extend m
+            include m
             match_unless_raises UnexpectedError do
               assert_equal expected, actual
             end