Integrate rustfix into clippy test suite

[killercup]

Let's test the suggestions clippy generates! (I talked about this with various people from time to time, but never opened an issue here for discussion.)

Recently, I refactored rustfix' test suite to basically not test the rustfix CLI but the suggestions and their fixes. You can find the test code here. It's pretty self-contained and only uses rustfix to get the suggestion from the compiler (but contains code to apply them). Indeed, the rustfix code base is very small and basically only a shim around parsing the compiler's json output and extracting suggestions. So in theory, I could easily make this a single test file (i.e., without dependencies on the rustfix repo) that you add as a job to clippy's CI.

The same test procedure would work very well with clippy's UI tests: If there is a foo.fixed.rs next to foo.rs, the test will apply rustfix to foo.rs and assert that it generates foo.fixed.rs (and that foo.fixed.rs compiles!). This would be a great way to ensure that clippy generates sensible suggestions, document edge cases, and also be a good starting point to specify which suggestions are auto-applicable (cf. https://github.com/killercup/rustfix/pull/44#issuecomment-357920563, rust-lang/rust#39254).

[killercup]

I originally thought about integrating this into compiletest (which already compiles the files and reads the compiler's JSON output). This is a pretty unique case, though: It's only useful to test the applied suggestions when you write lints.

I'll most likely have time to work on this on Saturday. Ping me and we can work on this together :)

cc @oli-obk @Manishearth

[oli-obk]

The only issue I see with the way everything is implemented atm is that you cannot use crate dependencies other than libstd. We do have some of these in our ui tests.

That said, compiletest is a total mess imo. It keeps breaking if you don't cargo clean every now and so often and is generally hard to configure. A cargo-based solution would be better here.

I once tried simply removing ui tests and using a crate that has a file in the examples directory for each test. That worked pretty well actually. We could do the same.

For now we can just keep the ui tests for everything that needs non-std dependencies and move everything else to rustfix tests

[oli-obk]

Oh we also need to publish rustfix in order to be able to have clippy depend on it. Out of some reason even dev-dependencies must be published.

[killercup]

Oh we also need to publish rustfix

If we want to depend on it directly, sure. Moving all the relevant rustfix code to the clippy repo is also a valid option IMHO.

Which features of compiletest is clippy using? Just the UI tests by now, or also the // ~ ERROR style matchers? It shouldn't be too hard to replicate the UI tests.

Which dependencies are clippy's test using?

FTR, the everything test currently executes clippy-driver for each file in the test/fixtures/ directory and passes a tempdir to its --out-dir flag. We could instead copy a default cargo config in there (with a bunch of dependencies) and drop in the test file as lib.rs. That would result in recompiling the dependencies over and over again, though.

The examples/ way sounds good. If we add a clippy_tests crate, we could even make everything its main binary which then calls cargo clippy --example foo --message-format=json in its own repo. Because that would be fun!

[oli-obk]

Moving all the relevant rustfix code to the clippy repo is also a valid option IMHO.

Rustc will want that at some point, too

Just the UI tests by now, or also the // ~ ERROR style matchers?

just the ui tests

Which dependencies are clippy's test using?

serde, regex, rustc, alloc, core,

Because that would be fun!

👍 best reason

[Manishearth]

Actually, I'd prefer to run this regardless of the existence of a fixtures file.

Or, perhaps, as a first pass, we can just focus on having it compile. Have a whitelist of tests we wish to rustfix, and slowly grow that till the suite is covered. Ensure they all compile without errors post-rustfix.

Then, change the test so that much like update-all-references the generated rs files get cached in the build dir, and we have tooling to update files under fixtures/ .

[killercup]

just the ui tests

Cool, we could easily make two runs based on the same structure (in parallel on CI even), one testing the human readable output, and one for rustfix.

Actually, I'd prefer to run this regardless of the existence of a fixtures file.

@Manishearth, the current everything.rs records the fixed source file if the env var RUSTFIX_TEST_RECORD_FIXED_RUST exists. We can change this to always apply all fixes, and always compile, but only turn compile failures into hard errors if there is a .fixed.rs file.

Ideally, we could also have a tidy script that makes sure that for lints that have a machine_applicable usage of span_lint_and_suggestion, there is a .fixed.rs. Since every case that triggers a lint can decide if it's machine_applicable or not, this doesn't need to be exhaustive and there can/should be multiple test files per lint.

[oli-obk]

one testing the human readable output, and one for rustfix.

the json output contains the human readable output. Just concatenate all of them, no need to build twice ;)

[killercup]

the json output contains the human readable output. Just concatenate all of them, no need to build twice ;)