wasm-bindgen icon indicating copy to clipboard operation
wasm-bindgen copied to clipboard

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

Open jquesada2016 opened this issue 3 years ago • 3 comments

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 avatar Dec 06 '21 15:12 jquesada2016

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

alexcrichton avatar Dec 13 '21 16:12 alexcrichton

@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 :-( )

siefkenj avatar Apr 29 '24 23:04 siefkenj

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 https://github.com/rustwasm/wasm-bindgen/pull/3899 for my benchmarks and proposed fix.

alecmocatta avatar Apr 30 '24 09:04 alecmocatta