[GR-18163] Fix Kernel#raise and don't override cause

PullRequest: truffleruby/4518

Author: andrykonchin

CHANGELOG.md

@@ -9,6 +9,7 @@ Bug fixes:
 
 Compatibility:
 
+* Support Timezone argument to `Time.{new,at}` and `Time#{getlocal,localtime}` (#1717, @patricklinpl, @manefz, @rwstauner).
 * Updated to Ruby 3.3.7 (@andrykonchin).
 * Implement `StringScanner#{peek_byte,scan_byte,scan_integer,named_captures}` methods (#3788, @andrykonchin).
 * Support String patterns in `StringScanner#{exist?,scan_until,skip_until,check_until,search_full}` methods (@andrykonchin).
@@ -18,6 +19,7 @@ Compatibility:
 * Support Digest plugins (#1390, @nirvdrum).
 * Joni has been updated from 2.2.1 to 2.2.6 (@andrykonchin).
 * Fix numeric coercing when `#coerce` method is not public (#3848, @andrykonchin).
+* Fix `Kernel#raise` and don't override `cause` at exception re-raising (#3831, @andrykonchin).
 
 Performance:
 
@@ -91,7 +93,6 @@ Compatibility:
 * Fix `Module#remove_const` and emit warning when constant is deprecated (@andrykonchin).
 * Add `Module#set_temporary_name` (#3681, @andrykonchin).
 * Modify `Float#round` to match MRI behavior (#3676, @andrykonchin).
-* Support Timezone argument to `Time.{new,at}` and `Time#{getlocal,localtime}` (#1717, @patricklinpl, @manefz, @rwstauner).
 
 Performance:
 

spec/ruby/core/kernel/raise_spec.rb

@@ -44,7 +44,53 @@
 
   it "raises an ArgumentError when only cause is given" do
     cause = StandardError.new
-    -> { raise(cause: cause) }.should raise_error(ArgumentError)
+    -> { raise(cause: cause) }.should raise_error(ArgumentError, "only cause is given with no arguments")
+  end
+
+  it "raises an ArgumentError when only cause is given even if it has nil value" do
+    -> { raise(cause: nil) }.should raise_error(ArgumentError, "only cause is given with no arguments")
+  end
+
+  it "raises an ArgumentError when given cause is not an instance of Exception" do
+    -> { raise "message", cause: Object.new }.should raise_error(TypeError, "exception object expected")
+  end
+
+  it "doesn't raise an ArgumentError when given cause is nil" do
+    -> { raise "message", cause: nil }.should raise_error(RuntimeError, "message")
+  end
+
+  it "allows cause equal an exception" do
+    e = RuntimeError.new("message")
+    -> { raise e, cause: e }.should raise_error(e)
+  end
+
+  it "doesn't set given cause when it equals an exception" do
+    e = RuntimeError.new("message")
+
+    begin
+      raise e, cause: e
+    rescue
+    end
+
+    e.cause.should == nil
+  end
+
+  it "raises ArgumentError when exception is part of the cause chain" do
+    -> {
+      begin
+        raise "Error 1"
+      rescue => e1
+        begin
+          raise "Error 2"
+        rescue => e2
+          begin
+            raise "Error 3"
+          rescue => e3
+            raise e1, cause: e3
+          end
+        end
+      end
+    }.should raise_error(ArgumentError, "circular causes")
   end
 
   it "re-raises a rescued exception" do
@@ -62,6 +108,101 @@
       end
     end.should raise_error(StandardError, "aaa")
   end
+
+  it "re-raises a previously rescued exception without overwriting the cause" do
+    begin
+      begin
+        begin
+          begin
+            raise "Error 1"
+          rescue => e1
+            raise "Error 2"
+          end
+        rescue => e2
+          raise "Error 3"
+        end
+      rescue
+        e2.cause.should == e1
+        raise e2
+      end
+    rescue => e
+      e.cause.should == e1
+    end
+  end
+
+  it "re-raises a previously rescued exception with overwriting the cause when it's explicitly specified with :cause option" do
+    e4 = RuntimeError.new("Error 4")
+
+    begin
+      begin
+        begin
+          begin
+            raise "Error 1"
+          rescue => e1
+            raise "Error 2"
+          end
+        rescue => e2
+          raise "Error 3"
+        end
+      rescue
+        e2.cause.should == e1
+        raise e2, cause: e4
+      end
+    rescue => e
+      e.cause.should == e4
+    end
+  end
+
+  it "re-raises a previously rescued exception without overwriting the cause when it's explicitly specified with :cause option and has nil value" do
+    begin
+      begin
+        begin
+          begin
+            raise "Error 1"
+          rescue => e1
+            raise "Error 2"
+          end
+        rescue => e2
+          raise "Error 3"
+        end
+      rescue
+        e2.cause.should == e1
+        raise e2, cause: nil
+      end
+    rescue => e
+      e.cause.should == e1
+    end
+  end
+
+  it "re-raises a previously rescued exception without setting a cause implicitly" do
+    begin
+      begin
+        raise "Error 1"
+      rescue => e1
+        raise
+      end
+    rescue => e
+      e.should == e1
+      e.cause.should == nil
+    end
+  end
+
+  it "re-raises a previously rescued exception that has a cause without setting a cause implicitly" do
+    begin
+      begin
+        raise "Error 1"
+      rescue => e1
+        begin
+          raise "Error 2"
+        rescue => e2
+          raise
+        end
+      end
+    rescue => e
+      e.should == e2
+      e.cause.should == e1
+    end
+  end
 end
 
 describe "Kernel#raise" do

spec/ruby/shared/kernel/raise.rb

@@ -49,21 +49,6 @@ def initialize(data)
     end
   end
 
-  it "does not allow message and extra keyword arguments" do
-    data_error = Class.new(StandardError) do
-      attr_reader :data
-      def initialize(data)
-        @data = data
-      end
-    end
-
-    -> { @object.raise(data_error, {a: 1}, b: 2) }.should raise_error(StandardError) do |e|
-      [TypeError, ArgumentError].should.include?(e.class)
-    end
-
-    -> { @object.raise(data_error, {a: 1}, [], b: 2) }.should raise_error(ArgumentError)
-  end
-
   it "raises RuntimeError if no exception class is given" do
     -> { @object.raise }.should raise_error(RuntimeError, "")
   end
@@ -74,7 +59,7 @@ def initialize(data)
   end
 
   it "raises a RuntimeError if string given" do
-    -> { @object.raise("a bad thing") }.should raise_error(RuntimeError)
+    -> { @object.raise("a bad thing") }.should raise_error(RuntimeError, "a bad thing")
   end
 
   it "passes no arguments to the constructor when given only an exception class" do
@@ -86,19 +71,32 @@ def initialize
   end
 
   it "raises a TypeError when passed a non-Exception object" do
-    -> { @object.raise(Object.new) }.should raise_error(TypeError)
+    -> { @object.raise(Object.new) }.should raise_error(TypeError, "exception class/object expected")
+    -> { @object.raise(Object.new, "message") }.should raise_error(TypeError, "exception class/object expected")
+    -> { @object.raise(Object.new, "message", []) }.should raise_error(TypeError, "exception class/object expected")
   end
 
   it "raises a TypeError when passed true" do
-    -> { @object.raise(true) }.should raise_error(TypeError)
+    -> { @object.raise(true) }.should raise_error(TypeError, "exception class/object expected")
   end
 
   it "raises a TypeError when passed false" do
-    -> { @object.raise(false) }.should raise_error(TypeError)
+    -> { @object.raise(false) }.should raise_error(TypeError, "exception class/object expected")
   end
 
   it "raises a TypeError when passed nil" do
-    -> { @object.raise(nil) }.should raise_error(TypeError)
+    -> { @object.raise(nil) }.should raise_error(TypeError, "exception class/object expected")
+  end
+
+  it "raises TypeError when passed a non-Exception object but it responds to #exception method that doesn't return an instance of Exception class" do
+    e = Object.new
+    def e.exception
+      Array
+    end
+
+    -> {
+      @object.raise e
+    }.should raise_error(TypeError, "exception object expected")
   end
 
   it "re-raises a previously rescued exception without overwriting the backtrace" do

src/main/ruby/truffleruby/core/kernel.rb

@@ -686,29 +686,41 @@ def warn(*messages, uplevel: undefined, category: nil)
 
   def raise(exc = undefined, msg = undefined, ctx = nil, cause: undefined, **kwargs)
     cause_given = !Primitive.undefined?(cause)
-    cause = cause_given ? cause : $!
 
-    if Primitive.undefined?(exc) and cause
-      raise ArgumentError, 'only cause is given with no arguments' if cause_given
-      exc = cause
+    if Primitive.undefined?(exc) && cause_given
+      raise ArgumentError, 'only cause is given with no arguments'
+    end
+
+    if Primitive.undefined?(exc) && $!
+      exc = $!
     else
-      unless kwargs.empty?
-        if Primitive.undefined?(msg)
-          msg = kwargs
-        else
-          raise ArgumentError, 'cannot give both message and extra keyword arguments'
+      if Primitive.undefined?(msg) && !kwargs.empty?
+        msg = kwargs
+      end
+
+      if cause_given
+        unless Primitive.is_a?(cause, ::Exception) || Primitive.nil?(cause)
+          Truffle::ExceptionOperations.exception_object_expected!
         end
+      else
+        cause = $!
       end
 
       exc = Truffle::ExceptionOperations.build_exception_for_raise(exc, msg)
 
       exc.set_backtrace(ctx) if ctx
       Primitive.exception_capture_backtrace(exc, 1) unless Truffle::ExceptionOperations.backtrace?(exc)
-      Primitive.exception_set_cause exc, cause unless Primitive.equal?(exc, cause)
+
+      if !Primitive.nil?(cause) && (cause_given || Primitive.nil?(exc.cause)) && !Primitive.equal?(cause, exc)
+        if Truffle::ExceptionOperations.circular_cause?(cause, exc)
+          raise ArgumentError, 'circular causes'
+        end
+
+        Primitive.exception_set_cause exc, cause
+      end
     end
 
     Truffle::ExceptionOperations.show_exception_for_debug(exc, 1) if $DEBUG
-
     Primitive.vm_raise_exception exc
   end
   module_function :raise

src/main/ruby/truffleruby/core/truffle/exception_operations.rb

@@ -20,7 +20,7 @@ def self.build_exception_for_raise(exc, msg)
           exc = exc.exception msg
         end
 
-        exception_class_object_expected! unless Primitive.is_a?(exc, ::Exception)
+        exception_object_expected! unless Primitive.is_a?(exc, ::Exception)
         exc
       elsif Primitive.is_a?(exc, ::String)
         ::RuntimeError.exception exc
@@ -29,6 +29,14 @@ def self.build_exception_for_raise(exc, msg)
       end
     end
 
+    def self.circular_cause?(cause, exception)
+      while cause && !Primitive.equal?(cause, exception)
+        cause = cause.cause
+      end
+
+      !Primitive.nil?(cause)
+    end
+
     def self.make_exception(args)
       case args.size
       when 0
@@ -50,7 +58,8 @@ def self.make_exception(args)
 
     def self.call_exception(exc, *args)
       res = Truffle::Type.check_funcall(exc, :exception, args)
-      raise TypeError, 'exception class/object expected' if Primitive.undefined?(res) || !Primitive.is_a?(res, Exception)
+      raise TypeError, 'exception class/object expected' if Primitive.undefined?(res)
+      raise TypeError, 'exception object expected' unless Primitive.is_a?(res, Exception)
       res
     end
 
@@ -64,6 +73,16 @@ def self.exception_class_object_expected!
       Primitive.vm_raise_exception exc
     end
 
+    # Avoid using #raise here to prevent infinite recursion
+    def self.exception_object_expected!
+      exc = ::TypeError.new('exception object expected')
+      Primitive.exception_capture_backtrace(exc, 1)
+
+      show_exception_for_debug(exc, 2) if $DEBUG
+
+      Primitive.vm_raise_exception exc
+    end
+
     def self.show_exception_for_debug(exc, uplevel)
       STDERR.puts "Exception: `#{Primitive.class(exc)}' at #{caller(uplevel + 1, 1)[0]} - #{exc.message}\n"
     end