[Roadmap] Turn cargo dev into Clippys ./x.py

[flip1995]

There's already cargo dev which makes Clippy development easier and more
pleasant. This can still be expanded, so that it covers more areas of the
development process.

Steps to completion:

• bless execute UI-tests and update reference files
• init include setup-toolchain.sh in clippy_dev (outdated)
• pr (?) Run everything (fmt, update_lints, ...?), so that the changes probably pass CI
• deprecate to deprecate a Clippy lint. This is especially useful if people want to uplift Clippy lints.
• Better integrate cargo dev in rust-lang/rust so that commands are also usable there
• Add git commit hooks running cargo dev commands before committing (Added cargo dev setup git-hook and updated cargo dev setup intellij including a remove command #7361)
• cargo dev rename_lint(See issue cargo dev rename_lint #7799)
• cargo dev dogfood --fix
• ... ideas?

These are some ideas, that I had, maybe we can even further expand the clippy_dev tool.

[phansch]

I'm going to work on adding the bless subcommand to clippy_dev this week.

[xFrednet]

@rustbot claim

[phansch]

@xFrednet Assuming, you want to work on the cargo dev bless integration into
x.py, here's a short summary of the current state of blessing in rustc and
Clippy:

Rustc's bless is implemented in the compiletest library living inside the rustc tree.
Clippy is using a custom fork of that library: compiletest-rs.

The custom fork that Clippy is using, currently doesn't have a bless
feature. Instead, Clippy's bless is a custom implementation in clippy_dev.

Two months later, I think that was a bad choice on my end. It would have been
much better to add bless support to compiletest-rs. This way we would have
more similarities between rustc tools and the tools Clippy is using. It would
have also allowed miri to get bless support for their testsuite, I think.
Finally, I believe it would also be easier to integrate it with x.py this way.

Considering all that, I think we would be better off throwing away our custom bless
implementation and adding bless support to compiletest-rs instead.

My proposal would be:

1. I'm opening a PR to compiletest-rs with the addition of the bless
   feature. That is mostly copy/pasting of code.
2. The interesting part then,
   would be to change cargo dev bless to use the blessing mechanism of
   compiletest-rs and then integrating cargo dev bless into x.py

cc @flip1995 for the general plan
and cc @camsteffen because we would probably lose the feature added in #6547

[xFrednet]

I've worked with compiletest-rs before in a different project and having a general bless feature as a part of it would definitely be beneficial to not just Clippy but for the entire Rust ecosystem. Therefore, I'm in favor of your proposal.

The first step sounds pretty straight forward. I'm happy to help if you need any support.

The second part is definitely the interesting part. This step will probably also depend on the bless interface implementation in compiletest-rs. I'll try to get more into compiletest-rs and the current bless implementation until that point.

Is there still some area that needs refinement when it comes to your proposal? It seems like you've already started on the implementation.

• Issue: Add basic bless support Manishearth/compiletest-rs#231
• PR: Adding bless support Manishearth/compiletest-rs#228

---

Thank you very much for the summary @phansch. It seemed a bit frightening at the beginning, but it's very clear, and gave me good overview! :)

[phansch]

Is there still some area that needs refinement when it comes to your proposal? It seems like you've already started on the implementation.

The only thing I can think of is a feature like #6547. I don't think that currently exists in any of the compiletest variants. This would first have to be added to Rustc's compiletest. Once that's merged, it could be copied over to compiletest-rs. I think that could be done somewhat independently of the other work as well.

Additionally, while we are waiting for Manishearth/compiletest-rs#232 and Manishearth/compiletest-rs#231 to be merged, I think it would be possible to work on a Clippy PR already that uses the new bless:

1. Check out Add basic bless support Manishearth/compiletest-rs#231 locally
2. Point Clippy's compiletest-rs dependency to your local checkout
3. Start reworking cargo dev bless to use the compiletest blessing mechanism

[xFrednet]

Hey @phansch congratulations on having the bless PR merged, that is really awesome!

You mentioned two tasks in your previous comment

1. work on a Clippy PR that uses the new bless

2. feature like Curse outdated test output #6547

Have you started to work on one of these? I'm not sure if we want to wait on the switch to the new bless for a feature like #6547. I would look into implementing #6547 into the Rustc's compiletest if you haven't started already and if you think that it's worth to wait until it has also reached into laumann/compiletest-rs. What is your opinion on it? 🙃

[phansch]

@xFrednet I haven't started any new work here, yet. To me it seems better to start with adding #6547 to Rustc's compiletest first.

Do you already have an idea how to implement it? I just know that the code would have to go somewhere here but I don't know on which condition it should skip writing the output file.
For Clippy the condition was easy: If the output file was changed before the last Clippy build, then don't update it again.
But Rustc's compiletest is used for rustdoc, rustc and a few other things, so we would have to check the timestamps of those binaries as well. I guess compiletest has to store the path to the executing binary somewhere, so that's something to check for sure.

[xFrednet]

Hmm, this definitely makes it more complicated. I expected that it is only used by one binary. My initial idea after your comment was to keep track of which test has been run in the last execution and only update those files. However, that idea seems pretty limiting on second thought.

Checking the timestamps on the binaries seems like a simple solution. But I'm not sure how generalizable it would be. We could think of adding a tag to the file that states the used binary and the binary compilation timestamps. The bless implementation could then specifically check if the test used the latest build. (This tag could be added in the first line of the test output or in a separate file. This tag would obviously be ignored by the comparison function).

Another idea I had was to add some kind of cli interface as it's likely that bless will be invoked by the user. It could look something like this:

> /update-references.sh --bless
Which tests do you want to update?
| No | Test                                  | Timestamp        |
|  0 | Abord                                 |                  |
|  1 | test/ui/atomic_ordering_int           | 2021-02-13 18:04 |
|  2 | test/ui/bind_instead_of_map_multipart | 2021-02-13 18:02 |
|  3 | test/ui/needless_doc_main             | 2021-02-13 18:01 |
|  4 | test/ui/neg_multiply                  | 2021-02-12 13:17 |
|  5 | test/ui/never_loop                    | 2021-02-12 13:01 |

This could also include some kind of auto feature which updates all files since the last build. However, I'm not sure how viable that option is. What are your thoughts on this? @phansch

[phansch]

Looking a bit more into this, I think there's still an easy way: It looks like compiletest has a rustc_path option that we can use to get the path to the binary here. To make it easier for us, we can also limit this feature to the 'UI' testsuite for a start:
Here you can check that using self.config.mode == Mode::Ui and then do the timestamp comparison.

The CLI interface looks nice, but I think it's something that would require an MCP first because it would potentially change the workflow for many Rust contributors.

[xFrednet]

Rusts x.py file also has the ability to install a commit hook for git. I think that would be a nice addition at least for new members. It could roughly run the following lines:

cargo dev update_lints
cargo dev fmt

And optionally also cargo test if somebody wants that :)