Skip to content

Conversation

@clubby789
Copy link
Contributor

@clubby789 clubby789 commented Dec 8, 2025

This implements the followup warning suggested in rust-lang/rfcs#3695
Lint against an empty cfg(any/all), suggest the boolean literal equivalents.
#149791 (comment)

Tracking issue: #131204

@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2025

Some changes occurred in check-cfg diagnostics

cc @Urgau

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 8, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2025

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rust-log-analyzer

This comment has been minimized.

@jdonszelmann
Copy link
Contributor

r? jdonszelmann

@rustbot rustbot assigned jdonszelmann and unassigned chenyukang Dec 8, 2025
@Urgau
Copy link
Member

Urgau commented Dec 8, 2025

Can you split-off the FALSE -> false suggestion into it's own PR?

As for the warning it should probably be a lint (so it can be enabled and disabled), but either way the warning will need to go through T-lang first, with a nomination and maybe a crater run.

@joshtriplett joshtriplett added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 8, 2025
@joshtriplett
Copy link
Member

With my T-lang hat on, this seems reasonable. Nominating for discussion of the new lint.

@rust-log-analyzer

This comment has been minimized.

@clubby789
Copy link
Contributor Author

Running into the above error where

    (@__items ($($not:meta,)*) ; ( ($($m:meta),*) ($($tokens:tt)*) ), $($rest:tt)*) => {
        #[cfg(all($($m,)* not(any($($not),*))))] cfg_if! { @__identity $($tokens)* }
        cfg_if! { @__items ($($not,)* $($m,)*) ; $($rest)* }
    };

is invoked with no $not, meaning we expand to not(any()) and raise the lint. Inclined to think that we should only lint for the literal any()/all() cases instead

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2025

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@clubby789
Copy link
Contributor Author

Implemented as a pre-expansion early lint

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@jdonszelmann jdonszelmann left a comment

Choose a reason for hiding this comment

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

Though I'm not T-lang, I agree with josh that this is a good lint to add. Just got some comments about the implementation but nothing too major. Just gotta clean some things up :)

View changes since this review

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@clubby789
Copy link
Contributor Author

Moved back to an attribute lint as ignoring spans from expansions resolves the above issue, and addressed the other comments

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@clubby789 clubby789 changed the title cfg_boolean_literal warnings Lint against cfg({any()/all()}) Dec 9, 2025
@rust-log-analyzer

This comment has been minimized.

Comment on lines 42 to 49
} else if list.is_empty() && meta.has_name(sym::all) {
span_lint_and_then(
cx,
NON_MINIMAL_CFG,
meta.span,
"unneeded sub `cfg` when there is no condition",
|_| {},
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed an overlapping case from Clippy

@rust-log-analyzer

This comment has been minimized.

@jdonszelmann
Copy link
Contributor

Alright, @rust-lang/lang r=me if you agree to accept this one

@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross added the P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang label Dec 9, 2025
@jdonszelmann
Copy link
Contributor

And ci passes haha

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

The Miri subtree was changed

cc @rust-lang/miri

@rustbot rustbot added the T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue. label Dec 9, 2025
@ChayimFriedman2
Copy link
Contributor

There is really no reason for the rust-analyzer changes, please revert them.

@clubby789 clubby789 removed the T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue. label Dec 9, 2025
@clubby789
Copy link
Contributor Author

My understanding was that the diagnostic changes would cause downstream failures; reverted 🙂

@ChayimFriedman2
Copy link
Contributor

It's not real Rust code, it's a string used for testing and parsed by r-a.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting.

We observed that applying this change would raise the MSRV of the code. Clippy has a mechanism for noting the MSRV of code and (optionally) not making suggestions that would raise it. rustc doesn't have that mechanism. We felt we didn't want to add this without such a mechanism.

(Separately, some folks felt that it should stay in clippy and not rustc even with a mechanism; that is a subject of ongoing discussion.)

@clubby789
Copy link
Contributor Author

I think for now I'll open a PR folding this into clippy::non_minimal_cfg, and change this PR to only removing cfg(any()/all()) from this repo (reverting the Miri and Clippy changes), which will make it easier to potentially uplift the lint in future.
I'll also look at a MSRV mechanism for rustc lints in another PR

@clubby789 clubby789 changed the title Lint against cfg({any()/all()}) Remove uses of cfg({any()/all()}) Dec 10, 2025
@clubby789
Copy link
Contributor Author

MSRV'd version of the lint in #149870

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.