Emit a special warning when nil is passed to raise_error (#1143)

* Add usage examples

* Add specific warning for nil passed to raise_error

Usage of `nil` with `raise_error` is with a high chance unintentional,
including negation.

fixes #1142

lib/rspec/matchers.rb

@@ -316,9 +316,9 @@ def be_truthy
     def be_falsey
       BuiltIn::BeFalsey.new
     end
-    alias_matcher :be_falsy,       :be_falsey
+    alias_matcher :be_falsy, :be_falsey
     alias_matcher :a_falsey_value, :be_falsey
-    alias_matcher :a_falsy_value,  :be_falsey
+    alias_matcher :a_falsy_value, :be_falsey
 
     # Passes if actual is nil
     def be_nil
@@ -379,7 +379,7 @@ def be_a_kind_of(expected)
       BuiltIn::BeAKindOf.new(expected)
     end
     alias_method :be_kind_of, :be_a_kind_of
-    alias_matcher :a_kind_of,  :be_a_kind_of
+    alias_matcher :a_kind_of, :be_a_kind_of
 
     # Passes if actual.between?(min, max). Works with any Comparable object,
     # including String, Symbol, Time, or Numeric (Fixnum, Bignum, Integer,
@@ -406,7 +406,7 @@ def be_within(delta)
       BuiltIn::BeWithin.new(delta)
     end
     alias_matcher :a_value_within, :be_within
-    alias_matcher :within,         :be_within
+    alias_matcher :within, :be_within
 
     # Applied to a proc, specifies that its execution will cause some value to
     # change.
@@ -492,8 +492,8 @@ def be_within(delta)
     def change(receiver=nil, message=nil, &block)
       BuiltIn::Change.new(receiver, message, &block)
     end
-    alias_matcher :a_block_changing,  :change
-    alias_matcher :changing,          :change
+    alias_matcher :a_block_changing, :change
+    alias_matcher :changing, :change
 
     # Passes if actual contains all of the expected regardless of order.
     # This works for collections. Pass in multiple args and it will only
@@ -511,7 +511,7 @@ def contain_exactly(*items)
       BuiltIn::ContainExactly.new(items)
     end
     alias_matcher :a_collection_containing_exactly, :contain_exactly
-    alias_matcher :containing_exactly,              :contain_exactly
+    alias_matcher :containing_exactly, :contain_exactly
 
     # Passes if actual covers expected. This works for
     # Ranges. You can also pass in multiple args
@@ -529,7 +529,7 @@ def cover(*values)
       BuiltIn::Cover.new(*values)
     end
     alias_matcher :a_range_covering, :cover
-    alias_matcher :covering,         :cover
+    alias_matcher :covering, :cover
 
     # Matches if the actual value ends with the expected value(s). In the case
     # of a string, matches against the last `expected.length` characters of the
@@ -544,8 +544,8 @@ def end_with(*expected)
       BuiltIn::EndWith.new(*expected)
     end
     alias_matcher :a_collection_ending_with, :end_with
-    alias_matcher :a_string_ending_with,     :end_with
-    alias_matcher :ending_with,              :end_with
+    alias_matcher :a_string_ending_with, :end_with
+    alias_matcher :ending_with, :end_with
 
     # Passes if <tt>actual == expected</tt>.
     #
@@ -559,7 +559,7 @@ def eq(expected)
       BuiltIn::Eq.new(expected)
     end
     alias_matcher :an_object_eq_to, :eq
-    alias_matcher :eq_to,           :eq
+    alias_matcher :eq_to, :eq
 
     # Passes if `actual.eql?(expected)`
     #
@@ -573,7 +573,7 @@ def eql(expected)
       BuiltIn::Eql.new(expected)
     end
     alias_matcher :an_object_eql_to, :eql
-    alias_matcher :eql_to,           :eql
+    alias_matcher :eql_to, :eql
 
     # Passes if <tt>actual.equal?(expected)</tt> (object identity).
     #
@@ -587,7 +587,7 @@ def equal(expected)
       BuiltIn::Equal.new(expected)
     end
     alias_matcher :an_object_equal_to, :equal
-    alias_matcher :equal_to,           :equal
+    alias_matcher :equal_to, :equal
 
     # Passes if `actual.exist?` or `actual.exists?`
     #
@@ -597,7 +597,7 @@ def exist(*args)
       BuiltIn::Exist.new(*args)
     end
     alias_matcher :an_object_existing, :exist
-    alias_matcher :existing,           :exist
+    alias_matcher :existing, :exist
 
     # Passes if actual's attribute values match the expected attributes hash.
     # This works no matter how you define your attribute readers.
@@ -617,7 +617,7 @@ def have_attributes(expected)
       BuiltIn::HaveAttributes.new(expected)
     end
     alias_matcher :an_object_having_attributes, :have_attributes
-    alias_matcher :having_attributes,           :have_attributes
+    alias_matcher :having_attributes, :have_attributes
 
     # Passes if actual includes expected. This works for
     # collections and Strings. You can also pass in multiple args
@@ -640,9 +640,9 @@ def include(*expected)
       BuiltIn::Include.new(*expected)
     end
     alias_matcher :a_collection_including, :include
-    alias_matcher :a_string_including,     :include
-    alias_matcher :a_hash_including,       :include
-    alias_matcher :including,              :include
+    alias_matcher :a_string_including, :include
+    alias_matcher :a_hash_including, :include
+    alias_matcher :including, :include
 
     # Passes if the provided matcher passes when checked against all
     # elements of the collection.
@@ -697,10 +697,10 @@ def all(expected)
     def match(expected)
       BuiltIn::Match.new(expected)
     end
-    alias_matcher :match_regex,        :match
+    alias_matcher :match_regex, :match
     alias_matcher :an_object_matching, :match
-    alias_matcher :a_string_matching,  :match
-    alias_matcher :matching,           :match
+    alias_matcher :a_string_matching, :match
+    alias_matcher :matching, :match
 
     # An alternate form of `contain_exactly` that accepts
     # the expected contents as a single array arg rather
@@ -761,20 +761,22 @@ def output(expected=nil)
     #   expect { do_something_risky }.to raise_error
     #   expect { do_something_risky }.to raise_error(PoorRiskDecisionError)
     #   expect { do_something_risky }.to raise_error(PoorRiskDecisionError) { |error| expect(error.data).to eq 42 }
+    #   expect { do_something_risky }.to raise_error { |error| expect(error.data).to eq 42 }
     #   expect { do_something_risky }.to raise_error(PoorRiskDecisionError, "that was too risky")
     #   expect { do_something_risky }.to raise_error(PoorRiskDecisionError, /oo ri/)
+    #   expect { do_something_risky }.to raise_error("that was too risky")
     #
     #   expect { do_something_risky }.not_to raise_error
-    def raise_error(error=nil, message=nil, &block)
+    def raise_error(error=BuiltIn::RaiseError::UndefinedValue, message=nil, &block)
       BuiltIn::RaiseError.new(error, message, &block)
     end
-    alias_method :raise_exception,  :raise_error
+    alias_method :raise_exception, :raise_error
 
-    alias_matcher :a_block_raising,  :raise_error do |desc|
+    alias_matcher :a_block_raising, :raise_error do |desc|
       desc.sub("raise", "a block raising")
     end
 
-    alias_matcher :raising,        :raise_error do |desc|
+    alias_matcher :raising, :raise_error do |desc|
       desc.sub("raise", "raising")
     end
 
@@ -788,7 +790,7 @@ def respond_to(*names)
       BuiltIn::RespondTo.new(*names)
     end
     alias_matcher :an_object_responding_to, :respond_to
-    alias_matcher :responding_to,           :respond_to
+    alias_matcher :responding_to, :respond_to
 
     # Passes if the submitted block returns true. Yields target to the
     # block.
@@ -809,7 +811,7 @@ def satisfy(description=nil, &block)
       BuiltIn::Satisfy.new(description, &block)
     end
     alias_matcher :an_object_satisfying, :satisfy
-    alias_matcher :satisfying,           :satisfy
+    alias_matcher :satisfying, :satisfy
 
     # Matches if the actual value starts with the expected value(s). In the
     # case of a string, matches against the first `expected.length` characters
@@ -824,8 +826,8 @@ def start_with(*expected)
       BuiltIn::StartWith.new(*expected)
     end
     alias_matcher :a_collection_starting_with, :start_with
-    alias_matcher :a_string_starting_with,     :start_with
-    alias_matcher :starting_with,              :start_with
+    alias_matcher :a_string_starting_with, :start_with
+    alias_matcher :starting_with, :start_with
 
     # Given no argument, matches if a proc throws any Symbol.
     #
@@ -850,7 +852,7 @@ def throw_symbol(expected_symbol=nil, expected_arg=nil)
       desc.sub("throw", "a block throwing")
     end
 
-    alias_matcher :throwing,        :throw_symbol do |desc|
+    alias_matcher :throwing, :throw_symbol do |desc|
       desc.sub("throw", "throwing")
     end
 
@@ -866,8 +868,8 @@ def throw_symbol(expected_symbol=nil, expected_arg=nil)
     def yield_control
       BuiltIn::YieldControl.new
     end
-    alias_matcher :a_block_yielding_control,  :yield_control
-    alias_matcher :yielding_control,          :yield_control
+    alias_matcher :a_block_yielding_control, :yield_control
+    alias_matcher :yielding_control, :yield_control
 
     # Passes if the method called in the expect block yields with
     # no arguments. Fails if it does not yield, or yields with arguments.
@@ -884,8 +886,8 @@ def yield_control
     def yield_with_no_args
       BuiltIn::YieldWithNoArgs.new
     end
-    alias_matcher :a_block_yielding_with_no_args,  :yield_with_no_args
-    alias_matcher :yielding_with_no_args,          :yield_with_no_args
+    alias_matcher :a_block_yielding_with_no_args, :yield_with_no_args
+    alias_matcher :yielding_with_no_args, :yield_with_no_args
 
     # Given no arguments, matches if the method called in the expect
     # block yields with arguments (regardless of what they are or how
@@ -914,8 +916,8 @@ def yield_with_no_args
     def yield_with_args(*args)
       BuiltIn::YieldWithArgs.new(*args)
     end
-    alias_matcher :a_block_yielding_with_args,  :yield_with_args
-    alias_matcher :yielding_with_args,          :yield_with_args
+    alias_matcher :a_block_yielding_with_args, :yield_with_args
+    alias_matcher :yielding_with_args, :yield_with_args
 
     # Designed for use with methods that repeatedly yield (such as
     # iterators). Passes if the method called in the expect block yields
@@ -935,8 +937,8 @@ def yield_with_args(*args)
     def yield_successive_args(*args)
       BuiltIn::YieldSuccessiveArgs.new(*args)
     end
-    alias_matcher :a_block_yielding_successive_args,  :yield_successive_args
-    alias_matcher :yielding_successive_args,          :yield_successive_args
+    alias_matcher :a_block_yielding_successive_args, :yield_successive_args
+    alias_matcher :yielding_successive_args, :yield_successive_args
 
     # Designed for use with methods that repeatedly yield (such as
     # iterators). Passes if the method called in the expect block yields

lib/rspec/matchers/built_in/raise_error.rb

@@ -9,13 +9,20 @@ module BuiltIn
       class RaiseError
         include Composable
 
-        def initialize(expected_error_or_message=nil, expected_message=nil, &block)
+        # Used as a sentinel value to be able to tell when the user did not pass an
+        # argument. We can't use `nil` for that because we need to warn when `nil` is
+        # passed in a different way. It's an Object, not a Module, since Module's `===`
+        # does not evaluate to true when compared to itself.
+        UndefinedValue = Object.new.freeze
+
+        def initialize(expected_error_or_message, expected_message, &block)
           @block = block
           @actual_error = nil
-          @warn_about_bare_error = expected_error_or_message.nil?
+          @warn_about_bare_error = UndefinedValue === expected_error_or_message
+          @warn_about_nil_error = expected_error_or_message.nil?
 
           case expected_error_or_message
-          when nil
+          when nil, UndefinedValue
             @expected_error = Exception
             @expected_message = expected_message
           when String
@@ -58,16 +65,19 @@ def matches?(given_proc, negative_expectation=false, &block)
             end
           end
 
-          warn_about_bare_error if warning_about_bare_error && !negative_expectation
-          eval_block if !negative_expectation && ready_to_eval_block?
+          unless negative_expectation
+            warn_about_bare_error! if warn_about_bare_error?
+            warn_about_nil_error! if warn_about_nil_error?
+            eval_block if ready_to_eval_block?
+          end
 
           expectation_matched?
         end
         # rubocop:enable MethodLength
 
         # @private
         def does_not_match?(given_proc)
-          warn_for_false_positives
+          warn_for_negative_false_positives!
           !matches?(given_proc, :negative_expectation) && Proc === given_proc
         end
 
@@ -131,29 +141,35 @@ def verify_message
           values_match?(@expected_message, @actual_error.message.to_s)
         end
 
-        def warn_for_false_positives
+        def warn_for_negative_false_positives!
           expression = if expecting_specific_exception? && @expected_message
                          "`expect { }.not_to raise_error(SpecificErrorClass, message)`"
                        elsif expecting_specific_exception?
                          "`expect { }.not_to raise_error(SpecificErrorClass)`"
                        elsif @expected_message
                          "`expect { }.not_to raise_error(message)`"
+                       elsif @warn_about_nil_error
+                         "`expect { }.not_to raise_error(nil)`"
                        end
 
           return unless expression
 
-          warn_about_negative_false_positive expression
+          warn_about_negative_false_positive! expression
         end
 
         def handle_warning(message)
           RSpec::Expectations.configuration.false_positives_handler.call(message)
         end
 
-        def warning_about_bare_error
+        def warn_about_bare_error?
           @warn_about_bare_error && @block.nil?
         end
 
-        def warn_about_bare_error
+        def warn_about_nil_error?
+          @warn_about_nil_error
+        end
+
+        def warn_about_bare_error!
           handle_warning("Using the `raise_error` matcher without providing a specific " \
                          "error or message risks false positives, since `raise_error` " \
                          "will match when Ruby raises a `NoMethodError`, `NameError` or " \
@@ -166,11 +182,24 @@ def warn_about_bare_error
                          "_positives = :nothing`")
         end
 
-        def warn_about_negative_false_positive(expression)
+        def warn_about_nil_error!
+          handle_warning("Using the `raise_error` matcher with a `nil` error is probably " \
+                         "unintentional, it risks false positives, since `raise_error` " \
+                         "will match when Ruby raises a `NoMethodError`, `NameError` or " \
+                         "`ArgumentError`, potentially allowing the expectation to pass " \
+                         "without even executing the method you are intending to call. " \
+                         "#{warning}"\
+                         "Instead consider providing a specific error class or message. " \
+                         "This message can be suppressed by setting: " \
+                         "`RSpec::Expectations.configuration.on_potential_false" \
+                         "_positives = :nothing`")
+        end
+
+        def warn_about_negative_false_positive!(expression)
           handle_warning("Using #{expression} risks false positives, since literally " \
                          "any other error would cause the expectation to pass, " \
-                         "including those raised by Ruby (e.g. NoMethodError, NameError " \
-                         "and ArgumentError), meaning the code you are intending to test " \
+                         "including those raised by Ruby (e.g. `NoMethodError`, `NameError` " \
+                         "and `ArgumentError`), meaning the code you are intending to test " \
                          "may not even get reached. Instead consider using " \
                          "`expect { }.not_to raise_error` or `expect { }.to raise_error" \
                          "(DifferentSpecificErrorClass)`. This message can be suppressed by " \

spec/rspec/matchers/built_in/raise_error_spec.rb

@@ -18,6 +18,16 @@
     expect { raise StandardError.new, 'boom' }.to raise_error
   end
 
+  it "issues a warning when `nil` is passed for an error class" do
+    expect_warning_with_call_site __FILE__, __LINE__+1, /with a `nil`/
+    expect { raise }.to raise_error(nil)
+  end
+
+  it "issues a warning when `nil` is passed for an error class when negated" do
+    expect_warning_with_call_site __FILE__, __LINE__+1, /raise_error\(nil\)/
+    expect { '' }.not_to raise_error(nil)
+  end
+
   it "issues a warning that does not include current error when it's not present" do
     expect(::Kernel).to receive(:warn) do |message|
       ex = /Actual error raised was/