Skip to content

Avoid prematurely choosing a glob import#153912

Merged
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
mu001999-contrib:fix-153842
Mar 24, 2026
Merged

Avoid prematurely choosing a glob import#153912
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
mu001999-contrib:fix-153842

Conversation

@mu001999
Copy link
Contributor

@mu001999 mu001999 commented Mar 15, 2026

Fixes #153842

Use the following without introducing trait to explain:

mod a {
    pub use crate::x::y as x; // single import #1
}

mod b {
    pub mod x {
        pub mod y {}
    }
}

use a::x; // single import #2
use b::*; // glob import #3

fn main() {}

In current implementation, when #1 is first resolved, crate::x is temporarily taken from glob import #3 as crate::b::x. This happens because single_import_can_define_name will see that #2 cannot define x (because it depends on #1 and #1 is ignored) and then return false. Later, during finalization, crate::x in #1 resolves through single import #2 instead, which no longer matches the initially cached module crate::b::x and triggers the ICE.

I think the resolver should keep this unresolved because #2 may still define x to avoid prematurely choosing a glob import.

r? petrochenkov

@rustbot 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. labels Mar 15, 2026
@petrochenkov
Copy link
Contributor

This code is a minefield, it is known to not be correct, it makes assumptions that do not hold, and the whole thing needs to be rewritten together with some correctness proof.
So I have zero confidence that the change from this PR is correct either.

@petrochenkov
Copy link
Contributor

Best I can do is to run crater on this and see what happens.
Although I'm not sure it's a good use of crater resources.
@bors try

rust-bors bot pushed a commit that referenced this pull request Mar 16, 2026
Avoid prematurely choosing a glob import
@rust-bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Glob imports that get selected first and then overwritten by single imports is something that is indeed done in the current system, even if it may be incorrect in corner cases, see e.g. the comments in select_glob_decl.

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2026
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 16, 2026

☀️ Try build successful (CI)
Build commit: b10dae3 (b10dae361a61486cfbdcd3aae3877ee17e00d192, parent: fe7294f8793d001bc3f9d197e7cdef6cdb46c15a)

@petrochenkov
Copy link
Contributor

Okay, the crater queue doesn't seem to be large at the moment.
@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-153912 created and queued.
🤖 Automatically detected try build b10dae3
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-153912 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-153912 is completed!
📊 4 regressed and 7 fixed (853200 total)
📊 4754 spurious results on the retry-regressed-list.txt, consider a retry1 if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

  1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-153912/retry-regressed-list.txt

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 21, 2026
@petrochenkov
Copy link
Contributor

No non-spurious regressions.
So my only concern now is that it can now accept more code that we may not want to accept.

@mu001999
Copy link
Contributor Author

mu001999 commented Mar 21, 2026

it can now accept more code that we may not want to accept

IIUC, single imports will be selected finally. This change is in single_import_can_define_name, will only avoid choosing a glob import when meeting both single and glob imports. It won't affect the final result of the resolution. So when we do the final resolutions, we should still deny the code that we won't accept currently.

But I have to say that I still don't have a complete mental model of resolver yet.

@petrochenkov
Copy link
Contributor

Okay, @bors r+

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 23, 2026

📌 Commit 5316bab has been approved by petrochenkov

It is now in the queue for this repository.

@rust-bors rust-bors bot 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 23, 2026
…rochenkov

Avoid prematurely choosing a glob import

Fixes rust-lang#153842

Use the following without introducing trait to explain:
```rust
mod a {
    pub use crate::x::y as x; // single import rust-lang#1
}

mod b {
    pub mod x {
        pub mod y {}
    }
}

use a::x; // single import rust-lang#2
use b::*; // glob import rust-lang#3

fn main() {}
```

In current implementation, when `rust-lang#1` is first resolved, `crate::x` is temporarily taken from glob import `rust-lang#3` as `crate::b::x`. This happens because `single_import_can_define_name` will see that `rust-lang#2` cannot define `x` (because it depends on `rust-lang#1` and `rust-lang#1` is ignored) and then return `false`. Later, during finalization, `crate::x` in `rust-lang#1` resolves through single import `rust-lang#2` instead, which no longer matches the initially cached module `crate::b::x` and triggers the ICE.

I think the resolver should keep this unresolved because `rust-lang#2` may still define `x` to avoid prematurely choosing a glob import.

r? petrochenkov
rust-bors bot pushed a commit that referenced this pull request Mar 23, 2026
…uwer

Rollup of 8 pull requests

Successful merges:

 - #122668 (Add APIs for dealing with titlecase)
 - #153041 (Remove `ATTRIBUTE_ORDER`)
 - #153912 (Avoid prematurely choosing a glob import)
 - #154093 (const validity checking: do not recurse to references inside MaybeDangling)
 - #154257 (Revert eagerly normalize in generalize)
 - #154179 (tests/ui/async-await/gat-is-send-across-await.rs: New regression test)
 - #154224 (Remove more `BuiltinLintDiag` variants)
 - #154245 (Allow applying autodiff macros to trait functions.)
@rust-bors rust-bors bot merged commit 924b911 into rust-lang:main Mar 24, 2026
12 checks passed
@rustbot rustbot added this to the 1.96.0 milestone Mar 24, 2026
@mu001999 mu001999 deleted the fix-153842 branch March 24, 2026 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

ICE: inconsistent resolution for an import

4 participants