Skip to content

Enforcing uniform "use" order per crate. #879

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

Closed
regexident opened this issue Apr 25, 2016 · 10 comments
Closed

Enforcing uniform "use" order per crate. #879

regexident opened this issue Apr 25, 2016 · 10 comments

Comments

@regexident
Copy link

When writing crates with lots of inner modules one ends up with a ton of pub use …; / use …; and pub mod …; / mod …; at the head of each file.

Without some automatic sanitizing this inevitably leads to inconsistencies in their ordering.

I for one prefer to have them sorted in groups of:

extern crate thirdparty;
pub mod;
mod;
pub use std::;
pub use thirdparty::;
pub use internal::;
use std::;
use thirdparty::;
use internal::;

I am aware that this is a very personal preference and thus plain wrong as a clippy lint (unless rust-lang starts "enforcing" a strict code style, at which point this behaviour would become part of rustc anyway).

My proposal thus is to add a lint that first scans all files of a crate and creates an ordering profile of the crate and then in a second phase emits warnings for all files that don't match the most common ordering in the crate.

This way we wouldn't need any config, nor would we enforce any specific pattern, but would instead derive the pattern from the existing crate files.

I already have a simple (Markov-based) implementation of such a profile extractor that can be trained on sequences of (one sequence per file):

enum Statement {
    ExtCrate,
    PubMod, PrivMod,
    PubStdUse, PubExtUse, PubOwnUse,
    PrivStdUse, PrivExtUse, PrivOwnUse,
}

I'll need to be able to distinguish between paths from std, an external crate as well as my own crate in order to map them to Statement.

Could you give me pointers as to which existing similar clippy lints or rustc::lint docs to look into?

@llogiq, mind mentoring me on this one? 😉

@regexident
Copy link
Author

This lint would probably be allow by default.

@llogiq
Copy link
Contributor

llogiq commented Apr 25, 2016

Yeah, it's too opinionated to be warn. Also I'd make it configurable so one could select the style – this way there's less moving parts and no risk of generating unwanted suggestions.

@Manishearth
Copy link
Member

I don't like the idea of being opinionated here; especially since many projects alphabetize instead of distinguishing between third party and internal.

That said, it might be a nice idea to have rustfmt auto-alphabetize things.

@llogiq
Copy link
Contributor

llogiq commented Apr 25, 2016

Agreed; this is better suited to rustfmt

@regexident
Copy link
Author

regexident commented Apr 25, 2016

@llogiq: By my approach it would suffice to simply write your first files in your desired ordering and have it enforced from then on. (The "moving parts" of the profile extractor are currently 50loc and could probably be reduce quite a bit.)

@Manishearth: This lint caters to all those who organize their file headers semantically and/or by frequency/importance of information ("what's used from outside" > "what's exposed to outside" > "what's inside") when browsing code files. I'm not talking about alphabetization at all here. Mind to elaborate? (By chance I just happened to file https://github.com/Manishearth/rust-clippy/issues/881 for exactly those people who happen to order their stuff by alphabet).

@Manishearth
Copy link
Member

I'm not talking about alphabetization at all here.

I know, I'm saying that a large portion of the community doesn't organize semantically, and instead organizes alphabetically.

@regexident
Copy link
Author

I know, I'm saying that a large portion of the community doesn't organize semantically, and instead organizes alphabetically.

In that case it wouldn't matter to them as the lint would be allow by default.
For all others the lint would be quite useful. Especially the aspect of zero-config.

@Manishearth
Copy link
Member

So the point is that while I'm okay with allow lints that the community doesn't agree with (as in, doesn't agree that it needs to be linted in the first place), I'm more wary of allow lints where there is at least one conflicting option that the community likes to use.

Additionally, such lints (either kind) are more suited to a tool like rustfmt. Servo uses a python script for alphabetization and it's quite annoying to fix the errors. The output could be improved, but really all you need is a tool that auto-rewrites it, which is rustfmt. Rustfmt would need to store some information about the extern crate declarations for the semantic sort, but it's still duable.

@Manishearth
Copy link
Member

As far as rustfmt is concerned the two options (semantic vs alphabetization) would be doable as a config option, too, perhaps off by default.

Nick might be able to mentor you on this in rustfmt if you want to work on it, not sure. I'd do so myself except that I'm very busy this week.

@regexident
Copy link
Author

regexident commented Apr 25, 2016

So the point is that […] I'm more wary of allow lints where there is at least one conflicting option that the community likes to use.

Fair point. (Especially as fixing these warnings would be quite high-maintenance and could easily automated by, say rustfmt)

As far as rustfmt is concerned the two options (semantic vs alphabetization) would be doable as a config option, too, perhaps off by default.

Yeah, that's what I thought, too. One could then just provide auto as a possible config, in addition to allowing one to provide a strict custom ordering.

Nick might be able to mentor you on this in rustfmt if you want to work on it, not sure. I'd do so myself except that I'm very busy this week.

Great! I'll turn this into an issue/PR at rust-lang-nursery/rustfmt and hit him up then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants