Skip to content

Refactor resolve resolution bindings #143734

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 1 commit into
base: master
Choose a base branch
from

Conversation

LorrensP-2158466
Copy link
Contributor

This pr does the work asked in #142547 (comment). This part:

move the (non)_glob_binding change

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 Jul 10, 2025
@LorrensP-2158466 LorrensP-2158466 force-pushed the refactor-resolve-resolution-bindings branch from bc863d6 to 9c91fb1 Compare July 10, 2025 13:52
@LorrensP-2158466
Copy link
Contributor Author

Had to fix the conflicts in the tests from the ambiguous_glob_imports pr. The author is different in that commit, should I change it?

@b-naber
Copy link
Contributor

b-naber commented Jul 10, 2025

Had to fix the conflicts in the tests from the ambiguous_glob_imports pr. The author is different in that commit, should I change it?

No it's fine. Thanks for the PRs.

@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 Jul 10, 2025
@LorrensP-2158466
Copy link
Contributor Author

So I kept the exact changes from b-naber as exact as possible, so I think most of these reviews are related to their changes. Should the "address review" commit be authored by them as well? Or doesn't this matter because the commits will probably be squashed?

@b-naber
Copy link
Contributor

b-naber commented Jul 10, 2025

I'll address the review either tomorrow or on the weekend. Can you give me write access to your branch?

@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented Jul 10, 2025

Yes! So i shouldn't address them or?

@b-naber
Copy link
Contributor

b-naber commented Jul 10, 2025

No, you can if you want to of course. It would just take some time to read into the logic of the code, but if you want to do that, feel free to address the review.

@LorrensP-2158466
Copy link
Contributor Author

Alright! I'll see what I can do today. I didn't find anything on branch-specific access, so I just added you to my fork.

@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented Jul 10, 2025

So, sorry, I didn't have much time and am pretty tired :). I only address the simple requests from Vadim.

Let me know if you'd like to take the actual logic changes. I'm perfectly fine doing them myself since it's good to learn the resolution process for my project. But I understand if you want this to go a little faster.

I should be able to finish this before the weekend is over, but I'm not sure, of course. :)

@bors

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor

since it's good to learn the resolution process for my project

Yeah, that's pretty much was the idea, getting experience doing something practical before going to harder refactorings needed for import parallelization.

@b-naber
Copy link
Contributor

b-naber commented Jul 11, 2025

So, sorry, I didn't have much time and am pretty tired :). I only address the simple requests from Vadim.

Let me know if you'd like to take the actual logic changes. I'm perfectly fine doing them myself since it's good to learn the resolution process for my project. But I understand if you want this to go a little faster.

I should be able to finish this before the weekend is over, but I'm not sure, of course. :)

No it's fine, I can wait until you've addressed the review.

@LorrensP-2158466 LorrensP-2158466 force-pushed the refactor-resolve-resolution-bindings branch from 1fe68dc to dd5c2bb Compare July 11, 2025 09:06
@LorrensP-2158466
Copy link
Contributor Author

So I edited the first commit to not introduce the faulty use of late_binding and changed the big diff in try_define to what it was before.

Then 2 extra commits (which should have been squashed, sorry), which address the rest of the review.

revert tests reverts the tests to what they were before because the changes of the first commit now failed with b-naber's changes.

@LorrensP-2158466
Copy link
Contributor Author

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

@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 Jul 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

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

@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 Jul 11, 2025
@petrochenkov
Copy link
Contributor

Thanks!
r=me after squashing commits.
@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 Jul 12, 2025
@LorrensP-2158466 LorrensP-2158466 force-pushed the refactor-resolve-resolution-bindings branch from d500de6 to 4b221e0 Compare July 12, 2025 13:05
@LorrensP-2158466 LorrensP-2158466 force-pushed the refactor-resolve-resolution-bindings branch from 4b221e0 to 9ed5378 Compare July 12, 2025 13:06
@LorrensP-2158466
Copy link
Contributor Author

I do not understand what you mean by r=me.
Second force push is because I forgot to set b-naber as author.

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

I do not understand what you mean by r=me.

https://rustc-dev-guide.rust-lang.org/walkthrough.html?highlight=r%3Dme#implementation

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 12, 2025

📌 Commit 9ed5378 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2025
fmease added a commit to fmease/rust that referenced this pull request Jul 13, 2025
…resolution-bindings, r=petrochenkov

Refactor resolve resolution bindings

This pr does the work asked in rust-lang#142547 (comment). This part:

> move the `(non)_glob_binding` change

r? `@petrochenkov`
bors added a commit that referenced this pull request Jul 13, 2025
Rollup of 14 pull requests

Successful merges:

 - #143301 (`tests/ui`: A New Order [26/N])
 - #143461 (make `cfg_select` a builtin macro)
 - #143519 (Check assoc consts and tys later like assoc fns)
 - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const)
 - #143634 (interpret/allocation: expose init + write_wildcards on a range)
 - #143679 (Preserve the .debug_gdb_scripts section)
 - #143685 (Resolve: merge `source_bindings` and `target_bindings` into `bindings`)
 - #143704 (Be a bit more careful around exotic cycles in in the inliner)
 - #143734 (Refactor resolve resolution bindings)
 - #143774 (constify `From` and `Into`)
 - #143785 (Add --compile-time-deps argument for x check)
 - #143786 (Fix fallback for CI_JOB_NAME)
 - #143825 (clippy: fix test filtering when TESTNAME is empty)
 - #143826 (Fix command trace)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2025
…resolution-bindings, r=petrochenkov

Refactor resolve resolution bindings

This pr does the work asked in rust-lang#142547 (comment). This part:

> move the `(non)_glob_binding` change

r? ``@petrochenkov``
bors added a commit that referenced this pull request Jul 13, 2025
Rollup of 13 pull requests

Successful merges:

 - #143301 (`tests/ui`: A New Order [26/N])
 - #143461 (make `cfg_select` a builtin macro)
 - #143519 (Check assoc consts and tys later like assoc fns)
 - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const)
 - #143634 (interpret/allocation: expose init + write_wildcards on a range)
 - #143679 (Preserve the .debug_gdb_scripts section)
 - #143685 (Resolve: merge `source_bindings` and `target_bindings` into `bindings`)
 - #143734 (Refactor resolve resolution bindings)
 - #143774 (constify `From` and `Into`)
 - #143785 (Add --compile-time-deps argument for x check)
 - #143786 (Fix fallback for CI_JOB_NAME)
 - #143825 (clippy: fix test filtering when TESTNAME is empty)
 - #143826 (Fix command trace)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2025
…resolution-bindings, r=petrochenkov

Refactor resolve resolution bindings

This pr does the work asked in rust-lang#142547 (comment). This part:

> move the `(non)_glob_binding` change

r? ```@petrochenkov```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2025
…resolution-bindings, r=petrochenkov

Refactor resolve resolution bindings

This pr does the work asked in rust-lang#142547 (comment). This part:

> move the `(non)_glob_binding` change

r? ````@petrochenkov````
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.

5 participants