Stabilize Option::expect_none(msg) and Option::unwrap_none()

[aeubanks]

Also update must_use message on is_none().

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[aeubanks]

Closes #62633
This would help clean up some of my tests

[dtolnay]

@rust-lang/libs:
@rfcbot fcp merge

impl<T: Debug> Option<T> {
    pub fn expect_none(self, msg: &str);
    pub fn unwrap_none(self);
}

The unwrap_none panic looks like:

thread 'main' panicked at 'called `Option::unwrap_none()` on a `Some` value: 100', src/main.rs:9:5

The expect_none panic in the case of e.g. Some(100).expect_none("msg msg") looks like:

thread 'main' panicked at 'msg msg: 100', src/main.rs:9:5

That formatting matches what Result::expect and Result::expect_err do with the user's message i.e. format!("{}: {:?}", msg, unexpected_value).

The naming of unwrap_none isn't 100% ideal because None isn't exactly "a wrapped ()", but it's probably the choice here.

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

Concerns:

• &self (Stabilize Option::expect_none(msg) and Option::unwrap_none() #73077 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[dtolnay]

One reasonable alternative we might want to consider is to make these both take &self instead of self. The methods do not require ownership of the Option since all they do is check for Some and debug print the value.

I don't think it's better enough to make up for the inconsistency with other expect/unwrap methods, but it would make these usable in more cases.

[Amanieu]

I think taking &self is better here. From the user's point of view there's isn't any value in being consistent with unwrap(self).

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[dtolnay]

Registering the previous two comments for a bit more discussion.
@rfcbot concern &self

[dtolnay]

I'm okay with either way. In the worst case if we go with self people would need to write .as_ref().unwrap_none() sometimes.

One place the difference would be observable would be when not using method syntax, such as v.iter().for_each(Option::unwrap_none). I don't have a good sense of which signature would be more commonly directly applicable.

If we change to &self would we still stabilize right away or better to give it another cycle or few in nightly?

[sfackler]

I think in this case it'd make sense to let it bake another release if we change the signature since we aren't sure which is the better option.

[dtolnay]

This is an example of the kind of code that would require an extra as_ref() if the methods took self:

pub struct Service {
    field: Option<...>,
}

impl Service {
    /// Precondition: xyz has not been frobbed yet.
    pub fn f(&self) {
        self.field.expect_none("...");
        ...
    }
}

[cuviper]

I prefer the API-symmetry of keeping self, especially since as_ref() is available for those that really don't want to consume it. I won't lose sleep if this changes to &self though.

[aeubanks]

I think being consistent with all the other similar methods is fairly important. If somebody wants to write something (macro? trait?) that works with all the unwrap* or expect* methods on Option/Result it would be awkward for one of them to stick out. And switching a return type from Option to Result or vice versa shouldn't require changes in other code due to self vs &self.

I see Option and Result as the same. Don't use expect/unwrap in Option where you wouldn't use it if it were a Result. If you would have used a match or if let with a Result, use that same thing with an Option. If you wouldn't (couldn't) reuse a Result because expect/unwrap consumed it by self earlier on, the same should apply if it's an Option.

[SOF3]

I think being consistent with all the other similar methods is fairly important. If somebody wants to write something (macro? trait?) that works with all the unwrap* or expect* methods on Option/Result it would be awkward for one of them to stick out. And switching a return type from Option to Result or vice versa shouldn't require changes in other code due to self vs &self.

Are there any practical examples where this would affect macros/traits? All self methods can still delegate to &self methods.
Another marginal case where this could make a difference is some borrowck for T in some unsafe code (I am not sure about details here), but a developer wouldn't idiomatically rely on such semantic anyway.

What I am thinking is that if unwrap_none accepts &self, maybe it shouldn't be named as such at all. The name unwrap_none is unintuitive independently (as @dtolnay suggested), because it was intended to be symmetric to unwrap_err from Result. If the receiver is not mirrored, perhaps the name needs not be mirrored either, to prevent assumption that they are symmetric.

[ogoffart]

How about assert_none ?

[withoutboats]

I have a general concern about the lack of counterpressure on adding new methods to the core standard library types. As these APIs get more and more methods, they become harder and hard to figure out how to use for new users, because the std API docs become more and more overwhelming. Every method has some utility, and I'm worried we don't have any mechanism to judge out whether that utility is worthwhile for the downside of adding more methods (in essence, I think we currently greatly undervalue the cost of adding methods).

That said, I read the internals thread and I saw that there was a lot of support for this functionality from many community members, mainly because they could add it to the end of a method chain.

But I'm not in love with the fact that we're adding two methods to do what today can be done in 1 pattern without imports. And I'm not compelled that there's a lot of symmetry with unwrap/expect.

So I think I would prefer to add just one method, assert_none, which takes self by reference and a string argument. For now, users who don't care about printing a string can pass the empty string. If we ever have default method arguments, it would be a prime case for having a default argument for assert_none.

That seems like the most parsiminous way to solve the user complaint with the assert!(x.is_none()) pattern.

[SOF3]

I think the advantage of being "symmetric" to Result, in contrary to what was mentioned above, is more helpful in terms of API discovery. assert_none is relatively harder to be discovered, and if it gets prevalent, people might instead ask "why is there no Result::assert_err that returns () and shuts up my warn(unused_results)".

Another cause that this method was requested to be added was the convenience of method call chains, but workarounds like this (instead of using assert_eq! macros properly) are more like a slippery slope to avoid something like rust-lang/rfcs#2442.

[bors]

☔ The latest upstream changes (presumably #74235) made this pull request unmergeable. Please resolve the merge conflicts.

[aeubanks]

Since there is major pushback and it likely won't get resolved, should I close this?