[L] Change in dart/sdk[main]: [dart2wasm] Update JS exception catching
Ömer Ağacan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Martin Kustermann
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted and added 1 comment![]( "Open in Gerrit")
- Martin Kustermann
- Srujan Gaddam
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 5 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
5 comments
/// JS exceptions are only caught as: `dynamic`, `Object`, `Object?`.That's not entirely true I suspect. So if a user writes
```
try {
foo();
} on JSObject (err, stack) {
..
}
```
then we probably turn this (due to lowering of JS extension types to representation types)
```
try {
foo();
} on JSValue (err, stack) {
..
}
```
(The user cannot write this as `JSValue` is not exposed from `dart:*`, but user can implicitly type it by using the JS interop types)
Now the above code isn't portable across dart2js & dart2wasm, but it can still be written.
So maybe what the below should test is whether a `JSValue` is a subtype of `guard`. That will answer true for `JSValue`, `Object`, Dynamic`
class NullThrownFromJavaScriptException {Should this `implement Exception` (just like the JS version)?
NullThrownFromJavaScriptException.fromUndefined() : _isNull = false;Consider making those const constructors.
JavaScriptStack(JS<WasmExternRef?>("() => new Error().stack"));Consider moving this logic to `JavaScriptStack` to have it in one place:
```
static StackTrace get current => JavaScriptStack.current;
```
} catch (_) {If you catch anything here, then we will never get to the outer `try/catch`, right?
But it seems this was the intention of the test, see the docstring:
```
// Similar to `jsExceptionTypeTest1`, but the type test should succeed in a
// parent try-catch.
Future<void> jsExceptionTypeTest2() async {
```
Maybe this `jsExceptionTypeTest2` isn't adding any value anymore over `jsExceptionTypeTest1`?
Related details
- Srujan Gaddam
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 4 comments![]( "Open in Gerrit")
/// JS exceptions are only caught as: `dynamic`, `Object`, `Object?`.That's not entirely true I suspect. So if a user writes
```
try {
foo();
} on JSObject (err, stack) {
..
}
```
then we probably turn this (due to lowering of JS extension types to representation types)
```
try {
foo();
} on JSValue (err, stack) {
..
}
```
(The user cannot write this as `JSValue` is not exposed from `dart:*`, but user can implicitly type it by using the JS interop types)
Now the above code isn't portable across dart2js & dart2wasm, but it can still be written.So maybe what the below should test is whether a `JSValue` is a subtype of `guard`. That will answer true for `JSValue`, `Object`, Dynamic`
I thought we don't want to catch as JS types, because that would require type testing the caught JS values, and I guess dart2js can be a bit more relaxed in the type tests? (which would introduce incompatbilitiy between dart2js and dart2wasm)
This is also how I summarized the plan in the issue. It's possible that I got it wrong though.
Let's wait for @sru...@google.com's feedback here.
Should this `implement Exception` (just like the JS version)?
Done
NullThrownFromJavaScriptException.fromUndefined() : _isNull = false;Consider making those const constructors.
Done
JavaScriptStack(JS<WasmExternRef?>("() => new Error().stack"));Consider moving this logic to `JavaScriptStack` to have it in one place:
```
static StackTrace get current => JavaScriptStack.current;
```
- Martin Kustermann
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
} catch (_) {If you catch anything here, then we will never get to the outer `try/catch`, right?
But it seems this was the intention of the test, see the docstring:
```
// Similar to `jsExceptionTypeTest1`, but the type test should succeed in a
// parent try-catch.
Future<void> jsExceptionTypeTest2() async {
```Maybe this `jsExceptionTypeTest2` isn't adding any value anymore over `jsExceptionTypeTest1`?
Good point, I'm also not sure what this is testing. I wonder if it could be testing nested `try-catch`s with yields in between?
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
1 comment
/// JS exceptions are only caught as: `dynamic`, `Object`, `Object?`.Ömer AğacanThat's not entirely true I suspect. So if a user writes
```
try {
foo();
} on JSObject (err, stack) {
..
}
```
then we probably turn this (due to lowering of JS extension types to representation types)
```
try {
foo();
} on JSValue (err, stack) {
..
}
```
(The user cannot write this as `JSValue` is not exposed from `dart:*`, but user can implicitly type it by using the JS interop types)
Now the above code isn't portable across dart2js & dart2wasm, but it can still be written.So maybe what the below should test is whether a `JSValue` is a subtype of `guard`. That will answer true for `JSValue`, `Object`, Dynamic`
I thought we don't want to catch as JS types, because that would require type testing the caught JS values, and I guess dart2js can be a bit more relaxed in the type tests? (which would introduce incompatbilitiy between dart2js and dart2wasm)
This is also how I summarized the plan in the issue. It's possible that I got it wrong though.
Let's wait for @sru...@google.com's feedback here.
We want users to use `} on Object catch (o) { if (o.isJSAny) { ... } else { ... }}` to ensure their code works across dart2js & dart2wasm.
That doesn't mean one cannot write `} on JSObject catch (o) { ... }`. There were talks about adding lints for that, but users could still write the code and it would compile.
If we wanted to make this a compile-time error, then it would be a possibly breaking language change.
Related details
- Nate Biggs
- Srujan Gaddam
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
} catch (_) {Ömer AğacanIf you catch anything here, then we will never get to the outer `try/catch`, right?
But it seems this was the intention of the test, see the docstring:
```
// Similar to `jsExceptionTypeTest1`, but the type test should succeed in a
// parent try-catch.
Future<void> jsExceptionTypeTest2() async {
```Maybe this `jsExceptionTypeTest2` isn't adding any value anymore over `jsExceptionTypeTest1`?
Good point, I'm also not sure what this is testing. I wonder if it could be testing nested `try-catch`s with yields in between?
I updated this test to rethrow in the inner handler and catch again in the outer handler. Then test the both the inner handler and the outer handler catches as expected.
Related details
- Martin Kustermann
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
It looks like Chrome is not giving you the full stack trace with `new Error().stack` (it gives you some number of top frames, not the full thing), and with the refactoring we now have more frames when get the stack trace, which causes test failures because of the missing frames in the stack. I've created a thread with the V8 devs on this in the internal chat room.
Related details
- Martin Kustermann
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
/// JS exceptions are only caught as: `dynamic`, `Object`, `Object?`.Ömer AğacanThat's not entirely true I suspect. So if a user writes
```
try {
foo();
} on JSObject (err, stack) {
..
}
```
then we probably turn this (due to lowering of JS extension types to representation types)
```
try {
foo();
} on JSValue (err, stack) {
..
}
```
(The user cannot write this as `JSValue` is not exposed from `dart:*`, but user can implicitly type it by using the JS interop types)
Now the above code isn't portable across dart2js & dart2wasm, but it can still be written.So maybe what the below should test is whether a `JSValue` is a subtype of `guard`. That will answer true for `JSValue`, `Object`, Dynamic`
Martin KustermannI thought we don't want to catch as JS types, because that would require type testing the caught JS values, and I guess dart2js can be a bit more relaxed in the type tests? (which would introduce incompatbilitiy between dart2js and dart2wasm)
This is also how I summarized the plan in the issue. It's possible that I got it wrong though.
Let's wait for @sru...@google.com's feedback here.
We want users to use `} on Object catch (o) { if (o.isJSAny) { ... } else { ... }}` to ensure their code works across dart2js & dart2wasm.
That doesn't mean one cannot write `} on JSObject catch (o) { ... }`. There were talks about adding lints for that, but users could still write the code and it would compile.
If we wanted to make this a compile-time error, then it would be a possibly breaking language change.
I still think that's not the intention with this change. If we allow JS types to be caught there will be discrepancies between dart2js and dart2wasm. Example:
```
import 'dart:js_interop';
import 'dart:typed_data';
test() {
throw Uint8List(10);
}main() {
try {
test();
} on JSUint8Array catch (e, st) {
print("Caught exception: $e");
}
print("OK");
}
```
dart2js catches this exception, but dart2wasm won't.When you don't allow interop types in `on` you avoid these issues when a Dart type is an interop type in dart2js but not in dart2wasm.
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
It looks like Chrome is not giving you the full stack trace with `new Error().stack` (it gives you some number of top frames, not the full thing), and with the refactoring we now have more frames when get the stack trace, which causes test failures because of the missing frames in the stack. I've created a thread with the V8 devs on this in the internal chat room.
I updated d8 invocations with the right flag to capture more frames, Chrome invocations left.
Related details
- Martin Kustermann
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
Ömer AğacanIt looks like Chrome is not giving you the full stack trace with `new Error().stack` (it gives you some number of top frames, not the full thing), and with the refactoring we now have more frames when get the stack trace, which causes test failures because of the missing frames in the stack. I've created a thread with the V8 devs on this in the internal chat room.
I updated d8 invocations with the right flag to capture more frames, Chrome invocations left.
Done
Related details
- Martin Kustermann
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
1 comment
Well, the way I think about this is the same as I think about `is` tests:
Users can write `Object object; if (object is Uint8List) {}` and will probably get `true` in dart2js and `false` in dart2wasm iff `object` is `JSUint8Array`. (Yes, users should not do that, they should rather write e.g. `if (object.isA<JSUint8Array>) {}` instead which will work for both)
So the same logic applies in try/catch: Users should use `on Object` but if they don't and isntead use `on JSUint8Array` they may get different behavior -- just like in the `is` tests.
So we get the same uniformity or differences in `is` and `} on catch () {`.
Related details
- Nate Biggs
- Srujan Gaddam
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |