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

refactor(resolve): splitting glob and non-glob name bindings #112659

Closed

bvanjoi wants to merge 1 commit into rust-lang:master from bvanjoi:split-glob-name-binding

Contributor

bvanjoi commented Jun 15, 2023

Fixes this comment.

r? @petrochenkov

rustbot assigned petrochenkov

rustbot added S-waiting-on-review T-compiler labels

Contributor

petrochenkov commented Jun 15, 2023 •

edited

Loading

Meta-comment: could you avoid changing and moving code (to compiler/rustc_resolve/src/name_resolution.rs) in a same commit? It makes it harder to review the changes.
Let's start with the changes only, and then the move can be done later in a separate commit (or not done at all).

Contributor Author

bvanjoi commented Jun 15, 2023

the move can be done later in a separate commit (or not done at all).

The reason I used a segregation file was to hide the non_glob_binding and glob_binding fields. However, I will move them back for a better review process.

This comment has been minimized.

Sign in to view

bvanjoi force-pushed the split-glob-name-binding branch from 879073d to c4e44c8 Compare

June 15, 2023 15:55

Contributor Author

bvanjoi commented Jun 15, 2023

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

It looks like there is something that is not tested in test/ui that I should fix first

bvanjoi marked this pull request as draft

June 15, 2023 16:00

This comment has been minimized.

Sign in to view

bvanjoi force-pushed the split-glob-name-binding branch from c4e44c8 to b083f8b Compare

June 15, 2023 18:39

This comment has been minimized.

Sign in to view

bvanjoi force-pushed the split-glob-name-binding branch from b083f8b to 289b640 Compare

June 15, 2023 18:45


          refactor(resolve): splitting glob and non-glob name bindings

f392f71

bvanjoi force-pushed the split-glob-name-binding branch from 289b640 to f392f71 Compare

June 16, 2023 01:46

bvanjoi marked this pull request as ready for review

June 16, 2023 02:26

Contributor Author

bvanjoi commented Jun 16, 2023

All tests had passed, @rustbot ready

Contributor

petrochenkov commented Jun 16, 2023

Contributor

petrochenkov commented Jun 16, 2023

This PR looks like it is in the right direction, but it is not exactly what I wanted to do.

The picture above shows how non-glob and glob names in a module should be integrated into name lookup as two different scopes.
In that case errors like GlobVsExpanded would be subsumed by more general errors like MoreExpandedVsOuter (because globs would be an outer scope), ignore_binding would be treated more consistently, and any other ad hoc treatment of non-glob and glob names would be replaced with regular scope walk.

I'm not sure whether non-glob and glob resolutions for the same name should be packed into the same NameResolution, maybe we should have separate module.non_glob_resolutions and module.glob_resolutions instead, that would depend on convenience of implementation.

petrochenkov reviewed

View reviewed changes

compiler/rustc_resolve/src/imports.rs

+                  }
+                  pub(crate) fn non_glob_binding(&self) -> Option<&'a NameBinding<'a>> {
+                      self.non_glob_binding.and_then(|binding| {

Contributor

petrochenkov Jun 16, 2023

.and_then(|...| Some(x) ) is .map(|...| x)

compiler/rustc_resolve/src/imports.rs

-                              None
-                          }
+                      self.non_glob_binding()
+                          .or_else(|| if self.single_imports.is_empty() { self.glob_binding() } else { None })

Contributor

petrochenkov Jun 16, 2023

This logic is likely incorrect and is a source of issues like #56593 (because it doesn't account for macros), but let's keep it as is in this PR.

I'd just want to rename it to something more explanatory, like determinate_binding, keep the removed comment and add a FIXME.

Contributor Author

bvanjoi Jun 16, 2023

Yes, I think that issue #56593 is interesting and could be the next task to work on.

compiler/rustc_resolve/src/imports.rs

                       })
                   }
-                  pub(crate) fn add_single_import(&mut self, import: &'a Import<'a>) {
-                      self.single_imports.insert(Interned::new_unchecked(import));
+                  pub(crate) fn available_binding(&self) -> Option<&'a NameBinding<'a>> {

Contributor

petrochenkov Jun 16, 2023

Suggested change

      
                pub(crate) fn available_binding(&self) -> Option<&'a NameBinding<'a>> {
          
                pub(crate) fn binding(&self) -> Option<&'a NameBinding<'a>> {

Assuming the other method is renamed to determinate_binding.

compiler/rustc_resolve/src/imports.rs

+                  pub(crate) fn non_glob_binding(&self) -> Option<&'a NameBinding<'a>> {
+                      self.non_glob_binding.and_then(|binding| {
+                          assert!(!binding.is_glob_import());

Contributor

petrochenkov Jun 16, 2023

I'm not sure these asserts are useful, there's literally one place where these fields are assigned and the glob-ness is checked there.
(I guess they could be kept during development and removed before merging.)

compiler/rustc_resolve/src/imports.rs

-                                          resolution.single_imports.remove(&Interned::new_unchecked(import));
-                                      });
+                                      let mut resolution = this.resolution(parent, key).borrow_mut();
+                                      resolution.single_imports.remove(&Interned::new_unchecked(import));

Contributor

petrochenkov Jun 16, 2023

update_resolution has some extra logic related to glob_importers.
Does it never run in this case? Or it runs but it's irrelevant?

Contributor Author

bvanjoi Jun 16, 2023 •

edited

Loading

It is a cleanup, because it seems will never meet other logic in update_resolution because the binding is not updated, and it will hit following code always:

 match resolution.binding() {
      _ if old_binding.is_some() => return t,
      None => return t,
      // xxxxxx
}

Of course, It should revert it back because it is irrelevant.

compiler/rustc_resolve/src/imports.rs

-                                  (false, false) => {
-                                      return Err(old_binding);
-                                  }
+                      if let Some(old_binding) = resolution.available_binding() && res == Res::Err && old_binding.res() != Res::Err {

Contributor

petrochenkov Jun 16, 2023

Could you keep the old code structure here when possible?
Otherwise it's hard to see what changes here.

compiler/rustc_resolve/src/build_reduced_graph.rs

    
            @@ -378,7 +379,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
          
                                  if !type_ns_only || ns == TypeNS {

                                      let key = BindingKey::new(target, ns);

                                      let mut resolution = this.resolution(current_module, key).borrow_mut();

                                      resolution.add_single_import(import);

                                      resolution.single_imports.insert(Interned::new_unchecked(import));

Contributor

petrochenkov Jun 16, 2023

Nit: unrelated change, could be moved to a separate cleanup commit.

compiler/rustc_resolve/src/ident.rs

-                      // If the primary binding is unusable, search further and return the shadowed glob
-                      // binding if it exists. What we really want here is having two separate scopes in
-                      // a module - one for non-globs and one for globs, but until that's done use this
-                      // hack to avoid inconsistent resolution ICEs during import validation.

Contributor

petrochenkov Jun 16, 2023

Could you keep this comment?
This PR doesn't introduce two separate scopes yet.

compiler/rustc_resolve/src/ident.rs

@@ @@ -900,7 +895,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { @@
                           }
                           // Forbid expanded shadowing to avoid time travel.
-                          if let Some(shadowed_glob) = resolution.shadowed_glob
+                          if let Some(shadowed_glob) = resolution.glob_binding()

Contributor

petrochenkov Jun 16, 2023

This line alone looks like a change in semantics - the binding is still glob now, but not necessarily shadowed.
But the binding.res() != shadowed_glob.res() condition below should ensure the old behavior, right?

tests/ui/resolve/non-glob-first.rs

+                  pub struct _S(Vec<P>);
+              }
+              fn main() {}

Contributor

petrochenkov Jun 16, 2023

Is there some specific issue for this test?

Contributor Author

bvanjoi Jun 16, 2023

No specific problem, it just tests/ui passed after refactoring but block by this comment. So I reduced it and added it into tests/ui after fixed.

btw, It is had some relative with #56593

petrochenkov added S-waiting-on-author and removed S-waiting-on-review labels

Contributor Author

bvanjoi commented Jun 16, 2023

it is not exactly what I wanted to do.

Apologize for the misunderstanding on this comment. I just think shaow_glob field to be a hack that should be removed, so I was splitting it into two separate fields.

In that case errors like ....

It appears that simply splitting non-glob-binding and glob-binding fields will not directly solve the problem.

we should have separate module.non_glob_resolutions and module.glob_resolutions

It appears that we may be able to delete the shadow_glob field in NameResolution once we have separated resolver.non_glob_resolution(module) and resolver.glob_resolutions(module). I will investigate this further soon.

Contributor Author

bvanjoi commented Jun 16, 2023 •

edited

Loading

I should clarify that I will not fix these comments temporarily because this refactoring does not appear to address the underlying issue.

Contributor

bors commented Jun 19, 2023

☔ The latest upstream changes (presumably #112774) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov mentioned this pull request

resolve: Always update globs importing from a module when bindings in that module change #113242

Closed

Contributor

bors commented Jul 6, 2023

☔ The latest upstream changes (presumably #113391) made this pull request unmergeable. Please resolve the merge conflicts.

Member

JohnCSimon commented Aug 27, 2023

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.

Contributor Author

bvanjoi commented Aug 27, 2023

No updates for now, I'll look at this after I fix the other issues, so close it for now.

bvanjoi closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author T-compiler