Repository navigation
Improve the rustc-side clippy development experience #76495
Description
When working on #75573, I ended up needing to modify Clippy (both a lint implementation and some UI tests). This process left a lot to be desired, and could be a significant roadblock to new contributors. I came up with some ideas for improving the process:
- Run Clippy tests on the PR builder (if we have the CI budget). My PR was initially approved, then unapproved due to a (correct) suspicion that Clippy UI tests would be broken. However, this could easily be missed on other PRs, leading to wasted merge queue time and failed rollups. By running these tests on the PR builder, we could catch Clippy changes prior to PR approval.
- Allow running Clippy tests in-tree from stage1. Currently, running
./x.py test src/tools/clippyrequires a full compiler bootstrap (e.g. building a stage2 compiler). This takes a significant amount of time, and interacts badly with incremental compilation. For contributors with lower-end machines, this may make the contribution process incredibly frustrating. Attempting to build Clippy against a stage1 compiler (which requires running./x.py test src/tools/clippy --stage 0for some reason) works, but the tests fail with a confusing error about LLVM being missing. - Integrate Clippy UI tests with
./x.py test --bless. Currently, blessing Clippy UI tests requires manually running scripts from insidesrc/tools/clippy. It would be nice if./x.py test --blessworked on all UI tests, allowing Clippy to be largely treated as 'just another directory'. - Document the interaction between
-D warningsand Clippy lints. It seems as though denying a built-in lint with-D warningsprevents Clippy lints from being run. Since Clippy UI tests are run with-D warnings, adding a new builtin Rust lint can end up preventing Clippy UI tests from testing Clippy lints. Ideally, we would allow both sets of lints to run - if this is not possible, we should document the workaround (adding#![allow(problematic_rustc_lint)]to the affected Clippy UI tests).
Activity
jyn514 commentedon Jan 1, 2021
Note that at some point
test --stage 0 src/tools/clippybroke: #78717RalfJung commentedon Jun 2, 2021
Last time I tried blessing clippy tests, that did not work, so I ended up manually adjusting stderr files. Maybe that's because I didn't try stage 2. Note that
./x.py test --bless(which is listed as 'working' in the list above) nowadays runs stage 1, so the OP text likely needs adjustments.@rust-lang/clippy this issue has remained open for more than half a year. The clippy-subtree-merger put the burden of keeping clippy working onto all rustc contributors, so having sub-par tooling like this is now a much bigger problem than it was back when only clippy devs had to use that tooling.
jyn514 commentedon Jun 2, 2021
What was the error? That should work, if not it's a bug.
flip1995 commentedon Jun 2, 2021
./x.py test --bless src/tools/clippyworks../x.py test --blessdoes not run the Clippy tests, so it also doesn't update them.RalfJung commentedon Jun 2, 2021
I think
./x.py test --bless src/tools/clippyis what I tried and got tons of strange linking errors. I figured this was expected so I didn't report a bug, sorry.RalfJung commentedon Jun 2, 2021
Ah, turns out what I actually tried is
./x.py test src/tools/clippy --bless --stage 0, since I did not want to wait for two full rustc builds. (Stage 1 clippy uses the stage 1 rustc libs, so those have to be built first.)flip1995 commentedon Jun 2, 2021
This is already done, if something in the
src/tools/clippyis touched by the PR. IIRC the decision was made that running it on every PR is not necessary, since most PRs won't cause problems with it.That would be great. Unfortunately most Clippy folks don't really know how the bootstrapping works exactly and therefore fixing this would be really hard for us to do.
Changes to rustc should not change Clippy tests, except for line number updates. The only exception I can think of is if a lint is being uplifted from Clippy to rustc. Fixing the issue where a new rust error/lint suppresses Clippy lints should be fixed by either adding
#[allow(problematic_lint)]or by fixing the issue in the test, if it doesn't change the semantics of the test.Yes, Clippy on
--stage 0doesn't work (yet). I don't know how to fix that.jyn514 commentedon Jun 2, 2021
The error that @RalfJung linked is not a bootstrapping issue. Clippy's fork of compile test needs to be fixed not to reuse the same target directory for tests as for building clippy itself.
RalfJung commentedon Jun 2, 2021
Actually that works, since stage 1 is what
x.py testuses by default -- right?Stage 0 doesn't work. That might be #78778, or it might be a separate problem.
RalfJung commentedon Jun 2, 2021
Clippy uses the same fork that Miri uses, and Miri doesn't have this particular problem I think (at least, Miri worked on stage 0 before #78778 got introduced). However, it looks like clippy actually supports ui tests that depend on 3rd-party crates, so that logic might be wrong -- Miri doesn't have anything like this.
flip1995 commentedon Jun 2, 2021
Yes using external crates in compiletest causes some problems also in Clippy. So this is most likely the reason, why it also causes problems in the Rust repo.
camsteffen commentedon Jun 10, 2021
I just opened rust-lang/rust-clippy#7343 to explain the problem from Clippy's side. I'd appreciate feedback from people with more rustc knowledge.
Rollup merge of rust-lang#96757 - jyn514:fewer-clippy-rebuilds, r=Mar…
jyn514 commentedon May 20, 2023
I have a prototype for running clippy tests without having to build rustc twice in #96798, which is currently blocked on changing the stage numbering for tools: https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Stage.20numbering.20for.20tools
See #96798 (comment) for more details.
jyn514 commentedon Nov 16, 2023
this is likely fixed since #87337, although i haven't tested
Auto merge of rust-lang#117595 - jyn514:x-clippy, r=albertlarsan68
Auto merge of rust-lang#117595 - jyn514:x-clippy, r=albertlarsan68