Added #[track_caller] and Improved Debug message for unwrap_throw()

[Billy-Sheppard]

Tested on firefox and edge (on fedora) with my own trait on top locally, I could not get it to run off a path dep due to

thread 'main' panicked at 'assertion failed: mid <= self.len()', /home/billy/.cargo/registry/src/github.com-1ecc6299db9ec823/wasm-bindgen-cli-support-0.2.80/src/decode.rs:43:27
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Help would be appreciated.

• Added #[track_caller]
• Using core::panic::location to retreive called file, line, col locations
• matches existing api

[Liamolucko]

Great idea! expect_throw should probably show the call site as well.

[Billy-Sheppard]

Great idea! expect_throw should probably show the call site as well.

Should it? Happy to include it, but my initial thinking was custom messages probably already indicate where in the code it was called from.

Will get on to those other fixes ASAP. Tks for the review.

EDIT: @Liamolucko, I've addressed those other points, feel free to leave any others otherwise lets get this merged in :).

[alexcrichton]

The purpose of unwrap_throw was originally to be very small on codegen, so specifically not having features like this (or the usage of format!). I think for this you'd instead want to use .unwrap() which has all the features baked in.

[Billy-Sheppard]

The purpose of unwrap_throw was originally to be very small on codegen, so specifically not having features like this (or the usage of format!). I think for this you'd instead want to use .unwrap() which has all the features baked in.

For me (on Edge and Firefox Fedora/Silverblue), .unwrap() doesn't provide line references. Outputs: Uncaught (in promise) RuntimeError: unreachable executed. Is there some other component to getting unwrap to work?

Perhaps this can be extracted to an optional feature or a separate function? I personally think it would be highly useful, but I understand why it potentially shouldn't be baked in. I have also updated the function to use core::fmt::write in place of format!, and as a result should work within #[no_std].

EDIT: I realise even using write won't work in no_std.
Maybe we should offer a new feature called fancy_unwrap that requires std, that offers this function instead. I have reverted to an earlier version of the code, that only implements my new function when the std feature is activated.
If that's not acceptable maybe the best way forward is to remake this into an external crate, with an almost analogous trait, that people can use.

[Liamolucko]

For me (on Edge and Firefox Fedora/Silverblue), .unwrap() doesn't provide line references. Outputs: Uncaught (in promise) RuntimeError: unreachable executed. Is there some other component to getting unwrap to work?

Yes - you need console_error_panic_hook to log panic messages to the console, which include the call site.

[Billy-Sheppard]

Thanks for that Liam, that does work for me, not sure if that's documented somewhere and I missed it somehow.

unwrap() does cause slightly different behavior to unwrap_throw() still however. For instance the console reports two errors, one with the rust stack, and one with Uncaught (in promise) RuntimeError: unreachable. I think there is still room for these improvements on unwrap_throw() to be used in conjunction with the combination of console_error_panic_hook and unwrap().

Perhaps my altered unwrap_throw could have #[cfg(all(feature = "std", debug_assertions)] on it?

[Pauan]

For the record, I think it would be really useful if unwrap_throw displayed filename and line numbers in debug mode (but not release mode).

[Billy-Sheppard]

After some productive discussion in this PR, it seems sensible to me to at least get some further explanation why this change couldn't be pushed through.

I've added debug assertions as I agree that's a sensible change.

[Billy-Sheppard]

Sorry to ping, @alexcrichton, do you have any further thoughts on having the filename tracking only when compiling with std and debug?

Also just following up on the discussion around console_error_panic_hook, is this documented somewhere?

I'm not sure about the majority of WASM crates but without PR'ing to the ones that do use unwrap_throw in place of unwrap, it can be difficult to debug some more sticky issues. Obviously that's more of an issue with those crates but perhaps the documentation/messaging can be improved?

[alexcrichton]

I think gating this behind debug_assertions is reasonable, yeah.

[Billy-Sheppard]

I've made those changes you've suggested there @alexcrichton, how does it look?

Is cfg!(all(debug_assertions, track_caller, std)) the correct if guard?

Also just following up on the discussion around console_error_panic_hook, is this documented somewhere?

This doesn't seem to be mentioned in the guide, perhaps I should draft another PR and add in some documentation on this for the guide?

[Liamolucko]

Is cfg!(all(debug_assertions, track_caller, std)) the correct if guard?

No, it should be cfg!(all(debug_assertions, feature = "std")).

[Billy-Sheppard]

Do the track caller attrs have to remain then as before (#[track_caller])? To cause rust to get the correct file/line/col reference?

Or is the full guard cfg!(all(debug_assertions, track_caller, feature = "std"))?

Won't that mean there will have to be two functions like before?

[Liamolucko]

Do the track caller attrs have to remain then as before (#[track_caller])? To cause rust to get the correct file/line/col reference?

No, they should be #[cfg_attr(debug_assertions, track_caller], which just means 'apply the #[track_caller] attribute if cfg(debug_assertions) is true'.

[Billy-Sheppard]

Perfect, thanks Liam, I had a bit of a brain fart and conflated those two changes Alex suggested. Should be working now.

[Billy-Sheppard]

Any further work required on this?