Convert lints to use a trait-based infrastructure

[kmcallister]

This untangles each lint's code and state from the overall Context and its Visitor implementation. It also replaces the enum Lint with an "extensible variant" based on the address of a static Lint struct.

Actually implementing lint plugins on top of this is only about 30 lines of code, but it's waiting on rust-lang/rfcs#89, so probably won't land for a few weeks.

I'd like to get the non-user-facing parts merged sooner, because it's a generally desirable cleanup, and it's a total pain to rebase across any other lint change.

[huonw]

cc me

[kmcallister]

Re-pushed with a rebase and some doc fixes.

[alexcrichton]

Can you profile lint passes running on something like rustc or servo? There are many many more virtual calls than there were before, and I want to make sure that this doesn't regress too much.

[huonw]

I'm not keen on this using Default so much, it ends up with a pile of extra Option<...>s for lints that need more information (and then a pile of .unwrap/.expects to handle them).

Could there be a initialise(...) -> Self method in the trait, which passes useful things for lints to initialise themselves with (like a reference to the tcx)?

Theoretically this would allow for phased lints, by making it something like

enum LintPhase { AstOnly, Resolved, TypeChecked }

trait LintPass {
    fn initialise(phase: LintPhase, ...) -> Option<Self>

and then lints that don't want to run for some particular phase would just return None. (Of course, it might be better to just store the lints separately for each stage, not really sure.)

[kmcallister]

Can you profile lint passes running on something like rustc or servo?

Sure, that's a good idea.

[kmcallister]

@huonw: There's only one Option field among the builtin lints I ported (in MissingDoc), and it's not really related to the use of Default.

Basically we're just using Default::default() as the constructor for builtin lints, because it can be auto-derived for the common case of unit structs. Lint passes that do have state don't derive Default, they just write their constructor as an impl Default. You can see this on some of the other lints and the other two fields on MissingDoc.

This is done just so that the add_builtin_lints! macro has a uniform interface to initialize builtin lints. I'm leaning towards scrapping this and just having two macros, one which does builtin::$name and one which does builtin::$name::new().

The reason for exported_items: Option<ExportedItems> is just that we don't pass the crate to register_builtin and through to these constructors. I could fix this.

For a plugin lint, the plugin registrar creates a trait object and does initialization there. If we want that initialization to have access to the crate, say, then we can provide that through the Registry. I think the use of trait objects means that a static method fn initialise() won't work.

I was thinking that phase would be an additional field in struct Lint.

[alexcrichton]

Thanks again for doing this by the way, this looks pretty awesome!

[kmcallister]

@huonw: There's only one Option field among the builtin lints I ported (in MissingDoc),

This is gone now, in 6512035.

[kmcallister]

Thanks for the many helpful comments everyone! By my accounting, the remaining tasks are:

• resolving the duplication between LintPass and Visitor. @alexcrichton, do you buy the argument that these are logically separate things, and we can de-duplicate the code at a later date?
• deciding whether -W help should load plugins. We can certainly put off some of the plumbing until lint plugins actually land. But it's already in place, and it'll be more work to rebase it out.
• possibly changing the syntax for declare_lint!
• profiling (I'm working on this now)
• rebasing and squashing
• opening a ticket for skipping LintPasses whose lints are all Allow

[alexcrichton]

@alexcrichton, do you buy the argument that these are logically separate things, and we can de-duplicate the code at a later date?

I do indeed!

deciding whether -W help should load plugins

My vote is for "not yet", but plausibly in the future.

rebasing and squashing

Thanks again for this!

[kmcallister]

I benchmarked this by building stage2 librustc, with optimization enabled, using a stage1 rustc built with optimization enabled. The time spent on lint checking increased from 0.677 s to 1.193 s, an increase of about 76%. However lint checking still takes only 11% as long as type checking, and 0.4% as long as the backend. Ratios for other crates like libsyntax are similar. So I think this is an acceptable regression.

The other pass timings remained roughly the same, as expected. The code being compiled is also different between these two runs, but the amount of code hasn't changed substantially.

This refactor also makes it much easier to skip lint passes that are set to Allow, which could improve performance in the future. And it makes it easier to run lints in parallel, although I'm not sure the gains would be worth the complexity.

[kmcallister]

deciding whether -W help should load plugins

My vote is for "not yet", but plausibly in the future.

What's the practical reason against it? The code is already written and it's not that ugly. The output lag is less than 1 second even on a huge crate, and rustc -W help with no crate filename is still instant as before.

Also the lint plugin RFC was accepted, so this is less of a speculative future thing.

[kmcallister]

If we don't implement -W help for plugins then the description strings provided by plugins aren't used anywhere, which is weird.

[kmcallister]

This is now #15024. I'm leaving the old branch alone to preserve discussion history.