Rewrite std_instead_of_core

[bushrat011899]

View all comments

Objective

• Fixes std_instead_of_core does not detect uses of non-fully qualified items #11159
• Fixes std_instead_of_core creates an invalid fix on stacked imports #12468
• Fixes std_instead_of_core/etc. miss macros #17260
• Alternative/followup to fix [std_instead_of_core]: false positive for multi-imports #17252

Solution

This PR is a major rewrite of std_instead_of_core which reduces allocations and hardens against possible changes to path segment resolution information, such as what is proposed in rust-lang/rust#159760. Instead of relying exclusively on check_path to collect possible lint points, we primarily rely on check_item for use statements. check_path is still used to catch instances such as std::boxed::Box::new(123), but such cases will contain resolution information for the full path, as the path is not shared across multiple items.

Lintcheck Interpretation

alloc_instead_of_core

• Vast majority of additions come from linting cases like fmt::Result when a crate does use std::fmt;. This is intended.

std_instead_of_alloc

• Additions and changes come from sync::{Arc, Mutex} and catching crate renames (extern crate std as alloc, etc.)
• Removal comes from crate renaming.

std_instead_of_core

• Some additions come from non-FQN paths such as hash::Hash.
• Further additions come from splitting incorrectly merged suggestion lints into multiple help emissions.
• Some removals come from catching crate renames (extern crate core as std, etc.)
• Other removals come from improper merges, such as std::fmt::{self, Debug} (while Debug is defined in core, std::fmt::self is alloc::fmt, so this cannot be a merged suggestion.
• Majority of changes come from fixing improper merges.
• Some changes come from macros having the first or the last call site linted to the user swapped.

---

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [std_instead_of_core], [std_instead_of_alloc], [alloc_instead_of_core]: fix false positive suggestions for multi-imports
changelog: [std_instead_of_core], [std_instead_of_alloc], [alloc_instead_of_core]: consider crate renames
changelog: [std_instead_of_core], [std_instead_of_alloc], [alloc_instead_of_core]: include relative paths
changelog: [std_instead_of_core], [std_instead_of_alloc], [alloc_instead_of_core]: include macros

[github-actions]

Lintcheck changes for f26808e

Lint	Added	Removed	Changed
clippy::alloc_instead_of_core	2465	0	0
clippy::std_instead_of_alloc	10	1	13
clippy::std_instead_of_core	116	12	6

This comment will be updated if you push new changes

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from Jarcho, llogiq, samueltardieu

[bushrat011899]

r? Jarcho

Please feel free to reverse this! I'm assigning it to you since this is an alternative to #17252 and based on your feedback to me, so I feel you'd be most appropriate to review. Again, thanks for all your help so far!

[Jarcho]

You beat me to finishing my first review by a few hours (I had it mostly typed up yesterday). You ended up doing one of the major things I was pointing out so that's cool.

[Jarcho]

You might find it nicer to handle list stems specially and turn lint points into a flattened tree. This will get you something like:

newtype_index! {
    pub struct NodeId {}
}

enum RootCrate {
    Std,
    Alloc,
}
enum NodeKind {
    Stem {
        did: DefId,
        /// Span of the final segment
        sp: Span,
        /// Index of the final path segment
        idx: u32,
    },
    Item(PerNs<Option<DefId>>),
    Glob,
}
struct Node {
    parent: NodeId,
    sp: Span,
    kind: NodeKind
}
struct Pass {
    root: Option<(RootCrate, Span)>,
    nodes: IndexVec<NodeId, Node>,
    skip_paths: u32,
}

fn get_parent_for(nodes: &IndexSlice<NodeId, Node>, p: &[PathSegment<'_>]) -> NodeId {
    let mut i = NodeId::from_usize(nodes.len() - 1);
    while i != NodeId::ZERO {
        let node = &nodes[i];
        if let Node::Stem { sp, idx } = node.kind
            && let Some(seg) = p.get(idx as usize)
            && seg.ident.span == s.span
        {
            return i;
        }
        i = node.parent;
    }
    i
}

fn check_item(...) {
    if let ItemKind::Use(path, kind) = item.kind
        && let (krate_seg, rest) = match path.segments {
            [root, krate, rest @ ..] if root.ident.name == kw::PathRoot => (krate, rest),
            [krate, rest @ ..] => (krate, rest),
            _ => return,
        }
        && let Some(krate) = RootCrate::from_name(krate_seg.ident.name)
    {
        // skip each path with a res only if there's a second path segment.
        self.skip_paths = path.res.map(|x| u32::from(x.is_some())).iter().sum::<u32>()
            & u32::from(rest.is_empty()).wrapping_sub(1);
        if self.root == Some((krate, krate_seg.ident.span)) {
            let last = path.segments.last().unwrap();
            self.nodes.push(Node {
                parent = get_parent_for(self.nodes, path.segments),
                sp: path.span,
                kind: match kind {
                    UseKind::ListStem => NodeKind::Seg {
                        did: last.res.def_id(),
                        sp: last.ident.span,
                        idx: path.segments.len() as u32 - 1
                    },
                    UseKind::Item(_) => NodeKind::Item(path.res.map(|x| x.and_then(|r| r.opt_def_id()))),
                    UseKind::Glob => NodeKind::Glob,
                },
            });
            return;
        }
        self.try_emit_use_tree();
        if let [.., last] = rest {
            match kind {
                UseKind::ListStem {
                    self.root = Some((krate, krate_seg.ident.span));
                    self.nodes.push(Node {
                        parent: NodeId::ZERO,
                        sp: path.span,
                        kind: NodeKind::ListStem {
                            did: last.res.def_id(),
                            sp: last.ident.span,
                            idx: path.segments.len() as u32 - 1,
                        },
                    });
                },
                UseKind::Item(_) | UseKind::Glob => {
                    // Lint like a single path, but with three potential namespaces
                },
            }
        }
    } else {
        self.try_emit_use_tree();
    }
}

fn check_path(...) {
    let (krate, last) = match path.segments {
        [root, krate, _, last] if root.ident.name == kw::PathRoot => (krate, last),
        [krate, _, last] => (krate, last),
        _ => return,
    };
    
    if let Some(krate) = RootCrate::from_name(krate_seg.ident.name) {
        if let Some(n) = self.skip_paths.checked_sub(1) {
            self.skip_paths = n;
            return;
        }
        // Lint a path
    }
}

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[bushrat011899]

You beat me to finishing my first review by a few hours (I had it mostly typed up yesterday). You ended up doing one of the major things I was pointing out so that's cool.

I'm so sorry for that! I figured this was a better version of my PR anyway, but I won't do q big change like this again without a comment and a ping first to check in.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[bushrat011899]

@rustbot ready
I believe I've addressed all your feedback Jarcho (thanks again for all your help!). I've not done exactly what you suggested regarding a flattened tree structure in favour of more aggressively only linting one path root at a time.