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

Update explainer on stack trace support in JS API #197

Merged
merged 5 commits into from
Feb 10, 2022

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Feb 4, 2022

This updates the explainer on

  • We add stack trace option to Exception class in the JS API
  • Exception can contain an optional externref to carry a stack trace
  • Exception is not a subclass of JS Error

The WebIDL syntax is suggested by @eqrion in
#183 (comment)
and
#183 (comment).

Addresses #183 and #189.

This updates the explainer on
- We add stack trace option to `Exception` class in the JS API
- `Exception` can contain an optional `externref` to carry a stack trace
- `Exception` is not a subclass of JS `Error`

The WebIDL syntax is suggested by @eqrion in
WebAssembly#183 (comment)
and
WebAssembly#183 (comment).

Addresses WebAssembly#183 and WebAssembly#189.
@aheejin
Copy link
Member Author

aheejin commented Feb 4, 2022

@eqrion Thanks for the suggestion! It looks I can't add you to reviewers, so cc'ing you here.

@eqrion
Copy link
Contributor

eqrion commented Feb 4, 2022

Thank you, this looks good to me!

@aheejin
Copy link
Member Author

aheejin commented Feb 10, 2022

Gentle ping :)

@Ms2ger Ms2ger merged commit d6f5074 into WebAssembly:main Feb 10, 2022
@aheejin aheejin deleted the stack_trace branch February 10, 2022 19:10
@aheejin
Copy link
Member Author

aheejin commented Feb 10, 2022

@Ms2ger I have a request :) Can you let me merge my own PRs going forward? Especially when there are multiple reviewers, sometimes I'd like to wait a little more. I also generally prefer letting authors merge PRs themselves in case they have the right access to the repo, which is what we do in our wasm toolchain repos (emscripten and binaryen)

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 11, 2022

Apologies, I'll try to remember that

Comment on lines +462 to +464
To preserve stack trace info when crossing the JS to Wasm boundary, `Exception`
can internally contain an optional `externref` value containing a stack trace
string, which is propagated when caught by `catch` and rethrown by `rethrow`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, is this normative text or is it just an implementation note? IIUC, "internally" means that the reference is not observable in Wasm. Does it even matter then whether it is an externref?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect it is less error-prone to acknowledge the extra data that needs to be preserved in the core spec, rather than monkey-patching it from the outside. I'm not sure if that means we need to spell out that it's an externref, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ms2ger, I'm not sure how we could do that without actually devolving into spec'ing how JS stack traces work – presumably, the trace also needs to be extended in certain situations. Giving the handwavy-ness of stack traces in JS itself, we can only get that wrong. I'd rather not go there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rossberg Do you not prefer taking the optional stack trace on/off option in WebAssembly.Exception constructor, or you are OK with that part but doesn't want to say anything about how it is achieved and we delegate that entirely to toolchains?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aheejin, the latter. Or at least I would keep the "how" slightly more abstract.

Suggested change
To preserve stack trace info when crossing the JS to Wasm boundary, `Exception`
can internally contain an optional `externref` value containing a stack trace
string, which is propagated when caught by `catch` and rethrown by `rethrow`.
To preserve stack trace info when crossing the JS to Wasm boundary, exceptions
can internally contain a stack trace, which is propagated when caught by `catch` and rethrown by `rethrow`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #203.

aheejin added a commit to aheejin/exception-handling that referenced this pull request Feb 15, 2022
aheejin added a commit that referenced this pull request Feb 28, 2022
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.

4 participants