Hacker News new | ask | show | jobs
by markus2012 3890 days ago
I completely agree. unwrap() is a huge mistake in Rust. There are brilliant ideas implemented in Rust (primarily the borrow checker) but their advantages seem to be wiped out by the kludge of unwrap.
3 comments

If we didn't provide it, someone would write it. (Actually, I suspect lots of libraries would just have it.) You can't force people to write good code.

This presupposes that the pattern that "unwrap" wraps is always a bad thing anyway, which it isn't. Sometimes the right thing to do is to panic your program if None is present. And sometimes the type system isn't sophisticated enough to express an invariant, and you need to use "unwrap" to work around it.

By the way, the borrow checker has nothing to do with unwrap().

Care to explain why? Whatever unwrap provides can be done with a match+panic. Which is more explicit, but that's just splitting hairs -- unwrap is pretty explicit anyway.
I understand the anger but at the same time I don't.

Overall unwrap() isn't good. Really it should be replaced by TODO WRITE ERROR HANDLING in 90% of its cases. That being said there are still good uses for it.

The main _safe_ use for unwrap() I find is when dealing with an iterator of Results/Options (which seems to happen a lot). The pattern:

      .filter( |x| x.is_some() )
      .map( |x| x.unwrap() )
Is safe, and I really don't know a more eloquent method of handling these operations.
In Swift you can solve this with a filterNone() method with an explicit type signature of [T?] -> [T], I assume that could also be implemented in Rust?
I'm not angry.

> Really it should be replaced by TODO WRITE ERROR HANDLING in 90% of its cases.

That's what the Rust community reads it as, mostly.

I've seen it occur very infrequently in libraries (aside from example/benchmark/test/mocking code, of course). When it does its usually for really out of whack errors like poisoned mutexes, crashed threads, etc (mind you, these are for unwrapping error values, not options, but it's almost the same thing). Or for cases where it's known to be valid due to invariants that can't be expressed in the normal way.

I did a quick grep of hyper, and the unwraps are almost all in "dev" code (tests/benchmarks/etc). The cases where they aren't are where there was an `is_some()` earlier (and due to some reasons it couldn't be restructured as a match, perhaps just because of rightward drift), or the "invariants" thing -- i.e. it often calls `<some url>.serialize_path().unwrap()`, which is OK, because hyper only ever deals with relative schemes (e.g. `http://` and `https://`, not `data:` or `mailto:` -- the latter two don't have paths), which is checked in the constructors, so this should NEVER panic. I didn't go through all the code, but in the main HTTP and HTTP2 code serialize_path was the only offender, which was one that was justified.

I see it more often in applications. In the following cases:

- Example applications in blog posts (because you want to explain something else, not spend too much time on error handling)

- Slightly less out of whack errors which are weird enough that they should close the app

- Normal errors which should close the app (e.g. "insufficient permissions" for a command line util, or a port-opening error on a network app). unwrap() prints out the error message so this is a very rudimentary form of error handling anyway.

- Laziness, or often with a TODO. Not too common.

So, it's mostly used in justifiable ways, and that's why it exists in the stdlib. It doesn't "wipe out" anything that the borrow checker provides (borrowchk and exhaustive match are two different things), and while it's a kludge if used often there are legitimate reasons to use it; enough reasons to have it in the stdlib.

> I really don't know a more eloquent method of handling these operations.

    .filter_map(|x| foo(x))

:)

(but yes, in general there are situations in which you know an Option is unwrappable, e.g. the serialize example above)

> filter_map(|x| foo(x))

Thanks :D

I've been really struggling to get iterators in rust. I don't have a very strong functional background :|

I have an open PR with documentation for just Iterator alone that has a diff so big github can't expand it. Hope they help.
Note that anytime you see a closure in any language that just takes a single argument, calls a single function with that argument, and returns the value of that function, then you can eliminate the closure entirely by just passing the function instead.

So `filter_map(|x| foo(x))` can be written more simply as just `filter_map(foo)`.

I know this but I can never get my code to compile when doing this. I always seem to get huge type errors even when I know the code is correct.

     map(|x|foo(x)) //clean compile
     map(foo) //type error
The number of times I've had this happen has just lead to me giving up on simply just passing the lambda as a variable instead of wrapping it in another.
This is a dramatic overreaction. `unwrap` is a convenience method whose trivially-obvious implementation is four lines long. Were it not provided by the stdlib then we'd have a preponderance of bespoke implementations in the ecosystem all with different names, which would make this pattern much harder to distinguish and discourage.

(Not to mention that `unwrap` in no way subverts memory safety, which makes mentioning it in the same breath as the borrow checker puzzling.)