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.

Sign up for GitHub

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

Jump to bottom

fix: modify the condition that resolve_imports stops #108729

Merged
merged 1 commit into from
Mar 21, 2023
Merged

fix: modify the condition that resolve_imports stops #108729

merged 1 commit into from
Mar 21, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Mar 4, 2023

close #97534

@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2023

r? @eholk

(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 Mar 4, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 4, 2023

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned eholk Mar 4, 2023
@petrochenkov
Copy link
Contributor

This assert is supposed to catch situations that shouldn't happen.
If it is triggered then something goes wrong, and compilation should never succeed in such case.

We need to figure out why it does happen with the example from #97534 instead of simply removing the assert.
@rustbot author

@rustbot rustbot 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 Mar 6, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 6, 2023 •
edited
Loading

We need to figure out why it does happen with the example from #97534 instead of simply removing the assert.

Thank you for helping me to change my mind about the problem. After checking the logs, I found that the problem with #97534 was that the resolve process from bar to foo was failing, which in turn was causing problems with the value of progress.

The current PR avoids this problem by changing the timing of resolve_importers execution, and it may fix the issue correctly.

Looking forward to your suggestions!

@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 Mar 6, 2023
@bvanjoi bvanjoi changed the title fix(rustc_expand): remove force mode error in retry resolve fix(rustc_expand): resolve imports during full expand Mar 7, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 7, 2023

cc @petrochenkov

petrochenkov
petrochenkov reviewed Mar 9, 2023
View reviewed changes
tests/ui/macros/nested-use-as-2015.rs Outdated Show resolved Hide resolved
tests/ui/macros/nested-use-as-2021.rs Outdated Show resolved Hide resolved
tests/ui/macros/nested-use-as-2021.rs Outdated Show resolved Hide resolved
tests/ui/macros/nested-use-as-2021.rs Outdated Show resolved Hide resolved
tests/ui/macros/nested-use-as-2021.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

I found that the problem with #97534 was that the resolve process from bar to foo was failing, which in turn was causing problems with the value of progress.

I'm still not sure how a failing import resolution may affect the value of progress and why this change fixes the issue, I need to do some testing.

@petrochenkov
Copy link
Contributor

Apparently either resolve_imports or resolve_macro_invocation are not idempotent and have some weird side effects, but I don't have time to investigate this deeper.

If the change in this PR somehow fixes this specific ICE, then let's land it, because it's just a reasonable optimization even in isolation.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 9, 2023

I'm still not sure how a failing import resolution may affect the value of progress.

To be precise, it is not the failure of resolve that affects the value of progress, but rather the fact that progress uses true from the last loop, and thus reports an error.

@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 Mar 9, 2023
@bvanjoi bvanjoi force-pushed the fix-issue-97534 branch 2 times, most recently from 23b73d4 to 5ef535c Compare March 10, 2023 15:54
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 10, 2023 •
edited
Loading

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 10, 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 Mar 18, 2023
@bvanjoi bvanjoi changed the title fix(resolve): once more resolve_imports fix: modify the condition that resolve_imports stops Mar 19, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 19, 2023

@rustbot ready

It was an unforgettable PR, the first time I applied what I learned in class to a complex project, thanks to your tireless correction of my problems and your mentor-like help. @petrochenkov

@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 Mar 19, 2023
petrochenkov
petrochenkov reviewed Mar 19, 2023
View reviewed changes
compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
library/core/tests/iter/sources.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

r=me after addressing remaining nits
@rustbot author

@rustbot rustbot 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 Mar 19, 2023
@petrochenkov petrochenkov mentioned this pull request Mar 19, 2023
Closed
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2023

📌 Commit 1775722 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 Mar 19, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 20, 2023
@matthiaskrgr
Rollup merge of rust-lang#108729 - bvanjoi:fix-issue-97534, r=petroch…
5abb423 
…enkov

fix: modify the condition that `resolve_imports` stops

close rust-lang#97534
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 20, 2023
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2023
@bors
Auto merge of rust-lang#109453 - matthiaskrgr:rollup-odn02wu, r=matth…
1db9c06 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#96391 (Windows: make `Command` prefer non-verbatim paths)
 - rust-lang#108164 (Drop all messages in bounded channel when destroying the last receiver)
 - rust-lang#108729 (fix: modify the condition that `resolve_imports` stops)
 - rust-lang#109336 (Constrain const vars to error if const types are mismatched)
 - rust-lang#109403 (Avoid ICE of attempt to add with overflow in emitter)
 - rust-lang#109415 (Refactor `handle_missing_lit`.)
 - rust-lang#109441 (Only implement Fn* traits for extern "Rust" safe function pointers and items)
 - rust-lang#109446 (Do not suggest bounds restrictions for synthesized RPITITs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ee330a3 into rust-lang:master Mar 21, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 21, 2023
@Alexendoo Alexendoo mentioned this pull request Mar 23, 2023
Closed
@bvanjoi bvanjoi deleted the fix-issue-97534 branch April 3, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.70.0
Development

Successfully merging this pull request may close these issues.

ICE with macros in macros and multiple use ... as ... with --edition=2021
6 participants
@bvanjoi @rustbot @petrochenkov @rust-log-analyzer @bors @eholk