Skip to content
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

fix(resolve): prevent infinite loop when glob-import self #110264

Closed
wants to merge 1 commit into from

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Apr 13, 2023

close #110164

@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@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 Apr 13, 2023
@compiler-errors
Copy link
Member

r? @petrochenkov

use _::a;
//~^ ERROR expected identifier, found reserved identifier `_`
//~| ERROR unresolved import `_`
use _::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does use _::* trigger the "glob-import a module into itself" in the first place?
I'd expect _ to resolve into Res::Err here.

Is it possible to reproduce the ICE without _::*, just with regular well-formed imports?

Copy link
Contributor Author

@bvanjoi bvanjoi Apr 17, 2023

Choose a reason for hiding this comment

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

Is it possible to reproduce the ICE without _::*, just with regular well-formed imports?

Will not occur, other cases will hit map_err

I'd expect _ to resolve into Res::Err here.

When executing new_key, special processing is performed for _ to ensure that _ each time the key is not the same, it is intended to solve the case of xxx as _

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, fascinating.
The underscore_disambiguator += 1 should only apply when we define names, not when we are retrieving already added definitions.

Copy link
Contributor

@petrochenkov petrochenkov Apr 17, 2023

Choose a reason for hiding this comment

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

I suggest adding BindingKey::new method that will be used everywhere by default, and rename new_key to new_disambiguated_key and use it only when adding use foo as _ imports to a module (in fn add_import I guess, but I'm not sure).

Note that if new_key is called for a single import twice in different contexts, then the resulting disambiguators are not consistent with each other, and it's something that is suspicious at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is independent from glob imports and we should do it first.

The glob change also looks reasonable, maybe it fixes some other actual issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I will open a new PR to rewrite new_key, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, new PR.

compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved
@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 Apr 17, 2023
@rust-log-analyzer

This comment has been minimized.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 18, 2023
…etrochenkov

fix(resolve): only disambiguate binding key during define

- close rust-lang#110164
- discussion: rust-lang#110264 (comment)

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2023
…rochenkov

fix(resolve): only disambiguate binding key during define

- close rust-lang#110164
- discussion: rust-lang#110264 (comment)

r? `@petrochenkov`
@JohnCSimon
Copy link
Member

@bvanjoi
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 17, 2023

this issue had been solved by another PR, so it can be closed.

@bvanjoi bvanjoi closed this Jun 17, 2023
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.

ICE: stack overflow in resolver
6 participants