Unflatten use statements in HIR

[oli-obk]

View all comments

The HIR and AST representations of use statements are now mirrored, instead of having the HIR flatten the AST representation and duplicating early parts of nested paths:

use a::b::{c, d};

used to get flattened in HIR to

use a::b::c;
use a::b::d;
use a::b::{};

which duplicated the a::b part several times (and many more if you have more nesting).

This alone is fine, it made some parts of the compiler simpler and some parts harder, but it also meant we were generating more items (which are also owners) and had to duplicate resolver information across the flattened items. All that is gone with this PR.

This PR is the minimal version we can land, but there are obvious further avenues for cleanups and improvements. I did some refactorings in rustdoc, but I think the import logic can generally be improved by not handling imports together with other items and instead having them in separate tables.

Similarly clippy can probably benefit from a check_use_tree method on LateLintPass, but they often need to track separate information anyway, so it may be better to just always manually recurse.

should unblock #159760

see related discussion on #t-compiler > partial_res_map vs per-owner tables @ 💬

part of rust-lang/goals#620

[oli-obk]

@bors try @rust-timer queue

[rust-bors]

💔 Test for bb18a8a failed: CI. Failed job:

• try - dist-x86_64-linux-quick (web logs, enhanced plaintext logs)

[oli-obk]

The CI failure makes no sense, the only way that assert is reachable is by there being UB somewhere afaict:

rust/compiler/rustc_metadata/src/rmeta/decoder.rs

Lines 1447 to 1452 in b588ef3

	match def_key.disambiguated_data.data {
	DefPathData::Ctor => {
	// Structure and variant constructors don't have any attributes encoded for them,
	// but we assume that someone passing a constructor ID actually wants to look at
	// the attributes on the corresponding struct or variant.
	assert_eq!(def_key.disambiguated_data.data, DefPathData::Ctor);

Also I can't reproduce locally 😨

[WaffleLapkin]

The backtrace doesn't make sense either:

  12:     0x7f7d92df5b99 - core[8ec35bcd8078f253]::panicking::assert_failed::<rustc_hir[5841a66ff274cc3a]::definitions::DefPathData, rustc_hir[5841a66ff274cc3a]::definitions::DefPathData>
  13:     0x7f7d946407c1 - rustc_metadata[69b7fa7d7ff41b69]::rmeta::decoder::cstore_impl::provide_extern::attrs_for_def

The assert is in CrateMetadata::get_item_attrs, but the backtrace skips it, even though it isn't marked with #[track_caller]. 🤔

And attrs_for_def shouldn't be able to panic like that on its own:

rust/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

Line 332 in f7d782a

attrs_for_def => { tcx.arena.alloc_from_iter(cdata.get_item_attrs(tcx, def_id.index)) }

rust/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

Lines 138 to 163 in f7d782a

	($tcx:ident, $def_id:ident, $other:ident, $cdata:ident, $name:ident => $compute:block) => {
	fn $name<'tcx>(
	$tcx: TyCtxt<'tcx>,
	def_id_arg: rustc_middle::queries::$name::Key<'tcx>,
	) -> rustc_middle::queries::$name::ProvidedValue<'tcx> {
	let _prof_timer =
	$tcx.prof.generic_activity(concat!("metadata_decode_entry_", stringify!($name)));
	#[allow(unused_variables)]
	let ($def_id, $other) = def_id_arg.into_args();
	assert!(!$def_id.is_local());
	// External query providers call `crate_hash` in order to register a dependency
	// on the crate metadata. The exception is `crate_hash` itself, which obviously
	// doesn't need to do this (and can't, as it would cause a query cycle).
	use rustc_middle::dep_graph::DepKind;
	if DepKind::$name != DepKind::crate_hash && $tcx.dep_graph.is_fully_enabled() {
	$tcx.ensure_ok().crate_hash($def_id.krate);
	}
	let cstore = CStore::from_tcx($tcx);
	let $cdata = cstore.get_crate_data($def_id.krate);
	$compute
	}
	};

[oli-obk]

The assert is in CrateMetadata::get_item_attrs, but the backtrace skips it, even though it isn't marked with #[track_caller]. 🤔

probably got inlined. but the assert message says it's in a different line

compiler/rustc_metadata/src/rmeta/decoder.rs:1404

I freshly rebased, so I assumed it should be fine, very odd

[oli-obk]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 00e1020 (00e1020c22d36a6629c7d764b1967a735a100e74)
Base parent: 526c36b (526c36b4c477994b7905ec6a084d6150e49076f5)

[rust-timer]

Finished benchmarking commit (00e1020): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.9%]	45
Regressions ❌ (secondary)	0.7%	[0.0%, 1.8%]	47
Improvements ✅ (primary)	-1.3%	[-18.7%, -0.1%]	80
Improvements ✅ (secondary)	-0.5%	[-1.4%, -0.1%]	12
All ❌✅ (primary)	-0.7%	[-18.7%, 0.9%]	125

Max RSS (memory usage)

Results (primary 1.0%, secondary 3.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.7%	[0.4%, 4.1%]	20
Regressions ❌ (secondary)	4.8%	[2.1%, 11.1%]	18
Improvements ✅ (primary)	-2.0%	[-11.4%, -0.4%]	12
Improvements ✅ (secondary)	-5.2%	[-7.7%, -2.7%]	2
All ❌✅ (primary)	1.0%	[-11.4%, 4.1%]	32

Cycles

Results (primary -5.9%, secondary 3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.3%, 3.2%]	2
Regressions ❌ (secondary)	3.2%	[2.2%, 3.8%]	11
Improvements ✅ (primary)	-8.0%	[-16.0%, -2.4%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-5.9%	[-16.0%, 3.2%]	10

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 470.08s -> 468.116s (-0.42%)
Artifact size: 400.00 MiB -> 400.03 MiB (0.01%)

[rust-bors]

📌 Commit a7c90ba has been approved by petrochenkov

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: petrochenkov
Duration: 3h 44m 26s
Pushing 0d38a84 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing f7575a9 (parent) -> 0d38a84 (this PR)

Test differences

Show 76 test diffs

76 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 0d38a842626a6e3b70e2c1efd76a8f22d2556d73 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. test-x86_64-gnu-aux: 1h 26m -> 2h 35m (+78.9%)
2. test-x86_64-gnu-debug: 1h 19m -> 2h 7m (+60.8%)
3. test-x86_64-rust-for-linux: 35m 33s -> 56m 44s (+59.6%)
4. test-pr-check-2: 26m 51s -> 42m 43s (+59.1%)
5. test-x86_64-gnu-stdlib-semver-check: 11m 50s -> 18m 19s (+54.8%)
6. dist-powerpc64-linux-musl: 1h 7m -> 1h 38m (+44.8%)
7. test-i686-gnu-nopt-2: 2h 18m -> 1h 19m (-42.6%)
8. dist-apple-various: 1h 28m -> 2h 5m (+42.2%)
9. dist-riscv64-linux-gnu: 1h 4m -> 1h 31m (+41.3%)
10. test-x86_64-gnu-tools: 49m 11s -> 1h 8m (+40.3%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (0d38a84): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 1.0%]	32
Regressions ❌ (secondary)	0.7%	[0.1%, 1.7%]	37
Improvements ✅ (primary)	-0.5%	[-1.0%, -0.2%]	77
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	12
All ❌✅ (primary)	-0.2%	[-1.0%, 1.0%]	109

Max RSS (memory usage)

Results (primary 1.9%, secondary 5.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.7%	[0.7%, 4.4%]	18
Regressions ❌ (secondary)	5.8%	[1.8%, 13.0%]	15
Improvements ✅ (primary)	-0.8%	[-1.2%, -0.5%]	6
Improvements ✅ (secondary)	-5.8%	[-5.8%, -5.8%]	1
All ❌✅ (primary)	1.9%	[-1.2%, 4.4%]	24

Cycles

Results (primary -0.8%, secondary 2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	2.9%	[0.8%, 4.3%]	8
Improvements ✅ (primary)	-2.2%	[-2.3%, -2.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-2.3%, 1.9%]	3

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.636s -> 491.128s (0.10%)
Artifact size: 406.16 MiB -> 406.18 MiB (0.01%)

[panstromek]

perf triage:

The results are fairly mixed, not clearly an improvement or regression. This was measured before merge with similar results, so I assume the impact is considered as justified. This is an architectural cleanup that unblocks other improvements, I think it makes sense to accept.

@rustbot label: +perf-regression-triaged