We should reject `#[test]` and `cfg(test)` in core

[asquared31415]

#[test] functions should not go in library/core, because they are not actually run. The crate library/coretests exists for this purpose instead. cfg(test) is similarly suspicious, because core doesn't end up being built under that cfg when running ./x.py test library.

We should have a lint against both of these constructs. It may also be desirable to lint in alloc and std, but there's significant use of the cfg in those crates, so presumably they're doing something.

This was discovered in practice due to the use of #[cfg(test)] in library/core/cell/covariant_unsafe_cell.rs causing CovariantUnsafeCell's variance to not actually be tested.

[asquared31415]

@rustbot label +T-bootstrap

[theemathas]

Github search for instances of this: https://github.com/search?q=repo%3Arust-lang%2Frust+path%3Alibrary%2Fcore%2F+%2Fcfg.*test%2F&type=code

[theemathas]

#160687 (comment) added the following comment
cc @bushrat011899

#[cfg(not(test))] // FIXME: splat interacts poorly with the double linking of `core` in tests

[zachs18]

Hmm, under bare rustc, rustc --check-cfg 'cfg()' warns on cfg(test), so cargo is passing --check-cfg 'cfg(test)'.

Ah, cargo had logic to mark test as an (un)expected cfg based on [lib] test = false in Cargo.toml (PR), but it wasn't clear exactly what test = false should mean, so the logic was reverted (PR), so currently test is always treated as expected. See this cargo issue for discussion

[bushrat011899]

#160687 (comment) added the following comment cc @bushrat011899

#[cfg(not(test))] // FIXME: splat interacts poorly with the double linking of core in tests

Yeah I think this should be looked at in much more detail before rejecting cfg(test)/cfg(not(test)) in core. While it's specific to splat, clearly cfg(not(test)) does do something, even if it's not well understood or intended.

[asquared31415]

I think that as long as the mechanism had a "i know what im doing" button, we could apply the restriction generally. The target audience for this is rustc internals, it can be a little rough. We just want to stop the people that assume "the test infrastructure works normally in core", and having to say the "i know what im doing" incantation would be enough friction IMO.

[asquared31415]

The rustc dev guide says

This can sometimes cause compilation errors while running tests. These errors can be avoided by using the cfg_attr attribute with the not(test) condition (it’s fine adding then for all rustc_diagnostic_item attributes as a preventive manner).

During the initial implementation of diagnostic items, this pattern was also used, so this is definitely a known weirdness. Maybe we could have an exception for diagnostic items specifically? There's so few in core though, that I would be fine with case-by-case manual exceptions.

Other than the diagnostics items though, there's a little bit of f16 and f128 code, and then the splat weirdness. All of these seem strange enough to me that it would be helpful to have a "i know what im doing, and here's why this is needed" (or i don't know why this is broken but it fixes it, put in a FIXME)

[asquared31415]

Also this is all under the assumption that core cannot be built under the normal testing machinery for some reason. If someone figures out "oh we can just tweak a flag and it all works perfectly" then that would be great, and we wouldn't need to reject use of cfg(test).

[bjorn3]

Also this is all under the assumption that core cannot be built under the normal testing machinery for some reason.

With a bunch of cfg to remove all lang items it technically can be, but we still shouldn't put tests in the same package as any standard library crate. The problem with that setup is that the cargo build for building the standard library and cargo test for testing need to use the exact same flags to prevent standard library crates getting duplicated between regular deps and libtest deps. That is why I also moved all liballoc tests into a separate crate and want to move all other tests out into separate packages.