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
• Fixes std_instead_of_core/etc. should emit MultiLine help messages #17261
• 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 9342210

Lint	Added	Removed	Changed
clippy::alloc_instead_of_core	2467	4	0
clippy::std_instead_of_alloc	18	55	32
clippy::std_instead_of_core	125	140	40

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

[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.

[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.

[Jarcho]

The reason for keeping the tree structure was to catch all imports in a use statement with a single multi-span lint. If it's going to be notably more work to implement it's fine to work on it separately (or just not do it). I'll try to get to a review some time this week.

[bushrat011899]

The reason for keeping the tree structure was to catch all imports in a use statement with a single multi-span lint. If it's going to be notably more work to implement it's fine to work on it separately (or just not do it).

Oh that makes sense! If you'd approve moving to a multi-span in this PR I'm happy to spend a little longer switching to the tree structure as you suggested. Sorry, I falsely assumed you weren't aware of that issue I opened (or were interested in acting on it!).

[Jarcho]

I don't mind either way. If you're adding it to this PR I'll wait a bit before reviewing.

[bushrat011899]

Yeah I'll mark as waiting on author and switch back when I'm done.
@rustbot author

[bushrat011899]

@rustbot ready

Ok I'm pretty happy with how I've adapted my rewrite to work with MultiSpans now. Instead of storing the lint points in a flattened tree, I've chosen two Vec<Span> buffers; one for UseKind::Single's defined in core, and one for alloc. Doing so means those two buffers can directly be used as the MultiSpans when emitting lints, avoiding additional intermediate allocations that would otherwise be required.

The way I emit the lint now is to emit on the root of a path (e.g., the std in std::foo::{bar, baz, bing, bong}), and (if a suggestion is not possible) use a MultiSpan for the help component of the lint pointing to the singles. I think this makes sense, since if it can be a suggestion, we lint on the root anyway. The only exception is for a single item that can't become a suggestion (e.g., the Result in use alloc::fmt; use fmt::Result;). Instead of emitting a spanned help for the single and spanning the lint over the root, I use an unspanned help and lint the single, which matches how the lint currently works. I added this exception because the emitted message looked messy.

[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.