Closed Simn closed 4 years ago
currently we have a filter for c#/java that sorts this stuff out. i think I even separated it from gencommon at some point.
it transforms try/catch so whatever is inherited from the base class is caught by native catches, and others are handled by is
checks in a catch-em-all. of course throw
also already wraps if needed.
See tryCatchWrapper.ml
I would like to avoid these filters. It shouldn't be necessary to transform it like that because we can check locally (at TThrow
and TTry
) what values we throw/catch and wrap accordingly.
Plus that filter doesn't work quite right:
class Main {
static public function main() {
try {
throw "string";
} catch(e:String) {
trace(e);
} catch(e:java.lang.Throwable) {
trace(e);
}
}
}
Generates:
try
{
try
{
throw haxe.lang.HaxeException.wrap("string");
}
catch (java.lang.Throwable e)
{
haxe.lang.Exceptions.setException(e);
haxe.Log.trace.__hx_invoke2_o(0.0, e, 0.0, new haxe.lang.DynamicObject(new java.lang.String[]{"className", "fileName", "methodName"}, new java.lang.Object[]{"Main", "src/Main.hx", "main"}, new java.lang.String[]{"lineNumber"}, new double[]{((double) (((double) (8) )) )}));
}
catch (java.lang.Throwable catchallException)
{
haxe.lang.Exceptions.setException(catchallException);
java.lang.Object realException = ( (( catchallException instanceof haxe.lang.HaxeException )) ? (((haxe.lang.HaxeException) (catchallException) ).obj) : (catchallException) );
if (( realException instanceof java.lang.String ))
{
java.lang.String e1 = haxe.lang.Runtime.toString(realException);
haxe.Log.trace.__hx_invoke2_o(0.0, e1, 0.0, new haxe.lang.DynamicObject(new java.lang.String[]{"className", "fileName", "methodName"}, new java.lang.Object[]{"Main", "src/Main.hx", "main"}, new java.lang.String[]{"lineNumber"}, new double[]{((double) (((double) (6) )) )}));
}
else
{
throw catchallException;
}
}
}
catch (java.lang.Throwable typedException)
{
throw haxe.lang.HaxeException.wrap(typedException);
}
Fails:
src\haxe\root\Main.java:53: error: exception Throwable has already been caught
catch (java.lang.Throwable catchallException)
^
1 error
Even worse: If we catch java.lang.Exception
instead it enters the wrong catch block.
I don't follow. Doesn't that filter do exactly that?
E.g it transforms:
try throw "hello" catch (e:SomeJavaThrowable) trace("a") catch (e:String) trace("b");
into
try throw new HaxeException("hello")
catch (e:SomeJavaThrowable) trace("a")
catch (e:Throwable) {
var actual = if (e is HaxeException) e.actual else e;
if (actual is String) {
trace("b")
} else {
throw e;
}
}
oops, you were faster :) i guess there are some bugs, but don't we still need a filter like that?
Even worse: If we catch java.lang.Exception instead it enters the wrong catch block.
I think it's used to error there...
Well, we need the functionality, but I don't want to add these extra expression passes if there's no good reason.
Maybe we can expose it in a way that we can just call its functionality from when we find a TThrow
or TTry
. But even then, we shouldn't have to work so much with the Haxe typed AST, especially if the native typing semantics are relevant.
I see, well yes, we can do this on the generator level of course, if that's easy.
Note the thing that I keep forgetting: when processing throw
, we want to do new HaxeException(value)
if we are sure that value is not Throwable
, but if the type is Dynamic
- we need to call HaxeException.wrap(value)
that does a run-time check for throwable to prevent double-wrapping.
Yep makes sense, thanks!
I think the correct algorithm for the TTry
catches would be something like this:
catch
.catch (java.lang.Throwable)
.instanceof
chain for remaining exceptions.else
case.Basically, we start with a list of types that we would have to instanceof
-check (worst case), then check which of these we can promote to a catch
without shadowing the others.
The only issue with this is the "assignable" check. I suppose we have to do that on Haxe types after all because otherwise we cannot unify... But we still have to consider the native semantics, in particular java.lang.Throwable
and java.lang.Exception
subsuming all exception types.
Well you don't need to unify there really? Just follow abstracts (if not yet), then if class: go up in the inheritance check to reach Throwable
. if not class or no Throwable
in the inheritance chain - it's "not assignable"
I've done the initial work, but this is going to require some optimization/cleanup still.
static function raise<T>(f:Void -> String) {
return try {
f();
} catch(e:NativeExceptionChild) {
'NativeExceptionChild: ${e.getMessage()}';
} catch(e:NativeExceptionBase) {
'NativeExceptionBase: ${e.getMessage()}';
} catch(e:String) {
'String: $e';
} catch(e:NativeExceptionOther) {
'NativeExceptionOther: ${e.getMessage()}';
} catch(e:Int) {
'Int: $e';
} catch(e:java.lang.Throwable) {
'Throwable ${e.getMessage()}';
} catch(e:Dynamic) {
'Dynamic: $e';
}
}
Decompiled generated jvm:
public static <T> String raise(MethodHandle f) {
try {
return f.invoke();
} catch (NativeExceptionChild var2) {
return Jvm.stringConcat("NativeExceptionChild: ", var2.getMessage());
} catch (NativeExceptionBase var3) {
return Jvm.stringConcat("NativeExceptionBase: ", var3.getMessage());
} catch (Throwable var4) {
Object var10000 = var4 instanceof Exception ? ((Exception)var4).value : (Object)var4;
if ((var4 instanceof Exception ? ((Exception)var4).value : (Object)var4) instanceof String) {
String e2 = (String)var10000;
return Jvm.stringConcat("String: ", e2);
} else if (var10000 instanceof NativeExceptionOther) {
NativeExceptionOther e3 = (NativeExceptionOther)var10000;
return Jvm.stringConcat("NativeExceptionOther: ", e3.getMessage());
} else if (var10000 instanceof Integer) {
int e4 = Jvm.toInt(var10000);
return Jvm.stringConcat("Int: ", Jvm.toString(e4));
} else if (var10000 instanceof Throwable) {
Throwable e5 = (Throwable)var10000;
return Jvm.stringConcat("Throwable ", e5.getMessage());
} else if (var10000 instanceof Object) {
Object e6 = var10000;
return Jvm.stringConcat("Dynamic: ", Std.string(e6));
} else {
throw Exception.wrap(var10000);
}
}
}
This shows the algorithm I outlined in the previous comment: It generates real catch
expressions while it can, then defers to an instanceof
chain. It's going to respect the lexical order this way and we can easily optimize for some cases, such as having a distinct exception type for String
.
One TODO is that I'm currently re-wrapping the caught exception. That wasn't really intentional, I just didn't realize that I still need the original expression so I ignored it.
The failing genjava example I posted now generates this:
try {
throw Exception.wrap("string");
} catch (Throwable var2) {
Object var10000 = var2 instanceof Exception ? ((Exception)var2).value : (Object)var2;
Object var10001;
DynamicObject var10002;
MethodHandle var4;
if ((var2 instanceof Exception ? ((Exception)var2).value : (Object)var2) instanceof String) {
String e = (String)var10000;
var4 = Log.trace;
var10001 = (Object)e;
var10002 = new DynamicObject();
var10002._hx_setField("fileName", "src/Main.hx");
var10002._hx_setField("lineNumber", 6);
var10002._hx_setField("className", "Main");
var10002._hx_setField("methodName", "main");
var4.invoke(var10001, var10002);
} else {
if (!(var10000 instanceof Throwable)) {
throw Exception.wrap(var10000);
}
Throwable e1 = (Throwable)var10000;
var4 = Log.trace;
var10001 = (Object)e1;
var10002 = new DynamicObject();
var10002._hx_setField("fileName", "src/Main.hx");
var10002._hx_setField("lineNumber", 8);
var10002._hx_setField("className", "Main");
var10002._hx_setField("methodName", "main");
var4.invoke(var10001, var10002);
}
}
At first I thought that the var10000 instanceof Throwable
check might be redundant, but that is not true because it checks the possibly unwrapped value here.
Another thing I want to check is if this is really the best pattern:
25: dup
26: instanceof #26 // class java/lang/String
29: ifeq 98
32: checkcast #26 // class java/lang/String
I'm pretty sure the JIT doesn't care either way, but maybe this can be done better regardless.
Something else to consider is the Exceptions attribute. The documentation says:
A method should throw an exception only if at least one of the following three criteria is met:
The exception is an instance of RuntimeException or one of its subclasses.
The exception is an instance of Error or one of its subclasses.
The exception is an instance of one of the exception classes specified in the exception_index_table just described, or one of their subclasses.
These requirements are not enforced in the Java Virtual Machine; they are enforced only at compile-time.
We can easily infer this for concrete occurrences of throw
, but it gets trickier if we have to consider calls to other functions. This would require a preliminary inter-procedural analysis step to determine who throws what.
I have no idea how strict their "should" is and what happens if these rules are violated.
Also related: Any invoke
mechanism can cause a Throwable
exception. That should just mean that whenever we invoke something unknown (through invokedynamic
once we finally understood that), we add Throwable
to the list of thrown exceptions.
This would require a preliminary inter-procedural analysis step to determine who throws what.
I wonder if this can be also used for diagnostics like "you forgot to handle this exception" and "this exception is never thrown". But I guess this won't be ever accurate with all the virtual and dynamic calls...
No more re-wrapping:
public static <T> String raise(MethodHandle f) {
try {
return f.invoke();
} catch (NativeExceptionChild var3) {
return Jvm.stringConcat("caught NativeExceptionChild: ", var3.getMessage());
} catch (NativeExceptionBase var4) {
return Jvm.stringConcat("caught NativeExceptionBase: ", var4.getMessage());
} catch (Throwable var5) {
Object var10000 = var5;
if (var5 instanceof Exception) {
var10000 = ((Exception)var5).value;
}
if (var10000 instanceof String) {
String e2 = (String)var10000;
return Jvm.stringConcat("caught String: ", e2);
} else if (var10000 instanceof NativeExceptionOther) {
NativeExceptionOther e3 = (NativeExceptionOther)var10000;
return Jvm.stringConcat("caught NativeExceptionOther: ", e3.getMessage());
} else if (var10000 instanceof Integer) {
int e4 = Jvm.toInt(var10000);
return Jvm.stringConcat("caught Int: ", Jvm.toString(e4));
} else if (var10000 instanceof Throwable) {
Throwable e5 = (Throwable)var10000;
return Jvm.stringConcat("caught Throwable: ", e5.getMessage());
} else if (var10000 instanceof Object) {
Object e6 = var10000;
return Jvm.stringConcat("caught Dynamic: ", Std.string(e6));
} else {
throw var5;
}
}
}
Reopening though so I don't forget checking that Exceptions attribute situation.
I don't really remember implementing it but it looks like we generate the Exceptions
attribute now. Should re-check after @RealyUniqueName's new exception handling is merged to make sure it's still there.
Re-checked - still there.
The straightforward approach is to always catch Throwable and do a series of Std.is checks. I wonder if it could be worth to create exception wrappers classes for thrown classes that are not aliased (no parent class or interface). This way we could have
StringException
and others and directly catch them through the JVM instruction.