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

Lint suggestion: Suggest separating (or combining) attributes which accept a list of parameters #11331

Open
clarfonthey opened this issue Aug 12, 2023 · 2 comments
Labels
A-lint Area: New lints

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Aug 12, 2023

What it does

Common practice in a lot of Rust code is to separate #![feature(...)], #![allow(...)], #![deny(...)], and #![warn(...)] attributes so that each item is a separate attribute, rather than a list of several. The proposal would be, for some potentially configurable list of attributes, suggest to either separate these attributes or combine them when applied to an item.

Advantage

Generally, separating onto multiple lines makes it easier to not "miss" items where a list is included on a single line. So, having a lint to enforce (and automatically fix) these cases would be very helpful.

However, because some people might also like the alternative method, and it's possible to format the attributes so that the list items are still on their own lines if the list is sufficiently large, I'm proposing to also add the ability to format in that way too.

Drawbacks

It might be too good.

Example

Valid combined:

#![allow(clippy::cursed_rust_patterns, live_code, rustc::eating_laundry)]
#![feature(cursed_types, java_style_inheritance, return_position_type_alias_impl_trait_in_struct)]

Valid separated:

#![allow(clippy::cursed_rust_patterns)]
#![allow(live_code)]
#![allow(rustc::eating_laundry)]
#![feature(cursed_types)]
#![feature(java_style_inheritance)]
#![feature(return_position_type_alias_impl_trait_in_struct)]

Invalid in both cases:

#![allow(clippy::cursed_rust_patterns, rustc::eating_laundry)]
#![allow(live_code)]
#![feature(cursed_types, return_position_type_alias_impl_trait_in_struct)]
#![feature(java_style_inheritance)]

Prior Art

This would have been helpful when working on rust-lang/rust#114766.

@clarfonthey clarfonthey added the A-lint Area: New lints label Aug 12, 2023
@est31
Copy link
Member

est31 commented Aug 13, 2023

I like the ability to grep for #![feature(something)] in rustc (or on grep.app to look for usages in the ecosystem). You can construct regexes but those are not always supported (grep.app limits regex support if it has too many results), and also more complicated to write. Only few search tools support multiline regexes (the feature attr could be multiline if it's a long list).

Same goes for #![allow(...)]. E.g. clippy had surveys ran on how many crates allow some specific lint. With combined attrs, it is harder to make such a survey. From a language development POV, I have also often been curious about where a lint was allowed.

As for grepping for the lint/feature name itself, is not always unique so I'm not sure it is a good idea. Generally, I think going with the separate option would be better, but e.g. with cfg_attr it makes a lot of sense to combine the attrs again, unless it's a simple cfg_attr. I'm not sure a lint outside of pedantic/restriction would be a good idea for this.

@Centri3
Copy link
Member

Centri3 commented Aug 13, 2023

This could probably fit in rustfmt as well, at least for feature, allow, etc., basically all attributes it knows won't change behavior when separated. This would allow this to be applied automatically.

For others however, like external attributes, clippy is probably best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

3 participants