libtest: Index tests by a unique TestId

[andersk]

This more robustly avoids problems with duplicate TestDesc. See #81852 and #82274.

Cc @Mark-Simulacrum.

[rust-highfive]

r? @KodrAus

(rust-highfive has picked a reviewer for you, use r? to override)

[KodrAus]

Thanks for the PR @andersk!

I just had one comment about an alternative approach that could reduce a bit of churn through the codebase here, but otherwise this seems like a good fix to me.

[KodrAus]

I guess an alternative here could be to add an id field to TestDesc. If all we need is to guarantee that different tests with the same description don't equate that should even be enough to leave the derived equality and hashing.

[andersk]

If we added id to TestDesc, the responsibility for generating it and ensuring its uniqueness would fall to the compiler (I’m not sure if this macro even can generate a unique ID):

rust/compiler/rustc_builtin_macros/src/test.rs

Lines 227 to 297 in 3e826bb

	cx.expr_struct(
	sp,
	test_path("TestDesc"),
	vec![
	// name: "path::to::test"
	field(
	"name",
	cx.expr_call(
	sp,
	cx.expr_path(test_path("StaticTestName")),
	vec![cx.expr_str(
	sp,
	Symbol::intern(&item_path(
	// skip the name of the root module
	&cx.current_expansion.module.mod_path[1..],
	&item.ident,
	)),
	)],
	),
	),
	// ignore: true | false
	field(
	"ignore",
	cx.expr_bool(sp, should_ignore(&cx.sess, &item)),
	),
	// allow_fail: true | false
	field(
	"allow_fail",
	cx.expr_bool(sp, should_fail(&cx.sess, &item)),
	),
	// should_panic: ...
	field(
	"should_panic",
	match should_panic(cx, &item) {
	// test::ShouldPanic::No
	ShouldPanic::No => {
	cx.expr_path(should_panic_path("No"))
	}
	// test::ShouldPanic::Yes
	ShouldPanic::Yes(None) => {
	cx.expr_path(should_panic_path("Yes"))
	}
	// test::ShouldPanic::YesWithMessage("...")
	ShouldPanic::Yes(Some(sym)) => cx.expr_call(
	sp,
	cx.expr_path(should_panic_path("YesWithMessage")),
	vec![cx.expr_str(sp, sym)],
	),
	},
	),
	// test_type: ...
	field(
	"test_type",
	match test_type(cx) {
	// test::TestType::UnitTest
	TestType::UnitTest => {
	cx.expr_path(test_type_path("UnitTest"))
	}
	// test::TestType::IntegrationTest
	TestType::IntegrationTest => {
	cx.expr_path(test_type_path("IntegrationTest"))
	}
	// test::TestPath::Unknown
	TestType::Unknown => {
	cx.expr_path(test_type_path("Unknown"))
	}
	},
	),
	// },
	],
	),

as well as librustdoc:

rust/src/librustdoc/doctest.rs

Lines 831 to 842 in 3e826bb

	desc: testing::TestDesc {
	name: testing::DynTestName(name),
	ignore: match config.ignore {
	Ignore::All => true,
	Ignore::None => false,
	Ignore::Some(ref ignores) => ignores.iter().any(|s| target_str.contains(s)),
	},
	// compiler failures are test failures
	should_panic: testing::ShouldPanic::No,
	allow_fail: config.allow_fail,
	test_type: testing::TestType::DocTest,
	},

and all the tests for libtest itself. Even if this is somehow possible, it wouldn’t reduce churn.

Another alternative would be to try using the address of the TestDesc as a unique identifier—but then instead of moving or cloning TestDesc we’d have to pass it by reference, which also at best wouldn’t reduce churn.

[bors]

☔ The latest upstream changes (presumably #83454) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC-zz]

r? @Amanieu

[Amanieu]

@bors r+

[bors]

📌 Commit 3c42d9f has been approved by Amanieu

[bors]

⌛ Testing commit 3c42d9f with merge 5e73bd1...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 5e73bd1 to master...