search

rust-lang / rust

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

Tracking Issue for Option::is_some_and and Result::is_{ok,err}_and #93050

Closed m-ou-se closed 1 year ago

m-ou-se commented 2 years ago

Feature gate: #![feature(is_some_and)]

This is a tracking issue for Option::is_some_and, Result::is_ok_and and Result::is_err_and.

Public API

impl<T> Option<T> {
    pub fn is_some_and(self, f: impl FnOnce(T) -> bool) -> bool;
}

impl<T, E> Result<T, E> {
    pub fn is_ok_and(self, f: impl FnOnce(T) -> bool) -> bool;
    pub fn is_err_and(self, f: impl FnOnce(E) -> bool) -> bool;
}

Steps / History

Unresolved Questions

m-ou-se commented 2 years ago

During the discussion in the libs-api meeting just now, there was not only some discussion about whether to take self by reference or value (which didn't reach a definitive conclusion), but also some on whether this method should exist at all.

@dtolnay brought up how he preferred for is_some_and not to be a specific method for Option, but instead for that "is some and" operation to written as three separate words using a new language feature, like is Some && (e.g. opt is Some(x) && x > 5).

This specific syntax was just an example, but the point we generally seemed to agree on was that it's a shame that our Option and Result type have so many methods, even though they're supposed to be these really simple types. Ideally, we'd have some kind of language feature making methods like is_some, map_err (etc. etc.) no longer necessary for ergonomics. Then questions like "should Poll::is_ready() exist?" are no longer relevant, and everyone's custom enums all become nicer to use.

The part that there were some different opinions on was if, for now, we should keep adding things like is_some_and, before such language feature happens (if it ever happens), or if adding methods like that makes things worse.

So, regardless for whether this feature goes through or not, I'd like to encourage ideas and exploration of a potential ergonomic language feature that would cover a large chunk of all Option and Result methods. (Not on this thread though! https://internals.rust-lang.org/ and Zulip are better places for that.)

joshtriplett commented 2 years ago

@m-ou-se I'm currently putting together a comprehensive list of which Option and Result methods would make sense to replace with such syntax. (There are still plenty that don't, but many do.)

jakoschiko commented 2 years ago

This reminds me of the let-expression RFC https://github.com/rust-lang/rfcs/pull/3159. It would allow to write this

assert!((let Some(x) = a) && (let Some(y) = b(x)) && x == y);
steffahn commented 2 years ago

I don’t quite fully understand what the benefit of this method over using matches is, and I don’t see it addressed anywhere in this discussion. I understand that something like foo.is_some() is more neat than matches!(foo, Some(_)), but once a closure gets involved and the name becomes long and non-intuitive like in this proposed method, I don’t see the benefit anymore.

One possible benefit is that it’s a postfix / method-style expression. But if we ever get the ability to do postfix / method-style macros, then optional_deadline.matches(Some(d) if d < now) is probably one of the first adaptors.

I could find this remark in the linked other issue

// Different ways to check if the `deadline` has passed (if there's a deadline), all a bit verbose or confusing:
let a = deadline.map(|d| d > now).unwrap_or(false);
let b = deadline.map_or(false, |d| d > now);
let c = matches!(deadline, Some(d) if d > now);
let d = deadline.map(|d| d > now) == Some(true);
let e = deadline.filter(|d| d > now).is_some();
let f = deadline.iter().any(|d| d > now);

Many of these are “confusing” because they use or combine functions beyond their main intended use case, or treat bools as values (rather than conditions) too much. In particular

However c uses matches exactly as it’s intended to be used. This is no weird case, corner case, not even a slight abuse of a tool that’s generally applied to do other kinds of things like .any typically handling iterators with multiple values, nor does it combine multiple things together in unexpected ways, in fact it’s a single-step process, a single call to matches with a straightforward pattern and a straightforward guard.

So all I can take away from that post is that, apparently / maybe, matches! is considered “a bit verbose”, since I can’t find any way to claim it “confusing”.

Comparing

matches!(deadline, Some(d) if d > now)

vs.

deadline.is_some_with(|d| d < now)

the former is … well … syntactically longer, though that’s mainly due to the words matches+if being longer than is+with; and, as mentioned above, matches! is not postfix (yet). So maybe… let’s just work on that (postfix macros)?

RalfJung commented 2 years ago

is_some_and can be written with matches!, that is true... the dual is_none_or cannot, though, I think?

steffahn commented 2 years ago

is_some_and can be written with matches!, that is true... the dual is_none_or cannot, though, I think?

I guess is_none_or is not straightforward with matches!, indeed. However, that would probably not be an argument in favor of introducing only is_some_and and not is_none_or as is the proposed first step here, as far as I understand. (In fact, following this argument, is_none_or should probably come first, have some merit (being a commonly useful method, etc…) on its own, and only then would a separate discussion about including is_some_and for consistency make sense.)

If we had … and I don’t know if that’s a good idea, and also that’s probably fairly off-topic in this thread … something like a general way to make a guard part of a pattern, then maybe deadline.is_none_or(|d| d < now) could be written as something like deadline.matches!(None | Some(d if d < now)) or deadline.matches!(None | (Some(d) if d < now)). Which reads similar “deadline matches None or …” with the “or”-pattern.

philpax commented 2 years ago

Hi - not sure what the best way to show support for this is, but weighing in to say that I'd very much appreciate this (or some variant of it, like the language extension proposed above.)

I am working with an ECS that lets me retrieve components through an Option; I find myself needing to evaluate predicates against those components reasonably often (e.g. get(tags).map_or(false, |t| t.contains(a_tag)))).

Unfortunately, the ways of currently doing this aren't easy to parse for a reader - map_or(false, predicate) or map(predicate).unwrap_or_default() both obscure the primary objective of the operation to me (it's not really clear what either of those are doing unless you've seen them before)

I would much rather be able to do get(tags).is_some_and(|t| t.contains(a_tag)) (or some variant thereof), which I find to be vastly more readable / semantically obvious. (In a sense, this is me responding directly to BurntSushi's comment from above; I think the proposed alternative is much clearer than what currently exists!)

That being said, if it is possible to replace the majority of the Option/Result combinators with something more general-purpose within the language itself, I would also love that. I'm really just throwing my weight behind the general principle 🙏

camsteffen commented 2 years ago

@camsteffen Apologies for the extra work, but would you be willing to make a draft PR that 1) modifies the signature of is_some_and (I saw that you had a PR for that previously), and then 2) includes an updated version of #98427 that shows what that would look like with a by-value signature?

Here you go! #99087

joshtriplett commented 2 years ago

Thank you, @camsteffen!

Looks to me like by-value improved the vast majority of cases.

kekeimiku commented 2 years ago

Suddenly it feels like having map_or is enough, it seems unnecessary to make Option Result more bloated if it's just for readability without any other advantages. (please feel free to ignore my statement

Amanieu commented 2 years ago

Looks to me like by-value improved the vast majority of cases.

If we do decide to go with that then the docs should have an example using .as_ref().

czipperz commented 2 years ago

I'd like is_none_or. I have a code block like this:

list.iter().filter(|x| {
            if let Some(symbols) = &symbols {
                symbols.contains(&x)
            } else {
                true
            }
})

it could instead be:

list.iter().filter(|x| {
            symbols.is_none_or(|symbols| symbols.contains(&x))
})

Alternatively right now I could write symbols.map(|symbols| symbols.contains(&x)).or(true) but it doesn't really read well imo.

joshtriplett commented 2 years ago

@rfcbot resolved ref @rfcbot concern should-take-self-by-value

jdahlstrom commented 2 years ago

(This is slightly tangential but also a potential argument against adding a new method when there's an existing, almost as concise alternative:)

Hmm. To me the obvious way to write "is_some_and()" has always been filter().is_some() rather than map_or:

optional_deadline.is_some_and(|d| d < now);

vs

optional_deadline.filter(|d| d < now).is_some();

but I haven't seen it mentioned in this (or the previous) thread. Maybe it's less idiomatic than I thought? I noticed that Option::filter has only been stable since 1.27 so there's maybe that?

RalfJung commented 2 years ago

I didn't even know filter existed, I would have thought you would have to into_iter first. Nice!

arniu commented 2 years ago

RFC: map_or_default in Option and Result

We should add a single map_or_default rather than is_some_and / is_ok_and.

Amanieu commented 2 years ago

I don't think map_or_default is the right solution here: if anything, it's less readable than a plain map_or.

jam1garner commented 2 years ago

I don't think map_or_default is the right solution here: if anything, it's less readable than a plain map_or.

Agreed, just to be another data point: I virtually never use the *_or_default methods for primitives. Using unwrap_or* as an example, not only is .unwrap_or(false) shorter than .unwrap_or_default(), but it is one less layer of cognitive indirection (in terms of what an internal monologue might looks like: "unwrap or default. what is the default? false". potentially worse if it takes a second to think about what type).

arniu commented 2 years ago

Maybe map_or_default is not the right solution here, but it's better than is_some_and / is_ok_and / is_err_and.

OliveIsAWord commented 2 years ago

Adding my own data point in support of is_none_or. My current use case is parsing a language with significant whitespace.

let foo = tokens.last().map(Token::kind);
let n = Token::Newline;
// `bar` should be true iff the latest token wasn't a newline or if this is the beginning of the source code.
let bar = match foo {
    Some(x) if x == n => false,
    _ => true,
};
let bar = foo.map_or(true, |x| x != n);
let bar = foo.is_none() || foo.unwrap() != n;
let bar = foo.is_none() || foo.is_some_and(|x| x != n);
let bar = foo.filter(|y| y == n).is_none();
let bar = foo.is_none_or(|x| x != n);

I hope you agree that using is_none_or provides the shortest and most readable solution.

m-ou-se commented 2 years ago

I'd be happy to approve a PR that adds is_none_or as an unstable function. :)

urben1680 commented 2 years ago

I would have a use case for this too. I have a VecDeque and at some point I want to find out if I need to pop the front. I would do this if:

  1. There is a second element
  2. The second element has a certain value
if self.entries.get(1).is_some_and(|second| second.value == value) {
  self.entries.pop_front();
}
flukejones commented 2 years ago

Hexerator is currently using this https://github.com/crumblingstatue/hexerator/commit/8d643c5153dbe366b55b607972031cabaa7705c2

SUPERCILEX commented 1 year ago

Since it is unclear to me that the past few examples are aware of matches (I certain wasn't!), here's some rehashing:

Niki4tap commented 1 year ago

Hey! Just passing by to say that the feature gate was renamed to is_some_and, but the tracking issue wasn't updated, still says is_some_with.

dogunbound commented 1 year ago

I have a use case for it too:

fn is_draw_full_cell_enabled(color_cell: Option<Rc<RefCell<ColorCell>>>) -> bool {
    if let Some(color_cell) = color_cell {
        color_cell.borrow().draw_full_cell
    } else {
        false
    }
}
match event {
    Event::MouseButtonPressed { button, x, y }
        if is_draw_full_cell_enabled(
            color_grid.mouse_position_to_cell_mut(Vector2::new(x, y)),
        ) => {}
    _ => {}
}

Could just be:

match event {
    Event::MouseButtonPressed { button, x, y }
        if color_grid
            .mouse_position_to_cell_mut(Vector2::new(x, y))
            .is_some_and(|cell| cell.borrow().draw_full_cell) => {}
    _ => {}
}

My code would be far simpler if I could just do that.

m-ou-se commented 1 year ago

Looks like this got stuck at some point. I believe the main thing left to do was to change it to take self by value, which already happened in #98354.

Let's start a new FCP.

@rfcbot cancel

@rfcbot merge

rfcbot commented 1 year ago

@m-ou-se proposal cancelled.

rfcbot commented 1 year ago

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

No concerns currently listed.

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.

SUPERCILEX commented 1 year ago

Was there a resolution to the concern that the same behavior could be achieved with matches?

joshtriplett commented 1 year ago

@SUPERCILEX is_some_and looks clearer and chains more easily. matches! requires going back to the beginning of the expression to open a matches!(, and looks more complex when embedding an if in it.

rfcbot commented 1 year ago

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

SUPERCILEX commented 1 year ago

@joshtriplett Fair enough, thanks!

rfcbot commented 1 year ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

jplatte commented 1 year ago

Is this just waiting for somebody to create a stabilization PR? I can probably do that if so.

Amanieu commented 1 year ago

Is this just waiting for somebody to create a stabilization PR? I can probably do that if so.

Yes. Feel free to go ahead.

asquared31415 commented 1 year ago

The description of this API in this tracking issue and in #110019 say that the closure takes &T but the implementation has the closure taking T.

Amanieu commented 1 year ago

We had a second FCP to change the closure to take T, but it seems that we forgot to update the issue description. I've just updated it.

jplatte commented 1 year ago

I totally forgot about all that.. Well I guess I will live with it. The draft PR to show that it made is_some_and calls "nicer" unfortunately didn't show any places where taking ownership of of the inner value was necessary, but maybe there are a few and it would probably be pretty annoying for some people for the combinator to not be applicable therethr same way I find the extra as_ref annoying where it is needed..

wrenger commented 1 year ago

Well, I don't know if these functions are really that essential. These checks can be made in several different ways.

let data = Some("foo");

if data == Some("foo") {
    //...
}
if data.is_some_and(|inner| inner == "foo") {
    //...
}
if let Some(inner) = data && inner == "foo" {
    //...
}
if matches!(data, Some(inner) if inner == "foo") {
    //...
}

The first might only be desirable in some cases, but the third one with let-chains https://github.com/rust-lang/rust/issues/53667 is a little bit more legible, in my opinion.

tnblumer commented 1 year ago

In which version should we expect this feature? I thought it was going to be in 1.70.0, but apparently not.

RalfJung commented 1 year ago

It is in 1.70.0 (see https://blog.rust-lang.org/2023/06/01/Rust-1.70.0.html)