Resolve: merge `source_bindings` and `target_bindings` into `bindings` #143685

LorrensP-2158466 · 2025-07-09T13:02:37Z

Attempts to merge the 2 fields source_bindings and target_bindings of ImportKind::Single into 1 field called bindings.

r? @petrochenkov

LorrensP-2158466 · 2025-07-09T13:05:50Z

I used std::task::Poll as Progress (is not as confusing and I liked it more than Option).

compiler/rustc_resolve/src/ident.rs

bjorn3 · 2025-07-09T14:13:17Z

compiler/rustc_resolve/src/imports.rs

@@ -40,6 +41,9 @@ use crate::{

 type Res = def::Res<NodeId>;

+// Poll is a weird name.
+use std::task::Poll as Progress;


It feels weird to reuse std::task::Poll when you are not using futures.

Yeah, Vadim said in #gsoc > Project: Parallel Macro Expansion @ 💬 it was semantically correct as well.

I find it to convey the message more clearly than Option<Option<...>>. Option::None doesn't say anything that we're still trying to resolve it, but Progress::Pending does. And Progres::Ready(None) is also clearer than Option::Some(None).

If this is something that is not done, I'll change it.

Defining your own Progress enum would work, right? But I will leave it up to Vadim if they agree.

Let's finish the correct implementation first, and then decide on the enum choice.

LorrensP-2158466 · 2025-07-09T17:27:00Z

Unfortunately, this didn't fix the fail. I'll investigate it tomorrow.

compiler/rustc_resolve/src/imports.rs

petrochenkov · 2025-07-11T14:06:38Z

compiler/rustc_resolve/src/lib.rs

@@ -891,6 +891,13 @@ impl<'ra> NameBindingData<'ra> {
        }
    }

+    fn maybe_source_binding(&self) -> Option<NameBinding<'ra>> {


Suggested change

fn maybe_source_binding(&self) -> Option<NameBinding<'ra>> {

fn import_source(&self) -> NameBinding<'ra> {

I don't think you ever need either one or another.

petrochenkov · 2025-07-11T14:06:47Z

compiler/rustc_resolve/src/lib.rs

+    fn maybe_source_binding(&self) -> Option<NameBinding<'ra>> {
+        match self.kind {
+            NameBindingKind::Import { binding, .. } => Some(binding),
+            _ => None,


Suggested change

_ => None,

_ => unreachable!(),

petrochenkov · 2025-07-11T14:08:45Z

compiler/rustc_resolve/src/imports.rs

@@ -24,7 +25,7 @@ use rustc_span::{Ident, Span, Symbol, kw, sym};
 use smallvec::SmallVec;
 use tracing::debug;

-use crate::Determinacy::{self, *};
+use crate::Determinacy::{self};


Suggested change

use crate::Determinacy::{self};

use crate::Determinacy;

Weird importing.

petrochenkov · 2025-07-11T14:11:47Z

compiler/rustc_resolve/src/imports.rs

                && source.name == kw::SelfLower
                // Silence `unresolved import` error if E0429 is already emitted
-                && let Err(Determined) = source_bindings.value_ns.get()
+                // `indings` here is "target_bindings", if it is "Err(Determined)" then `source_bindings` as well.


This comment is only useful in context of this PR, and can be removed.

petrochenkov · 2025-07-11T14:15:28Z

compiler/rustc_resolve/src/imports.rs

                } else {
                    return;
                };

                let parent = import.parent_scope.module;
-                match source_bindings[ns].get() {
-                    Ok(binding) => {
+                match bindings[ns].get() {


I think this match can be merged into the match above, they are exactly the same.

petrochenkov · 2025-07-11T14:27:13Z

compiler/rustc_resolve/src/imports.rs

@@ -1340,7 +1346,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
            let mut full_path = import.module_path.clone();
            full_path.push(Segment::from_ident(ident));
            self.per_ns(|this, ns| {
-                if let Ok(binding) = source_bindings[ns].get() {
+                if let Progress::Ready(Some(binding)) = bindings[ns].get() {


maybe_source_binding was not applied here.

LorrensP-2158466 · 2025-07-11T21:03:17Z

compiler/rustc_resolve/src/imports.rs

@@ -838,22 +832,28 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
        let mut indeterminate_count = 0;
        self.per_ns(|this, ns| {
            if !type_ns_only || ns == TypeNS {
-                if let Err(Undetermined) = source_bindings[ns].get() {
-                    let binding = this.maybe_resolve_ident_in_module(
+                if let Progress::Pending = bindings[ns].get() {


I have a strange feeling I should do the source_binding extraction here as well? And if not, why shouldn't it be?

LorrensP-2158466 · 2025-07-11T21:34:21Z

@rustbot ready

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 9, 2025

rustbot assigned petrochenkov Jul 9, 2025

This comment has been minimized.

Sign in to view

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2025

petrochenkov reviewed Jul 9, 2025

View reviewed changes

compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved

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 9, 2025

bjorn3 reviewed Jul 9, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

petrochenkov reviewed Jul 10, 2025

View reviewed changes

compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

LorrensP-2158466 added 4 commits July 11, 2025 15:00

merge source and target bindings into single field

cba97a9

catch both Pending and Ready(None)

3c23b46

extract source_binding out of bindings

89322c9

cleanup

564c205

LorrensP-2158466 force-pushed the merge-import-bindings branch from e6d1789 to 564c205 Compare July 11, 2025 13:12

LorrensP-2158466 marked this pull request as ready for review July 11, 2025 13:13

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 changed the title ~~[WIP] Resolve: merge source_bindings and target_bindings into 1 field~~ Resolve: merge source_bindings and target_bindings into 1 field Jul 11, 2025

LorrensP-2158466 changed the title ~~Resolve: merge source_bindings and target_bindings into 1 field~~ Resolve: merge source_bindings and target_bindings into bindings Jul 11, 2025

This comment has been minimized.

Sign in to view

petrochenkov reviewed Jul 11, 2025

View reviewed changes

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 commented Jul 11, 2025

View reviewed changes

address review

b71fc0c

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

	fn maybe_source_binding(&self) -> Option<NameBinding<'ra>> {
	fn import_source(&self) -> NameBinding<'ra> {

Resolve: merge source_bindings and target_bindings into bindings #143685

Are you sure you want to change the base?

Resolve: merge source_bindings and target_bindings into bindings #143685

Conversation

LorrensP-2158466 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LorrensP-2158466 commented Jul 9, 2025

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 commented Jul 9, 2025

Uh oh!

This comment has been minimized.

Uh oh!

This comment was marked as resolved.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 commented Jul 11, 2025

Uh oh!

Uh oh!

Resolve: merge `source_bindings` and `target_bindings` into `bindings` #143685

Resolve: merge `source_bindings` and `target_bindings` into `bindings` #143685

LorrensP-2158466 commented Jul 9, 2025 •

edited

Loading

LorrensP-2158466 Jul 11, 2025 •

edited

Loading