Repository navigation
Improve the rustc-side clippy development experience #76495
Description
Activity
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.
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.
What was the error? That should work, if not it's a bug.
./x.py test --bless src/tools/clippyworks../x.py test --blessdoes not run the Clippy tests, so it also doesn't update them.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.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.)Run Clippy tests on the PR builder (if we have the CI budget)
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.Allow running Clippy tests in-tree from stage1
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.
Document the interaction between -D warnings and Clippy lints.
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.Ah, turns out what I actually tried is
./x.py test src/tools/clippy --bless --stage 0Yes, Clippy on
--stage 0doesn't work (yet). I don't know how to fix that.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.
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.
Allow running Clippy tests in-tree from stage1
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.
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.Clippy's fork of compile test needs to be fixed not to reuse the same target directory for tests as for building clippy itself.
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.
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.
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.
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.
Reacted by Oli Scherer and Philipp Krones- 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).
this is likely fixed since #87337, although i haven't tested
Reacted by Fridtjof Stoldt- Document the interaction between
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:
./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../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'.-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).