[GR-33908] [GR-34933] Improve handling of foreign exception and make Polyglot::ForeignException a class inheriting from Exception

PullRequest: truffleruby/3210

Author: eregon

doc/user/polyglot.md

@@ -117,16 +117,14 @@ Where boolean value is expected (e.g., in `if` conditions) the foreign value is
 ### Rescuing Foreign Exceptions
 
 Foreign exceptions can be caught by `rescue Polyglot::ForeignException => e` or by `rescue foreign_meta_object`.
-It is possible to rescue any exception (Ruby or foreign) with `rescue Object => e`.
+It is possible to rescue any exception (Ruby or foreign) with `rescue Exception => e`.
 
 This naturally stems from the ancestors of a foreign exception:
 ```ruby
 Java.type("java.lang.RuntimeException").new.class.ancestors
-# => [Polyglot::ForeignExceptionClass, Polyglot::ForeignException, Polyglot::ForeignObject, Object, Kernel, BasicObject]
+# => [Polyglot::ForeignException, Polyglot::ExceptionTrait, Polyglot::ObjectTrait, Exception, Object, Kernel, BasicObject]
 ```
 
-Note `Polyglot::ForeignException` is a module and not class, so to ensure it's included for all foreign exceptions.
-
 ## Accessing Java Objects
 
 TruffleRuby's Java interoperability interface is similar to the interface from the Nashorn JavaScript implementation, as also implemented by GraalVM's JavaScript implementation.

spec/ruby/core/exception/full_message_spec.rb

@@ -15,13 +15,19 @@
   it "supports :highlight option and adds escape sequences to highlight some strings" do
     e = RuntimeError.new("Some runtime error")
 
-    full_message = e.full_message(highlight: true, order: :bottom)
-    full_message.should include "\e[1mTraceback\e[m (most recent call last)"
-    full_message.should include "\e[1mSome runtime error (\e[1;4mRuntimeError\e[m\e[1m)"
+    full_message = e.full_message(highlight: true, order: :top).lines
+    full_message[0].should.end_with? "\e[1mSome runtime error (\e[1;4mRuntimeError\e[m\e[1m)\e[m\n"
 
-    full_message = e.full_message(highlight: false, order: :bottom)
-    full_message.should include "Traceback (most recent call last)"
-    full_message.should include "Some runtime error (RuntimeError)"
+    full_message = e.full_message(highlight: true, order: :bottom).lines
+    full_message[0].should == "\e[1mTraceback\e[m (most recent call last):\n"
+    full_message[-1].should.end_with? "\e[1mSome runtime error (\e[1;4mRuntimeError\e[m\e[1m)\e[m\n"
+
+    full_message = e.full_message(highlight: false, order: :top).lines
+    full_message[0].should.end_with? "Some runtime error (RuntimeError)\n"
+
+    full_message = e.full_message(highlight: false, order: :bottom).lines
+    full_message[0].should == "Traceback (most recent call last):\n"
+    full_message[-1].should.end_with? "Some runtime error (RuntimeError)\n"
   end
 
   it "supports :order option and places the error message and the backtrace at the top or the bottom" do
@@ -35,22 +41,34 @@
   it "shows the caller if the exception has no backtrace" do
     e = RuntimeError.new("Some runtime error")
     e.backtrace.should == nil
-    full_message = e.full_message(highlight: false, order: :top)
-    full_message.should include("#{__FILE__}:#{__LINE__-1}:in `")
-    full_message.should include("': Some runtime error (RuntimeError)\n")
+    full_message = e.full_message(highlight: false, order: :top).lines
+    full_message[0].should.start_with?("#{__FILE__}:#{__LINE__-1}:in `")
+    full_message[0].should.end_with?("': Some runtime error (RuntimeError)\n")
   end
 
   it "shows the exception class at the end of the first line of the message when the message contains multiple lines" do
     begin
       line = __LINE__; raise "first line\nsecond line"
     rescue => e
       full_message = e.full_message(highlight: false, order: :top).lines
-      full_message[0].should include("#{__FILE__}:#{line}:in `")
-      full_message[0].should include(": first line (RuntimeError)\n")
+      full_message[0].should.start_with?("#{__FILE__}:#{line}:in `")
+      full_message[0].should.end_with?(": first line (RuntimeError)\n")
       full_message[1].should == "second line\n"
     end
   end
 
+  it "highlights the entire message when the message contains multiple lines" do
+    begin
+      line = __LINE__; raise "first line\nsecond line\nthird line"
+    rescue => e
+      full_message = e.full_message(highlight: true, order: :top).lines
+      full_message[0].should.start_with?("#{__FILE__}:#{line}:in `")
+      full_message[0].should.end_with?(": \e[1mfirst line (\e[1;4mRuntimeError\e[m\e[1m)\e[m\n")
+      full_message[1].should == "\e[1msecond line\e[m\n"
+      full_message[2].should == "\e[1mthird line\e[m\n"
+    end
+  end
+
   it "contains cause of exception" do
     begin
       begin

spec/ruby/core/thread/shared/exit.rb

@@ -66,14 +66,34 @@
     ScratchPad.recorded.should == nil
   end
 
+  it "does not reset $!" do
+    ScratchPad.record []
+
+    exc = RuntimeError.new("foo")
+    thread = Thread.new do
+      begin
+        raise exc
+      ensure
+        ScratchPad << $!
+        begin
+          Thread.current.send(@method)
+        ensure
+          ScratchPad << $!
+        end
+      end
+    end
+    thread.join
+    ScratchPad.recorded.should == [exc, exc]
+  end
+
   it "cannot be rescued" do
     thread = Thread.new do
       begin
         Thread.current.send(@method)
       rescue Exception
         ScratchPad.record :in_rescue
       end
-     ScratchPad.record :end_of_thread_block
+      ScratchPad.record :end_of_thread_block
     end
 
     thread.join

spec/tags/truffle/interop/polyglot/foreign_exception_tags.txt

@@ -0,0 +1 @@
+slow:Polyglot::ForeignException when reaching the top-level is printed like a Ruby exception

spec/truffle/interop/foreign_inspect_to_s_spec.rb

@@ -51,8 +51,8 @@
         -> {
           integer_class.valueOf("abc")
         }.should raise_error(Polyglot::ForeignException) { |exc|
-          exc.inspect.should =~ /\A#<Polyglot::ForeignExceptionClass\[Java\] java\.lang\.NumberFormatException:0x\h+: For input string: "abc">\z/
-          exc.to_s.should == '#<Polyglot::ForeignExceptionClass[Java] java.lang.NumberFormatException: For input string: "abc">'
+          exc.inspect.should =~ /\A#<Polyglot::ForeignException\[Java\] java\.lang\.NumberFormatException:0x\h+: For input string: "abc">\z/
+          exc.to_s.should == '#<Polyglot::ForeignException[Java] java.lang.NumberFormatException: For input string: "abc">'
         }
       end
     end
@@ -145,8 +145,8 @@
   describe "exception" do
     it "gives a similar representation to Ruby" do
       exc = Truffle::Debug.foreign_exception("foo")
-      exc.inspect.should =~ /\A#<Polyglot::ForeignExceptionClass:0x\h+: foo>\z/
-      exc.to_s.should == '#<Polyglot::ForeignExceptionClass [foreign exception]>'
+      exc.inspect.should =~ /\A#<Polyglot::ForeignException:0x\h+: foo>\z/
+      exc.to_s.should == '#<Polyglot::ForeignException [foreign exception]>'
     end
   end
 

spec/truffle/interop/polyglot/class_spec.rb

@@ -14,7 +14,7 @@
 
     Truffle::Debug.foreign_hash.class.should == Polyglot::ForeignHash
     Truffle::Debug.foreign_array.class.should == Polyglot::ForeignArray
-    Truffle::Debug.foreign_exception("").class.should == Polyglot::ForeignExceptionClass
+    Truffle::Debug.foreign_exception("").class.should == Polyglot::ForeignException
     Truffle::Debug.foreign_executable(14).class.should == Polyglot::ForeignExecutable
     # ForeignClass, ForeignMetaObject
     Truffle::Debug.foreign_iterable.class.should == Polyglot::ForeignIterable
@@ -31,7 +31,7 @@
   it "gives a Ruby Class to Ruby objects behind a foreign proxy" do
     Truffle::Interop.proxy_foreign_object({}).class.should == Polyglot::ForeignHashIterable
     Truffle::Interop.proxy_foreign_object([1, 2, 3]).class.should == Polyglot::ForeignArray
-    Truffle::Interop.proxy_foreign_object(Exception.new("")).class.should == Polyglot::ForeignExceptionClass
+    Truffle::Interop.proxy_foreign_object(Exception.new("")).class.should == Polyglot::ForeignException
     Truffle::Interop.proxy_foreign_object(-> { nil }).class.should == Polyglot::ForeignExecutable
     Truffle::Interop.proxy_foreign_object(String).class.should == Polyglot::ForeignClass
     Truffle::Interop.proxy_foreign_object(Enumerable).class.should == Polyglot::ForeignMetaObject
@@ -51,7 +51,7 @@
       Truffle::Interop.to_java_map({ a: 1 }).class.should == Polyglot::ForeignHash
       Truffle::Interop.to_java_array([1, 2, 3]).class.should == Polyglot::ForeignArray
       Truffle::Interop.to_java_list([1, 2, 3]).class.should == Polyglot::ForeignArray
-      Java.type('java.lang.RuntimeException').new.class.should == Polyglot::ForeignExceptionClass
+      Java.type('java.lang.RuntimeException').new.class.should == Polyglot::ForeignException
       # ForeignExecutable
       Java.type('java.math.BigInteger').class.should == Polyglot::ForeignClass
       Java.type('java.math.BigInteger')[:class].class.should == Polyglot::ForeignClass

spec/truffle/interop/polyglot/foreign_exception_spec.rb

@@ -13,6 +13,11 @@
     @foreign = Truffle::Debug.foreign_exception("exception message")
   end
 
+  it "subclasses Exception" do
+    @foreign.class.should == Polyglot::ForeignException
+    @foreign.class.superclass.should == Exception
+  end
+
   it "supports #message" do
     @foreign.message.should == "exception message"
   end
@@ -21,6 +26,15 @@
     @foreign.cause.should == nil
   end
 
+  it "supports #full_message" do
+    -> {
+      raise @foreign
+    }.should raise_error(Polyglot::ForeignException) {
+      full_message = @foreign.full_message(highlight: false, order: :top).lines
+      full_message[0].should == "#{__FILE__}:#{__LINE__-3}:in `Kernel#raise': exception message (Polyglot::ForeignException)\n"
+    }
+  end
+
   it "supports rescue Polyglot::ForeignException" do
     begin
       raise @foreign
@@ -29,6 +43,14 @@
     end
   end
 
+  it "supports rescue Exception" do
+    begin
+      raise @foreign
+    rescue Exception => e # rubocop:disable Lint/RescueException
+      e.should.equal?(@foreign)
+    end
+  end
+
   it "supports rescue Object" do
     begin
       raise @foreign
@@ -67,4 +89,16 @@
       entry.label.should.is_a?(String)
     end
   end
+
+  describe "when reaching the top-level" do
+    it "is printed like a Ruby exception" do
+      out = ruby_exe('raise Truffle::Debug.foreign_exception "main"', args: "2>&1", exit_status: 1, escape: false)
+      out.should == "-e:1:in `Kernel#raise': main (Polyglot::ForeignException)\n" \
+        "\tfrom -e:1:in `<main>'\n"
+
+      out = ruby_exe('at_exit { raise Truffle::Debug.foreign_exception "at exit" }', args: "2>&1", exit_status: 1, escape: false)
+      out.should == "-e:1:in `Kernel#raise': at exit (Polyglot::ForeignException)\n" \
+        "\tfrom -e:1:in `block in <main>'\n"
+    end
+  end
 end

src/main/java/org/truffleruby/RubyContext.java

@@ -29,6 +29,7 @@
 import com.oracle.truffle.api.RootCallTarget;
 import com.oracle.truffle.api.TruffleLanguage.ContextReference;
 import com.oracle.truffle.api.TruffleLogger;
+import com.oracle.truffle.api.exception.AbstractTruffleException;
 import com.oracle.truffle.api.nodes.EncapsulatingNodeReference;
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.utilities.AssumedValue;
@@ -66,7 +67,6 @@
 import org.truffleruby.language.RubyBaseNode;
 import org.truffleruby.language.SafepointManager;
 import org.truffleruby.language.backtrace.BacktraceFormatter;
-import org.truffleruby.language.control.RaiseException;
 import org.truffleruby.language.dispatch.DispatchNode;
 import org.truffleruby.language.globals.GlobalVariableStorage;
 import org.truffleruby.language.loader.CodeLoader;
@@ -334,9 +334,9 @@ protected boolean patch(Env newEnv) {
     protected boolean patchContext(Env newEnv) {
         try {
             return patch(newEnv);
-        } catch (RaiseException e) {
+        } catch (AbstractTruffleException e) {
             getDefaultBacktraceFormatter()
-                    .printRubyExceptionOnEnvStderr("Exception during RubyContext.patch():\n", e.getException());
+                    .printRubyExceptionOnEnvStderr("Exception during RubyContext.patch():\n", e);
             throw e;
         } catch (Throwable e) {
             System.err.println("Exception during RubyContext.patch():");

src/main/java/org/truffleruby/core/CoreLibrary.java

@@ -23,6 +23,7 @@
 import java.util.concurrent.ConcurrentMap;
 
 import com.oracle.truffle.api.RootCallTarget;
+import com.oracle.truffle.api.exception.AbstractTruffleException;
 import org.graalvm.collections.Pair;
 import org.jcodings.specific.USASCIIEncoding;
 import org.jcodings.transcode.EConvFlags;
@@ -54,7 +55,6 @@
 import org.truffleruby.core.string.ImmutableRubyString;
 import org.truffleruby.language.Nil;
 import org.truffleruby.language.NotProvided;
-import org.truffleruby.language.control.RaiseException;
 import org.truffleruby.language.globals.GlobalVariableReader;
 import org.truffleruby.language.globals.GlobalVariables;
 import org.truffleruby.language.loader.CodeLoader;
@@ -786,10 +786,9 @@ public void loadRubyCoreLibraryAndPostBoot() {
             }
         } catch (IOException e) {
             throw CompilerDirectives.shouldNotReachHere(e);
-        } catch (RaiseException e) {
-            context.getDefaultBacktraceFormatter().printRubyExceptionOnEnvStderr(
-                    "Exception while loading core library:\n",
-                    e.getException());
+        } catch (AbstractTruffleException e) {
+            context.getDefaultBacktraceFormatter()
+                    .printRubyExceptionOnEnvStderr("Exception while loading core library:\n", e);
             throw CompilerDirectives.shouldNotReachHere("couldn't load the core library", e);
         }
     }

src/main/java/org/truffleruby/core/VMPrimitiveNodes.java

@@ -208,15 +208,17 @@ public abstract static class VMRaiseExceptionNode extends PrimitiveArrayArgument
         protected Object vmRaiseException(RubyException exception,
                 @Cached ConditionProfile reRaiseProfile) {
             final Backtrace backtrace = exception.backtrace;
-            if (reRaiseProfile.profile(backtrace != null && backtrace.getRaiseException() != null)) {
+            RaiseException raiseException = null;
+            if (reRaiseProfile.profile(backtrace != null && (raiseException = backtrace.getRaiseException()) != null)) {
                 // We need to rethrow the existing RaiseException, otherwise we would lose the
                 // TruffleStackTrace stored in it.
-                assert backtrace.getRaiseException().getException() == exception;
+                assert raiseException.getException() == exception;
 
                 if (getContext().getOptions().BACKTRACE_ON_RAISE) {
-                    getContext().getDefaultBacktraceFormatter().printRubyExceptionOnEnvStderr("raise: ", exception);
+                    getContext().getDefaultBacktraceFormatter().printRubyExceptionOnEnvStderr("raise: ",
+                            raiseException);
                 }
-                throw backtrace.getRaiseException();
+                throw raiseException;
             } else {
                 throw new RaiseException(getContext(), exception);
             }