-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add clippy to this repository #40921
Comments
For those of you playing along at home, the reason we're moving (part of) clippy into rustc is for stable clippy. The plan is to do basically the same thing that RLS does -- move the unstable bits into the Rust distribution, so that it can be used from stable Rust even though it doesn't use stable features. I'm unsure if we should make it installed by default or something that you With that plan, clippy could still live out of tree, but this means that rustc devs will have to go through a ritual each time clippy breaks due to an unstable API, so after discussing with Alex, vendoring in tree (in fact, moving core development to in-tree) is the way to go.
To be clear, cargo clippy and the wiki generation stuff will continue to live out of tree in the original repo. This may involve stabilizing I don't know if we should move issue tracking. We have a LOT of issues filed on clippy, mostly triaged, and tagged for difficulty. I'm fine with continuing to track out of tree, or slowly migrating by encouraging new filings on rust-lang/rust, or migrating at once, or whatever. |
Just as a general question, what's the probability of clippy moving back out of tree at some point? For example, if rust pins down some stable api for clippy (and other things) to use. It's just interesting that on one hand there's "what if" longer term discussions about libstd moving out of tree, then there's the reality of things like clippy moving in tree. And the latter is because clippy happens to be a thing that's widely used and this is the easiest solution to make it stable. I'm all in favour of practical solutions and it'd be really good if clippy worked on stable so I'm very much not objecting - just wondering what the (very) long term vision is, given there are downsides to being in-tree as well (clippy being bottlenecked by the rust release cycle, the slower CI cycle etc). |
A related stable API that is in the process of being created is the rls-API. It might allow us to move a lot of lints to such a stable API. Also I've heard some talk that the linting API gets revamped, because it doesn't fit nicely into the on-demand system the compiler is moving to. Everything is in a lot of flux, so right now there's no way to stabilize such an API. But the desire to have stable clippy is strong enough to ignore the downsides for now and move in-tree. I would very much like to see the linting API becoming stable in the future, but I definitely don't see it on the roadmap this year. |
I know nothing about current linting API and on-demand system, but there are indeed some interesting problems in integrating linting and IDEs. For Java, IntelliJ IDEA has a lot of clippy-like inspections, and it faces an interesting tradeoff. On the one hand, you want the inspections to be local and very incremental (for example, highlighting unused local variables). On the other hand, some inspections want to access the whole project (for example, highlighting unused public methods). The first category nicely fits into the incremental, highlight as you type framework. The second category needs either to be run on the whole project at once explicitly (much like clippy now), or to apply some heuristics to give approximate results, but without recompiling everything. |
I personally feel like we're not quite ready to do this just yet, AFAIK there hasn't been much discussion of:
|
I thought we'd had those discussions already? If not, they can be had here. I don't recall seeing much discussion for RLS either, and AIUI clippy and RLS were both part of the same plan.
The clippy plugin is a part of the Rust distribution, and cargo-clippy is a thin wrapper (which you can install via
From what I recall from earlier discussions, this is basically necessary since y'all want the CI to run the same steps as nightlies (each CI build is a potential nightly build), so components like these would have to be included as well. I'm okay with keeping clippy out of tree, but then the nightlies will break every now and then.
FWIW most of the times clippy rustup stuff is just APIs changing; APIs that are used by the rest of rustc anyway, so this is just like adding and maintaining another internal |
Any updates? RLS is chugging along, and I was under the impression that clippy would be able to use the same method of becoming stable. We should try and make this happen. |
There's an IRC meeting on the topic on May 4th, 16:00 UTC-04:00 (21:00 CET) |
@oli-obk So 22:00 CEST? I'm not sure if "4pm Boston time" includes some kind of summer time in it 😅 |
I agree that we need some solution here, but I think moving Clippy in tree is definitely the wrong one. To be clear, the RLS does not live in-tree, it lives in its own repo and there is a submodule in-tree which is only used for dist builds. That seems like the upper limit on integration we should accept for Clippy. The goal here is to make Clippy available on stable, IMO it would be great to do that with as little integration with rustc as possible. Ideally, we could use rustup to install Clippy (thus giving it access to the compiler's unstable API, whilst itself being stable) without any involvement in the Rust repo at all (although it might turn out that a submodule is the easiest way). I think there are questions on what being 'part of the distro' means, neither the RLS nor Clippy will be installed by default for example, and obviously we don't want Rustup distributing a broken Clippy, but there are different ways that can be achieved, I think. |
So @brson and @alexcrichton and I have discussed this in the past and the reason the solution was moving it in tree was because the nightly dist build should never fail, basically, and they wanted nightly builds to be the same as any other CI build. I am okay with the submodule, but the issue was that this creates friction for people who want to make breaking changes to APIs clippy consumes, so the proposal was to move it in-tree to make it easier to contribute to. "rustup to install clippy" isn't going to work well because there's usually a strict clippy-version-to-nightly mapping, we can't just try to install master and hope it works. It has to be part of the distribution. By "part of the distribution" we just mean that it should be compiled in a dist build and put on the server for folks to fetch via rustup. |
The default plan here is not to have any "integration with rustc", but to make clippy a subfolder in the Rust repository that can be optionally built that CI gates on. The resultant dylib will just float around in the build dir. Rustc won't be aware of it. (Well, rustc will need to be aware of it a tiny bit so that |
After discussing with @nrc on IRC, we're leaning towards this proposal. It's different from the original: Clippy is a submodule (or a pinned commit, w/e). Built on each commit, with notifications to clippy devs or something when it breaks. Built in dist, if it builds it is included, if not the dist is partial (but still published!). The notifications bit isn't necessary for this design, but would be a way to prevent partial dists. If you have run Clippy will still live in its own repo and be developed there. The "I am cargo but also rustc u wot m8" hacks in cargo-clippy can go away. The dist will contain a clippy dylib. Rustc will need a small fix that lets We'd need to handle the clippy.toml stuff by extending It would be nice if we could also make it stable to do Thoughts? |
Sgtm. I wonder if we could automatically load the plugin if anyone writes |
Good idea – warning on lints has some precedent in stable, and the clippy plugin could be loaded on demand. |
In #45285 (comment) I've discovered something slightly more worrisome (but perhaps not so bad in the long run?). Right now clippy is its own workspace with its own Cargo.lock, but in general we're trying to get all our tools into the same workspace to ensure dependencies are the same. This is mostly a problem for dependencies like Cargo where we don't want the Folding clippy into the rustc workspace is difficult, however, because clippy itself is already a workspace. I'm not sure how to best solve this... I believe the only downside of allowing clippy to have its own Cargo.lock is that it may pick up different versions of dependencies than the rest of the tools in the Rust repo. This means larger build times for us (as we're probably building different versions). If clippy were to pick up a dependency on larger tools (aka rls, rustfmt, cargo, etc), then we'd run the risk of version drift. I don't think there's plans for that though? |
Nope. I think clippy doesn't particularly need a lockfile, really. It's fine if y'all use your own. |
Oh the worry is less so about lock files but the fact that clippy is its own workspace (so it's not a member of the main rustc workspace) |
@oli-obk if we remove the clippy workspace will that break things? |
Nope. Let's do it. That was just an optimization for developing inside the clippy repo. But it's not really necessary. |
Can we also add clippy to |
not sure, I have never used |
Yeah, given that RLS is already shipping with optional clippy we should ship the CLI tool as well. |
I am swamped with miri regressions. I'd be very delighted if someone else would tackle that |
I think this issue is outdated, maybe we can just close it. @oli-obk @Manishearth |
Skip the discussion until #40921 (comment)
The plan has been updated accordingly
Broad plan
src/bootstrap/dist.rs
,src/bootstrap/lib.rs
andsrc/bootstrap/step.rs to bundle clippy as a rustup component with rustc if clippy builds
src/tools/build-manifest/src/main.rs
Open Questions
clippy.toml
right now-Zextra-plugins=clippy(clippy.toml)
?The text was updated successfully, but these errors were encountered: