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

UnwrapThrowExt should propagate the error for Result<T, E> #2732

Closed
jquesada2016 opened this issue Dec 6, 2021 · 3 comments · Fixed by #4049
Closed

UnwrapThrowExt should propagate the error for Result<T, E> #2732

jquesada2016 opened this issue Dec 6, 2021 · 3 comments · Fixed by #4049
Labels

Comments

@jquesada2016
Copy link

Describe the Bug

I was debating between creating this issue as a feature request or as a bug, but I do believe it is a bug, as it is not what I would believe to be the intended behavior.

Currently, when calling unwrap_throw() or expect_throw() on an Err(E) variant, where E: Debug, only the message is printed, and not the debug value of E, as it does for the regular expect()/unwrap() counterparts. This makes the thrown error much less usable in diagnostics, especially since the unwrap_throw does not provide the std::panic:Location to hint as to what went wrong and where.

If this is, perhaps, a security concern, a feature flag might make this functionality available, and keep the old behavior when this flag is not set. Although, I am behind the idea of mimicking the original functions behavior when it comes to the thrown message.

Steps to Reproduce

  1. Try the following code:
use wasm_bindgen::prelude::*;

#[wasm_bindgen(start)]
fn start() {
  let res: Result<(), String> = Err("This message should be displayed when thrown".to_string());
  
  res.expect_throw("Throwing error...");
  // or
  res.unwrap_throw();
}

Expected Behavior

The unwrap_throw()/expect_throw() trait methods should mimic the behavior of the core/std library, displaying a debug view of the contained error value.

Actual Behavior

The expect_throw() method only displays the message in the thrown error, and neither unwrap_throw() nor expect_throw() display a std::panic::Location to trace the origin of the thrown error.

@jquesada2016 jquesada2016 changed the title UnwrapTrhowExt should propagate the error for Result<T, E> UnwrapThrowExt should propagate the error for Result<T, E> Dec 6, 2021
@alexcrichton
Copy link
Contributor

I think it'd be fine to update this to basically behave the same as the Rust standard library

@siefkenj
Copy link

@alexcrichton Based on the closed PR #2835, is this bug being labeled as wontfix, or is there a different implementation you'd be happy with? (I just spent an hour expanding macros to discover my real error was hidden by an unwrap_throw :-( )

@alecmocatta
Copy link
Contributor

For many/most uses the size savings of unwrap_throw aren't worth the extra debugging cost. In my case it was giving opaque errors, as well as triggering UB that led to red herring errors elsewhere.

See #3899 for my benchmarks and proposed fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants