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, DiagMessage};
 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_session::impl_lint_pass;
 use rustc_span::symbol::kw;
-use rustc_span::{Span, sym};
+use rustc_span::{Ident, Span, Symbol, sym};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -93,129 +93,181 @@ impl_lint_pass!(StdReexports => [
 ]);
 
 pub struct StdReexports {
-    lint_points: Option<(Span, Vec<LintPoint>)>,
+    root: Span,
+    lint_points: Vec<LintPoint>,
+    has_conflicts: bool,
     msrv: Msrv,
 }
 
 impl StdReexports {
     pub fn new(conf: &'static Conf) -> Self {
         Self {
-            lint_points: Option::default(),
+            root: Span::default(),
+            lint_points: Vec::new(),
+            has_conflicts: false,
             msrv: conf.msrv,
         }
     }
 
-    fn lint_if_finish(&mut self, cx: &LateContext<'_>, krate: Span, lint_point: LintPoint) {
-        match &mut self.lint_points {
-            Some((prev_krate, prev_lints)) if prev_krate.overlaps(krate) => {
-                prev_lints.push(lint_point);
-            },
-            _ => emit_lints(cx, self.lint_points.replace((krate, vec![lint_point]))),
+    /// If all the contained [`LintPoint`]s refer to the same lint _and_ there
+    /// aren't any conflicts (e.g, multi-imports which aren't covered by this lint),
+    /// combine them into a single lint suitable for a suggestion.
+    fn combine_lints(&self) -> Option<LintPoint> {
+        if !self.has_conflicts
+            && let Some(lint_point) = self
+                .lint_points
+                .iter()
+                .copied()
+                .map(Some)
+                .reduce(|prev, next| prev?.combine(next?))
+                .flatten()
+        {
+            Some(LintPoint {
+                span: self.root,
+                ..lint_point
+            })
+        } else {
+            None
         }
     }
-}
 
-#[derive(Debug)]
-enum LintPoint {
-    Available(Span, &'static Lint, &'static str, &'static str),
-    Conflict,
+    /// Emit any currently stored [`LintPoint`]s and reset internal storage.
+    fn emit(&mut self, cx: &LateContext<'_>) {
+        if let Some(lint_point) = self.combine_lints() {
+            span_lint_and_sugg(
+                cx,
+                lint_point.lint,
+                lint_point.span,
+                lint_point.message(),
+                lint_point.help(),
+                lint_point.replace_with.to_string(),
+                Applicability::MachineApplicable,
+            );
+        } else {
+            for lint_point in &self.lint_points {
+                span_lint_and_help(
+                    cx,
+                    lint_point.lint,
+                    lint_point.span,
+                    lint_point.message(),
+                    None,
+                    lint_point.help(),
+                );
+            }
+        }
+
+        self.root = Span::default();
+        self.lint_points.clear();
+        self.has_conflicts = false;
+    }
 }
 
 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;
-                },
-            };
+        // Path must resolve to a definition where the first segment is at least a module.
+        let (
+            Res::Def(def_kind, def_id),
+            Some(&PathSegment {
+                ident: ref first_segment @ Ident { name: used, .. },
+                res: Res::Def(DefKind::Mod, crate_def_id),
+                ..
+            }),
+            Some(PathSegment {
+                ident: last_segment, ..
+            }),
+        ) = (path.res, get_first_segment(path), path.segments.last())
+        else {
+            return;
+        };
 
-            self.lint_if_finish(
-                cx,
-                first_segment.ident.span,
-                LintPoint::Available(last_segment.ident.span, lint, used_mod, replace_with),
-            );
+        // Paths within or from macros do not need to be linted.
+        if path.span.in_external_macro(cx.sess().source_map()) || is_from_proc_macro(cx, first_segment) {
+            return;
+        }
+
+        // If the first segment of the path is not a crate (std, core, alloc, etc.), then
+        // we cannot determine if it should be replaced.
+        if !crate_def_id.is_crate_root() {
+            return;
+        }
+
+        // Exclude paths that refer to macros.
+        if matches!(def_kind, DefKind::Macro(_)) {
+            return;
+        }
+
+        // Determine if this path may be a candidate for linting.
+        let potential_lint_point = LintPoint::new(last_segment.span, used, cx.tcx.crate_name(def_id.krate));
+
+        // Emit existing lint points if this lint refers to a new path.
+        if !self.root.overlaps(first_segment.span) {
+            self.emit(cx);
+            self.root = first_segment.span;
+        }
+
+        match potential_lint_point {
+            Some(lint_point) if is_stable(cx, def_id, self.msrv) => {
+                self.lint_points.push(lint_point);
+            },
+            _ => {
+                self.has_conflicts = true;
+            },
         }
     }
 
     fn check_block_post(&mut self, cx: &LateContext<'tcx>, _: &Block<'tcx>) {
-        emit_lints(cx, self.lint_points.take());
+        self.emit(cx);
     }
 
     fn check_body_post(&mut self, cx: &LateContext<'tcx>, _: &Body<'tcx>) {
-        emit_lints(cx, self.lint_points.take());
+        self.emit(cx);
     }
 
     fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
-        emit_lints(cx, self.lint_points.take());
+        self.emit(cx);
     }
 }
 
-fn emit_lints(cx: &LateContext<'_>, lint_points: Option<(Span, Vec<LintPoint>)>) {
-    let Some((krate_span, lint_points)) = lint_points else {
-        return;
-    };
+#[derive(Debug, Clone, Copy)]
+struct LintPoint {
+    span: Span,
+    lint: &'static Lint,
+    used: Symbol,
+    replace_with: Symbol,
+}
 
-    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;
-            },
-        }
-    }
+impl LintPoint {
+    fn new(span: Span, used: Symbol, replace_with: Symbol) -> Option<Self> {
+        let lint = match (used, replace_with) {
+            (sym::std, sym::core) => Some(STD_INSTEAD_OF_CORE),
+            (sym::std, sym::alloc) => Some(STD_INSTEAD_OF_ALLOC),
+            (sym::alloc, sym::core) => Some(ALLOC_INSTEAD_OF_CORE),
+            _ => None,
+        }?;
 
-    if !has_conflict && let Some((lint, used_mod, replace_with)) = lint {
-        span_lint_and_sugg(
-            cx,
+        Some(Self {
+            span,
             lint,
-            krate_span,
-            format!("used import from `{used_mod}` instead of `{replace_with}`"),
-            format!("consider importing the item from `{replace_with}`"),
-            (*replace_with).to_string(),
-            Applicability::MachineApplicable,
-        );
-        return;
+            used,
+            replace_with,
+        })
     }
 
-    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}`"),
-        );
+    /// Combine two [`LintPoint`]s if they cover the same lint.
+    /// Does not check if they refer to a meaningfully similar span.
+    fn combine(self, other: Self) -> Option<Self> {
+        core::ptr::eq(self.lint, other.lint).then_some(Self {
+            span: Span::default(),
+            ..self
+        })
+    }
+
+    fn message(&self) -> impl Into<DiagMessage> {
+        format!("used import from `{}` instead of `{}`", self.used, self.replace_with)
+    }
+
+    fn help(&self) -> impl Into<DiagMessage> {
+        format!("consider importing the item from `{}`", self.replace_with)
     }
 }
 
@@ -236,29 +288,30 @@ fn get_first_segment<'tcx>(path: &Path<'tcx>) -> Option<&'tcx PathSegment<'tcx>>
 /// or now stable moves that were once unstable.
 ///
 /// Does not catch individually moved items
-fn is_stable(cx: &LateContext<'_>, mut def_id: DefId, msrv: Msrv) -> bool {
-    loop {
-        if let Some(stability) = cx.tcx.lookup_stability(def_id) {
-            match stability.level {
-                // Workaround for items from `core::intrinsics` with a stable export in a different module.
-                // Not that we ignore the `since` field as we are already accessing the item in question.
-                StabilityLevel::Stable {
-                    allowed_through_unstable_modules: Some(_),
-                    ..
-                } => return true,
-                StabilityLevel::Stable { since, .. } => match since {
-                    StableSince::Version(v) if !msrv.meets(cx, v) => return false,
-                    StableSince::Current if msrv.current(cx).is_none() => return false,
-                    StableSince::Err(_) => return false,
-                    StableSince::Version(_) | StableSince::Current => {},
-                },
-                StabilityLevel::Unstable { .. } => return false,
-            }
-        }
+fn is_stable(cx: &LateContext<'_>, def_id: DefId, msrv: Msrv) -> bool {
+    iter_parents(cx, def_id)
+        .find_map(|def_id| match cx.tcx.lookup_stability(def_id)?.level {
+            StabilityLevel::Unstable { .. } => Some(false),
+            StabilityLevel::Stable {
+                since,
+                allowed_through_unstable_modules,
+            } => {
+                let stable = match since {
+                    StableSince::Version(v) => msrv.meets(cx, v),
+                    StableSince::Current => msrv.current(cx).is_none(),
+                    StableSince::Err(_) => false,
+                };
 
-        match cx.tcx.opt_parent(def_id) {
-            Some(parent) => def_id = parent,
-            None => return true,
-        }
-    }
+                (!stable || allowed_through_unstable_modules.is_some()).then_some(stable)
+            },
+        })
+        .unwrap_or(true)
+}
+
+/// Iterate (inclusively) over all ancestors of the provided [`DefId`].
+fn iter_parents(cx: &LateContext<'_>, mut def_id: DefId) -> impl Iterator<Item = DefId> {
+    core::iter::once(def_id).chain(core::iter::from_fn(move || {
+        def_id = cx.tcx.opt_parent(def_id)?;
+        Some(def_id)
+    }))
 }

tests/ui/std_instead_of_core_unfixable.rs

@@ -13,6 +13,14 @@ fn issue15143() {
     use std::{error::Error, vec::Vec, fs::File};
     //~^ std_instead_of_core
     //~| std_instead_of_alloc
+
+    use std::sync::{
+        Arc,
+        //~^ std_instead_of_alloc
+        Mutex,
+        Weak,
+        //~^ std_instead_of_alloc
+    };
 }
 
 #[rustfmt::skip]

tests/ui/std_instead_of_core_unfixable.stderr

@@ -27,20 +27,36 @@ LL |     use std::{error::Error, vec::Vec, fs::File};
    = help: to override `-D warnings` add `#[allow(clippy::std_instead_of_alloc)]`
 
 error: used import from `std` instead of `alloc`
-  --> tests/ui/std_instead_of_core_unfixable.rs:21:17
+  --> tests/ui/std_instead_of_core_unfixable.rs:18:9
+   |
+LL |         Arc,
+   |         ^^^
+   |
+   = help: consider importing the item from `alloc`
+
+error: used import from `std` instead of `alloc`
+  --> tests/ui/std_instead_of_core_unfixable.rs:21:9
+   |
+LL |         Weak,
+   |         ^^^^
+   |
+   = help: consider importing the item from `alloc`
+
+error: used import from `std` instead of `alloc`
+  --> tests/ui/std_instead_of_core_unfixable.rs:29:17
    |
 LL |         borrow::Cow,
    |                 ^^^
    |
    = help: consider importing the item from `alloc`
 
 error: used import from `std` instead of `alloc`
-  --> tests/ui/std_instead_of_core_unfixable.rs:23:22
+  --> tests/ui/std_instead_of_core_unfixable.rs:31:22
    |
 LL |         collections::BTreeSet,
    |                      ^^^^^^^^
    |
    = help: consider importing the item from `alloc`
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors