Auto merge of #112847 - bvanjoi:fix-112831, r=Nilstrieb

Revert #112758 and add test case

Fixes #112831.

Cannot unwrap `update_resolution` for `resolution.single_imports.remove(&Interned::new_unchecked(import));` because there is a relationship between the `Import` and `&NameBinding` in `NameResolution`. This issue caused by my unfamiliarity with the data structure and I apologize for it.

This PR had been reverted, and test case have been added.

r? `@Nilstrieb`
cc `@petrochenkov`

3 files changed, 86 insertions, 40 deletions

diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs
index 23ef9bf53a1..47d8e5993fd 100644
--- a/compiler/rustc_resolve/src/imports.rs
+++ b/compiler/rustc_resolve/src/imports.rs
@@ -304,23 +304,21 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         let res = binding.res();
         self.check_reserved_macro_name(key.ident, res);
         self.set_binding_parent_module(binding, module);
-
-        let mut resolution = self.resolution(module, key).borrow_mut();
-        let old_binding = resolution.binding();
-        let mut t = Ok(());
-        if let Some(old_binding) = resolution.binding {
-            if res == Res::Err && old_binding.res() != Res::Err {
-                // Do not override real bindings with `Res::Err`s from error recovery.
-            } else {
+        self.update_resolution(module, key, |this, resolution| {
+            if let Some(old_binding) = resolution.binding {
+                if res == Res::Err && old_binding.res() != Res::Err {
+                    // Do not override real bindings with `Res::Err`s from error recovery.
+                    return Ok(());
+                }
                 match (old_binding.is_glob_import(), binding.is_glob_import()) {
                     (true, true) => {
                         if res != old_binding.res() {
-                            resolution.binding = Some(self.ambiguity(
+                            resolution.binding = Some(this.ambiguity(
                                 AmbiguityKind::GlobVsGlob,
                                 old_binding,
                                 binding,
                             ));
-                        } else if !old_binding.vis.is_at_least(binding.vis, self.tcx) {
+                        } else if !old_binding.vis.is_at_least(binding.vis, this.tcx) {
                             // We are glob-importing the same item but with greater visibility.
                             resolution.binding = Some(binding);
                         }
@@ -332,7 +330,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             && key.ns == MacroNS
                             && nonglob_binding.expansion != LocalExpnId::ROOT
                         {
-                            resolution.binding = Some(self.ambiguity(
+                            resolution.binding = Some(this.ambiguity(
                                 AmbiguityKind::GlobVsExpanded,
                                 nonglob_binding,
                                 glob_binding,
@@ -344,12 +342,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         if let Some(old_binding) = resolution.shadowed_glob {
                             assert!(old_binding.is_glob_import());
                             if glob_binding.res() != old_binding.res() {
-                                resolution.shadowed_glob = Some(self.ambiguity(
+                                resolution.shadowed_glob = Some(this.ambiguity(
                                     AmbiguityKind::GlobVsGlob,
                                     old_binding,
                                     glob_binding,
                                 ));
-                            } else if !old_binding.vis.is_at_least(binding.vis, self.tcx) {
+                            } else if !old_binding.vis.is_at_least(binding.vis, this.tcx) {
                                 resolution.shadowed_glob = Some(glob_binding);
                             }
                         } else {
@@ -357,27 +355,53 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         }
                     }
                     (false, false) => {
-                        t = Err(old_binding);
+                        return Err(old_binding);
                     }
                 }
+            } else {
+                resolution.binding = Some(binding);
             }
-        } else {
-            resolution.binding = Some(binding);
-        };
 
+            Ok(())
+        })
+    }
+
+    fn ambiguity(
+        &self,
+        kind: AmbiguityKind,
+        primary_binding: &'a NameBinding<'a>,
+        secondary_binding: &'a NameBinding<'a>,
+    ) -> &'a NameBinding<'a> {
+        self.arenas.alloc_name_binding(NameBinding {
+            ambiguity: Some((secondary_binding, kind)),
+            ..primary_binding.clone()
+        })
+    }
+
+    // Use `f` to mutate the resolution of the name in the module.
+    // If the resolution becomes a success, define it in the module's glob importers.
+    fn update_resolution<T, F>(&mut self, module: Module<'a>, key: BindingKey, f: F) -> T
+    where
+        F: FnOnce(&mut Resolver<'a, 'tcx>, &mut NameResolution<'a>) -> T,
+    {
         // Ensure that `resolution` isn't borrowed when defining in the module's glob importers,
         // during which the resolution might end up getting re-defined via a glob cycle.
-        let (binding, t) = match resolution.binding() {
-            _ if old_binding.is_some() => return t,
-            None => return t,
-            Some(binding) => match old_binding {
-                Some(old_binding) if ptr::eq(old_binding, binding) => return t,
-                _ => (binding, t),
-            },
+        let (binding, t) = {
+            let resolution = &mut *self.resolution(module, key).borrow_mut();
+            let old_binding = resolution.binding();
+
+            let t = f(self, resolution);
+
+            match resolution.binding() {
+                _ if old_binding.is_some() => return t,
+                None => return t,
+                Some(binding) => match old_binding {
+                    Some(old_binding) if ptr::eq(old_binding, binding) => return t,
+                    _ => (binding, t),
+                },
+            }
         };
 
-        drop(resolution);
-
         // Define `binding` in `module`s glob importers.
         for import in module.glob_importers.borrow_mut().iter() {
             let mut ident = key.ident;
@@ -396,18 +420,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         t
     }
 
-    fn ambiguity(
-        &self,
-        kind: AmbiguityKind,
-        primary_binding: &'a NameBinding<'a>,
-        secondary_binding: &'a NameBinding<'a>,
-    ) -> &'a NameBinding<'a> {
-        self.arenas.alloc_name_binding(NameBinding {
-            ambiguity: Some((secondary_binding, kind)),
-            ..primary_binding.clone()
-        })
-    }
-
     // Define a dummy resolution containing a `Res::Err` as a placeholder for a failed
     // or indeterminate resolution, also mark such failed imports as used to avoid duplicate diagnostics.
     fn import_dummy_binding(&mut self, import: &'a Import<'a>, is_indeterminate: bool) {
@@ -757,8 +769,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                 .emit();
                         }
                         let key = BindingKey::new(target, ns);
-                        let mut resolution = this.resolution(parent, key).borrow_mut();
-                        resolution.single_imports.remove(&Interned::new_unchecked(import));
+                        this.update_resolution(parent, key, |_, resolution| {
+                            resolution.single_imports.remove(&Interned::new_unchecked(import));
+                        });
                     }
                 }
             }
diff --git a/tests/ui/resolve/auxiliary/issue-112831-aux.rs b/tests/ui/resolve/auxiliary/issue-112831-aux.rs
new file mode 100644
index 00000000000..df436fb9929
--- /dev/null
+++ b/tests/ui/resolve/auxiliary/issue-112831-aux.rs
@@ -0,0 +1,13 @@
+// force-host
+// no-prefer-dynamic
+
+#![crate_type = "proc-macro"]
+
+extern crate proc_macro;
+
+struct Zeroable;
+
+#[proc_macro_derive(Zeroable)]
+pub fn derive_zeroable(_: proc_macro::TokenStream) -> proc_macro::TokenStream {
+  proc_macro::TokenStream::default()
+}
diff --git a/tests/ui/resolve/issue-112831.rs b/tests/ui/resolve/issue-112831.rs
new file mode 100644
index 00000000000..ffd83ea8bc1
--- /dev/null
+++ b/tests/ui/resolve/issue-112831.rs
@@ -0,0 +1,20 @@
+// check-pass
+// aux-build:issue-112831-aux.rs
+
+mod zeroable {
+    pub trait Zeroable {}
+}
+
+use zeroable::*;
+
+mod pod {
+    use super::*;
+    pub trait Pod: Zeroable {}
+}
+
+use pod::*;
+
+extern crate issue_112831_aux;
+use issue_112831_aux::Zeroable;
+
+fn main() {}