Track partial_res_map per owner#159760
Conversation
still keep around the global map for within the resolver, but only forward the per-owner maps to ast lowering
They are not used anywhere, as we use the import_map everywhere for the actual resolutions. The import_map stores the resolution of the last segment of an import, the previous segments' resolution are only used within the resolver itself.
|
cc @rust-lang/clippy |
|
Interesting! I'm still getting my head around how the compiler works, so I'm not super confident I can say what the impact of this PR would be on the viability of this lint. But I do want to highlight I have an open PR for a total rewrite anyway to work directly with the use items instead of just the paths, since there was already issues with how multi imports were being handled. That PR is here. I'm not sure if that makes things better or worse, but I'd be happy to try and make my PR work with yours once it's merged. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The clippy lint only needs each segment to have a resolution the first time it appears. Are the
You mean to put the whole |
|
The hir ids are different. Only the node ids are shared. But as you saw, even a rustc internal lint is having this problem. So I will have to see if I can get the information preserved in some manner |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Track `partial_res_map` per owner
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (30ec28b): comparison URL. Overall result: ❌ regressions - 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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.8%, secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary 2.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 486.724s -> 489.686s (0.61%) |
cc rust-lang/rust-project-goals#620
r? @petrochenkov
Except for imports, everything using the partial_res_map in ast lowering works per-owner already. So, as a way to land this without a major refactoring, we turn all resolutions of path segments in use trees other than the first into
Res::Err, which works just fine except for this one clippy lint.Some background:
use statements are expanded from ast -> hir by flattening trees, so
is turned into
but the
foo::barpart is only resolved in the first expanded import and theboois only resolved in the second one (the one ending ina). Onmainthe way this works out is that we ast->hir lower each of those paths for every expanded import by looking up the resolution again.So when we start having per-owner maps, the second and third expanded import can't access the entry for
fooanymore, as it's only available in the first map.As discussed in #t-compiler > partial_res_map vs per-owner tables @ 💬, reworking the representation of use statements is entirely on the table. So I'm planning to just flatten the entire tree as we do today, but only have one owner (
ItemKind::Use) and everything else just lives within that.cc @bushrat011899 as a recent contributor to the clippy lint.
rustdoc and stability checking are unaffected, as the former uses
import_res_map, and the latter still correctly reports on path segments via the first expansion of a use tree.