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

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

Closed
8 tasks done
m-ou-se opened this issue Jan 18, 2022 · 92 comments · Fixed by #110019
Closed
8 tasks done

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

m-ou-se opened this issue Jan 18, 2022 · 92 comments · Fixed by #110019
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Jan 18, 2022

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 m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jan 18, 2022
@the8472
Copy link
Member

the8472 commented Jan 18, 2022

What are the use-cases? And won't that largely be covered by if-let chains?

if let Some(foo) = opt && foo > bar {
   
}

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 18, 2022

Lots of functions on Option and Result are already 'covered' by match or if let, but it's still very useful to have methods for the most common cases. It makes things more readable and less verbose, and also easier to write (especially with auto completion).

let deadline_passed = optional_deadline.is_some_with(|d| d < now);

let deadline_passed = matches!(optional_deadline, Some(d) if d < now);

let deadline_passed = if let Some(d) = optional_deadline && d < now { true } else { false };

@the8472
Copy link
Member

the8472 commented Jan 18, 2022

I assume deadline_passedwould then flow into another if in which case one could do an if-let-chain (once that's stable) directly rather than having a separate variable for that. I mean as I understand it the purpose of if-let-chain is to make such towers of conditionals more readable.
Maybe it's useful inside a filter closure? But then one could flatten() first and then filter instead.

I think a code example with a bit more context that would benefit from this and wouldn't be replaced by if-let-chain in the future would be helpful.

Naming. Alternatives:

Or maybe just is? Assuming we want to spend our small budget of meaningful two-letter words on this.
But I like is_some_and since it is consistent with the way the if is written.

@camsteffen
Copy link
Contributor

camsteffen commented Jan 18, 2022

is or any name ending in "is" will not flow well with function names in the argument: opt.and_is(char::is_lowercase), dog.and_is(Dog::has_a_bone), etc.

is_some_and flows better. I would consider the shorter some_and as well.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 18, 2022

I assume deadline_passedwould then flow into another if in which case one could do an if-let-chain (once that's stable) directly rather than having a separate variable for that.

This example about a deadline: Option<Instant> comes directly from code I worked on recently. Checking some condition on a Some is a pattern that occurs reguarly there. In some cases it's just used in an if directly, but in many cases the value is stored in some struct first, or used by multiple ifs, or passed to a function, serialized and sent over a socket, etc. In some cases it's only used in a single if, but part of a much more complicated condition. Various parts of that condition are first assigned to named bools to keep things clear. (Just like why people use matches!() rather than a match or if let in some situations. Or x.is_some() rather than if let Some(_) = x. Or x.then() instead of if x {}. And so on.)

Or maybe just is?

I've added is and has to the alternatives.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 18, 2022

There are 117 cases of .map_or(false, in the compiler/ directory in the rust repository. Most of them are probably good examples of use cases for these methods.

@jakoschiko
Copy link
Contributor

The name is_some_and would allow us to add its counterpart is_none_or in the future.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 22, 2022

is_some_and

After thinking about it a bit more, I like this name. It very clearly states that it first checks is_some, and then also checks a condition afterwards.

@camsteffen
Copy link
Contributor

FWIW this was attempted in #75298.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 11, 2022
Rename is_{some,ok,err}_with to is_{some,ok,err}_and.

This renames `is_{some,ok,err}_with` to `is_{some,ok,err}_and`. This was discussed on the [tracking issue](rust-lang#93050).
csgordon added a commit to csgordon/ir441 that referenced this issue Mar 13, 2022
@m-ou-se m-ou-se changed the title Tracking Issue for Option::is_some_with and Result::is_{ok,err}_with Tracking Issue for Option::is_some_and and Result::is_{ok,err}_and Mar 29, 2022
@jam1garner
Copy link
Contributor

As someone who finds map_or both overly verbose for this (I need this version far more than the general case of map_or) and generally unpleasant (due to argument order not matching the name, although I 100% understand that decision even if I don't love it) I would be thrilled for this.

I think the worry about overlap with if/let is valid but for filtering and other combinators this composes really well (having played with it some now)

Also +1 to is_some_and for the name, very much a fan

@filip-hejsek
Copy link

I often define similar functions in my code as an extension trait, and is_some_or is exactly the name i used (before it was added to nightly rust). I think it's a very intuitive name.

I would like to also see the is_none_or counterpart added to Rust. Note that is_none_or cannot be easily expressed using if-let chains.

@AmitPr
Copy link

AmitPr commented Jun 19, 2022

Would love to see this stabilized. My usecase is checking if a submitted value matches the last value, if there was a last value:

//Earlier, for example:
let last : Option<u64>;
let next : u64;
//In logic
if last.is_some_and(|v| v == next) {
    return Err(());
}

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 20, 2022

@AmitPr if last == Some(next) should also work, right?

@SrTobi
Copy link

SrTobi commented Jun 20, 2022

@m-ou-se: not if next is expensive.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 20, 2022

It seems like everybody is happy with the current names for these methods. I think it's time to consider stabilization.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jun 20, 2022

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 20, 2022
@SrTobi
Copy link

SrTobi commented Jun 20, 2022

what is with is_none_or?

Btw, I found this issue here by explicitly searching for is_none_or approximately 30min after I discovered is_some_and just because the later implicated the former's existence.

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 20, 2022

is_none_or is a separate method that has been suggested but isn't part of this tracking issue.

@SrTobi
Copy link

SrTobi commented Jun 20, 2022

Thx, does this mean we have to create a new issue for that method or is it somehow possible to attach it to this issue? I can also do the impl, just not sure how the process is.

@camsteffen
Copy link
Contributor

I think the function should take the self instead of &self to be consistent with other function-taking methods like map_or. That makes this function strictly more useful since the predicate can take ownership of the inner value.
It might be safe to say that predicate functions usually don't need ownership, but the API shouldn't impose that. There will be some cases where this is required or more efficient. You can always use as_ref().is_some_and(..) to avoid consuming the Option if needed. is_some doesn't take self because there would be no benefit in doing so, and it would force users to write as_ref().is_some() in some cases.

@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Mar 7, 2023
@SUPERCILEX
Copy link
Contributor

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

@joshtriplett
Copy link
Member

@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 rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 24, 2023
@rfcbot
Copy link

rfcbot commented Mar 24, 2023

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

@SUPERCILEX
Copy link
Contributor

@joshtriplett Fair enough, thanks!

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 3, 2023
@rfcbot
Copy link

rfcbot commented Apr 3, 2023

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
Copy link
Contributor

jplatte commented Apr 5, 2023

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

@Amanieu
Copy link
Member

Amanieu commented Apr 5, 2023

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

Yes. Feel free to go ahead.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 6, 2023
@asquared31415
Copy link
Contributor

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
Copy link
Member

Amanieu commented Apr 6, 2023

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
Copy link
Contributor

jplatte commented Apr 7, 2023

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..

@bors bors closed this as completed in b6f6104 Apr 7, 2023
qutax added a commit to pace-running/pace3 that referenced this issue May 8, 2023
for more information see issue #93050:
rust-lang/rust#93050

Signed-off-by: Knut Borchers <[email protected]>
@wrenger
Copy link

wrenger commented May 19, 2023

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 #53667 is a little bit more legible, in my opinion.

@tnblumer
Copy link

tnblumer commented Jun 5, 2023

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

@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.