fix: [std_instead_of_core] false positive for multi-imports

Author: bushrat011899

clippy_lints/src/std_instead_of_core.rs

@@ -2,14 +2,14 @@ use clippy_config::Conf;
 use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
 use clippy_utils::is_from_proc_macro;
 use clippy_utils::msrvs::Msrv;
-use rustc_errors::Applicability;
+use rustc_errors::{Applicability, MultiSpan};
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
 use rustc_hir::{Block, Body, HirId, Path, PathSegment, StabilityLevel, StableSince};
-use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::impl_lint_pass;
 use rustc_span::symbol::kw;
-use rustc_span::{Span, sym};
+use rustc_span::{Span, Symbol, sym};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -116,44 +116,34 @@ impl StdReexports {
 }
 
 #[derive(Debug)]
-enum LintPoint {
-    Available(Span, &'static Lint, &'static str, &'static str),
-    Conflict,
+struct LintPoint {
+    span: Span,
+    used_from: Symbol,
+    available_from_core: bool,
+    available_from_alloc: bool,
 }
 
 impl<'tcx> LateLintPass<'tcx> for StdReexports {
     fn check_path(&mut self, cx: &LateContext<'tcx>, path: &Path<'tcx>, _: HirId) {
         if let Res::Def(def_kind, def_id) = path.res
             && let Some(first_segment) = get_first_segment(path)
-            && is_stable(cx, def_id, self.msrv)
             && !path.span.in_external_macro(cx.sess().source_map())
             && !is_from_proc_macro(cx, &first_segment.ident)
             && !matches!(def_kind, DefKind::Macro(_))
             && let Some(last_segment) = path.segments.last()
             && let Res::Def(DefKind::Mod, crate_def_id) = first_segment.res
             && crate_def_id.is_crate_root()
         {
-            let (lint, used_mod, replace_with) = match first_segment.ident.name {
-                sym::std => match cx.tcx.crate_name(def_id.krate) {
-                    sym::core => (STD_INSTEAD_OF_CORE, "std", "core"),
-                    sym::alloc => (STD_INSTEAD_OF_ALLOC, "std", "alloc"),
-                    _ => {
-                        self.lint_if_finish(cx, first_segment.ident.span, LintPoint::Conflict);
-                        return;
-                    },
-                },
-                sym::alloc if cx.tcx.crate_name(def_id.krate) == sym::core => (ALLOC_INSTEAD_OF_CORE, "alloc", "core"),
-                _ => {
-                    self.lint_if_finish(cx, first_segment.ident.span, LintPoint::Conflict);
-                    return;
-                },
+            let is_stable = is_stable(cx, def_id, self.msrv);
+
+            let lint_point = LintPoint {
+                span: last_segment.ident.span,
+                used_from: first_segment.ident.name,
+                available_from_core: is_stable && is_available_from(cx, sym::core, path),
+                available_from_alloc: is_stable && is_available_from(cx, sym::alloc, path),
             };
 
-            self.lint_if_finish(
-                cx,
-                first_segment.ident.span,
-                LintPoint::Available(last_segment.ident.span, lint, used_mod, replace_with),
-            );
+            self.lint_if_finish(cx, first_segment.ident.span, lint_point);
         }
     }
 
@@ -171,27 +161,77 @@ impl<'tcx> LateLintPass<'tcx> for StdReexports {
 }
 
 fn emit_lints(cx: &LateContext<'_>, lint_points: Option<(Span, Vec<LintPoint>)>) {
-    let Some((krate_span, lint_points)) = lint_points else {
+    let Some((krate_span, mut lint_points)) = lint_points else {
         return;
     };
 
-    let mut lint: Option<(&'static Lint, &'static str, &'static str)> = None;
-    let mut has_conflict = false;
-    for lint_point in &lint_points {
-        match lint_point {
-            LintPoint::Available(_, l, used_mod, replace_with)
-                if lint.is_none_or(|(prev_l, ..)| l.name == prev_l.name) =>
-            {
-                lint = Some((l, used_mod, replace_with));
-            },
-            _ => {
-                has_conflict = true;
-                break;
-            },
+    // It's possible for multiple items to come from the same path.
+    // For example, `std::vec` refers to a macro and a module.
+    // In these cases, it's possible for them to have different availabilities.
+    // `std::vec` as a macro and a module are both defined in `alloc` and unavailable in `core`.
+    // Whereas, `std::env` is not available in `alloc`, and only the macro is available in `core`.
+    // Since we aren't checking which of the shadowed items the user needs, we take the intersection
+    // of these availabilities to ensure we don't provide the user a false positive.
+    lint_points.sort_by_key(|lint_point| lint_point.span);
+    lint_points.dedup_by(|a, b| {
+        if a.span == b.span {
+            b.available_from_alloc &= a.available_from_alloc;
+            b.available_from_core &= a.available_from_core;
+            true
+        } else {
+            false
+        }
+    });
+
+    let mut core_span = MultiSpan::new();
+    let mut alloc_span = MultiSpan::new();
+    let mut all_from_std = true;
+    let mut all_from_alloc = true;
+    let mut all_core = true;
+    let mut all_alloc = true;
+
+    for lint_point in lint_points {
+        all_from_std &= lint_point.used_from == sym::std;
+        all_from_alloc &= lint_point.used_from == sym::alloc;
+
+        if lint_point.available_from_core {
+            core_span.push_primary_span(lint_point.span);
+        } else {
+            all_core = false;
+        }
+
+        if !lint_point.available_from_alloc {
+            all_alloc = false;
+        }
+
+        if !lint_point.available_from_core && lint_point.available_from_alloc {
+            alloc_span.push_primary_span(lint_point.span);
         }
     }
 
-    if !has_conflict && let Some((lint, used_mod, replace_with)) = lint {
+    let mut helps = Vec::new();
+    let mut suggestions = Vec::new();
+
+    if all_from_std {
+        if all_core {
+            suggestions.push((STD_INSTEAD_OF_CORE, &sym::std, &sym::core));
+            helps.push((STD_INSTEAD_OF_ALLOC, &sym::std, &sym::alloc, alloc_span));
+        } else if all_alloc {
+            suggestions.push((STD_INSTEAD_OF_ALLOC, &sym::std, &sym::alloc));
+            helps.push((STD_INSTEAD_OF_CORE, &sym::std, &sym::core, core_span));
+        } else {
+            helps.push((STD_INSTEAD_OF_CORE, &sym::std, &sym::core, core_span));
+            helps.push((STD_INSTEAD_OF_ALLOC, &sym::std, &sym::alloc, alloc_span));
+        }
+    } else if all_from_alloc {
+        if all_core {
+            suggestions.push((ALLOC_INSTEAD_OF_CORE, &sym::alloc, &sym::core));
+        } else {
+            helps.push((ALLOC_INSTEAD_OF_CORE, &sym::alloc, &sym::core, core_span));
+        }
+    }
+
+    for (lint, used_mod, replace_with) in suggestions {
         span_lint_and_sugg(
             cx,
             lint,
@@ -201,24 +241,91 @@ fn emit_lints(cx: &LateContext<'_>, lint_points: Option<(Span, Vec<LintPoint>)>)
             (*replace_with).to_string(),
             Applicability::MachineApplicable,
         );
-        return;
     }
 
-    for lint_point in lint_points {
-        let LintPoint::Available(span, lint, used_mod, replace_with) = lint_point else {
-            continue;
-        };
-        span_lint_and_help(
-            cx,
-            lint,
-            span,
-            format!("used import from `{used_mod}` instead of `{replace_with}`"),
-            None,
-            format!("consider importing the item from `{replace_with}`"),
-        );
+    for (lint, used_mod, replace_with, span) in helps {
+        for &span in span.primary_spans() {
+            span_lint_and_help(
+                cx,
+                lint,
+                span,
+                format!("used import from `{used_mod}` instead of `{replace_with}`"),
+                None,
+                format!("consider importing the item from `{replace_with}`"),
+            );
+        }
     }
 }
 
+/// Checks if the provided `path` could have its crate replaced with `krate` and still be valid.
+fn is_available_from(cx: &LateContext<'_>, krate: Symbol, path: &Path<'_>) -> bool {
+    // Determine DefId for krate
+    let Some(krate) = cx
+        .tcx
+        .crates(())
+        .iter()
+        .find(|&&krate_num| cx.tcx.is_user_visible_dep(krate_num) && cx.tcx.crate_name(krate_num) == krate)
+        .map(|&krate| {
+            Res::Def(
+                DefKind::Mod,
+                DefId {
+                    index: rustc_span::def_id::CRATE_DEF_INDEX,
+                    krate,
+                },
+            )
+        })
+    else {
+        return false;
+    };
+
+    path.segments
+        .iter()
+        .enumerate()
+        .skip_while(|(_, segment)| {
+            // Skip `::crate` and `crate` segments
+            segment.ident.name == kw::PathRoot || segment.res.opt_def_id().is_some_and(DefId::is_crate_root)
+        })
+        .scan(krate, |cursor, (index, segment)| {
+            // Some, but not all, items from core and reexported by alloc.
+            // To find these cases, we must walk the alloc and core crates along the provided path
+            // to see if the desired item is accessible.
+            *cursor = cx
+                .tcx
+                .module_children({
+                    // TyCtxt::module_children will ICE if the provided DefId isn't "module like".
+                    cursor.module_like_def_id()?
+                })
+                .iter()
+                .filter(|child| {
+                    // Since this lint is only designed to work on `std`, `core`, and `alloc` from a
+                    // user's crate, only public items are of concern.
+                    child.vis.is_public()
+                })
+                .filter(|child| {
+                    // We search by name to allow cases like `core::str` and `std::str`.
+                    // `std::str` is actually a reexport of `alloc::str`, but `alloc::str`
+                    // reexports the contents of `core::str`, so many instances of `std::str`
+                    // can be replaced with `core::str`, even if they are distinct items.
+                    child.ident.name == segment.ident.name
+                })
+                .map(|child| child.res.map_id(|x| x))
+                .find(|child| {
+                    if index + 1 == path.segments.len() {
+                        // The last segment must resolve to the same item as the original path.
+                        *child == path.res
+                    } else {
+                        // We allow intermediate segments to resolve to different items,
+                        // but we only care about module-like items.
+                        child.module_like_def_id().is_some()
+                    }
+                })?;
+
+            Some(*cursor)
+        })
+        .last()
+        .is_some_and(|cursor| cursor == path.res)
+}
+
 /// Returns the first named segment of a [`Path`].
 ///
 /// If this is a global path (such as `::std::fmt::Debug`), then the segment after [`kw::PathRoot`]

tests/ui/std_instead_of_core.fixed

@@ -116,3 +116,23 @@ fn issue13158_msrv_1_80(_: &dyn std::error::Error) {}
 #[clippy::msrv = "1.81"]
 fn issue13158_msrv_1_81(_: &dyn core::error::Error) {}
 //~^ std_instead_of_core
+
+#[warn(clippy::std_instead_of_alloc)]
+fn pr17252() {
+    use core::result::{self, Iter, Result};
+    //~^ std_instead_of_core
+
+    use alloc::str::{self as _};
+    //~^ std_instead_of_alloc
+
+    use alloc::str::{self as _, FromStr as _};
+    //~^ std_instead_of_alloc
+    //~| std_instead_of_core
+
+    use alloc::fmt::{self, Write};
+    //~^ std_instead_of_alloc
+    //~| std_instead_of_core
+
+    // Macros aren't currently linted.
+    use std::writeln;
+}

tests/ui/std_instead_of_core.rs

@@ -116,3 +116,23 @@ fn issue13158_msrv_1_80(_: &dyn std::error::Error) {}
 #[clippy::msrv = "1.81"]
 fn issue13158_msrv_1_81(_: &dyn std::error::Error) {}
 //~^ std_instead_of_core
+
+#[warn(clippy::std_instead_of_alloc)]
+fn pr17252() {
+    use std::result::{self, Iter, Result};
+    //~^ std_instead_of_core
+
+    use std::str::{self as _};
+    //~^ std_instead_of_alloc
+
+    use std::str::{self as _, FromStr as _};
+    //~^ std_instead_of_alloc
+    //~| std_instead_of_core
+
+    use std::fmt::{self, Write};
+    //~^ std_instead_of_alloc
+    //~| std_instead_of_core
+
+    // Macros aren't currently linted.
+    use std::writeln;
+}

tests/ui/std_instead_of_core.stderr

@@ -109,5 +109,45 @@ error: used import from `std` instead of `core`
 LL | fn issue13158_msrv_1_81(_: &dyn std::error::Error) {}
    |                                 ^^^ help: consider importing the item from `core`: `core`
 
-error: aborting due to 17 previous errors
+error: used import from `std` instead of `core`
+  --> tests/ui/std_instead_of_core.rs:122:9
+   |
+LL |     use std::result::{self, Iter, Result};
+   |         ^^^ help: consider importing the item from `core`: `core`
+
+error: used import from `std` instead of `alloc`
+  --> tests/ui/std_instead_of_core.rs:125:9
+   |
+LL |     use std::str::{self as _};
+   |         ^^^ help: consider importing the item from `alloc`: `alloc`
+
+error: used import from `std` instead of `alloc`
+  --> tests/ui/std_instead_of_core.rs:128:9
+   |
+LL |     use std::str::{self as _, FromStr as _};
+   |         ^^^ help: consider importing the item from `alloc`: `alloc`
+
+error: used import from `std` instead of `core`
+  --> tests/ui/std_instead_of_core.rs:128:31
+   |
+LL |     use std::str::{self as _, FromStr as _};
+   |                               ^^^^^^^
+   |
+   = help: consider importing the item from `core`
+
+error: used import from `std` instead of `alloc`
+  --> tests/ui/std_instead_of_core.rs:132:9
+   |
+LL |     use std::fmt::{self, Write};
+   |         ^^^ help: consider importing the item from `alloc`: `alloc`
+
+error: used import from `std` instead of `core`
+  --> tests/ui/std_instead_of_core.rs:132:26
+   |
+LL |     use std::fmt::{self, Write};
+   |                          ^^^^^
+   |
+   = help: consider importing the item from `core`
+
+error: aborting due to 23 previous errors
 

tests/ui/std_instead_of_core_unfixable.rs

@@ -25,3 +25,26 @@ fn pr16964() {
         ffi::OsString,
     };
 }
+
+#[warn(clippy::alloc_instead_of_core)]
+#[rustfmt::skip]
+fn pr17252() {
+    extern crate alloc;
+
+    use alloc::str::{self as _, FromStr as _};
+    //~^ alloc_instead_of_core
+
+    use std::sync::{
+        Arc,
+        Mutex,
+        Weak,
+        atomic::{
+            AtomicPtr,
+            Ordering,
+        },
+    };
+    //~^^^^^^^^ std_instead_of_alloc
+    //~^^^^^^^ std_instead_of_alloc
+    //~^^^^^^ std_instead_of_core
+    //~^^^^^^ std_instead_of_core
+}