Rollup merge of #122373 - surechen:fix_121331, r=petrochenkov

Fix the conflict problem between the diagnostics fixes of lint `unnecessary_qualification` and `unused_imports` fixes #121331 For an `item` that triggers lint unnecessary_qualification, if the `use item` which imports this item is also trigger unused import, fixing the two lints at the same time may lead to the problem that the `item` cannot be found. This PR will avoid reporting lint unnecessary_qualification when conflict occurs. r? ``@petrochenkov``
2024-11-22 06:44:35 +00:00 · 2024-03-14 20:00:20 +01:00 · 2024-03-14 20:00:20 +01:00 · b200108bc5
commit b200108bc5
parent 54a5a49af0 1a81a941ad
17 changed files with 269 additions and 42 deletions
--- a/compiler/rustc_lint_defs/src/builtin.rs
+++ b/compiler/rustc_lint_defs/src/builtin.rs
@ -557,6 +557,7 @@ declare_lint! {
    /// fn main() {
    ///     use foo::bar;
    ///     foo::bar();
+    ///     bar();
    /// }
    /// ```
    ///
--- a/compiler/rustc_resolve/src/check_unused.rs
+++ b/compiler/rustc_resolve/src/check_unused.rs
@ -23,18 +23,19 @@
 //  - `check_unused` finally emits the diagnostics based on the data generated
 //    in the last step

-use crate::imports::ImportKind;
+use crate::imports::{Import, ImportKind};
 use crate::module_to_string;
 use crate::Resolver;

-use crate::NameBindingKind;
+use crate::{LexicalScopeBinding, NameBindingKind};
 use rustc_ast as ast;
 use rustc_ast::visit::{self, Visitor};
 use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
 use rustc_data_structures::unord::UnordSet;
 use rustc_errors::{pluralize, MultiSpan};
 use rustc_hir::def::{DefKind, Res};
-use rustc_session::lint::builtin::{MACRO_USE_EXTERN_CRATE, UNUSED_EXTERN_CRATES, UNUSED_IMPORTS};
+use rustc_session::lint::builtin::{MACRO_USE_EXTERN_CRATE, UNUSED_EXTERN_CRATES};
+use rustc_session::lint::builtin::{UNUSED_IMPORTS, UNUSED_QUALIFICATIONS};
 use rustc_session::lint::BuiltinLintDiag;
 use rustc_span::symbol::{kw, Ident};
 use rustc_span::{Span, DUMMY_SP};
@ -514,8 +515,59 @@ impl Resolver<'_, '_> {
            }
        }

+        let mut redundant_imports = UnordSet::default();
        for import in check_redundant_imports {
-            self.check_for_redundant_imports(import);
+            if self.check_for_redundant_imports(import)
+                && let Some(id) = import.id()
+            {
+                redundant_imports.insert(id);
+            }
+        }
+
+        // The lint fixes for unused_import and unnecessary_qualification may conflict.
+        // Deleting both unused imports and unnecessary segments of an item may result
+        // in the item not being found.
+        for unn_qua in &self.potentially_unnecessary_qualifications {
+            if let LexicalScopeBinding::Item(name_binding) = unn_qua.binding
+                && let NameBindingKind::Import { import, .. } = name_binding.kind
+                && (is_unused_import(import, &unused_imports)
+                    || is_redundant_import(import, &redundant_imports))
+            {
+                continue;
+            }
+
+            self.lint_buffer.buffer_lint_with_diagnostic(
+                UNUSED_QUALIFICATIONS,
+                unn_qua.node_id,
+                unn_qua.path_span,
+                "unnecessary qualification",
+                BuiltinLintDiag::UnusedQualifications { removal_span: unn_qua.removal_span },
+            );
+        }
+
+        fn is_redundant_import(
+            import: Import<'_>,
+            redundant_imports: &UnordSet<ast::NodeId>,
+        ) -> bool {
+            if let Some(id) = import.id()
+                && redundant_imports.contains(&id)
+            {
+                return true;
+            }
+            false
+        }
+
+        fn is_unused_import(
+            import: Import<'_>,
+            unused_imports: &FxIndexMap<ast::NodeId, UnusedImport>,
+        ) -> bool {
+            if let Some(unused_import) = unused_imports.get(&import.root_id)
+                && let Some(id) = import.id()
+                && unused_import.unused.contains(&id)
+            {
+                return true;
+            }
+            false
        }
    }
 }
--- a/compiler/rustc_resolve/src/imports.rs
+++ b/compiler/rustc_resolve/src/imports.rs
@ -1306,7 +1306,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
        None
    }

-    pub(crate) fn check_for_redundant_imports(&mut self, import: Import<'a>) {
+    pub(crate) fn check_for_redundant_imports(&mut self, import: Import<'a>) -> bool {
        // This function is only called for single imports.
        let ImportKind::Single {
            source, target, ref source_bindings, ref target_bindings, id, ..
@ -1317,12 +1317,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {

        // Skip if the import is of the form `use source as target` and source != target.
        if source != target {
-            return;
+            return false;
        }

        // Skip if the import was produced by a macro.
        if import.parent_scope.expansion != LocalExpnId::ROOT {
-            return;
+            return false;
        }

        // Skip if we are inside a named module (in contrast to an anonymous
@ -1332,7 +1332,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
        if import.used.get() == Some(Used::Other)
            || self.effective_visibilities.is_exported(self.local_def_id(id))
        {
-            return;
+            return false;
        }

        let mut is_redundant = true;
@ -1375,7 +1375,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                format!("the item `{source}` is imported redundantly"),
                BuiltinLintDiag::RedundantImport(redundant_spans, source),
            );
+            return true;
        }
+
+        false
    }

    fn resolve_glob_import(&mut self, import: Import<'a>) {
--- a/compiler/rustc_resolve/src/late.rs
+++ b/compiler/rustc_resolve/src/late.rs
@ -580,6 +580,15 @@ impl MaybeExported<'_> {
    }
 }

+/// Used for recording UnnecessaryQualification.
+#[derive(Debug)]
+pub(crate) struct UnnecessaryQualification<'a> {
+    pub binding: LexicalScopeBinding<'a>,
+    pub node_id: NodeId,
+    pub path_span: Span,
+    pub removal_span: Span,
+}
+
 #[derive(Default)]
 struct DiagMetadata<'ast> {
    /// The current trait's associated items' ident, used for diagnostic suggestions.
@ -4654,20 +4663,16 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
            let ns = if i + 1 == path.len() { ns } else { TypeNS };
            let res = self.r.partial_res_map.get(&seg.id?)?.full_res()?;
            let binding = self.resolve_ident_in_lexical_scope(seg.ident, ns, None, None)?;
-
-            (res == binding.res()).then_some(seg)
+            (res == binding.res()).then_some((seg, binding))
        });

-        if let Some(unqualified) = unqualified {
-            self.r.lint_buffer.buffer_lint_with_diagnostic(
-                lint::builtin::UNUSED_QUALIFICATIONS,
-                finalize.node_id,
-                finalize.path_span,
-                "unnecessary qualification",
-                lint::BuiltinLintDiag::UnusedQualifications {
-                    removal_span: path[0].ident.span.until(unqualified.ident.span),
-                },
-            );
+        if let Some((seg, binding)) = unqualified {
+            self.r.potentially_unnecessary_qualifications.push(UnnecessaryQualification {
+                binding,
+                node_id: finalize.node_id,
+                path_span: finalize.path_span,
+                removal_span: path[0].ident.span.until(seg.ident.span),
+            });
        }
    }
 }
--- a/compiler/rustc_resolve/src/lib.rs
+++ b/compiler/rustc_resolve/src/lib.rs
@ -68,7 +68,7 @@ use std::fmt;

 use diagnostics::{ImportSuggestion, LabelSuggestion, Suggestion};
 use imports::{Import, ImportData, ImportKind, NameResolution};
-use late::{HasGenericParams, PathSource, PatternSource};
+use late::{HasGenericParams, PathSource, PatternSource, UnnecessaryQualification};
 use macros::{MacroRulesBinding, MacroRulesScope, MacroRulesScopeRef};

 use crate::effective_visibilities::EffectiveVisibilitiesVisitor;
@ -372,7 +372,7 @@ impl<'a> From<&'a ast::PathSegment> for Segment {
 /// This refers to the thing referred by a name. The difference between `Res` and `Item` is that
 /// items are visible in their whole block, while `Res`es only from the place they are defined
 /// forward.
-#[derive(Debug)]
+#[derive(Debug, Copy, Clone)]
 enum LexicalScopeBinding<'a> {
    Item(NameBinding<'a>),
    Res(Res),
@ -1105,6 +1105,8 @@ pub struct Resolver<'a, 'tcx> {

    potentially_unused_imports: Vec<Import<'a>>,

+    potentially_unnecessary_qualifications: Vec<UnnecessaryQualification<'a>>,
+
    /// Table for mapping struct IDs into struct constructor IDs,
    /// it's not used during normal resolution, only for better error reporting.
    /// Also includes of list of each fields visibility
@ -1464,6 +1466,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
            local_macro_def_scopes: FxHashMap::default(),
            name_already_seen: FxHashMap::default(),
            potentially_unused_imports: Vec::new(),
+            potentially_unnecessary_qualifications: Default::default(),
            struct_constructors: Default::default(),
            unused_macros: Default::default(),
            unused_macro_rules: Default::default(),
--- a/tests/ui/lint/lint-qualification.fixed
+++ b/tests/ui/lint/lint-qualification.fixed
@ -1,5 +1,6 @@
 //@ run-rustfix
 #![deny(unused_qualifications)]
+#![deny(unused_imports)]
 #![allow(deprecated, dead_code)]

 mod foo {
@ -21,8 +22,10 @@ fn main() {
    //~^ ERROR: unnecessary qualification
    //~| ERROR: unnecessary qualification

-    use std::fmt;
-    let _: fmt::Result = Ok(()); //~ ERROR: unnecessary qualification
+    
+    //~^ ERROR: unused import: `std::fmt`
+    let _: std::fmt::Result = Ok(());
+    // don't report unnecessary qualification because fix(#122373) for issue #121331

    let _ = <bool as Default>::default(); // issue #121999
    //~^ ERROR: unnecessary qualification
--- a/tests/ui/lint/lint-qualification.rs
+++ b/tests/ui/lint/lint-qualification.rs
@ -1,5 +1,6 @@
 //@ run-rustfix
 #![deny(unused_qualifications)]
+#![deny(unused_imports)]
 #![allow(deprecated, dead_code)]

 mod foo {
@ -22,7 +23,9 @@ fn main() {
    //~| ERROR: unnecessary qualification

    use std::fmt;
-    let _: std::fmt::Result = Ok(()); //~ ERROR: unnecessary qualification
+    //~^ ERROR: unused import: `std::fmt`
+    let _: std::fmt::Result = Ok(());
+    // don't report unnecessary qualification because fix(#122373) for issue #121331

    let _ = <bool as ::std::default::Default>::default(); // issue #121999
    //~^ ERROR: unnecessary qualification
--- a/tests/ui/lint/lint-qualification.stderr
+++ b/tests/ui/lint/lint-qualification.stderr
@ -1,5 +1,5 @@
 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:11:5
+  --> $DIR/lint-qualification.rs:12:5
   |
 LL |     foo::bar();
   |     ^^^^^^^^
@ -16,7 +16,7 @@ LL +     bar();
   |

 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:12:5
+  --> $DIR/lint-qualification.rs:13:5
   |
 LL |     crate::foo::bar();
   |     ^^^^^^^^^^^^^^^
@ -28,7 +28,7 @@ LL +     bar();
   |

 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:17:13
+  --> $DIR/lint-qualification.rs:18:13
   |
 LL |     let _ = std::string::String::new();
   |             ^^^^^^^^^^^^^^^^^^^^^^^^
@ -40,7 +40,7 @@ LL +     let _ = String::new();
   |

 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:18:13
+  --> $DIR/lint-qualification.rs:19:13
   |
 LL |     let _ = ::std::env::current_dir();
   |             ^^^^^^^^^^^^^^^^^^^^^^^
@ -52,7 +52,7 @@ LL +     let _ = std::env::current_dir();
   |

 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:20:12
+  --> $DIR/lint-qualification.rs:21:12
   |
 LL |     let _: std::vec::Vec<String> = std::vec::Vec::<String>::new();
   |            ^^^^^^^^^^^^^^^^^^^^^
@ -64,7 +64,7 @@ LL +     let _: Vec<String> = std::vec::Vec::<String>::new();
   |

 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:20:36
+  --> $DIR/lint-qualification.rs:21:36
   |
 LL |     let _: std::vec::Vec<String> = std::vec::Vec::<String>::new();
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -75,20 +75,20 @@ LL -     let _: std::vec::Vec<String> = std::vec::Vec::<String>::new();
 LL +     let _: std::vec::Vec<String> = Vec::<String>::new();
   |

-error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:25:12
+error: unused import: `std::fmt`
+  --> $DIR/lint-qualification.rs:25:9
   |
-LL |     let _: std::fmt::Result = Ok(());
-   |            ^^^^^^^^^^^^^^^^
+LL |     use std::fmt;
+   |         ^^^^^^^^
   |
-help: remove the unnecessary path segments
-   |
-LL -     let _: std::fmt::Result = Ok(());
-LL +     let _: fmt::Result = Ok(());
+note: the lint level is defined here
+  --> $DIR/lint-qualification.rs:3:9
   |
+LL | #![deny(unused_imports)]
+   |         ^^^^^^^^^^^^^^

 error: unnecessary qualification
-  --> $DIR/lint-qualification.rs:27:13
+  --> $DIR/lint-qualification.rs:30:13
   |
 LL |     let _ = <bool as ::std::default::Default>::default(); // issue #121999
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
--- a/tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.fixed
+++ b/tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.fixed
@ -0,0 +1,50 @@
+//@ run-rustfix
+//@ edition:2021
+#![deny(unused_qualifications)]
+#![deny(unused_imports)]
+#![feature(coroutines, coroutine_trait)]
+
+use std::ops::{
+    Coroutine,
+    CoroutineState::{self},
+    //~^ ERROR unused import: `*`
+};
+use std::pin::Pin;
+
+#[allow(dead_code)]
+fn finish<T>(mut amt: usize, mut t: T) -> T::Return
+    where T: Coroutine<(), Yield = ()> + Unpin,
+{
+    loop {
+        match Pin::new(&mut t).resume(()) {
+            CoroutineState::Yielded(()) => amt = amt.checked_sub(1).unwrap(),
+            CoroutineState::Complete(ret) => {
+                assert_eq!(amt, 0);
+                return ret
+            }
+        }
+    }
+}
+
+
+mod foo {
+    pub fn bar() {}
+}
+
+pub fn main() {
+
+    use foo::bar;
+    bar();
+    //~^ ERROR unnecessary qualification
+    bar();
+
+    // The item `use std::string::String` is imported redundantly.
+    // Suppress `unused_imports` reporting, otherwise the fixed file will report an error
+    #[allow(unused_imports)]
+    use std::string::String;
+    let s = String::new();
+    let y = std::string::String::new();
+    // unnecessary qualification
+    println!("{} {}", s, y);
+
+}
--- a/tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.rs
+++ b/tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.rs
@ -0,0 +1,50 @@
+//@ run-rustfix
+//@ edition:2021
+#![deny(unused_qualifications)]
+#![deny(unused_imports)]
+#![feature(coroutines, coroutine_trait)]
+
+use std::ops::{
+    Coroutine,
+    CoroutineState::{self, *},
+    //~^ ERROR unused import: `*`
+};
+use std::pin::Pin;
+
+#[allow(dead_code)]
+fn finish<T>(mut amt: usize, mut t: T) -> T::Return
+    where T: Coroutine<(), Yield = ()> + Unpin,
+{
+    loop {
+        match Pin::new(&mut t).resume(()) {
+            CoroutineState::Yielded(()) => amt = amt.checked_sub(1).unwrap(),
+            CoroutineState::Complete(ret) => {
+                assert_eq!(amt, 0);
+                return ret
+            }
+        }
+    }
+}
+
+
+mod foo {
+    pub fn bar() {}
+}
+
+pub fn main() {
+
+    use foo::bar;
+    foo::bar();
+    //~^ ERROR unnecessary qualification
+    bar();
+
+    // The item `use std::string::String` is imported redundantly.
+    // Suppress `unused_imports` reporting, otherwise the fixed file will report an error
+    #[allow(unused_imports)]
+    use std::string::String;
+    let s = String::new();
+    let y = std::string::String::new();
+    // unnecessary qualification
+    println!("{} {}", s, y);
+
+}
--- a/tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.stderr
+++ b/tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.stderr
@ -0,0 +1,31 @@
+error: unused import: `*`
+  --> $DIR/lint-unnecessary-qualification-issue-121331.rs:9:28
+   |
+LL |     CoroutineState::{self, *},
+   |                            ^
+   |
+note: the lint level is defined here
+  --> $DIR/lint-unnecessary-qualification-issue-121331.rs:4:9
+   |
+LL | #![deny(unused_imports)]
+   |         ^^^^^^^^^^^^^^
+
+error: unnecessary qualification
+  --> $DIR/lint-unnecessary-qualification-issue-121331.rs:37:5
+   |
+LL |     foo::bar();
+   |     ^^^^^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/lint-unnecessary-qualification-issue-121331.rs:3:9
+   |
+LL | #![deny(unused_qualifications)]
+   |         ^^^^^^^^^^^^^^^^^^^^^
+help: remove the unnecessary path segments
+   |
+LL -     foo::bar();
+LL +     bar();
+   |
+
+error: aborting due to 2 previous errors
+
--- a/tests/ui/resolve/issue-113808-invalid-unused-qualifications-suggestion.fixed
+++ b/tests/ui/resolve/issue-113808-invalid-unused-qualifications-suggestion.fixed
@ -18,6 +18,16 @@ impl Index<str> for A {
    }
 }

+// This is used to make `use std::ops::Index;` not unused_import.
+// details in fix(#122373) for issue #121331
+pub struct C;
+impl Index<str> for C {
+    type Output = ();
+    fn index(&self, _: str) -> &Self::Output {
+        &()
+    }
+}
+
 mod inner {
    pub trait Trait<T> {}
 }
@ -29,4 +39,5 @@ use inner::Trait;
 impl Trait<u8> for () {}
 //~^ ERROR unnecessary qualification

+impl Trait<A> for A {}
 fn main() {}
--- a/tests/ui/resolve/issue-113808-invalid-unused-qualifications-suggestion.rs
+++ b/tests/ui/resolve/issue-113808-invalid-unused-qualifications-suggestion.rs
@ -18,6 +18,16 @@ impl ops::Index<str> for A {
    }
 }

+// This is used to make `use std::ops::Index;` not unused_import.
+// details in fix(#122373) for issue #121331
+pub struct C;
+impl Index<str> for C {
+    type Output = ();
+    fn index(&self, _: str) -> &Self::Output {
+        &()
+    }
+}
+
 mod inner {
    pub trait Trait<T> {}
 }
@ -29,4 +39,5 @@ use inner::Trait;
 impl inner::Trait<u8> for () {}
 //~^ ERROR unnecessary qualification

+impl Trait<A> for A {}
 fn main() {}
--- a/tests/ui/resolve/issue-113808-invalid-unused-qualifications-suggestion.stderr
+++ b/tests/ui/resolve/issue-113808-invalid-unused-qualifications-suggestion.stderr
@ -16,7 +16,7 @@ LL + impl Index<str> for A {
   |

 error: unnecessary qualification
-  --> $DIR/issue-113808-invalid-unused-qualifications-suggestion.rs:29:6
+  --> $DIR/issue-113808-invalid-unused-qualifications-suggestion.rs:39:6
   |
 LL | impl inner::Trait<u8> for () {}
   |      ^^^^^^^^^^^^^^^^
--- a/tests/ui/resolve/unused-qualifications-suggestion.fixed
+++ b/tests/ui/resolve/unused-qualifications-suggestion.fixed
@ -16,8 +16,10 @@ fn main() {
    use foo::bar;
    bar();
    //~^ ERROR unnecessary qualification
+    bar();

    use baz::qux::quux;
    quux();
    //~^ ERROR unnecessary qualification
+    quux();
 }
--- a/tests/ui/resolve/unused-qualifications-suggestion.rs
+++ b/tests/ui/resolve/unused-qualifications-suggestion.rs
@ -16,8 +16,10 @@ fn main() {
    use foo::bar;
    foo::bar();
    //~^ ERROR unnecessary qualification
+    bar();

    use baz::qux::quux;
    baz::qux::quux();
    //~^ ERROR unnecessary qualification
+    quux();
 }
--- a/tests/ui/resolve/unused-qualifications-suggestion.stderr
+++ b/tests/ui/resolve/unused-qualifications-suggestion.stderr
@ -16,7 +16,7 @@ LL +     bar();
   |

 error: unnecessary qualification
-  --> $DIR/unused-qualifications-suggestion.rs:21:5
+  --> $DIR/unused-qualifications-suggestion.rs:22:5
   |
 LL |     baz::qux::quux();
   |     ^^^^^^^^^^^^^^