Skip to content

fix(resolve): update shadowed_glob more precision #112495

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 14, 2023
Merged

fix(resolve): update shadowed_glob more precision #112495

merged 1 commit into from
Jun 14, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jun 10, 2023 •
edited
Loading

Why does it panic?

We use #109153 as an illustration.

The process of resolve_imports is:

Iter resolve resolution of (Mod(root), Ident(bar) in type ns)
0 use foo::* binding -> foo::bar, shallowed_glob -> None
1 use bar::bar binding -> foo::bar::bar, shallowed_glob -> foo::bar
2 use bar::* binding -> foo::bar::bar, shallowed_glob -> foo::bar::bar::bar

So during finalize_import, the root::bar in use bar::bar had been pointed to foo::bar::bar::bar, which is different from the initial_module valued of foo::bar, therefore, the panic had been triggered.

Try to solve it

I think #109153 should check-pass rather than throw an ambiguous error. Following this idea, there are two ways to solve this problem:

1. Give up the initial_module and update import.imported_module after each resolution update. However, I think this method may have too much impact.
2. Do not update the shadowed_glob when it is defined.

To be honest, I am not sure if this is the right way to solve this ICE. Perhaps there is a better resolution.

Edit: we had made the resolution.shadowed_glob update more detailed.

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 Jun 10, 2023
bvanjoi
bvanjoi commented Jun 11, 2023
View reviewed changes
petrochenkov
petrochenkov reviewed Jun 13, 2023
View reviewed changes
@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 Jun 13, 2023
bvanjoi
bvanjoi commented Jun 13, 2023
View reviewed changes
bvanjoi
bvanjoi commented Jun 13, 2023
View reviewed changes
@bvanjoi bvanjoi changed the title fix(resolve): skip re-assign shadowed_glob when there had diff res for TypeNS fix(resolve): do not re-assign shadowed_glob if it is defined Jun 13, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 13, 2023

All checks had passed, so @rustbot ready

@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 Jun 13, 2023
@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 Jun 13, 2023
bvanjoi
bvanjoi commented Jun 13, 2023
View reviewed changes
@bvanjoi bvanjoi changed the title fix(resolve): do not re-assign shadowed_glob if it is defined fix(resolve): update shadowed_glob more precision Jun 13, 2023
petrochenkov
petrochenkov reviewed Jun 13, 2023
View reviewed changes
tests/ui/resolve/issue-105069.stderr
@@ -4,17 +4,19 @@ error[E0659]: `V` is ambiguous
LL | use V;
| ^ ambiguous name
|
= note: ambiguous because of multiple potential import sources
note: `V` could refer to the variant imported here
= note: ambiguous because of multiple glob imports of a name in the same module
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you just fixed this FIXME - https://github.com/rust-lang/rust/pull/112086/files#diff-54ebaa0715c94a2acf974077e838aa584186e187f3d01edb1791fd71c78408caR937-R939 and we can now remove AmbiguityKind::Import in #112086.

@rust-log-analyzer

This comment has been minimized.

# to view
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 13, 2023

📌 Commit f7330eb has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 13, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 14, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2023
@bors
Auto merge of rust-lang#112609 - matthiaskrgr:rollup-er6weld, r=matth…
57c215b 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#112197 (Erase regions even if normalization fails in writeback (in new solver))
 - rust-lang#112495 (fix(resolve): update shadowed_glob more precision)
 - rust-lang#112520 (Fix the overflow issue for transmute_generic_consts)
 - rust-lang#112571 (rustdoc-search: search never type with `!`)
 - rust-lang#112581 ([rustdoc] Fix URL encoding of % sign)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 98f6e96 into rust-lang:master Jun 14, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 14, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.

ICE: assertion failed: import.imported_module.get().is_none()' ICE inconsistent resolution for an import
5 participants
@bvanjoi @rust-log-analyzer @petrochenkov @bors @rustbot