Add MINIMAL_CFG_CONDITION lint

[GuillaumeGomez]

I encountered a few cases where some code had:

#[cfg(any(unix))]

In this case, the any is useless. This lint checks this and also for the all condition.

changelog: [`non_minimal_cfg`]: Add new `non_minimal_cfg` lint

[rustbot]

r? @Jarcho

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

[GuillaumeGomez]

Surprisingly, I needed to update one ui test locally but I had to revert this change because the updated output fails in the CI.

[asquared31415]

As-is, this lint does not emit any output for empty lists cfg(any()) or cfg(all()). Both are syntactically valid, and any() means "never enable this" where all() means "always enable this". Is it intentional that this lint does not output anything in these cases?

Note that sometimes any() is used to disable something for debugging purposes, should that affect whether this lint does anything?

[GuillaumeGomez]

It is intentional to not handle empty any and empty all as I'm not sure if it's used for voluntarily or not. That could be an interesting other lint though.

Note that sometimes any() is used to disable something for debugging purposes, should that affect whether this lint does anything?

Do you have some examples by chance?

[asquared31415]

#[cfg(any())]
fn uwu() {
    let () = 1; // type error
    let b = a; // use missing identifier
    "a" - 100.0 == Vec // it parses, but definitely not valid
}

// redefinition doesn't cause an issue since the other one doesn't exist
fn uwu() {
    // let's try an alternate implementation without updating any other code
    // or needing to delete the old code
}

Code that's cfg'd out only has to parse, but otherwise doesn't act like it exists at all. Sometimes this is useful for seeing if parts of code are needed, or for swapping out implementations for testing without needing to handle multiline comments or deleting the code. It's probably a moderately uncommon use of cfg but it's not unheard of.

[GuillaumeGomez]

I see. Well then, definitely better to not lint about this part. So I suppose the current lint is valid then since it doesn't prevent this use of cfg()?

[bors]

☔ The latest upstream changes (presumably #10691) made this pull request unmergeable. Please resolve the merge conflicts.

[Jarcho]

A name like non_minimal_cfg would allow the lint to be extended to other reductions. With that #[cfg(all())] should be linted as well.

#[cfg(any())] can't easily be detected anyways as the item doesn't appear in the AST. There's no need to worry about it.

[GuillaumeGomez]

I fixed the merge conflict, updated the lint name to MINIMAL_CFG_CONDITION and added checks for empty any and all.

[Jarcho]

Looks like I was wrong and failed predicates do appear in the AST. It's only the HIR tree they don't appear in. It would be best to ignore any() then as there is no other way to type it. all() should still be linted as it's the same as having no condition.

[GuillaumeGomez]

Only kept the check for all().

[Jarcho]

You have failing fixed code. You can move the test case into a *_unfixable.rs test without rustfix. Perfectly acceptable here given how unlikely an empty any() is in the first place. Or you can add a suggestion to remove the cfg attribute.

[GuillaumeGomez]

I split the test in 2 where I kept the "fixable" one in the first file and the all() check in the other. Hopefully the CI should be happy this time. ^^'

[GuillaumeGomez]

Yep, CI is happy now. \o/

[Jarcho]

Looks good. Thank you. @bors r+

[bors]

📌 Commit dbc76a7 has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit dbc76a7 with merge 940ffdf...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 940ffdf to master...