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

Convert lints to use a trait-based infrastructure #14804

Closed
wants to merge 24 commits into from

Conversation

kmcallister
Copy link
Contributor

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
Copy link
Member

huonw commented Jun 10, 2014

cc me

Keegan McAllister added 7 commits June 10, 2014 17:45
We're going to have more modules under lint, and the paths get unwieldy. We
also plan to have lints run at multiple points in the compilation pipeline.
The immediate benefits are

* moving the state specific to a single lint out of Context, and
* replacing the soup of function calls in the Visitor impl with
  more structured control flow

But this is also a step towards loadable lints.
Also change some code formatting.

lint::builtin becomes a sibling of lint::context in order to ensure that lints
implemented there use the same public API as lint plugins.
@kmcallister
Copy link
Contributor Author

Re-pushed with a rebase and some doc fixes.

@alexcrichton
Copy link
Member

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
Copy link
Member

huonw commented Jun 12, 2014

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
Copy link
Contributor Author

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

Sure, that's a good idea.

@kmcallister
Copy link
Contributor Author

@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
Copy link
Member

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

Keegan McAllister added 16 commits June 12, 2014 22:22
It wasn't a very appropriate use of the trait. Instead, just enumerate
unit structs and those with a `fn new()` separately.
Suggested viewing: git show -w
The proper, more general solution is to skip a lint pass entirely when all the
lints it could emit are Allow. That should probably be a ticket once this
lands.
It's really too early to be thinking about this as a separate "public" API.
None of the builtin lints use this, and it's now available through the Context.
@kmcallister
Copy link
Contributor Author

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

This is gone now, in 6512035.

@kmcallister
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@kmcallister kmcallister reopened this Jun 18, 2014
@kmcallister
Copy link
Contributor Author

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

bors added a commit that referenced this pull request Jun 24, 2014
This is a rebase of #14804 with two new commits on top to implement and test lint plugins.

r? @alexcrichton @huonw: Can you take a look at the new commits, and also weigh in about any issues from the old PR that you feel are still unresolved? I'm leaving the old branch alone to preserve discussion history.
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

Successfully merging this pull request may close these issues.

5 participants