Lint against empty cfg(any()/all())

[clubby789]

View all comments

Detect usages of cfg(any()) and cfg(all()) and suggest the literal boolean equivalents. When --lint-rust-version is available, gate the diagnostic appropriately.
Will need to be updated for #158134

Implements the lint suggested in rust-lang/rfcs#3695 (Tracking: #131204)
Tracking issue: #157574

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

r? @Kivooeo

rustbot has assigned @Kivooeo.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 19 candidates

[rustbot]

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

cc @rust-lang/miri

[mejrs]

Does this lint fire if the cfg predicate is produced in a macro expansion? I can see some valid use cases for producing all() etc in a macro.

[clubby789]

Does this lint fire if the cfg predicate is produced in a macro expansion? I can see some valid use cases for producing all() etc in a macro.

No;

if list.is_empty() && !list.span.from_expansion() {

gates the emission.

[rustbot]

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

cc @rust-lang/clippy

[clubby789]

This does overlap somewhat with clippy::non_minimal_cfg

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

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.

[JonathanBrouwer]

r=me after T-lang approves

View changes since this review

[traviscross]

Suggested change

	pub EMPTY_CFG_PREDICATE,
	pub EMPTY_CFG_PREDICATES,

Per our RFC 0344 guidance, this should be in the plural.

Also, the naming seems a bit loose. This is targeting two specific cfg predicates, not empty cfg predicates in general (and, of course, we might later add more cfg predicates that we might not want to lint when empty). Have any more targeted ideas for a name?

View changes since the review

[clubby789]

Maybe EMPTY_CFG_COMBINATORS? Or EMPTY_CFG_LISTS (but that's a little less precise)

[ada4a]

Or MANUAL_CFG_BOOL?

[clubby789]

That's probably my preferred suggestion so far (or MANUAL_CFG_BOOLS?), but open to others

[nikomatsakis]

My take is:

• Personally, I think cargo clippy being a distinct tool instead of a lint-group etc is bad ux. I don't care to try and addrress this. I love the name clippy and it has an amazing brand. I have heard others feel differently than me. It's not really the high-order bit here.
• We do need some sense of what kind of lints we want to have be warn/deny by default versus not and so forth. I think we should have clear rules and that warn-by-default should be used sparingly, it's better than people "opt in" to stylistic or pedantic lints.

I would like us to develop checklists and guidelines. We've tried a few times. We should just put something on the lang-team page and adjust it over time. I think there is generally settled precedent that...

Warn-by-default:

• Likely to be a bug
• Things that will change in the future (forward compat warnings) -- these additionally signal in dependencies if they will impact you even as a crate consumer
• Key stylistic conventions (e.g., capitalization), particularly those that help us avoid bugs

Deny-by-default:

• Overwhelmingly likely to be a bug

Allow-by-default:

• Fits into the above categories but only for narrow sets of crates; obvious discoverability problem, for this, named groups are useful, this story is undeveloped.

None of these categories covers this lint. But I think there is another category of lint that may merit warn-by-default, which is to nudge people away from older style. This was compared in the meeting to an edition idiom lint, which makes sense to me, I think edition idiom lints probably could or should be warn-by-default in the new editions in the same way.

[nikomatsakis]

Personally I think putting our "thumb on the scale" to push people to adopt "new and better Rust" is a good idea, it benefits the whole ecosystem if there is a consistent idiomatic Rust and not a wide spread of usage. Many long-time Rust users will not learn about features any other way. I think this is why we warn for key stylistic conventions as well.

I do think clippy, in practice, largely serves this purpose though -- e.g., for consistency, we may want to flag on failure to use if let-conditions and so forth. Perhaps then clippy is the right answer, but this bring us back to discoverability. This seems a bit more extreme than that one, in that cfg(any()) is legitimately confusing.

[nikomatsakis]

Try this again...

@rfcbot fcp merge lang

We discussed this in the lang-team meeting. Everyone agreed the newer style is better. There was some question about how far we want to go in stylistic lints and where the line between rustc and clippy falls.

I'm moving to merge to help move the discussion forward. @traviscross mentioned in the meeting that he would raise a concern about finalizing the naming, which makes sense. I think he or others may also raise a concern to talk about the bigger question (I'll give my thoughts on that separately).

[rust-rfcbot]

@nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• consistency-and-plan (Lint against empty cfg(any()/all()) #158136 (comment))
• naming (Lint against empty cfg(any()/all()) #158136 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

@rfcbot concern naming

There's an open discussion on the name for this in #158136 (comment). Since the name is part of what we accept in the FCP, I'd like to let this settle first.

[traviscross]

@rfcbot concern consistency-and-plan

This lint goes beyond what we normally do with warn-by-default rustc lints. It's not linting against a likely bug or typographical error. It's not pushing a key stylistic convention, such as capitalization, that we lean on as a language matter. It's not something with unclear or shifting semantics that we're trying to drive out of the language. It's a minor stylistic point.

I'm not sure that we should light up CI for people on this.

That said, I agree that it can sometimes make sense to push our perspective on preferred usage with linting. If we want to absorb more of Clippy into rustc by creating appropriate lint groups and discussing whether lints in those groups should be allow-by-default or warn-by-default, I'm open to that. I'd want to finish that discussion first and agree on a plan (probably in the form of an RFC) before accepting lints that break from our established conventions and only make sense in that world.

[tmandry]

In today's lang meeting I mentioned waiting for --lint-rust-version to become available, but I decided against filing a concern. cfg(false) and friends are not something I expect to see often in the published version of crates carrying an MSRV. We don't need that level of UX polish for a rare edge case.