Skip to content

Suggest A && B for if A { B } else { false } #14865

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.

Sign up for GitHub

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

Jump to bottom
Open
scottmcm opened this issue May 21, 2025 · 5 comments · May be fixed by #14904
Open

Suggest A && B for if A { B } else { false } #14865

scottmcm opened this issue May 21, 2025 · 5 comments · May be fixed by #14904
Assignees
@barun511
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy

Comments

@scottmcm
Copy link
Member

scottmcm commented May 21, 2025 •
edited by rustbot
Loading

What it does

Inspired by this code in the standard library: https://doc.rust-lang.org/src/std/collections/hash/set.rs.html#864-866

    pub fn is_subset(&self, other: &HashSet<T, S>) -> bool {
        if self.len() <= other.len() { self.iter().all(|v| other.contains(v)) } else { false }
    }

which could instead be

    pub fn is_subset(&self, other: &HashSet<T, S>) -> bool {
        self.len() <= other.len() && self.iter().all(|v| other.contains(v))
    }

When the "then" block is a single simple expression (rather than a complex thing with a bunch of lets, say), seeing else { false } is a hint that the code can be simplified without changing behaviour or performance.

Playground repro that this is not linted today: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=90c86a06c5fce6c0f816f5f4ec170cc5

Similarly, suggesting A || B instead of if A { true } else { B } would also be good.

Advantage

  • Shorter and arguably simpler code
  • Makes it clearer that this is an "and"
    • By having things that are just &&s be &&s it helps emphasize the places that do need the if-else for their less-common operation.

Drawbacks

If either condition is particularly complicated, splitting it up into the two pieces might help a reader grok the intent.

Example

    pub fn is_subset(&self, other: &HashSet<T, S>) -> bool {
        if self.len() <= other.len() { self.iter().all(|v| other.contains(v)) } else { false }
    }

Could be written as:

    pub fn is_subset(&self, other: &HashSet<T, S>) -> bool {
        self.len() <= other.len() && self.iter().all(|v| other.contains(v))
    }
@scottmcm scottmcm added the A-lint Area: New lints label May 21, 2025
@Jarcho
Copy link
Contributor

Jarcho commented May 22, 2025 •
edited
Loading

Complicated conditions tend to be longer, so at least they would get formatted across lines. I don't think I would ever see the if version as simpler to understand, but maybe someone would.

Do you have a suggestion for the name. I'm at either ifs_as_logical_ops or if_then_bool_else_false. I'd rather the disjunction form also be the same lint which rules out the second name.

@barun511
Copy link
Contributor

Hm, brand new lint seems challenging - but would love to give it a go.

@barun511
Copy link
Contributor

@Jarcho are we aligned that this is something we want to do? happy to have a first crack. for the name, I like ifs_as_logical_ops better.

I'd probably write a first version to only do the conjunctive lint, and then flup with the disjunctive version
@rustbot claim

@Jarcho
Copy link
Contributor

Jarcho commented May 23, 2025

The lint itself is fine. There is a question as to what category to put it in, but that can wait till a PR is made.

@Jarcho Jarcho added the good first issue These issues are a good way to get started with Clippy label May 23, 2025
@scottmcm
Copy link
Member Author

Personally I would propose category style for this. There's nothing wrong with the code, there's just a slightly better way to write it. I guess you could call it manual_logical_and or something.

(It's not correctness or performance, at least.)

@barun511 barun511 linked a pull request May 26, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@barun511 barun511

Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add ifs_as_logical_ops lint barun511/rust-clippy
3 participants
@Jarcho @barun511 @scottmcm