Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross-target exception handling #9124

Merged

Conversation

RealyUniqueName
Copy link
Member

@RealyUniqueName RealyUniqueName commented Feb 4, 2020

Closes #6966
Closes #4159

This PR implements a common cross-target exception handling.

The behavior is configured with the following platform config on ocaml side:

type exceptions_config = {
	(* Base types which may be thrown from Haxe code without wrapping. *)
	ec_native_throws : path list;
	(* Base types which may be caught from Haxe code without wrapping. *)
	ec_native_catches : path list;
	(* Path of a native class or interface, which can be used for wildcard catches. *)
	ec_wildcard_catch : path;
	(*
		Path of a native base class or interface, which can be thrown.
		This type is used to cast `haxe.Exception.thrown(v)` calls to.
		For example `throw 123` is compiled to `throw (cast Exception.thrown(123):ec_base_throw)`
	*)
	ec_base_throw : path;
}

Suggested API on Haxe side: https://github.com/RealyUniqueName/haxe/blob/feature/haxe-error/std/haxe/Exception.hx

Implemented approach brings following benefits:

  • no need to implement exception handling in target generators;
  • no need to store an exception information into a static var ([js] haxe.CallStack.lastException is potentially leaking #6966);
  • custom exception types may have native behavior for 3rd-party code on target platforms;
  • compiler becomes aware of native wildcard catch types (e.g. reports unreachable second catch here: catch(e:java.lang.Throwable) {} catch(e:Dynamic) {})
  • "natural" catch generation (no need for if(Std.is(e, CatchType)-fest) on targets where haxe.Exception extends a native exception type;
  • rethrowing (add "rethrow" functionality #4159) and chaining exceptions.

Example

Here is an example:

class MyException extends haxe.Exception {}

try {
  throw new MyException('oops');
} catch(e:MyException) {
  trace(e.message); 
  throw e; // rethrow
}

Currently that sample is compiled into following pseudo-code for PHP (and for almost any target):

try {
  throw new HxException(new MyException("oops"));
} catch(_g1:php.Throwable) { //catches everything
  haxe.CallStack.saveException(_g1);
  var _g2 = Std.is(_g1, HxException) ? (cast hxe:HxException).value : _g1);
  if(Std.is(_g2, MyException)) { 
    var e:MyException = _g2;
    trace(e.message);
    throw new HxException(e); // no real rethrow - throws new exception
  } else {
    throw _g1;
  }
}

If a target platform supports typed catch, then with this PR compiled code would be:

try {
  throw new MyException("oops");
} catch(e:MyException) {
  trace(e.message);
  throw e; // real rethrow
}

If a target platform does not support typed catch (e.g. javascript), the generated code is:

try {
  throw new MyException("oops"); 
} catch(_g1) { 
  var _g2 = haxe.Exception.caught(_g1);
  if(Std.is(_g2, MyException)) {
    var e:MyException = _g2;
    trace(e.message);
    throw e; //this is actually the same value caught into _g1, so it's a real rethrow
  } else {
    throw _g1;
  }
}

Wildcard catch

haxe.Exception itself is treated as a wildcard catch:

try {
  throwSomething();
} catch(e:haxe.Exception) {
  trace(e.exception);
}

compiled into

try {
  throwSomething();
} catch(_g1:BaseNativeExceptionClass) { // <- wildcard catch on target platform
  var e = haxe.Exception.caught(_g1);
  trace(e.message);
}

Backward compatibility

The new approach still supports throwing and catching values of arbitrary types (like strings and enums), so existing Haxe code would not require any updates.
Non-haxe.Exception types get wrapped into haxe.ValueException (which extends haxe.Exception) and automatically unwrapped on catch:

try {
  throw "terrible error";
} catch(e:Dynamic) {
  trace(e);
}

compiled into

try {
  throw haxe.Exception.thrown("terrible error"); // wraps into ValueException under the hood
} catch(_g1:BaseNativeExceptionClass) { // <- wildcard catch on target platform
  haxe.CallStack.saveException(_g1);
  var e:Dynamic = haxe.Exception.caught(_g1).unwrap();
  trace(e); // "terrible error"
}

However, with this PR merged we should encourage users to throw and catch only haxe.Exception-based values.

Checklist

Implementation for each target mostly boils down to removing exception handling from the generator and to implementing haxe.Exception API

  • php
  • js
  • java
  • jvm
  • cs
  • python
  • lua
  • eval
  • flash
  • cpp
  • neko
  • hashlink
  • custom exceptions' stacks point to "throw" positions
  • optimize for DCE

@RealyUniqueName RealyUniqueName added this to the Release 4.1 milestone Feb 4, 2020
@RealyUniqueName RealyUniqueName marked this pull request as ready for review March 5, 2020 09:32
@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Mar 5, 2020

This PR is ready for review \o/
CI failure is not related.

@hughsando @ncannasse @Simn please, check the changes related to your targets.

@Simn
Copy link
Member

Simn commented Mar 5, 2020

It looks fine, but could you add an adapted version of this test please: https://github.com/Simn/genjvm/blob/7075f8bd4e25d5de50cb879bc589103835e05440/tests/genjvm/src/test/TestExceptions.hx#L25-L32

@RealyUniqueName
Copy link
Member Author

@Simn added

@Simn Simn self-requested a review March 5, 2020 20:47
@nadako
Copy link
Member

nadako commented Mar 5, 2020

Hmm, throw new haxe.Exception("Hallo") generates 545 lines of JS. Not saying that we should do anything about it right now, but is it technically possible to improve that?

@RealyUniqueName
Copy link
Member Author

That's because throwing a type makes dce to keep toString of that type which leads to keep Boot.__string_rec and co.
And Exception.toString() also stringifies a stack, which adds a few more methods from haxe.CallStack.
I'll try to figure something out, but I'm not sure if it's possible without changing that dce behavior and/or reducing Exception.toString to just return this.message

@Simn
Copy link
Member

Simn commented Mar 6, 2020

I for one don't care much for JS hello-throw optimizations. It's fair that throwing an exception comes with a bit of run-time support, so as long as we don't have random throws in our hello world programs I don't think there's a problem here.

@ncannasse
Copy link
Member

  • For the "used internally" APIs, I would generate @:privateAccess if needed but not make the methods public.
  • instead of using if(Std.is(_g2, MyException)) var e:MyException = _g2; maybe using var e:MyException = Std.downcast(_g2); if( e != null ) which does a single cast ?

@RealyUniqueName
Copy link
Member Author

instead of using if(Std.is(_g2, MyException)) var e:MyException = _g2; maybe using var e:MyException = Std.downcast(_g2); if( e != null ) which does a single cast ?

Std.is(a, Type) is optimized to something like a instanceof Type on some targets (js, php), while Std.downcast would resort to a more involved check. We can work to optimize Std.downcast, but that's out of the scope of this PR.
Besides that I doubt exceptions handling is used in performance critical code often.

@Simn
Copy link
Member

Simn commented Mar 10, 2020

Please resolve the conflicts.

... and sorry for causing conflicts!

@RealyUniqueName
Copy link
Member Author

Done.
I'm going to merge this on March 16 if no changes requested until then.

@Simn
Copy link
Member

Simn commented Mar 10, 2020

I was going to merge it this week but we can also wait a bit longer if you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[js] haxe.CallStack.lastException is potentially leaking add "rethrow" functionality
4 participants