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.

Sign up for GitHub

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

Jump to bottom

add "std-unwrap" feature to defer to std's unwrap in unwrap_throw #3899

Closed
wants to merge 1 commit into from
Closed

add "std-unwrap" feature to defer to std's unwrap in unwrap_throw #3899

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,7 +1327,7 @@ pub fn anyref_heap_live_count() -> u32 {
pub trait UnwrapThrowExt<T>: Sized {
/// Unwrap this `Option` or `Result`, but instead of panicking on failure,
/// throw an exception to JavaScript.
#[cfg_attr(debug_assertions, track_caller)]
#[cfg_attr(any(debug_assertions, wasm_bindgen_unwrap_throw_debug), track_caller)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be best if you just remove all guards on track_caller, which would also improve native support.

fn unwrap_throw(self) -> T {
if cfg!(all(debug_assertions, feature = "std")) {
let loc = core::panic::Location::caller();
Expand All @@ -1346,16 +1346,26 @@ pub trait UnwrapThrowExt<T>: Sized {
/// Unwrap this container's `T` value, or throw an error to JS with the
/// given message if the `T` value is unavailable (e.g. an `Option<T>` is
/// `None`).
#[cfg_attr(debug_assertions, track_caller)]
#[cfg_attr(any(debug_assertions, wasm_bindgen_unwrap_throw_debug), track_caller)]
fn expect_throw(self, message: &str) -> T;
}

impl<T> UnwrapThrowExt<T> for Option<T> {
#[cfg_attr(debug_assertions, track_caller)]
#[cfg(wasm_bindgen_unwrap_throw_debug)]
#[track_caller]
fn unwrap_throw(self) -> T {
self.unwrap()
}
Comment on lines +1354 to +1358
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm aware that this is the change proposed in OP, but is the debug_assertions output not good enough, or why do you actually need unwrap()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spend quite a bit of time triaging errors, and in that context consistency and recognisability is important. In this instance, deferring to std maintains consistency:

  • across build targets. same error under wasm, wasi, emscripten, linux, macos and so on.

  • across calls to the _throw or non-_throw variant. same error and importantly same behaviour, panicking rather than throwing.

We've implemented manual poisoning in our panic hook until #3687 is resolved. The throw inside .unwrap_throw() bypasses that and results in unsoundness that is unavoidable without patching leptos wasm-bindgen-futures and so on to respect our poison. That's rather painful, hence this fork where we panic rather than throw. We do also implement poison in window.addEventListener("error", ...) and window.addEventListener("unhandledrejection", ...) for such throws but I believe e.g. wasm-bindgen-futures calling queueMicrotask can preempt that callback, so can't fully mitigate the unsoundness.

Anecdotally since switching to this fork various "impossible" errors e.g. rust-lang/regex#1176 have stopped occurring, so in retrospect I think the throwing plus #3687 plus leptos or wasm-bindgen-futures calling queueMicrotask and similar was likely the cause.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm questioning if wasm-bindgen should do something here, unwrap_throw() should explicitly throw, if it panics it would be doing something different then advertised.

I do agree that the lack of module poisoning is quite problematic, unfortunately I don't know a "quick" fix for it apart from what you have been doing.

I have a couple of suggestions:

  • Maybe we can figure out a way to fix wasm-bindgen-futures:
    • Now that unwinding is actually supported in Rust Wasm we may want to think about the implementation of wasm-bindgen-futures more carefully.
    • If we can't find a good solution, I'm happy to add a cfg flag there to panic instead of throwing.
  • Other libraries might do whats suggested here, e.g. switch to panicking behind a cfg flag.
  • Module poisoning needs a solution, I'm happy to look at a PR here in wasm-bindgen, even if it should probably be solved in Rustc instead.

Please let me know what you think, I'm sympathetic to your goals but I would like to find a better solution.


#[cfg_attr(any(debug_assertions, wasm_bindgen_unwrap_throw_debug), track_caller)]
fn expect_throw(self, message: &str) -> T {
if cfg!(all(
target_arch = "wasm32",
not(any(target_os = "emscripten", target_os = "wasi"))
not(any(
wasm_bindgen_unwrap_throw_debug,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above. If you want to include the file, line and column as in unwrap_throw() I think it would be a nice improvement.

target_os = "emscripten",
target_os = "wasi"
))
)) {
match self {
Some(val) => val,
Expand All @@ -1371,11 +1381,15 @@ impl<T, E> UnwrapThrowExt<T> for Result<T, E>
where
E: core::fmt::Debug,
{
#[cfg_attr(debug_assertions, track_caller)]
#[cfg_attr(any(debug_assertions, wasm_bindgen_unwrap_throw_debug), track_caller)]
fn expect_throw(self, message: &str) -> T {
if cfg!(all(
target_arch = "wasm32",
not(any(target_os = "emscripten", target_os = "wasi"))
not(any(
wasm_bindgen_unwrap_throw_debug,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above.

target_os = "emscripten",
target_os = "wasi"
))
)) {
match self {
Ok(val) => val,
Expand Down