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

Use is_some_and when appropriate #9125

Closed
pcd1193182 opened this issue Jul 5, 2022 · 24 comments · Fixed by #11030
Closed

Use is_some_and when appropriate #9125

pcd1193182 opened this issue Jul 5, 2022 · 24 comments · Fixed by #11030
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@pcd1193182
Copy link

pcd1193182 commented Jul 5, 2022

What it does

The lint would detect cases where the new Option function is_some_and would be appropriate. Examples would be places where map() converts an Option<T> into an Option<bool> and then unwrap_or(false) is used. The library function is still experimental, but once it's stable, this would be a useful modernization suggestion.

Lint Name

option_is_some_with

Category

complexity

Advantage

Reduces function calls (even if they would optimize down), increases clarity.

Drawbacks

Low priority, since the existing approaches are functional, if less idiomatic/aesthetic.

Example

let x: Option<int> = Some(7);
x.map(|val| val > 5).unwrap_or(false)

Could be written as:

let x: Option<int> = Some(7);
x.is_some_and(|val| val > 5)
@pcd1193182 pcd1193182 added the A-lint Area: New lints label Jul 5, 2022
@giraffate
Copy link
Contributor

There is no method is_some_with on Option. Do you mean is_some_and?

@pcd1193182
Copy link
Author

pcd1193182 commented Jul 6, 2022

Sorry, that's correct. The feature is called is_some_with, which is what confused me.

EDIT: Updated the original comment.

@pcd1193182 pcd1193182 changed the title Use is_some_with when appropriate Use is_some_and when appropriate Jul 6, 2022
@giraffate giraffate added good-first-issue These issues are a good way to get started with Clippy labels Jul 7, 2022
@JoelMon
Copy link
Contributor

JoelMon commented Jul 12, 2022

Is there a hurry to get this done? I would love to learn how to implement lint's.

If this is a good match for a beginner I would love to learn how to implement this.

@Rqnsom
Copy link
Contributor

Rqnsom commented Jul 12, 2022

I'm also looking for good-first-issues since I'm also a beginner, but I feel that a good-first-issue items are never supposed to be urgent. So that we the beginners could take our time to do it at our pace - since learning should not be stressful. So, I think you are free to claim it and see how it goes! :-)

Clippy veterans can correct me if I said anything wrong.

@xFrednet
Copy link
Member

Hey, welcome to Clippy 👋. This issue is not urgent, so you're welcome to claim it and take your time. If you have questions, you're also welcome to ask. Please don't hesitate to ping anyone of us (sometimes we miss questions in issues, if nobody mentions us)

Some more context, about what is urgent, in case you're interested: A new version of Clippy is released every 6 weeks. There are only very few things which are kind of urgent, and these are mostly handled by maintainers. (like writing the changelog and syncing changes with the rust repo). Some issues have a slightly higher priority, like crashes or very common false positives. But even these can be handled differently if nobody wants to fix them. The wonderful part is that Clippy and other rust projects are about building cool software and very little about deadlines and releases :)

@JoelMon
Copy link
Contributor

JoelMon commented Jul 12, 2022

@Rqnsom, @xFrednet Thank you for being so welcoming! :)

How do I claim a lint? Do I do something like @bors claim? I think I saw someone use a command like that once to claim a lint but I can't seem to find it in https://bors.rust-lang.org/

@xFrednet
Copy link
Member

For that, we have our lovely rustbot. Just include @rustbot claim in a comment, to asking an issue to you. See https://rustc-dev-guide.rust-lang.org/rustbot.html in case you're interested in what else you can do with @rustbot 🙃

@JoelMon
Copy link
Contributor

JoelMon commented Jul 12, 2022

@rustbot claim

@JoelMon
Copy link
Contributor

JoelMon commented Jul 12, 2022

I'm noticing that the documentation is pretty fragmented... That looks like a good easy (non technical) project to look at next. lol

@xFrednet
Copy link
Member

Yeah, Clippy sadly also has some areas which are not documented at all. We're recently made some good progress on that, but documentation improvements are always good and welcome!

@kadiwa4
Copy link
Contributor

kadiwa4 commented Jul 21, 2022

This should also trigger on .map_or(false, …), .map(…).unwrap_or_default(), .map(…) == Some(true) and .filter(…).is_some().

@xFrednet
Copy link
Member

It would be cool if all of these causes would be caught. For the start, it might be better to focus on one and add the others later :)

@TheCodingWombat
Copy link

@JoelMon Any progress on this? I would also love to try it as my first clippy lint, and my first contribution to an open source project.

@JoelMon
Copy link
Contributor

JoelMon commented Dec 14, 2022

@JoelMon Any progress on this? I would also love to try it as my first clippy lint, and my first contribution to an open source project.

I haven't been able to take a crack at it and won't be able to any time soon. Go ahead and give it a go. Let me know if I have to do someone on my end to give up claim or what not.

Hope you success and enjoyment to your first contribution!

@TheCodingWombat
Copy link

@rustbot claim

@rustbot rustbot assigned TheCodingWombat and unassigned JoelMon Dec 14, 2022
@TheCodingWombat
Copy link

TheCodingWombat commented Dec 16, 2022

@xFrednet I was trying to implement this lint, but then I found out about an existing lint option_map_unwrap_or. Should this be its own self contained lint (and if so, how do we then make clippy use this lint's suggestion instead of that of option_map_unwrap_or)? Or should we just add some different suggestions for different values of the unwrap_or argument to option_map_unwrap_or?

@xFrednet
Copy link
Member

Hey @TheCodingWombat, good catch! This lint is more specific in the sense that it specially detects boolean conversions. I think adding logic to the mentioned lint, to suggest is_some_and() if the map is used for a boolean conversion, would be the best step.

Small sidenote, the lint has been renamed and the current name is clippy::map_unwrap_or :)

@TheCodingWombat
Copy link

@xFrednet What command do I use to run a single uitest? I tried cargo uitest -- --test-args=test_name.rs, but that doesn't work, also not without the --, even though this was the suggestion of the test output. I think this is because programs are passed to one another and the CLI arguments need to be passed from one to the other, but I don't know how.

@xFrednet
Copy link
Member

xFrednet commented Dec 17, 2022

@TheCodingWombat The interface is a bit weird, as the framework we use doesn't support cli arguments. Instead, a TESTNAME environment value is used. Try TESTNAME=test_file.rs cargo uitest

@frewsxcv
Copy link
Member

Here's another usage that could use is_some_and:

https://github.com/rust-lang/rust/blob/0978711950b77582e4f8f334f6e9848d48ab7790/compiler/rustc_lint/src/nonstandard_style.rs#L69-L72

This could be:

if u.next().is_some_and(|u| l != u) {
    return true;
}

@xFrednet
Copy link
Member

A start of this implementation can be found in this PR #10102 which was closed due to inactivity. If someone else wants to pick it up, I'm happy to help where I can :)

@chansuke
Copy link
Contributor

@rustbot claim

@disco07
Copy link
Contributor

disco07 commented May 3, 2023

@rustbot claim

@rustbot rustbot assigned disco07 and unassigned chansuke May 3, 2023
@darklyspaced
Copy link
Contributor

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.