[js] rework exception handling (closes #6458) #6713

nadako · 2017-10-26T00:19:49Z

This reworks JS exception handling in a pre-analyzer/pre-dce filter that handles wrapping/unwrapping, single-catch + Std.is transformation, rethrowing, haxe.CallStack, etc.

E.g.

try {
    throw "epuc fial";
} catch (e:String) {
    trace(e);
} catch (e:Bool) {
    trace(e);
    js.Lib.rethrow();
}

is now generated as

try {
    throw new js__$Boot_HaxeError("epuc fial");
} catch( e ) {
    var e1 = (e instanceof js__$Boot_HaxeError) ? e.val : e;
    if(typeof(e1) == "string") {
        console.log("Main.hx:7:",e1);
    } else if(typeof(e1) == "boolean") {
        console.log("Main.hx:9:",e1);
        throw e;
    } else {
        throw e;
    }
}

Let's see what travis has to say about this...

nadako · 2017-10-26T00:23:10Z

std/js/_std/Reflect.hx

@@ -26,6 +26,7 @@
 		return js.Object.prototype.hasOwnProperty.call(o, field);
 	}

+	@:pure


This (and other similar ones) are not inferred as pure anymore, because the filter adds haxe.CallStack.lastException assignment. It might make sense to move injection of that assignment into a separate post-analyzer/dce filter.

Probably should do that...

nadako · 2017-10-26T00:50:18Z

std/js/Lib.hx

+
+		Calling this is only possible inside a catch statement.
+	**/
+	public static function getOriginalException():Dynamic {


This one allows for implementing functions like this:

static function catchToCallback<T>(fn:()->T, callback:(error:Any, result:Null<T>)->Void) { var result = try fn() catch (e:Any) return callback(js.Lib.getOriginalException(), null); callback(null, result); }

which will be compiled nicely into

Main.catchToCallback = function(fn,callback) { var result; try { result = fn(); } catch( e ) { callback(e,null); return; } callback(null,result); };

(note how we don't unwrap e here, because we requested "original exception")

It looks like you will lose haxe.CallStack.exceptionsStack() functionality.

Not really, it's just not generated in a simple case where haxe.CallStack is not used.

nadako · 2017-10-26T01:03:29Z

tests/optimization/src/TestJs.hx

@@ -172,12 +172,12 @@ class TestJs {
 		try throw false catch (e:Dynamic) {}
 	}

-	@:js('try {throw new js__$Boot_HaxeError(false);} catch( e ) {if (e instanceof js__$Boot_HaxeError) e = e.val;TestJs.use(e);}')
+	@:js('try {throw new js__$Boot_HaxeError(false);} catch( e ) {var e1 = (e instanceof js__$Boot_HaxeError) ? e.val : e;var e2 = e1;TestJs.use(e2);}')


This var e2 = e1; is unfortunate, but I think that's something the analyzer should handle.

It probably considers the variable to be a user-var. Try adding Meta.CompilerGenerated to it.

Will try, thanks!

That helped!

nadako · 2017-10-26T01:04:19Z

tests/optimization/src/TestJs.hx

 	static function testHaxeErrorUnwrappingWhenUsed() {
 		try throw false catch (e:Dynamic) use(e);
 	}

-	@:js("try {throw new js__$Boot_HaxeError(false);} catch( e ) {if (e instanceof js__$Boot_HaxeError) e = e.val;if( js_Boot.__instanceof(e,Bool) ) {} else throw(e);}")
+	@:js('try {throw new js__$Boot_HaxeError(false);} catch( e ) {var e1 = (e instanceof js__$Boot_HaxeError) ? e.val : e;if(typeof(e1) != "boolean") {throw e;}}')


This was actually incorrect before (an instance of #6458)

nadako · 2017-10-26T01:06:09Z

tests/optimization/src/TestJs.hx

@@ -62,7 +62,7 @@ class TestJs {
 		return v + v2;
 	}

-	@:js("var a = [];var tmp;try {tmp = a[0];} catch( e ) {tmp = null;}tmp;")
+	@:js("var a = [];var tmp;try {tmp = a[0];} catch( e ) {var e1 = (e instanceof js__$Boot_HaxeError) ? e.val : e;var e2 = e1;tmp = null;}tmp;")


More generated code because @:analyzer(no_local_dce) is being respected, since this is now inserted as proper AST instead of doing some checks at the generator level.

RealyUniqueName · 2017-10-26T05:26:22Z

src/filters/jsExceptions.ml

+ *)
+
+(*
+	This filter handles everything related to expressions for the JavaScript target:


Maybe "related to exceptions"?

Oops, yeah :)

RealyUniqueName · 2017-10-26T05:37:37Z

src/filters/jsExceptions.ml

+let follow = Abstract.follow_with_abstracts
+
+let rec is_js_error t =
+	match t with


Looks like follow_with_abstracts should be used here.

We should add a test that catches something which is typedeffed to/abstracts over js.Error.

This function is meant to run with a followed type already. In fact it should be called with a tclass, hmmm...

… filter, so it doesn't interfere with purity inference and can make more informed decision whether it's actually needed

nadako · 2017-10-26T10:58:51Z

I think it's pretty good now. I moved haxe.CallStack.lastException storing into a separate post-DCE filter, so it doesn't interfere with purity inference and only injects assignments when CallStack.exceptionStack() is actually used.

[js] rework exception handling (closes #6458)

a6e7b48

nadako commented Oct 26, 2017

View reviewed changes

[js] add js.Lib.getOriginalException

ebc339e

nadako commented Oct 26, 2017

View reviewed changes

nadako requested a review from Simn October 26, 2017 00:51

nadako self-assigned this Oct 26, 2017

nadako added bug enhancement platform-javascript Everything related to JS / JavaScript labels Oct 26, 2017

[js] adjust optimization tests for try/catch

c4cd6d7

nadako commented Oct 26, 2017

View reviewed changes

[js] add tests for generated code for getOriginalException and rethrow

42d3535

RealyUniqueName reviewed Oct 26, 2017

View reviewed changes

minor improvements

12102a8

skial mentioned this pull request Oct 26, 2017

Haxe Roundup 405 skial/haxe.io#443

Closed

nadako added 2 commits October 26, 2017 13:20

inject haxe.CallStack.lastException assignment in a separate post-DCE…

7b2e17b

… filter, so it doesn't interfere with purity inference and can make more informed decision whether it's actually needed

don't forget to recurse into try/catch expressions

6b50e1b

nadako merged commit b224999 into development Oct 26, 2017

nadako deleted the js_exceptions branch October 26, 2017 11:36

ncannasse mentioned this pull request Nov 24, 2017

Control flow with exceptions #6754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[js] rework exception handling (closes #6458) #6713

[js] rework exception handling (closes #6458) #6713

nadako commented Oct 26, 2017

nadako Oct 26, 2017

nadako Oct 26, 2017

nadako Oct 26, 2017

RealyUniqueName Oct 26, 2017 •

edited

Loading

nadako Oct 26, 2017

nadako Oct 26, 2017

Simn Oct 26, 2017

nadako Oct 26, 2017

nadako Oct 26, 2017

nadako Oct 26, 2017

nadako Oct 26, 2017 •

edited

Loading

RealyUniqueName Oct 26, 2017

nadako Oct 26, 2017

RealyUniqueName Oct 26, 2017

Simn Oct 26, 2017

nadako Oct 26, 2017

nadako commented Oct 26, 2017

[js] rework exception handling (closes #6458) #6713

[js] rework exception handling (closes #6458) #6713

Conversation

nadako commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealyUniqueName Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadako Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadako commented Oct 26, 2017

RealyUniqueName Oct 26, 2017 •

edited

Loading

nadako Oct 26, 2017 •

edited

Loading