Skip to content

Resolver: Batched Import Resolution #145108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

LorrensP-2158466
Copy link
Contributor

Transforms the current algorithm for resolving imports to a batched algorithm. Every import in the indeterminate_imports set is resolved in isolation. This is the only real difference from the current algorithm.

r? petrochenkov

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 8, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@LorrensP-2158466 LorrensP-2158466 marked this pull request as ready for review August 11, 2025 08:37
@rustbot rustbot 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 Aug 11, 2025
@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov 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 Aug 11, 2025
@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented Aug 11, 2025

Code looks a little better now and should be more correct than the previous commits, but I have yet to find a way to resolve the current test failures.

@rust-log-analyzer

This comment has been minimized.

@LorrensP-2158466
Copy link
Contributor Author

Okey, I did fix core not being built, but those other errors happened again. We now resolve the prelude import path when we create such an import at build_reduced_graph phase.

@LorrensP-2158466 LorrensP-2158466 force-pushed the batched-import-resolution branch from a3f8ae2 to 4a2a0dc Compare August 11, 2025 20:37
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Could you update the tests to make CI green, so I can see the difference?
(Even if the changes do not seem correct.)

@petrochenkov 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 Aug 12, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is definitely wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is wrong as well.

@petrochenkov
Copy link
Contributor

Moving prelude_import processing to build_reduced_graph may also be necessary for #139493.

@LorrensP-2158466
Copy link
Contributor Author

I'll create a pr for it.

@petrochenkov
Copy link
Contributor

Maybe the commit loop will have to be split into two loops, the first one just setting glob_importers (maybe also imported_module or something else), and the second one doing everything else.

@bors

This comment was marked as resolved.

@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented Aug 20, 2025

The libcore errors appear when glob_importers are moved from resolve_glob_import to commit (the last commit 5583ebc in master...petrochenkov:rust:batched-import-resolution).

I do not entirely agree with this. The errors started happening when we began using SideEffect and applying them in order of resolution, instead of separately applying the side effects (see a52656d). But yeah, those changes were necessary, because there were failing UI tests.

Reverting those changes did not resolve some of the errors; it even introduced new ones.

Try minimizing core into a small #![no_core] crate that reproduces the problem.

I've not been successful with this, but I should be getting close.

Maybe the commit loop will have to be split into two loops, the first one just setting glob_importers (maybe also imported_module or something else), and the second one doing everything else.

Thought of that as well, and this produces other errors.

I've also noticed that meddling with the commit order produces a lot of different errors that make no more sense than the current ones.

The most successful I've been in producing as few errors as possible is by first committing the glob side effects and then the single import side effects. The only errors that stay present are resolutions made of protable_simd types.

@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented Aug 20, 2025

Try tracing accesses to glob_importers before and after that commit to understand what happens.
Try minimizing core into a small #![no_core] crate that reproduces the problem.

Currently doing this.

fn resolve_import<'r>(mut self: CmResolver<'r, 'ra, 'tcx>, import: Import<'ra>) -> usize {
/// Meanwhile, if resolution is successful, the side effect of the resolution is returned.
fn resolve_import<'r>(
self: &mut CmResolver<'r, 'ra, 'tcx>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self: &mut CmResolver<'r, 'ra, 'tcx>,
&self,

@@ -1484,66 +1583,47 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
false
}

fn resolve_glob_import(&mut self, import: Import<'ra>) {
fn resolve_glob_import<'r>(
self: &mut CmResolver<'r, 'ra, 'tcx>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self: &mut CmResolver<'r, 'ra, 'tcx>,
&self,

@LorrensP-2158466 LorrensP-2158466 force-pushed the batched-import-resolution branch from 3b2e50e to 4efdee8 Compare August 21, 2025 15:08
@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@LorrensP-2158466
Copy link
Contributor Author

Original resolver errors are resolved, but...
Compiling libcore now fails where some traits are not implemented for some types, but the code does implement them. Most notable Copy, I have a feeling its derive macro is not being resolved correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants