search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.79k stars 12.5k forks source link

Tracking issue for Option::expect_none(msg) and unwrap_none() #62633

Closed cuviper closed 3 years ago

cuviper commented 5 years ago 
impl<T: fmt::Debug> Option<T> {
    pub fn expect_none(self, msg: &str);
    pub fn unwrap_none(self);
}

Steps:

trevyn commented 3 years ago

all this method can do is either succeed with a () or panic, so is an assert

I think there's a subtle error in this statement that affects judgement — unwrap_none has an equivalency to a sequence of is_none + assert, but it is not an assert.

Multiplication is equivalent to a sequence of additions, but it is not a sequence of additions.

Equivalency alone is not a meaningful standard, because we can write equivalent code to all of std.

buzmeg commented 3 years ago

The core team's original rationale for closing was "all this method can do is either succeed with a () or panic, so is an assert, but that's already handled by the assert! macro."

That's an interesting point: Is it documented somewhere that assert!() MUST return ()? Or is that simply an accident of implementation? If assert!() becomes able to function in a postfix way for chaining, that will change, no?

SOF3 commented 3 years ago

The core team's original rationale for closing was "all this method can do is either succeed with a () or panic, so is an assert, but that's already handled by the assert! macro."

That's an interesting point: Is it documented somewhere that assert!() MUST return ()? Or is that simply an accident of implementation? If assert!() becomes able to function in a postfix way for chaining, that will change, no?

If postfix macros are implemented, both unwrap and expect are not required anyway, because we could just option.expect!(), option.expect!("message"), option.expect!("message {}", args), etc. the same way.

buzmeg commented 3 years ago

Ah, good point. Shame that postfix macros have basically been stuck for four years now with no end in sight.

steveklabnik commented 3 years ago

I just want to comment here, as someone who doesn't have a super strong opinion in any direction, and is not on the libs team:

at the end of the day, procedurally speaking, the libs team is empowered to make decisions about what goes into the standard library. We do not develop Rust based on comments, upvotes, or reactions on GitHub issues. If we did, there would be no reason to have the teams. The reason (well, one of many reasons that) we have the teams is that sometimes, hard decisions need to be made.

This means comments characterizing this as "most people want this method, therefore consensus is closer to merging than not" is a category error. Many people may want this method, but for the purpose of accepting or rejecting, it is the opinion of the teams that counts. If literally every other person in the world is 👍 and the team members are 👎 , then the consensus is 👎 .

On a personal note, I sometimes disagree with teams decisions too. But, not every single person will ever agree with every possible decision. That's just how things go.

steveklabnik commented 3 years ago

Okay, with my legalese hat off:

Is there anything blocking people from implementing this method themselves if they want it? Normally what happens in these scenarios is that people do so, make a package, and then then the ecosystem popularity can help inform decisions.

As a concrete example of this working, before Rust 1.0, a function named ref_slice was removed. I thought it would still be helpful, and so I made a package for it: https://crates.io/crates/ref_slice

Eventually, it was demonstrated that this was useful enough to bring back, and so it was: https://github.com/rust-lang/rust/issues/45703

If people feel very passionate about this API, building such a package might allow them to get the benefits they want today, without needing to convince the libs team of anything. It also may help build the case for an eventual reversal of the decision.

trevyn commented 3 years ago

Is there anything blocking people from implementing this method themselves if they want it?

Yes — In practice, it's an ergonomics issue, and having the overhead of another crate and having to use it everywhere negates the ergonomics benefit.

We do not develop Rust based on comments, upvotes, or reactions on GitHub issues...Many people may want this method, but for the purpose of accepting or rejecting, it is the opinion of the teams that counts. If literally every other person in the world is 👍 and the team members are 👎 , then the consensus is 👎 .

That does not sound... inclusive and empowering.

I'm not saying there should be a direct popular vote, but I think something of the spirit of Rust is missing here.

CryZe commented 3 years ago

Another way to go about this is to possibly add a .assert postfix completion to rust-analyzer. That probably makes sense to have anyway.

trevyn commented 3 years ago

Another way to go about this is to possibly add a .assert postfix completion to rust-analyzer. That probably makes sense to have anyway.

This partially addresses "writing" ergonomics, but not "reading" ergonomics.

CryZe commented 3 years ago

The problem with this method, is that it opens a whole can of worms where you'd basically establish this as a new Rust pattern where every method returning a boolean should also have a .unwrap_*() / .expect_*() variation. Vec::is_empty? Better add Vec::unwrap_empty() / expect_empty() too. Metadata::is_dir / unwrap_dir / expect_dir. ptr::is_null / unwrap_null / expect_null and so on. You might argue those make sense, but you'd open up a huge can of worms leading to not just a minimal addition to std, but quite a lot, with arguably very little advantage compared to an assert other than it being in postfix position. It honestly makes a lot more sense to push the postfix macros RFC more rather than establishing this new pattern.

trevyn commented 3 years ago

Hmm, a crate that implements a whole bunch of useful postfix traits on many standard types is not a bad idea; it would amortize the overhead of use to the point where it could work. (I note that steveklabnik's ref_slice example actually contains four functions, two of which are aliased to alternate names as well.)

@CryZe For reference, I proposed in #82383 a detailed concrete definition and mental model for the word "unwrap" that is coherent to existing usage and would not imply its presence on bool, Vec, etc.

naiveai commented 3 years ago

@CryZe devil's advocate, most of the people here are arguing for this because of the "hole" implied by unwrap and unwrap_err. However I do find this argument compelling.

@trevyn If such a crate existed, I can nearly guarantee the vast majority of its users would be using solely for unwrap_none/expect_none, which kind of defeats the point of such an amortization, because then they'd be bringing in a whole bunch of other code. You could perhaps deal with it by using features, but I don't see much point in that.

rickvanprim commented 3 years ago

The problem with this method, is that it opens a whole can of worms where you'd basically establish this as a new Rust pattern where every method returning a boolean should also have a .unwrap_*() / .expect_*() variation. Vec::is_empty? Better add Vec::unwrap_empty() / expect_empty() too. Metadata::is_dir / unwrap_dir / expect_dir. ptr::is_null / unwrap_null / expect_null and so on. You might argue those make sense, but you'd open up a huge can of worms leading to not just a minimal addition to std, but quite a lot, with arguably very little advantage compared to an assert other than it being in postfix position. It honestly makes a lot more sense to push the postfix macros RFC more rather than establishing this new pattern.

I'd expect the methods to be on bool not Vec for that example, my_vec.is_empty().expect_true(), which feels reasonable to me stylistically. I'm not suggesting that those methods be added, but I do like the concept of postfix assertions 🙂

aclysma commented 3 years ago

@CryZe @rickvanprim Vec, Metadata, ptr, and bool do not have unwrap or expect functions, so I don't think it makes as much sense to put unwrap_* or expect_* on them as it does for Option and Result. (Result already has them)

@steveklabnik I understand and respect that this is not a "vote" and that the lib team is free to form their own consensus among themselves. That consensus may differ from the consensus of those who participated in FCP. I was expecting a similar process to be followed for FCP as was stated here. "there should not be a strong consensus against that position outside of the subteam" Does this not apply because this is not an RFC?

That being said, if the lib team holds the privilege to overrule what the community is saying, I would hope that it is coupled with a responsibility to consider all feedback from the community (even the +1s) and to maintain a very high bar for choosing when to overrule it. If that bar to overrule was being met, then I would expect the lib team could articulate stronger reasons for why they are against this stabilization (and to respond in more detail to the comments left during FCP that were in favor of merging.)

rickvanprim commented 3 years ago

@CryZe @rickvanprim Vec, Metadata, ptr, and bool do not have unwrap or expect functions, so I don't think it makes as much sense to put unwrap_* or expect_* on them as it does for Option and Result. (Result already has them)

I read it more as an assertion on the underlying value, so you're asserting that Vec::is_empty() returned true, similar to Option::unwrap() (in my mind) asserting that it's Some and returning the underlying T.

My only usage of Option::unwrap_none() is as an assertion that the entry I'm inserting into HashMap is a new entry not a replacement, so maybe there are better ways of expressing that, like using the Entry syntax.

aclysma commented 3 years ago

is_empty() has no side effects, so I don't mind so much putting that in assert!(). Options are frequently returned from things that do have side effects, similarly to result. (such as inserting into a hash map)

steveklabnik commented 3 years ago

My theme in this comment is "I was not clear enough before", heh. My bad.

Yes — In practice, it's an ergonomics issue, and having the overhead of another crate and having to use it everywhere negates the ergonomics benefit. - @trevyn

Sorry, I wasn't clear before. I meant technical reasons. Sometimes, things like coherence rules prevent an out of tree implementation. That doesn't mean this ergonomics thing isn't real, and you're right that there's a tradeoff here, but it is fundamentally possible. Different people will way this tradeoff differently, but it being possible matters.

I would hope that it is coupled with a responsibility to consider all feedback from the community - @aclysma

It is. However,

and to maintain a very high bar for choosing when to overrule it.

What I am trying to do here, and again, I wasn't super clear before, so my bad, is provide you all with the way to make your arguments in the strongest way. The fact of the matter is, presenting this GitHub thread as "all feedback from the community" is also a category error. Is it a signal? Yes. Is it a particularly strong one? No. The reason being that, first of all, we're talking about a couple of people in the grand scheme of things, but also, the list of people in this thread is heavily biased towards people that want to see this added. That is, after all, the point of this thread. So you can't really take this that strongly.

You also have to couple this with the responsibility of the libs team, and that is to steward the standard library, forever. The default is always no. We can add things, but never remove them. This means that there is a high bar for justifying adding things.

But also, at the same time, sometimes, reasonable people disagree with each other. And sometimes, this changes over time too. I remember being really sad that postfix await was chosen by the language team. At the time, I thought it was a huge mistake. I campaigned against it very hard. A significant amount of the community (and that decision had a much broader audience than this thread) agreed with me (and others).

In the end, though, now that I've lived with that decision for a while? They were absolutely right, and I was wrong. It was a tough call, and it wasn't popular at the time, but it was the right thing for Rust. This doesn't mean this is always the case; I bring this story up partially because it's sort of a counterpoint to the story I laid out earlier about slice_ref, where a decision was reversed later, because that was the right call eventually.

That does not sound... inclusive and empowering. - @trevyn

Taking feedback into account is being inclusive. That the community is privy to this process at all is radically different than many other languages. Making decisions that may be unpopular isn't inherently un-inclusive.

(that leads into @aclysma this time)

I would expect the lib team could articulate stronger reasons for why they are against this stabilization (and to respond in more detail to the comments left during FCP that were in favor of merging.)

This is a good procedural point, for sure. I do think that some more elaboration and a direct response to these points would be nice. At the same time, we all have to remember that we're all human, and we're all here for the same purpose: to make Rust better. The teams have so, so, so much work on their plate, and sometimes this means that a particular thread may not get as much love as any given individual may think it should. I do generally agree that the more heat a thread is, the more communicative the team should be about the reason it made the decision that was made. But I will also note that this hasn't actually been closed yet. That opportunity is still here.

"there should not be a strong consensus against that position outside of the subteam" Does this not apply because this is not an RFC? - @aclysma

Well, there is also this right before:

That does not require consensus amongst all participants in the RFC thread (which is usually impossible).

I personally think that that sentence is a bit strongly worded, but also, yes. The bar gets higher the bigger something is. This is already a lot of stuff for a very small API. (To be honest, other than the fact that I am a governance nerd, and a general desire to de-escalate threads) I am not even 100% sure why I am spending so much time on this thread; I don't feel strongly that this should be included or not included, but here I am writing a ton of words on it...)

Anyway. That's just my thoughts. Again, I am not on the relevant team here, so I'm just a random commenter as much as anyone. That's how I see this situation. I will probably leave this here :)

ijackson commented 3 years ago

On a personal note, I sometimes disagree with teams decisions too. But, not every single person will ever agree with every possible decision. That's just how things go.

Quite. The pile-on in this issue is rather unedifying. I had hoped we would treat our libs team with more respect. Most of the individual messages would be OK by themselves, but I think the cumulative effect is not. If you just want to +1 something, do that.

Personally I like the idea in this MR but if the libs team don't like it there is no reason it couldn't be done in an extension trait as you suggested.

Maybe someone would like to enhance boolinator to have this feature? boolinator already has an extension trait for bool providing expect. It seems to me like it would be a natural home for an extension trait for Option providing expect_none etc. It could also usefully be enhanced with expect_false on bool.

buzmeg commented 3 years ago

Quite. The pile-on in this issue is rather unedifying. I had hoped we would treat our libs team with more respect.

A) Leave the "pile-on" and commentary for elsewhere, please.

B) As for edifying, I have seen quite a few more substantive options and comments about alternatives after the fcp than before it. Had some of these options been mooted and discussed prior to simply just "fcp close", my guess is that a "reject on aesthetic grounds" would have gone over much better. Had some of these things appeared above the "fcp close", I, personally, probably wouldn't even have commented.

C) Until some of us started jumping up and down, there was radio silence from the libs team about why something which seemed like a shoe-in had a single in-private meeting and suddenly was dead.

People got surprised and that's not going to go over well. And I think both sides got surprised. The people in favor of this issue got surprised when it suddenly went from "looks like its going in" to "reject" very suddenly. And I suspect that the dev team got surprised that people cared about this more than they realized.

@steveklabnik wrote:

The reason being that, first of all, we're talking about a couple of people in the grand scheme of things, but also, the list of people in this thread is heavily biased towards people that want to see this added. That is, after all, the point of this thread. So you can't really take this that strongly.

@steveklabnik You are claiming that you should ignore feedback simply because the people involved are actively engaged in the topic at hand? Maybe you adjust your Bayesian prior down some for enthusiastic engagement, but you don't just set it to zero. Rust issues with negative feedback are not at all uncommon, and if that were the case here people would certainly be pointing to it for reinforcing their position. So, positive feedback, lack of negative feedback, etc. really is signal rather than noise.

What you choose to do with that signal is a different matter, but you can't just dismiss it as not being signal.

steveklabnik commented 3 years ago

I specifically did not say that it should be ignored, or that it is not a signal. I said that it is not a strong enough signal to be presented as some kind of "community consensus."

BurntSushi commented 3 years ago

but you don't just set it to zero [...snip...] What you choose to do with that signal is a different matter, but you can't just dismiss it as not being signal.

As I said above:

Utility to folks is and should be a factor in our decision. Folks have made their case. I personally remain unswayed.

This issue being turned into a debate about procedure is incredibly draining. I think @steveklabnik has roughly described my understanding of our governance structure. I would like to re-emphasize here that the legalese is that the teams do not have to make decisions based on voting by others or any other such thing. However, in practice, teams do take community feedback into account, because that's what conscientious people in a healthy project do. This does not necessarily mean that a few vocal folks along with a ~dozen positive emoji reactions will get their way in adding an API though. There's a lot of room between "we take your feedback into account" and "you aren't listening to upvotes and a few of us emphasizing that we have strong opinions on this matter."

In terms of emoji reactions, I will just say that I personally put very little weight on them. They are a signal for sure, but a weak one. All it takes is for someone to link a github thread on twitter or some other social media web site to skew that signal in ways that make it nearly useless. (And this doesn't even need to be done maliciously. Sometimes you really do just want to get more eyes on something.) And in general, it's impossible to know when that has and hasn't happened. This doesn't mean all emoji reactions are useless. But in terms of their impact on my personal decision with regard to std APIs, they at best say, "take a closer look."

In terms of the folks voicing opinions in this thread in favor of this API, I have engaged with them. I've explained why I found the arguments in favor of this API to be uncompelling. I also agree with @m-ou-se's commentary on the matter. An argument that I don't believe I've addressed head on is the purported ergonomic benefit to this API. I personally don't see much if any benefit to this, and even if I did, I'm not sure it would out-weigh the downside to adding this API. In particular, I find the notion of unwrapping a None value weird and surprising, for reasons I've already covered.

Now, in terms of the FCP surprising folks, I went back and looked it over. And I can see how it came across that way. I can't see any dissenting opinions before the disposition to close was proposed. Indeed, the libs team really should be presenting their argumentation in favor or against before calling an FCP disposition so that others have time to respond. I personally wasn't at the libs meeting when the disposition was decided, but my guess is that this was considered such a small API addition that dismissing it without much ceremony wasn't given much thought. So I agree that there were perhaps surprises on both sides here.

Back in the old days when the libs team was first formed, we actually had a stronger habit of discussing new APIs in our meetings (often debating amongst ourselves) instead of having the debate openly and asynchronously on the issue tracker. This wound up causing problems very similar to what we're seeing in this thread, and it was why (I believe) Aaron Turon pushed us towards not debating the APIs themselves in the meetings. But rather, having that debate on the issue tracker. I think since then, we have historically done that.

m-ou-se commented 3 years ago

As a general comment to those who might be unaware: Please note that none of us on the library team work on this full-time, but sometimes a few hours or a day a week. We usually have a meeting once a week which is also the moment in the week most of us spend some time on issues like this. Only six days have passed since @buzmeg's first comment on this thread, so talking about 'radio silence' after just a few days feels a bit out of place. Things can take quite a bit longer in Rust, being run as an open source project by volunteers.

davepacheco commented 3 years ago

Reading this thread and from my own experience, it seems like much of the disagreement here relates to side effects in assertions. If Rust already has a position on that (not just what's legal, but what's idiomatic), then maybe clarifying that would go a long way towards obviating the need for this?

(Specifically: I got here from the same BTreeMap::insert use case that seems to have brought more than one other person here. That last comment, plus several others express doubt about side effects in assertions (which is also how I felt coming into this). On the other hand, that side effects can be part of assertions seems to have been a key point underlying the decision from the Libs team meeting and was echoed here.)

The only technical reason I know to avoid side effects within assertions is that in C, this was likely to result in those side effects not happening in non-debug builds. [Edit: I've seen this behavior in non-C languages or libraries as well.] I think this idea permeates a lot of folks' understanding, not just people who write C, but I don't know of another technical reason to avoid this practice. Having been excited about this feature, I'm coming around to the other side: the awkwardness in semantics around unwrapping a None is about the same as the awkwardness I felt about an assertion with side effects. If Rust already embraces the latter, then I have no need for these functions.

Still, the evidence in this thread suggests that if assertions with side effects are not just legal but idiomatic in Rust, that could be clearer. I think it would help if the messaging around assert was more explicit about both side effects and being different from C (or "other languages", if that's better). Since a main use case for this issue seems to be BTreeMap::insert(), an explicit note in the docs recommending this pattern might make people more comfortable with it. (The example already does include an insert within an assertion. But it looks like code that's testing or demonstrating the functionality. It's not obvious to me that it's saying "this pattern is fine in production code".) Maybe there are other ways to better communicate this idiom to people too, but this is starting to get off-topic.

buzmeg commented 3 years ago

Quoth @m-ou-se:

Only six days have passed since @buzmeg's first comment on this thread, so talking about 'radio silence' after just a few days feels a bit out of place. Things can take quite a bit longer in Rust, being run as an open source project by volunteers.

That's fair.

But please note that @aclysma, @trevyn (both of whom commented earlier than me), and I commented once over the 10 day FCP, waited patiently for a response during a time while the team actively was ripping this out of the Rust codebase, and I, personally, didn't start jumping up and down until the bot said "The final comment period, with a disposition to close, as per the review above, is now complete."

If the granularity of the team is 7 days (perfectly normal), then the FCP should probably be adjusted to 2 1/2 weeks instead of 1 1/2 weeks to reflect that reality.

cuviper commented 3 years ago

@buzmeg

while the team actively was ripping this out of the Rust codebase

I'm not sure what you're referring to. The stabilization PR was closed, but I haven't seen any "ripping out".

But you did get me to go look for usage in the compiler. I don't see any unwrap_none, but there are 14 expect_none:

$ git grep -w expect_none compiler
compiler/rustc_codegen_ssa/src/coverageinfo/map.rs:67:        self.counters[id].replace(region).expect_none("add_counter called with duplicate `id`");
compiler/rustc_codegen_ssa/src/coverageinfo/map.rs:99:            .expect_none("add_counter_expression called with duplicate `id_descending_from_max`");
compiler/rustc_middle/src/ich/impls_syntax.rs:48:            tokens.as_ref().expect_none("Tokens should have been removed during lowering!");
compiler/rustc_middle/src/mir/interpret/allocation.rs:342:        src.next().expect_none("iterator was longer than it said it would be");
compiler/rustc_mir/src/interpret/memory.rs:857:                src.next().expect_none("iterator said it was empty but returned an element");
compiler/rustc_mir/src/interpret/memory.rs:883:                src.next().expect_none("iterator said it was empty but returned an element");
compiler/rustc_mir/src/interpret/memory.rs:897:        src.next().expect_none("iterator was longer than it said it would be");
compiler/rustc_mir/src/transform/coverage/debug.rs:289:                .expect_none(
compiler/rustc_mir/src/transform/coverage/debug.rs:482:            edge_to_counter.insert((from_bcb, to_bb), counter_kind.clone()).expect_none(
compiler/rustc_mir/src/transform/deduplicate_blocks.rs:89:                duplicates.insert(bb, value).expect_none("key was already inserted");
compiler/rustc_span/src/hygiene.rs:121:            expn_data.orig_id.replace(self.as_u32()).expect_none("orig_id should be None");
compiler/rustc_span/src/hygiene.rs:205:            data.orig_id.replace(raw_id).expect_none("orig_id should be None");
compiler/rustc_span/src/hygiene.rs:1421:                    .expect_none("Hash collision after disambiguator update!");
compiler/rustc_span/src/lib.rs:2003:                cache[index].replace(sub_hash).expect_none("Cache slot was filled");
cuviper commented 3 years ago

(Sorry, I don't mean to pile on this heated thread, but I realized another valuable thing I wanted to point out...)

The methods also require T: Debug, and any unexpected Some value is printed in the panic message. So if you want to completely replace these methods with assertions, you would need to save the value so it can be referenced twice, like:

let option = some.expression();
assert!(option.is_none(), "your message: {:?}", option.unwrap());

Or replicate the actual method implementation:


if let Some(value) = some.expression() {
    panic!("your message: {:?}", value);
}
CryZe commented 3 years ago

So in that case would it maybe make sense to simply only have .expect_none() instead?

cuviper commented 3 years ago

unwrap_none() also includes a message, called `Option::unwrap_none()` on a `Some` value, and the Debug value.

m-ou-se commented 3 years ago

We discussed this issue again in the library team meeting yesterday.

Thanks for all the valuable feedback and insights. We discussed the arguments in more detail. However, we still stand by our decision to not stabilize these functions. Our main arguments against stabilizing these are:

Having them as optional "features" is a strange limbo state. That creates confusion as to whether they're staying around or going away.

This is very true, and we should be better at not keeping unstable features in such a state for a long time, as it clearly causes confusion. We will more actively try to track unstable features and make sure they are either removed or stabilized within reasonable time. We're sorry we might have given the impression this feature was close to stabilizing, causing surprise when that turned out to not be the case.

During our discussion, the idea of an Option::err_or was brought up. In examples where panicking on a Some makes sense, the Some is considered an error. So, a function that would convert an Option::Some to a Result::Err might make sense in some of those cases, such that that Result can then be handled in the usual ways, such as by unwrapping it. We didn't discuss this option in detail, so there's no concensus on this yet, but it might be an interesting direction to explore separately from unwrap_none/expect_none.

RalfJung commented 3 years ago

FWIW, I really like(d) these methods -- see https://github.com/rust-lang/miri/pull/1734 for how removing the methods make the code either harder to read [*] or more verbose.

[] I consider it bad style for assert! to have side-effects. The intention of an assertion is to check that something holds true; as such I expect the check to be side-effect-free. I know that assertions are always compiled in Rust, but just because I can* rely on their side-effects always happening, doesn't mean I should.

m-ou-se commented 3 years ago

In all those cases it's about panicking on insert()ing a duplicate key. It makes me wonder whether insert() should've returned a Result instead of using Some to indicate the duplicate key error. (Not that we can really change that now.)

RalfJung commented 3 years ago

In all those cases it's about panicking on insert()ing a duplicate key.

Indeed, this is really my primary motivation here: I think there should be a nice way to say "insert a key and assert that it's fresh". expect_none() did exactly that. assert! with side-effect doesn't qualify as "nice" IMO (see above).

BurntSushi commented 3 years ago

I've personally never thought of asserts with side-effects as being bad style in Rust.

SOF3 commented 3 years ago

Having a complex expression in an assertion macro might not work well with tools like rustfmt. Although rustfmt already lexically identifies assert_eq!, I am not sure if it is compatible with macro aliases. This is offtopic from this issue though.

m-ou-se commented 3 years ago

I opened https://github.com/rust-lang/rfcs/issues/3092 for the insert() issue.

aclysma commented 3 years ago

@m-ou-se was stabilizing just expect_none discussed? Many of the objections to unwrap_none did not apply to expect_none.

m-ou-se commented 3 years ago

@aclysma Yes. The argument of not wanting to have i32::expect_positive, Vec::expect_empty etc. still holds. We also don't want to break the symmetry of expect/unwrap.

(It's also interesting to note that the expect() methods would probably not have existed if #[track_caller] happened sooner. Before #[track_caller], you wouldn't be able to distinguish different .unwrap() calls from their panic message. .expect() with a message helped out with that.)

RalfJung commented 3 years ago

@m-ou-se asked elsewhere

I'd be curious to hear why you think this is bad style though.

I'm not sure to say here beyond what I said above: when I read code I like to consider assert! as "executable documentation", it says "some thing is always true here" and backs that statement up with a runtime check. A comment can be removed without breaking the code; assert! is a stronger form of comment but still more a comment than actually part of the behavior of the function, as far as I am concerned.

Assertions with side effects are quite common in rustc

I know, and I've had trouble reading such code in the past.^^

in Rust it's not much different than a panic!() in an if.

I view assert! as having a semantic connotation that goes well beyond a panic in an if, so I don't view the two as equivalent.

tspiteri commented 3 years ago

For people wanting chaining, I think there's always .ok_or(()).unwrap_err().

m-ou-se commented 3 years ago

Here's an alternative solution for map.insert(..).unwrap_none(): https://github.com/rust-lang/rust/pull/82764

cuviper commented 3 years ago
  • assert_eq!(option, None); already shows the value inside the Some in the panic message.

True, although it requires PartialEq. Maybe we should have assert_matches!(expr, pat)?

ijackson commented 3 years ago

Here's an alternative solution for map.insert(..).unwrap_none(): #82764

Thank you. That is a good addition.

TBH I still see value in unwrap_none(). I don't find the arguments against it particularly convincing. But I don't think discussing that further in this thread is going to be helpful or fun.

To those in this thread who want unwrap_none(): should I bother with providing this as an out-of-stdlib extension trait somewhere? Would you rather have it as a part of an existing crate (and if so which one), or a crate of its own?

m-ou-se commented 3 years ago

Maybe we should have assert_matches!(expr, pat)?

@cuviper Yes, I think so! I was about to make a PR for assert_match!(), but then got distracted by HashMap::try_insert. :)

cuviper commented 3 years ago

@m-ou-se great! FWIW, I used plural to go with the existing matches! macro.

RalfJung commented 3 years ago

It's not plural, I think, it's the third-person-singular-suffix-"s": "X matches Y".

cuviper commented 3 years ago

@RalfJung sorry, you're right. It doesn't help that "matches" is also a plural noun. Still, we should try to be consistent.

m-ou-se commented 3 years ago

assert_matches!() implementation: https://github.com/rust-lang/rust/pull/82770

Bendrien commented 3 years ago

I hope it is fine that im writing here. I just stumbled over this api and was wondering about the Debug constrain. I want to use expect_none on a generic option but dont care about the value itself, I just want to give some context msg why a None is expected. Since this is still an unstable feature I thought it makes sense to share my opinion with you.

RalfJung commented 3 years ago

@Bendrien expect_none was removed recently, see https://github.com/rust-lang/rust/pull/83349 and the comments in this thread.

mankinskin commented 1 year ago

My god, what is the harm in adding unwrap_none and expect_none(msg)? So many people are asking for this very API because they find it useful.

People here seem to completely miss the idea behind using it. It is obviously not about unwrapping to get some value, but about panicking when it is not none. maybe "unwrap_none" sounds a bit odd, but "expect_none" is very clear. Also "unwrap_none" makes sense when the wrapper is empty.

Having to wrap an Option in an assert increases code complexity when you are mostly using functional adapters which chain behind oneanother.