Remove #[cfg(not(test))] gates in core

[thaliaarchi]

These gates are unnecessary now that unit tests for core are in a separate package, coretests, instead of in the same files as the source code. They previously prevented the two core versions from conflicting with each other.

[rustbot]

r? @thomcc

rustbot has assigned @thomcc.
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

[thaliaarchi]

I only found this comment explaining it after opening this. It seems like doc tests and macros require their own gates, and this commented gate is for everything else.

rust/library/core/src/lib.rs

Lines 46 to 56 in 30f168e

	// Since core defines many fundamental lang items, all tests live in a
	// separate crate, coretests (library/coretests), to avoid bizarre issues.
	//
	// Here we explicitly #[cfg]-out this whole crate when testing. If we don't do
	// this, both the generated test artifact and the linked libtest (which
	// transitively includes core) will both define the same set of lang items,
	// and this will cause the E0152 "found duplicate lang item" error. See
	// discussion in #50466 for details.
	//
	// This cfg won't affect doc tests.
	#![cfg(not(test))]

I still want to see whether CI passes, as this PR essentially tests the doc test case.

[thaliaarchi]

Interesting. CI passed with #[cfg(not(test))] gates removed on impl blocks in core::slice. I've now pushed a change to try removing those gates throughout all of core.

[thaliaarchi]

It seems these gates were added in #50466 (rustbuild: Allow quick testing of libstd and libcore at stage0, 2018-05-05), including the comment which I quoted above.

[tgross35]

Cc @bjorn3 who knows a lot about the test infra

[bjorn3]

These were necessary way back when libcore unit tests were in the same files as the libcore source code to prevent the two libcore versions from conflicting with each other. They are no longer necessary anymore as the libcore unit tests are now in a separate coretests package (and even before that got moved into a separate integration test crate years ago).

[thaliaarchi]

So it sounds like we can get rid of them! I've identified all the other occurrences and removed them now. Since the PR description will be in the merge, I've updated it with some context.

Are these mentioned issues relevant?

• Is there a gentler way to land the assert_matches macro? #82913
• Re-land early syntax feature gating (was: Some features can no longer be controlled by conditional compilation) #65860

rust/library/core/src/lib.rs

Lines 228 to 240 in 30f168e

	#[cfg(not(test))] // See #65860
	#[macro_use]
	mod macros;
	// We don't export this through #[macro_export] for now, to avoid breakage.
	// See https://github.com/rust-lang/rust/issues/82913
	#[cfg(not(test))]
	#[unstable(feature = "assert_matches", issue = "82775")]
	/// Unstable module containing the unstable `assert_matches` macro.
	pub mod assert_matches {
	#[unstable(feature = "assert_matches", issue = "82775")]
	pub use crate::macros::{assert_matches, debug_assert_matches};
	}

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[thaliaarchi]

A Cranelift patch needed to be regenerated

[tgross35]

There are some compiler tie-ins also gated behind not(test), e.g. #[cfg_attr(not(test), lang = "String")] and #[cfg_attr(not(test), rustc_diagnostic_item = "Vec")]. Are those removable for the same reason?

[thaliaarchi]

Interestingly, some of those are in std like #[cfg_attr(not(test), rustc_diagnostic_item = "HashMap")]. The rustc_diagnostic_items in core and std are inconsistent whether they are gated and occur both ways.

[bjorn3]

The ones in libstd do still need to be gated as libstd has a fair amount of unit tests that aren't split into a separate package like is the case for libcore. I'm trying to move unit tests to integration tests for libstd too.

[thomcc]

Very nice. @bors r+

[bors]

📌 Commit 638b226 has been approved by thomcc

It is now in the queue for this repository.

[thaliaarchi]

I've now checked alloc to see whether it could benefit from this same cleanup. Looks like bjorn3 already handled it in #136642. The only modules with #[cfg(not(test))] gates and related are those which are #[path = …]-included and still require them.