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

replace uwraps with expect #5

Open
jaheba opened this issue Apr 4, 2017 · 7 comments
Open

replace uwraps with expect #5

jaheba opened this issue Apr 4, 2017 · 7 comments

Comments

@jaheba
Copy link
Contributor

jaheba commented Apr 4, 2017

The code has a lot of .unwrap()s, which should be replaced with .expect(...).

E.g.:

fn pop_into<U>(&mut self) -> U
            where U: From<T>
        {
            self.pop().unwrap().into()
        }

Better:

fn pop_into<U>(&mut self) -> U
            where U: From<T>
        {
            self.pop().expect("Pop from empty stack.").into()
        }
@ltratt
Copy link
Contributor

ltratt commented Apr 5, 2017

In general, I don't think this is a good thing to do. I tend to think of unwrap as roughly equivalent to an assert: if it fires, something's gone wrong and the only useful info is "it failed at line L in file F". A user message doesn't really make anything better, but it does make the code harder to read, the binary bigger, and introduces more chances for the string and the code to become out of sync. [There are probably exceptions, of course, but I can't think of anywhere so far where I'd have used expect over unwrap myself.]

@jaheba
Copy link
Contributor Author

jaheba commented Apr 5, 2017

Sadly the "it failed at line L in file F" looks like this:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:329

So it actually doesn't tell you where you try to unwrap. If you have multiple unwraps within the same function, it actually is really hard to identify which one caused the failure. If you use expect, you at least get a hint.

@ltratt
Copy link
Contributor

ltratt commented Apr 5, 2017

I just do RUST_BACKTRACE=1 cargo run ... and (generally) it works reasonably well.

@jaheba
Copy link
Contributor Author

jaheba commented Apr 5, 2017

This won't tell you which one failed.

Example:

fn two(a: Option<u32>, b: Option<u32>) {
    a.unwrap();
    b.unwrap();
}


fn main() {
    two(None, Some(1));
}

You cannot tell from the stack-trace which one failed:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:329
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: <core::option::Option<T>>::unwrap
   1: none::two
   2: none::main

@ltratt
Copy link
Contributor

ltratt commented Apr 5, 2017

It should give you line numbers, in general. Maybe you need to use RUST_BACKTRACE=full?

@jaheba
Copy link
Contributor Author

jaheba commented Apr 5, 2017

No, it doesn't; even if you run it with full:

   0:        0x10a821043 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hbdeac7eba2f064c6
   1:        0x10a8221dd - std::panicking::default_hook::{{closure}}::h9e0a6ca9bb64b479
   2:        0x10a821da4 - std::panicking::default_hook::h9043ae80af471c9f
   3:        0x10a824887 - std::panicking::rust_panic_with_hook::h05996066754c6be9
   4:        0x10a824774 - std::panicking::begin_panic::h9fecf34da42eb910
   5:        0x10a824692 - std::panicking::begin_panic_fmt::he5aad713258a67c3
   6:        0x10a8245f7 - rust_begin_unwind
   7:        0x10a844eb0 - core::panicking::panic_fmt::he26d734b771c5b2c
   8:        0x10a844db4 - core::panicking::panic::h09943c653e3821eb
   9:        0x10a81d684 - <core::option::Option<T>>::unwrap::hb671ac437168fbb3
  10:        0x10a81d710 - none::two::hd2ba063fc908b04a
  11:        0x10a81d759 - none::main::hfb0944a09355cbaf
  12:        0x10a8245b5 - std::panicking::try::do_call::h24a2756282b9a31c
  13:        0x10a82639a - __rust_maybe_catch_panic
  14:        0x10a824b40 - std::rt::lang_start::hd19f94db0c6a490e
  15:        0x10a81d789 - main

@ltratt
Copy link
Contributor

ltratt commented Apr 5, 2017

Ah, this may be a flaw in Rust's OS X support. If you try this on Linux, I'm pretty sure you'll see line numbers. [Though I might be wrong!]

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

No branches or pull requests

2 participants