Lint to check alphabetical order of uses, mods & crates

[wafflespeanut]

fixes #7451

[wafflespeanut]

@Manishearth There are two things left. For now, this only gets the names of extern crates and uses and puts them into a vector. So, how shall I go about getting the names of mods? And, how should I sort the strings in the vectors? (so that I can compare them later?).

[nox]

This has the exact same problem as the Python lint, in which import lists aren't checked for sortedness, right?

I mean things like:

use mod::{foo, bar, qux};

[wafflespeanut]

@nox No, only the individual declarations. I think checking for sortedness in import lists is probably a bad idea, since some of the giant files have a lot of lists and arranging them would be painful :)

[Manishearth]

If the lint outputs a sorted list to be used, it wouldn't be so bad. But this PR is fine too, we can discuss whether or not to augment the functionality.

[Manishearth]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{components/plugins/lints/sorter.rs, line 37 [r1] (raw file):}
We may want to glob import in the future, so blocking std::prelude is a better idea here. But this is okay too.

---

Comments from the review on Reviewable.io

[wafflespeanut]

Oh yeah, right. I now remember that we'll be actually suggesting how the stuff should be sorted :)

[wafflespeanut]

Now, there's a InternedString <--> String problem

[Manishearth]

You don't need to put r=Manishearth in your commits. Homu will do that in the merge commit

[wafflespeanut]

@Manishearth I'm not sure whether it's pretty, but we have a working lint! ^_^

Here's how crates are checked (same goes for mods)...

Here's how use lists are checked (cc @nox)...

[wafflespeanut]

This still needs more polishing...

[jdm]

Calling the lint sorted_declarations means that deny(sorted_declarations) actually means "disallow unsorted declarations", which is a bit confusing.

[wafflespeanut]

Yeah, right. That should be deny(unsorted_declarations) :)

[Manishearth]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

^{components/plugins/lints/sorter.rs, line 34 [r2] (raw file):}
Also ignore mods with attributes

---

^{components/plugins/lints/sorter.rs, line 55 [r2] (raw file):}
You need to sort it so that self comes at the beginning of the list

---

^{components/plugins/lints/sorter.rs, line 87 [r3] (raw file):}
&Vec should be &[..]

Also, T: Deref<Target=String> should be enough, reallyx

---

^{components/plugins/lints/sorter.rs, line 93 [r2] (raw file):}
Error message should be better.

Something like

{} should be in alphabetical order\n\texpected: {}\n\tfound: {}

where the first {} is either "use statements", "module declarations", or "crate declarations"

---

Comments from the review on Reviewable.io

[wafflespeanut]

Now, all that's left is to sort the declarations in the existing files (which is gonna be a pain!)

[Manishearth]

This should be Warn. Deny is going to be very annoying when you're fiddling with imports temporarily before checking in.

[Manishearth]

Not like this.

Use lists should be in alphabetical order.
try:
use foo::bar::baz::{a,b,c,d};

print out a fully copyable use statement

[Manishearth]

ditto.

[wafflespeanut]

This is still incomplete. Firstly, I gotta cripple the lint, so that it doesn't walk into servo/target/... and check the generated DOM bindings. And second, I haven't yet given any preference to the pub stuff. So far, the sorting is based on the names of the declarations.

Sidenote: I'm still not sure whether this is efficient (I've tried my best), and so, I welcome suggestions :)

[jdm]

Alternatively, we could fix the generated bindings.

[wafflespeanut]

This just keeps getting better and better! Now, we gotta filter the additional attributes that come clinging with the mod declarations (for instance, this shows #[allow(non_camel_case_types)] even though it's not actually declared there, but here).

Also, the lint doesn't care about renamed declarations (for now).

extern crate foobar as foo    // this is interpreted as `extern crate foo`

... which should also be fixed as well.

[wafflespeanut]

(current status)

[bors-servo]

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

[wafflespeanut]

Alright, tidy doesn't give up. It expects pub mod before mod, but our sorting is something like #[macro_use] stuff at the start, followed by the private mods and then followed by pub mods.

[jdm]

It seems to me that the main downside of doing this check in a lint vs. in the python script is that we lose the immediate feedback that travis provides, so we won't notice that a PR needs amending until it fails to merge.

[wafflespeanut]

@jdm Agreed. Also, since the lint only throws warnings, we can't know whether a PR has merged with warnings. (sigh) after all that work... :/

@Manishearth maybe we should go back to throwing errors! :)

[Manishearth]

No. No errors. Go does this with unused imports, and it's extremely annoying when this happens while you're developing. So much that a myriad of solutions, ranging from auto-import-fixers to modified compilers have come out.

Style lints should not be errors.

[wafflespeanut]

Okay, since this lint doesn't fit very well here (nor rustfmt), I've planned to move this elsewhere. I'll just submit a PR for the files (well, I can't just abandon all the work, it's consumed a lot of my time! :P)