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

Add needless_if lint #10921

Merged
merged 6 commits into from
Jun 12, 2023
Merged

Add needless_if lint #10921

merged 6 commits into from
Jun 12, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 10, 2023

first off: Sorry about the large diff. Seems a ton of tests do this (understandably so).

this is basically everything I wanted in #10868, while it doesn't lint all unnecessary empty blocks, it lints needless if statements; which are basically the crux of the issue (for me) anyway. I've committed code that includes this far too many times 😅 hopefully clippy can help me out soon

closes #10868

changelog: New lint [needless_if]

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2023

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 10, 2023
@Centri3 Centri3 force-pushed the needless_if branch 2 times, most recently from 1d90048 to 0fc3b31 Compare June 10, 2023 12:07
@Manishearth
Copy link
Member

@blyxyas want to take a crack at reviewing this?

(cc @xFrednet )

@blyxyas
Copy link
Member

blyxyas commented Jun 11, 2023

Yep, Will review it!

add description
tests/ui/needless_if.rs Outdated Show resolved Hide resolved
tests/ui/needless_if.fixed Outdated Show resolved Hide resolved
clippy_lints/src/needless_if.rs Outdated Show resolved Hide resolved
clippy_lints/src/needless_if.rs Outdated Show resolved Hide resolved
don't lint on `if let`
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nitpick and I think this is done. Thanks! ❤️

cc @xFrednet

clippy_lints/src/needless_if.rs Show resolved Hide resolved
@Alexendoo
Copy link
Member

Some cases to check:

if true {
    #[cfg(any())]
    foo;
}
macro_rules! m {
    ($($t:tt)*) => {
        if true {
            $($t)*
        }
    }
}

m!();

@Manishearth
Copy link
Member

@bors r=blyxyas,Manishearth

looks great, you can ignore the comment i left or make a followup PR if you agree with me

@bors
Copy link
Contributor

bors commented Jun 12, 2023

📌 Commit 108c04a has been approved by blyxyas,Manishearth

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 12, 2023

⌛ Testing commit 108c04a with merge 21e6235...

@bors
Copy link
Contributor

bors commented Jun 12, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas,Manishearth
Pushing 21e6235 to master...

@bors bors merged commit 21e6235 into rust-lang:master Jun 12, 2023
@Centri3
Copy link
Member Author

Centri3 commented Jun 12, 2023

Some cases to check:

if true {
    #[cfg(any())]
    foo;
}
macro_rules! m {
    ($($t:tt)*) => {
        if true {
            $($t)*
        }
    }
}

m!();

Sorry I didn't get to this 😅 I'll do this as a followup, though I'm pretty sure it'll lint on both currently.

@Alexendoo
Copy link
Member

Opened #10935 for that + a few other cases I thought of/ran into

bors added a commit that referenced this pull request Jun 13, 2023
Don't lint non-statement/faux empty `needless_if`s

Also has a basic fall-back for `if` statements that have attributes applied to them and incorporates #10921 (review) while I was there

r? `@Manishearth`

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint to warn on empty blocks
7 participants