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

Load lints from plugins #6746

Closed
wants to merge 8 commits into from
Closed

Load lints from plugins #6746

wants to merge 8 commits into from

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Feb 16, 2021

This PR gives Clippy the ability to load lints from shared libraries, i.e., "plugins."

For a quick demo, try the following:

  1. cargo build
  2. cargo build --manifest-path plugin_examples/allow_clippy_lints/Cargo.toml
  3. cargo run --bin cargo-clippy '' --plugin plugin_examples/allow_clippy_lints/target/debug/liballow_clippy_lints.so

The benefit of this feature is that it allows one to develop lints without having to fork the entire Clippy codebase.

rustc has (had?) a plugin feature, but the idea seems to have fallen out of favor (rust-lang/rust#29597). Moreover, most do not run rustc directly, but instead allow cargo to do the dirty work of setting up rustc's arguments. Combine this with the fact that Clippy is the de-facto standard for linting Rust codebases. This makes Clippy the ideal platform for reviving the rustc plugin functionality as it applies to linting.

Note that if this PR is accepted, the clippy_utils crate should be published on crates.io.

@rust-highfive
Copy link

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2021
@flip1995 flip1995 added S-needs-discussion Status: Needs further discussion before merging or work can be started and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 16, 2021
@flip1995
Copy link
Member

I'm..... not sure what I should think of this. The plugin interface was removed for a reason. I think we don't want the plugin interface in Clippy for the same reasons that rustc removed it.

The benefit of this feature is that it allows one to develop lints without having to fork the entire Clippy codebase.

If you want to write your own lint tool, you can do this with the rustc API today. No need to fork Clippy, just to write lints.

I'd rather make it easier to write lint tools based on the rustc API than hacking something like a plugin interface into Clippy, making the clippy-driver even harder to understand and maintain. I think this has to be solved with a plan in rustc, not hacked into Clippy.


Overall I think Clippy should stay an end-product, rather than a Rust front end that makes lint writing easy. I don't think we have the resources to maintain Clippy as a library/Rust frontend/plugin interface.

So -1 from my side on this


I'm not against publishing a clippy_utils / lint_utils crate though.

@smoelius Was there previous discussion on doing this?

@rust-lang/clippy Your opinions on this?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 16, 2021

I agree with @flip1995

We should not expose the compiler's plugin interface at all. If anything, we should design our entirely own lint plugin scheme. I have broad ideas on this topic, and none of them end up exposing compiler internals to users. Instead we'll have an entirely independent scheme that does not use the rustc plugin scheme and not necessarily even use any kind of dylib scheme.

If you want to make writing your own lints easier, I think the resources would be best invested in making it trivial to create custom compiler drivers.

@smoelius
Copy link
Contributor Author

smoelius commented Feb 16, 2021

Was there previous discussion on doing this?

There's no previous discussion that I am aware of.

If you want to write your own lint tool, you can do this with the rustc API today. No need to fork Clippy, just to write lints.

But then you would have to write your own driver, right?

From an ergonomics perspective, I think it would be more convenient to have:

<one-tool> <options> <path-to-lint>

than to have multiple drivers. For example, wouldn't it be great if, to write a new lint, all you had to do was fork something as small as this and start editing? https://github.com/smoelius/rust-clippy/tree/master/plugin_examples/allow_clippy_lints

I am under the impression that writing a driver is non-trivial. Since that work is already done in Clippy, it would seem a shame to have another tool that essentially duplicates that code.

Am I over exaggerating the work required to write/maintain a driver?

Re compiler APIs, I would think Clippy could take a "caveat emptor" approach, i.e., Clippy will get your plugin loaded, but that's it. Any compiler APIs you use could change out from under you, just as they could change out from under Clippy's built-in lints. So, proceed at your own risk.

I am sympathetic to the additional maintenance burden, however. This change would add some code to clippy-driver. I don't know how to avoid that, unfortunately.

@Manishearth
Copy link
Member

Highly against publishing clippy-driver.

At some point it would be nice to have a coccinelle-esque textual format for defining lints ("your lint looks like the code it catches") but I don't think the solution is publishing clippy-driver.

I feel like this is the realm of a separately maintained and published tool. Feel free to copy code from clippy-driver, and I'll point out most custom drivers, Clippy or otherwise, have mostly the same code with minor deviances. It may be possible to write a shim API that has hooks for everything.

@Manishearth
Copy link
Member

Am I over exaggerating the work required to write/maintain a driver?

Custom drivers are pretty easy to write, it's just a bunch of boilerplate

@smoelius
Copy link
Contributor Author

I'm not against publishing a clippy_utils / lint_utils crate though.

Should I then submit a reduced PR that only factors out the clippy_utils / lint_utils crate? What name does the team prefer?

@Manishearth
Copy link
Member

Manishearth commented Feb 17, 2021

I'm okay with the refactor, but I am very wary of publishing anything without a clear understanding of guarantees. Currently rustfmt is having issues because they publish rustfmt-nightly but don't really want to, and I do not wish to get us in the same situation.

If we do this I think we should not provide any guarantees for the published crate, including any guarantee of regular updates for new nightlies. I do not think we have the capacity to do this properly, and would prefer not getting into this mess in the first place. Perhaps just making it a separate crate that people can load as a git dependency would work.

@smoelius
Copy link
Contributor Author

That sounds reasonable to me. Thanks, @Manishearth.

@smoelius smoelius closed this Feb 17, 2021
bors added a commit that referenced this pull request Feb 24, 2021
Factor out `clippy_utils` crate

As discussed in #6746, this PR factors out `clippy_lints::utils` as its own crate, `clippy_utils` .

This change will allow `clippy_utils` to be used in lints outside of Clippy.

There is no plan to publish this crate on `crates.io` (see #6746 (comment)). Dependent crates should obtain it from GitHub.

changelog: Factor out `clippy_utils` so it can be used by external tools (not published)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants