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

Put glaring warnings and many capital letters in the docs of option::get() and option::unwrap(). #3606

Closed
bblum opened this issue Sep 27, 2012 · 14 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@bblum
Copy link
Contributor

bblum commented Sep 27, 2012

I realised this when replying to @eholk on my blog: http://winningraceconditions.blogspot.com/2012/09/rust-0-index-and-conclusion.html?showComment=1348721405975#c9047445512681989061

Basically, I worry that novices who are familiar with null pointer checks but not familiar with the functional "option" idiom will start learning rust, then pop into the channel or go to their friends asking "How do I get the T out of an Option<T>??", and start using get() or unwrap() indiscriminately, when really they should be learning about match and how to properly handle the None case.

I think a big step here would be to write something like "WARNING: If you are not SURE that the optional value is not None, use a match statement instead and handle the None case explicitly!" in the documentation for these functions.

@brson
Copy link
Contributor

brson commented Sep 27, 2012

This seems like a good use for typestate. option::get is usually paired with option::is_some or is_none, and that seems like a pretty reasonable restriction. Can we support just enough to do this in the most palatable way possible?

@brson
Copy link
Contributor

brson commented Sep 27, 2012

I bet the system we had supported this pretty well :)

@bblum
Copy link
Contributor Author

bblum commented Sep 27, 2012

I remember myself using get and unwrap a lot and cringing every time I did - though looking back on it now, most of the cases could now be solved with match move to replace is_some and unwrap (others would be replaced with oneshot closures).

I dunno. I'm not sure it's worth more language features to track in the cases where you actually do need to be imperative about it.

@AngryLawyer
Copy link

I admit to using unwrap a lot - Rust's the first language that I've seen Option types in (the only other functional language I really know is Erlang). I've just done a grep through the code and seen how it's supposed to be, and now feel embarrassed.

Those who aren't exposed to the correct way of doing it are likely to do it wrong - I support the idea of making the documentation a bit more explicit about the correct way of getting things out of Options

@catamorphism
Copy link
Contributor

Is there ever a legitimate reason to use is_some followed by get? I've seen code like this and wondered why people hated match expressions so much.

@eholk
Copy link
Contributor

eholk commented Sep 27, 2012

I think match versus is_some followed by get is just a personal style issue. The latter idiom is more familiar to imperative programmers, which I think many Rust programmers are or will be.

Still, using match doesn't quite solve the problem, as it's still really easy to add a None => fail clause. Error handling and recovery is hard. To me it's a similar issue to how C programmers rarely check the return value of functions that might return an error, and those that do often just terminate the program when an error is detected. I have a feeling all the language features in the world can't make programmers always write robust software.

@bblum
Copy link
Contributor Author

bblum commented Sep 27, 2012

While I think match is objectively better style than if is_some() - the only difference being that the compiler checks you in one of them..! - I am with eric on this.

Hence the suggestion to add docs rather than a small extra language feature (also perhaps rather than removing get() from the library altogether, which would be terrible).

@bblum
Copy link
Contributor Author

bblum commented Sep 27, 2012

@catamorphism I can't think of any. I used to use is_some followed by unwrap a lot (still visible in the task spawn code), but now match move provides that.

I do think get() should stay. People would just write it themselves.

@Dretch
Copy link
Contributor

Dretch commented Sep 27, 2012

FWIW the if-based code is smaller and introduces only one new indentation level instead of two:

match x {
    Some(y) => {
        y.do_some_stuff(some_args, oh_its_quite_long);
    }
    None => {}
}

vs

if x.is_some() {
    x.get().do_some_stuff(some_args, oh_its_quite_long);
}

@burg
Copy link

burg commented Sep 28, 2012

In my experience, I would write .is_some() and .get() because of Dretch's example---if I'm already several matches deep and then match on Option, the extra indentation seems gratuitous and takes up a lot of source space despite being a very simple operation.

The pattern `if is.is_none() { /* do nothing; return */ } mimics the early returns that are common in C++ code like SpiderMonkey. In both cases, the intention is reduce indentation and the associated cognitive load.

@erickt
Copy link
Contributor

erickt commented Sep 28, 2012

@Dretch: option::iter is even shorter, and is my favorite way of dealing with this:

do x.iter |y| {
    y.do_some_stuff(some_args, oh_its_quite_long);
}

@bblum
Copy link
Contributor Author

bblum commented Sep 28, 2012

@erickt I think maybe what he meant was something like

if x.is_none() {
    return something_early;
}
if y.is_none() {
    return here_too;
}
if z.is_none() {
    return also_here;
}
this_would_otherwise_be_indented_three_times();

which is super "imperative". iter is definitely the functional idiom.

@brson
Copy link
Contributor

brson commented Oct 4, 2012

I beefed up the docs a little in c83218d by adding Safety note sections to get, get_ref, and unwrap. I didn't make the language as ominous as requested in the OP though:

    In general, because this function may fail, its use is discouraged
    (calling `get` on `None` is akin to dereferencing a null pointer).
    Instead, prefer to use pattern matching and handle the `None`
    case explicitly. 

@bblum
Copy link
Contributor Author

bblum commented Oct 5, 2012

That looks super.

@bblum bblum closed this as completed Oct 5, 2012
RalfJung pushed a commit to RalfJung/rust that referenced this issue May 19, 2024
Rustup, aligned heap-allocations on wasm

Pulls in rust-lang#125003 so allocation tests should now work on wasm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

8 participants