Conditionally mark the test cfg as a well known cfg

[Urgau]

What does this PR try to resolve?

This PR conditionally mark the test cfg as a well known cfg depending on the target unit "test" field (ie lib.test = false, [[bin]] test = false and others).

This is related to rust-lang/rust#117778 and https://users.rust-lang.org/t/cargo-what-is-the-purpose-of-lib-test-false/102361.

When defining lib.test = false (and others), any use of cfg(test) will trigger the unexpected_cfgs lint.

[lib]
test = false  # will now warn on cfg(test) 

How should we test and review this PR?

Best reviewed commit by commit. Second commit removes the test cfg from the --check-cfg args.

Additional information

T-compiler MCP#785 and #14963 were of preparatory work.

r? @epage

[weihanglo]

If user has specified --lib, doesn't it mean that they requested to test the lib crate so cfg(test) is expected to be there?

[Urgau]

What the command line arguments are shouldn't matter, otherwise users would get a different behavior between a "normal" cargo test and one with arguments like --all-targets.

[weihanglo]

However, lib.test = false doesn't mean there is no test. It, as of today, is defined as "test" won't run by default. cfg(test) is still relevant regardless if a test is in the default test set.

Maybe I should put the question this way: Why is cfg(test) in a test=false crate unexpected?

[Urgau]

IMO having test = false meaning not tested by default compared to not tests at all is unexpected, at least before reading the documentation I would have assumed that it disabled all the testing.

@jplatte made a further argument in his comment:

Hm, maybe. I guess it depends on what the intention of lib.test = false is. I think the vast majority of projects that set it don't have unit tests at all, and if any were to be introduced, getting a warning would be valuable (whether it results in the test author removing the flag or moving the test code into unit tests). I wonder if there are any projects that use lib.test = false while having unit tests, intentionally.

A similar comment was also put in the URLO thread.

[weihanglo]

Thanks for linking the thread here. Yeah I've read that, and agree that lib.test = false largely implies no test in lib crate at all. That is a valid use case and deserves a lint or warning.

However, fields under Cargo targets are mostly about including in or excluding from the default crate set, and are largely affected by the command-line argument. If Cargo has started emitting unexpected warning for Cargo targets, it may also bite users that disabling tests for other reasons. For example, I've seen people setting test=false because some tests are not expected to run by default for daily development, but only on CI or other special environments.

While that kind of scenario I guess is far less than who don't want tests at all, it is still a valid use case aligning with what the current doc describes. Treating unexpected_cfgs as an indirect way of banning tests in a crate doesn't seem to be the best venue. It is more like a hack because there is no lint for really banning tests.

That being said, I am not surprised if Cargo targets settings don't meet people's expectation. It is always a source of confusions¹ 😞.

Footnotes

1. I can actually name a lot of issues here. Let's just have a little peek of them:
   * https://github.com/rust-lang/cargo/issues/8338
   * https://github.com/rust-lang/cargo/issues/10936
   * https://github.com/rust-lang/cargo/issues/10958
   * https://github.com/rust-lang/cargo/issues/13668
   * https://github.com/rust-lang/cargo/issues/13437
   * https://github.com/rust-lang/cargo/issues/13828 ↩

[Urgau]

The unexpected_cfgs lint do trigger on #[test] but only when passed with --test.

$ rustc +nightly --crate-type=lib --test --check-cfg='cfg()' src/lib.rs
warning: unexpected `cfg` condition name: `test`
 --> src/lib.rs:9:5
  |
9 |     #[test]
  |     ^^^^^^^
  |
  = help: expected names are: `clippy`, `debug_assertions`, `doc`, `doctest`, `fmt_debug`, `miri`, `over
flow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generaliz
e_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_e
nv`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `ub_checks`, `unix`, and `windows`
  = note: using a cfg inside a attribute macro will use the cfgs from the destination crate and not the ones from the defining crate
  = help: try referring to `test` crate for guidance on how handle this unexpected cfg
  = help: to expect this configuration use `--check-cfg=cfg(test)`
  = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more information about checking conditional configuration
  = note: `#[warn(unexpected_cfgs)]` on by default
  = note: this warning originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: 1 warning emitted

We can probably make the lint fire without the --test argument.

[epage]

Unless I missed something in catching up on this, I feel like we'd get more out of test = false being consistent, independent of the build-target type. I'm not too concerned about false positives from people who have #[test] but set test = false.

[weihanglo]

My main point is the meaning of test = false will change after this PR. I don't mind shipping the particular change. Just want to make sure people aware of and feel good with it. And probably need doc update as well.

[Urgau]

I have adjusted the documentation with the new behavior.

[kpreid]

There are multiple ways which a target may be tested even when it has test = false, notably, cargo test --all-targets and cargo test some_test_name_filter. Therefore, I believe this change is problematic (and have filed #15131 for tracking of that).

[epage]

Talked it over with @weihanglo. While this is an observable change in behavior it seems relatively innocuous and could likely be revisited if needed.