Overhaul `UsePath` #141741

nnethercote · 2025-05-29T16:18:09Z

It currently uses SmallVec<[Res; 3]> which is really weird. Details in the individual commits.

r? @petrochenkov

rustbot · 2025-05-29T16:18:13Z

fmease is not on the review rotation at the moment.
They may take a while to respond.

rustbot · 2025-05-29T16:18:15Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in match checking

cc @Nadrieril

nnethercote · 2025-05-29T16:18:47Z

@fmease: please ignore the first two commits, they are from #141725.

Please feel free to reassign if you're not a good reviewer for this change.

nnethercote · 2025-05-29T16:20:13Z

fmease is not on the review rotation at the moment.

Oh dear... how about:

r? @lcnr

Not sure?

petrochenkov · 2025-05-29T16:27:26Z

(I'm the author of the current setup.)

nnethercote · 2025-05-29T16:38:31Z

Ok then: r? @petrochenkov

rustbot · 2025-05-29T16:38:33Z

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

rustbot · 2025-05-30T00:12:46Z

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

Noratrieb · 2025-05-30T09:26:46Z

compiler/rustc_hir/src/hir.rs

@@ -347,7 +347,7 @@ pub struct Path<'hir, R = Res> {
 }

 /// Up to three resolutions for type, value and macro namespaces.
-pub type UsePath<'hir> = Path<'hir, SmallVec<[Res; 3]>>;


now the comment doesn't add much information :)

Seems ok as a quick reminder of what PerNS is.

compiler/rustc_ast_lowering/src/path.rs

compiler/rustc_ast_lowering/src/item.rs

compiler/rustc_lint/src/unqualified_local_imports.rs

petrochenkov · 2025-05-30T10:41:48Z

UseKind:ListStem item gets no Res results

There are two kinds of list stems

Those that need to be lowered into HIR and checked, those always get a Res.
- Example: a::b in use a::b::{ /* empty */ };
Those that would ideally not be lowered into HIR at all, but we cannot just drop them if DefId was previously allocated for them, so they are lowered into an empty node with empty res and therefore loops over res, including HIR visitors, never visit them.
- Example a::b in use a::b::{not, empty};

So this

This is what we
want, but was more by luck than design.

is indeed design.

compiler/rustc_ast_lowering/src/item.rs has comments about this, perhaps they could be more detailed.

petrochenkov · 2025-05-30T10:46:47Z

This does make things more "statically typed", but I don't really like this aesthetically.
Res already knows its namespace (or has no namespace, like Res::Err), so PerNS<Res> is redundant.
The processing logic is more complex.

It's not too important though, because HIR imports shouldn't be used for anything load-bearing (except stability checking at the moment), and lints working on them typically do not work fully correctly (they should either work at name resolution time or use module_children instead of imports).

So I'd generally classify this as "neutral change, not an improvement".

petrochenkov · 2025-05-30T11:00:28Z

The previous lowering (before #104963) generated up to 3 different import items in HIR, so in #104963 the visitor loop over 3 items naturally evolved into a loop over 3 resolutions to minimize changes.

petrochenkov · 2025-05-30T11:01:23Z

@bors try @rust-timer queue

Overhaul `UsePath` It currently uses `SmallVec<[Res; 3]>` which is really weird. Details in the individual commits. r? `@petrochenkov`

bors · 2025-05-30T11:02:34Z

⌛ Trying commit 8e5356c with merge 99c6aa3...

bors · 2025-05-30T13:25:41Z

☀️ Try build successful - checks-actions
Build commit: 99c6aa3 (99c6aa342b84bfd40603c85195231d76dd1a1d61)

rust-timer · 2025-05-30T16:03:14Z

Finished benchmarking commit (99c6aa3): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-0.7%	[-1.1%, -0.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-1.1%, -0.3%]	2

Max RSS (memory usage)

Results (primary 0.3%, secondary -0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-0.9%	[-3.0%, -0.4%]	11
All ❌✅ (primary)	0.3%	[-1.3%, 1.8%]	2

Cycles

Results (secondary 0.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.9%	[0.5%, 2.0%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-1.0%, -0.5%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 370.26 MiB -> 370.29 MiB (0.01%)

petrochenkov · 2025-05-31T17:21:14Z

Let's merge this after all.
But perhaps some operations on PerNS are used more than once now and can be factored out into methods?
@bors rollup=maybe
@rustbot author

rustbot · 2025-05-31T17:21:17Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

I find it much easier to think about in the positive sense.

nnethercote · 2025-06-02T03:05:04Z

I have addressed the specific comments and also added a new commit that streamlines a couple of PerNS uses.

@bors ready

compiler/rustc_ast_lowering/src/lib.rs

petrochenkov · 2025-06-02T12:18:47Z

r=me after addressing the remaining nits and optionally #141741 (comment).
@rustbot author

`UsePath` contains a `SmallVec<[Res; 3]>`. This holds up to three `Res` results, one per namespace (type, value, or macro). `lower_import_res` takes a `PerNS<Option<Res<NodeId>>>` result and lowers it into the `SmallVec`. This is pretty weird. The input `PerNS` makes it clear which `Res` belongs to which namespace, but the `SmallVec` throws that information away. And code that operates on the `SmallVec` tends to use iteration (or even just grabbing the first entry!) without knowing which namespace the `Res` belongs to. Even weirder! Also, `SmallVec` is an overly flexible type to use here, because it can contain any number of elements (even though it's optimized for 3 in this case). This commit changes `UsePath` so it also contains a `PerNS<Option<Res<HirId>>>`. This type preserves more information and is more self-documenting. The commit also changes a lot of the use sites to access the result for a particular namespace. E.g. if you're looking up a trait, it will be in the `Res` for the type namespace if it's present; it's silly to look in the `Res` for the value namespace or macro namespace. Overall I find the new code much easier to understand. However, some use sites still iterate. These now use `present_items` because that filters out the `None` results. Also, `redundant_pub_crate.rs` gets a bigger change. A `UseKind:ListStem` item gets no `Res` results, which means the old `all` call in `is_not_macro_export` would succeed (because `all` succeeds on an empty iterator) and the `ListStem` would be ignored. This is what we want, but was more by luck than design. The new code detects `ListStem` explicitly. The commit generalizes the name of that function accordingly. Finally, the commit also removes the `use_path` arena, because `PerNS<Option<Res>>` impls `Copy` (unlike `SmallVec`) and it can be allocated in the arena shared by all `Copy` types.

nnethercote · 2025-06-02T23:40:31Z

I addressed the nits. I left the questionable comment behind, I figure it doesn't hurt.

@bors r=petrochenkov

bors · 2025-06-02T23:40:33Z

📌 Commit 8747ccb has been approved by petrochenkov

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - #140715 (Clarify &mut-methods' docs on sync::OnceLock) - #141677 (Async drop - type instead of async drop fn, fixes #140484) - #141741 (Overhaul `UsePath`) - #141873 (Fixed a typo in `ManuallyDrop`'s doc) - #141876 (Don't declare variables in `ExprKind::Let` in invalid positions) - #141886 (Add missing 2015 edition directives) - #141889 (Add missing `dyn` keywords to tests that do not test for them) - #141891 (Fix borrowck mentioning a name from an external macro we (deliberately) don't save) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #141741 - nnethercote:overhaul-UsePath, r=petrochenkov Overhaul `UsePath` It currently uses `SmallVec<[Res; 3]>` which is really weird. Details in the individual commits. r? `@petrochenkov`

rustbot assigned fmease May 29, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 29, 2025

This comment has been minimized.

Sign in to view

rustbot assigned lcnr and unassigned fmease May 29, 2025

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this May 29, 2025

fmease unassigned lcnr May 29, 2025

Noratrieb reviewed May 30, 2025

View reviewed changes

petrochenkov reviewed May 30, 2025

View reviewed changes

compiler/rustc_ast_lowering/src/path.rs Show resolved Hide resolved

compiler/rustc_ast_lowering/src/item.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/unqualified_local_imports.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 30, 2025

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2025

bors added a commit that referenced this pull request May 30, 2025

Auto merge of #141741 - nnethercote:overhaul-UsePath, r=<try>

99c6aa3

Overhaul `UsePath` It currently uses `SmallVec<[Res; 3]>` which is really weird. Details in the individual commits. r? `@petrochenkov`

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 30, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 31, 2025

nnethercote added 2 commits June 2, 2025 09:53

Factor out repeated code into is_mod_inherent.

bb063e6

Invert the sense of is_not_macro_export.

176c34a

I find it much easier to think about in the positive sense.

nnethercote force-pushed the overhaul-UsePath branch from 8e5356c to 2475522 Compare June 2, 2025 03:04

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 2, 2025

petrochenkov reviewed Jun 2, 2025

View reviewed changes

compiler/rustc_ast_lowering/src/lib.rs Show resolved Hide resolved

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2025

nnethercote force-pushed the overhaul-UsePath branch from 2475522 to 8747ccb Compare June 2, 2025 23:39

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 2, 2025

matthiaskrgr mentioned this pull request Jun 3, 2025

Rollup of 8 pull requests #141944

Merged

bors merged commit 8db6881 into rust-lang:master Jun 3, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 3, 2025

nnethercote deleted the overhaul-UsePath branch June 3, 2025 09:59

Overhaul UsePath #141741

Overhaul UsePath #141741

Uh oh!

Conversation

nnethercote commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 29, 2025

Uh oh!

rustbot commented May 29, 2025

Uh oh!

This comment has been minimized.

nnethercote commented May 29, 2025

Uh oh!

nnethercote commented May 29, 2025

Uh oh!

This comment has been minimized.

petrochenkov commented May 29, 2025

Uh oh!

nnethercote commented May 29, 2025

Uh oh!

rustbot commented May 29, 2025

Uh oh!

rustbot commented May 30, 2025

Uh oh!

Noratrieb May 30, 2025

Choose a reason for hiding this comment

Uh oh!

petrochenkov Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented May 30, 2025

Uh oh!

This comment has been minimized.

bors commented May 30, 2025

Uh oh!

bors commented May 30, 2025

Uh oh!

This comment has been minimized.

rust-timer commented May 30, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

petrochenkov commented May 31, 2025

Uh oh!

rustbot commented May 31, 2025

Uh oh!

nnethercote commented Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Jun 2, 2025

Uh oh!

nnethercote commented Jun 2, 2025

Uh oh!

bors commented Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

Overhaul `UsePath` #141741

Overhaul `UsePath` #141741

nnethercote commented May 29, 2025 •

edited

Loading

petrochenkov commented May 30, 2025 •

edited

Loading

petrochenkov commented May 30, 2025 •

edited

Loading

petrochenkov commented May 30, 2025 •

edited

Loading