Allow all unused lints inside ui tests

[xFrednet]

Clippy currently allows some unused lints inside UI tests, but not all of them during the compilation tests of the .fixed files. As a result, we often have attributes like #[allow(unused)]... in our tests. This PR allows all lints inside the unused group during Clippy's execution and the compilation test.

Other lints are already allowed during this compilation, like unused_imports. I sadly couldn't find where they are allowed and by I'm guessing that they are allowed by default in compiletest_rs. Therefore, I opted to pass the allow flag to the compiler inside compile-test.rs.

An example can be tested by removing #[allow(dead_code, unused_assignments)] from line 3 in tests/ui/assign_ops.rs on master (The compilation test fails) and then on this branch (cargo uitest is happy). Reproduction:

# Cleanup (Warning can delete files and changes)
git reset --hard
cargo clean

# Normal test
cargo uitest

# Remove allow arrtibute from file
cargo uitest # Fails due to line changes
cargo dev bless # Update `.stderr` and `.fixed` files

# Final test
cargo uitest
# Red on master due to the `unused-assignments` lint
# Green on my branch

I thought about removing all the now unused #[allow()] attributes, but that resulted in changes to a lot of files with little benefit. If this is accepted, it might be worth to do this in a followup PR or so. 🙃

---

changelog: none

r? @flip1995 I'm guessing that you have the most knowledge about this right now.

cc: @camsteffen since you've also worked a bit with compile test inside Clippy

Hope you're having a good day after reading this 🙃

[flip1995]

compiletest_rs already allows the unused group in all UI tests by default.

So what this PR does is to also allow those in the .fixed tests?

[xFrednet]

Yes 👍

[xFrednet]

Now that you're referencing the compiletest_rs code, I'm wondering if this should maybe be implemented there 🤔

[flip1995]

The question is, if we apply a suggestion and it leads to an unused warning, should the suggestion then be MaybeIncorrect? Or in other words: If we disable the unused group for rustfix tests by default, we won't catch when a suggestion results in unused leftovers.

Currently the allow(unused) mark places where this happens and kind of documents in the tests that this is intentional / expected when applying the suggestion.

Not sure if this may lead to problems in testing suggestions or if I'm just overthinking this. 🤔

[flip1995]

Now that you're referencing the compiletest_rs code, I'm wondering if this should maybe be implemented there

Yes, me too. Maybe as a config option for rust-fix tests.

[xFrednet]

If we disable the unused group for rustfix tests by default, we won't catch when a suggestion results in unused leftovers.

We have a bunch of files who allow unused lints inside .fixed file and so far it worked out well to my knowledge. I also believe that we would notice inside the lint message if it suggests adding a new value or something similar. Another option would be to only allow a few lints like dead-code, unused-variables, unused-assignments to still catch the other unused lints.

Now that you're referencing the compiletest_rs code, I'm wondering if this should maybe be implemented there

Yes, me too. Maybe as a config option for rust-fix tests.

That might be an option, I'll look into it 🙃

[flip1995]

Another option would be to only allow a few lints like dead-code, unused-variables, unused-assignments to still catch the other unused lints.

Yeah, I think this would be a better approach.

[xFrednet]

Do we maybe want to also limit the unused lints inside the normal ui tests? While testing, I noticed that we have some unused-braces, unused-imports and unused-parens diagnostics which are currently suppressed.

[flip1995]

Do we maybe want to also limit the unused lints inside the normal ui tests?

I don't think so. I really don't care about unused stuff in tests.

[xFrednet]

Okay, I'll open an issue regarding unused lints in compiletest and look at implementing is there 🙃

[camsteffen]

Don't we ultimately want to be consistent with rustc tests?

[flip1995]

Yeah, the compiletest-rs crate should not deviate too much from the Rust compiletest. But I'm really unsure what the current state of both crates currently is.

[xFrednet]

There is an issue for unifying compiletest-rs with the one from Rust. Manishearth/compiletest-rs#238 It only lists removing the dependency on test for that. Not sure if that's up-to-date though 🤔

[flip1995]

The maintainers of the Rust compiletest don't want to move it out of the compiler for maintainability reasons. But we of course want it to be unified, because we would get all the great things that are implemented/fixed/... there. So we shouldn't change that much in compiletest-rs.

One relevant question here: How many allows (an estimate is enough) are there for .fixed tests?

[xFrednet]

So we shouldn't change that much in compiletest-rs.

Understandable. I opened Manishearth/compiletest-rs#243 also asking if this should be implemented in the upstream version first

One relevant question here: How many allows (an estimate is enough) are there for .fixed tests?

About 81 based on a quick search