search

rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.45k stars 1.54k forks source link

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

Open flip1995 opened 4 years ago

flip1995 commented 4 years ago

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:

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

phansch commented 3 years ago

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

xFrednet commented 3 years ago

@rustbot claim

phansch commented 3 years ago

@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 https://github.com/rust-lang/rust-clippy/pull/6547

xFrednet commented 3 years ago

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.

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 commented 3 years ago

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 https://github.com/rust-lang/rust-clippy/pull/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 https://github.com/laumann/compiletest-rs/pull/232 and https://github.com/laumann/compiletest-rs/pull/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 https://github.com/laumann/compiletest-rs/pull/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 commented 3 years ago

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 #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? :upside_down_face:

phansch commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 :)