Add disabled-in-tests configuration to disable arbitrary lints in tests

[smoelius]

Adds a catch-all configuration to disable lints in tests, so that one-offs like #8802 are not needed.

To illustrate, the following two clippy.toml files have essentially the same effect:

allow-unwrap-in-tests = true # existing configuration

disabled-in-tests = ["unwrap_used"] # configuration added by this PR

The approach is to store a disabled lints set in clippy_utils::diagnostics. When a lint is in the set and --test is passed to rustc, diagnostics are not emitted for the lint.

Two other approaches I tried were:

• Don't register the lint's pass. This works but disables the other lints in the pass as well. For example, unwrap_used is in the methods::Methods pass. So not registering that pass causes expect_used, should_implement_trait, etc. to be disabled as well.
• Don't register the lint. E.g., filter the lint from declared_lints::LINTS. This works but causes rustc to to produce "unknown lint" warnings on attributes like #[allow(unwrap_used)].

The present PR's approach is also much simpler than the two just described. I could share code for the other approaches if desired, though.

I expect this PR to be subject to the feature freeze.

changelog: Add disabled-in-tests configuration to disable arbitrary lints in tests

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[y21]

To illustrate, the following two clippy.toml files have essentially the same effect:

They work quite differently as currently implemented though. allow-unwrap-in-tests checks if it's contained within an item marked with #[cfg(test)] and only then avoids emitting a warning, as opposed to this which disables the lint in the entire crate (even outside of test code) when --test is passed.

To me this seems like the wrong thing to check for, and this type of check has also lead to some confusion in the past when combining #[expect] and --all-targets, see for example #13358 and rust-lang/rust#130021.

For example, if I have a crate with

fn foo(opt: Option<i32>) {
  #[expect(clippy::unwrap_used)]
  opt.unwrap();
}

and set disabled-in-tests = ["unwrap_used"] and run cargo clippy --all-targets, then this will display an "unfulfilled lint expectation" warning, even though this isn't test code. Technically, it's not wrong I guess, but I'd expect the use case is to specifically only disable lints in test code but still emit it anywhere else (i.e. what allow-unwrap-in-tests does today), not if --test is passed.

[smoelius]

@y21 Thanks for your prompt response.

Can you see anything else wrong with this approach besides breaking expect?

One way to fix the expect problem could be to have a mechanism in the rust compiler to mark a diagnostic as silenced. Then, the diagnostic could be recorded for the sake of expect, but not actually shown to the user.

But if this PR's approach breaks more than just expect, that idea could be too narrow.

[smoelius]

I pushed some commits and changed the PR to draft in the hope of getting your feedback.

Suppose I could get the changes in this post-expect-filter branch merged into rustc. Do you think then that the present PR might be considered for merger into Clippy?

To be more specific, should I pursue getting that branch merged into rustc?

Please note that I added a test that very closely resembles #15600 (comment): https://github.com/rust-lang/rust-clippy/blob/a9e61900d0f9b30e0c823e165c5f9bf4cdc4ad92/tests/ui-toml/disabled_in_tests_expect/expect.rs

[y21]

I don't think this needs the changes in rustc. If you want to manually fulfill an #[expect] without showing it (I assume that's what it's for, correct?), we already do that in a bunch of places. You can look at how clippy_utils::fulfill_or_allowed is implemented, it uses cx.fulfill_expectation(..).

---

It would technically fix the case I'd mentioned, but checking --test still seems fundamentally like the wrong thing to me IMO. There won't be a false "unfulfilled expectation" warning like before, but now we won't get any warnings for the snippet in #15600 (comment) when removing the #[expect] and run with --test, even though it's not test code.

I still think the correct thing would be to not check for a test session but where the expression being linted actually is located. What do you think? Maybe this is something worth discussing on Zulip on what the general expectation is on how exactly this type of configuration would be used. Perhaps people do think it's fine that "disabled-in-tests" really means "disable in test sessions" as is implemented, instead of "disabled in test code" (what e.g. allow-unwrap-in-tests does).

The current behavior of disabled-in-tests = ["unwrap_used"] is also identical to #![cfg_attr(test, allow(clippy::unwrap_used)] at the crate root, right?

View changes since this review

[y21]

For my own understanding, shouldn't this condition be inverted? The idea AFAIK is to essentially ignore disabled-in-tests and return an empty set when not in a test crate, that is, we don't want to register this expect hook when not in a test crate, right? Unfortunately I don't have an easy way to test this branch atm since it's based on a modified rustc

[smoelius]

Oof. You're right.

[smoelius]

You can look at how clippy_utils::fulfill_or_allowed is implemented, it uses cx.fulfill_expectation(..).

Thanks. I wasn't aware of LintContext::fulfill_expectation. But to use that, wouldn't one need to know the LintExpectationIds?

...but now we won't get any warnings for the snippet in #15600 (comment) when removing the #[expect] and run with --test, even though it's not test code.

That's correct. But I think that's fine, because one will get warnings for the non-test code when one builds without --test.

Just to be explicit:

• If one runs cargo clippy --tests, they'll see no warnings for non-test code.
• If one runs cargo clippy or cargo clippy --all-targets, they will see warnings for non-test code.

I still think the correct thing would be to not check for a test session but where the expression being linted actually is located. What do you think?

I disagree. The current approach of searching for #[cfg(test)] is a heuristic. Moreover, the notion of "test code" isn't well defined. Suppose a module is marked with #[cfg(any(test, feature = "foo"))] and the user compiles with --test and --features=foo. Is the module "test code" because --test was passed, or non-test code because --features=foo was passed?

Another reason to use --test is: there are currently seven allow-*-in-test configurations, and an open PR to add an eighth. I think that trajectory is unsustainable.

And to state the obvious, merging this PR wouldn't mean that allow-unwrap-in-tests would have to go away. People could continue to use it. But, hopefully, adding similar configurations for other lints would become unnecessary.

Maybe this is something worth discussing on Zulip on what the general expectation is on how exactly this type of configuration would be used. Perhaps people do think it's fine that "disabled-in-tests" really means "disable in test sessions" as is implemented, instead of "disabled in test code" (what e.g. allow-unwrap-in-tests does).

I will start a conversation on Zulip, but I would be fine with changing the name if you think disabled-in-tests is misleading. Maybe disabled-when-checking-tests?

The current behavior of disabled-in-tests = ["unwrap_used"] is also identical to #![cfg_attr(test, allow(clippy::unwrap_used)] at the crate root, right?

Ignoring things like #[warn(clippy::unwrap_used)] in submodules, I think that's right.

[smoelius]

Zulip thread (for my own reference): https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/.60disabled-in-tests.60.20configuration.20.28PR.2015600.29