Create automatic clippy tidiness tests

[eerii]

Now that most clippy warnings are dealt with, it would be interesting to discuss if an automatic clippy lint would make sense for the project.

Upsides:

• Catch bugs
• Produce cleaner code
• Avoid big cleanups in the future

Downsides:

• Increase the tidy check run time
• False positives
• Can be annoying at times

Proposal

One way it could work is having it as an extra check of test-tidy, with a flag such as --clippy false to skip the clippy tests. The clippy warnings could be treated as information, printing them so you can fix (or ignore) them, but so that they don't fail the CI. clippy errors probably should still be errors and make the test fail, because they could indicate a more serious issue. The main problem I see with this approach is that test-tidy could take longer, specially on a first build, since it needs to compile the code to run clippy.

Other options

Some other ideas I had but I'm not convinced with were:

• Running clippy just before the pr enters the merge queue, as a final check. This can slow down merging prs quite a bit.
• Running clippy on the pull request CI, sending a bot comment with the suggestions. This feels very spammy.

---

This feels like a difficult design decision with quite some pros and cons, and there may be much better ways of handling this, that's why I opened this issue to discuss how should we proceed.

[mrobinson]

I think this is a great idea. I agree that running clippy when running tidy would be too slow, but how about just adding a new GitHub CI "lint" job that runs tidy and also runs clippy? It would be nice to add more features to the lint job as well, such as producing inline GitHub annotations on the PR. This just requires special processors for console output, but should be pretty straight-forward. Here's an example of an action that does this with clippy output: https://github.com/actions-rs/clippy-check. We'd probably need a special processor for ./mach tidy as well as it produces its own output as well as output from cargo fmt.

[eerii]

I had no idea that annotations on the PR existed, they look perfect for this! I can try to work something out with them, it would be so nice to see the lint suggestions directly on the pr.

[sagudev]

One thing to note is that clippy is a rustc driver. When you run cargo clippy what actually happens is that cargo check is run with additional lints provided by clippy-driver.

Ideally we could just run clippy lints as part of build/compilation and I this could somehow be achieved with crown also reexporting clippy lints (or just making clippy to be also run with crown). But this is just idea and I am not sure if it would even work.

EDIT: That would not be possible as clippy requires disabled mir optimizations: https://github.com/rust-lang/rust-clippy/blob/2795a6018944a5918b7d276267165484f5d62d6a/src/driver.rs#L163

[sagudev]

Also given that there was quite some PRs that introduced warnings and then fixup PRs that fixed them, I think we should make them fail CI (at least for rustc warnings).

[eerii]

Is there an easy way of testing Github Actions? I'm thinking of making a hard copy (not just a fork) of the repository and make test prs there. However, having to change the action, commit, push, wait for it to set up everything and run can be a bit cumbersome and time intensive in my experience, so I was wondering if there was a better way to at least catch some issues locally.

[sagudev]

I have GitHub Actions extension in VSCode, but that is just for syntax. I am not aware of any faster way.

[mrobinson]

@eerii You might have some success with https://github.com/nektos/act. I usually test things by running things in my personal repository and using the extension that @sagudev mentioned.

[eerii]

Thank you!! I'll give those a try :D