Auto merge of #8699 - Jarcho:vec_init_then_push_7071, r=dswij,xFrednet

Don't lint `vec_init_then_push` when further extended

fixes #7071

This will still lint when a larger number of pushes are done (four currently). The exact number could be debated, but this is more readable then a sequence of pushes so it shouldn't be too large.

changelog: Don't lint `vec_init_then_push` when further extended.
changelog: Remove `mut` binding from `vec_init_then_push` when possible.

clippy_lints/src/vec_init_then_push.rs

@@ -1,19 +1,30 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::higher::{get_vec_init_kind, VecInitKind};
 use clippy_utils::source::snippet;
-use clippy_utils::{path_to_local, path_to_local_id};
+use clippy_utils::visitors::for_each_local_use_after_expr;
-use if_chain::if_chain;
+use clippy_utils::{get_parent_expr, path_to_local_id};
+use core::ops::ControlFlow;
 use rustc_errors::Applicability;
-use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, Stmt, StmtKind};
+use rustc_hir::def::Res;
+use rustc_hir::{
+    BindingAnnotation, Block, Expr, ExprKind, HirId, Local, Mutability, PatKind, QPath, Stmt, StmtKind, UnOp,
+};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::Span;
+use rustc_span::{Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for calls to `push` immediately after creating a new `Vec`.
     ///
+    /// If the `Vec` is created using `with_capacity` this will only lint if the capacity is a
+    /// constant and the number of pushes is greater than or equal to the initial capacity.
+    ///
+    /// If the `Vec` is extended after the initial sequence of pushes and it was default initialized
+    /// then this will only lint after there were at least four pushes. This number may change in
+    /// the future.
+    ///
     /// ### Why is this bad?
     /// The `vec![]` macro is both more performant and easier to read than
     /// multiple `push` calls.
@@ -43,26 +54,88 @@ pub struct VecInitThenPush {
 struct VecPushSearcher {
     local_id: HirId,
     init: VecInitKind,
-    lhs_is_local: bool,
+    lhs_is_let: bool,
-    lhs_span: Span,
+    let_ty_span: Option<Span>,
+    name: Symbol,
     err_span: Span,
-    found: u64,
+    found: u128,
+    last_push_expr: HirId,
 }
 impl VecPushSearcher {
     fn display_err(&self, cx: &LateContext<'_>) {
-        match self.init {
+        let required_pushes_before_extension = match self.init {
             _ if self.found == 0 => return,
-            VecInitKind::WithLiteralCapacity(x) if x > self.found => return,
+            VecInitKind::WithConstCapacity(x) if x > self.found => return,
+            VecInitKind::WithConstCapacity(x) => x,
             VecInitKind::WithExprCapacity(_) => return,
-            _ => (),
+            _ => 3,
         };
 
-        let mut s = if self.lhs_is_local {
+        let mut needs_mut = false;
+        let res = for_each_local_use_after_expr(cx, self.local_id, self.last_push_expr, |e| {
+            let Some(parent) = get_parent_expr(cx, e) else {
+                return ControlFlow::Continue(())
+            };
+            let adjusted_ty = cx.typeck_results().expr_ty_adjusted(e);
+            let adjusted_mut = adjusted_ty.ref_mutability().unwrap_or(Mutability::Not);
+            needs_mut |= adjusted_mut == Mutability::Mut;
+            match parent.kind {
+                ExprKind::AddrOf(_, Mutability::Mut, _) => {
+                    needs_mut = true;
+                    return ControlFlow::Break(true);
+                },
+                ExprKind::Unary(UnOp::Deref, _) | ExprKind::Index(..) if !needs_mut => {
+                    let mut last_place = parent;
+                    while let Some(parent) = get_parent_expr(cx, parent) {
+                        if matches!(parent.kind, ExprKind::Unary(UnOp::Deref, _) | ExprKind::Field(..))
+                            || matches!(parent.kind, ExprKind::Index(e, _) if e.hir_id == last_place.hir_id)
+                        {
+                            last_place = parent;
+                        } else {
+                            break;
+                        }
+                    }
+                    needs_mut |= cx.typeck_results().expr_ty_adjusted(last_place).ref_mutability()
+                        == Some(Mutability::Mut)
+                        || get_parent_expr(cx, last_place)
+                            .map_or(false, |e| matches!(e.kind, ExprKind::AddrOf(_, Mutability::Mut, _)));
+                },
+                ExprKind::MethodCall(_, [recv, ..], _)
+                    if recv.hir_id == e.hir_id
+                        && adjusted_mut == Mutability::Mut
+                        && !adjusted_ty.peel_refs().is_slice() =>
+                {
+                    // No need to set `needs_mut` to true. The receiver will be either explicitly borrowed, or it will
+                    // be implicitly borrowed via an adjustment. Both of these cases are already handled by this point.
+                    return ControlFlow::Break(true);
+                },
+                ExprKind::Assign(lhs, ..) if e.hir_id == lhs.hir_id => {
+                    needs_mut = true;
+                    return ControlFlow::Break(false);
+                },
+                _ => (),
+            }
+            ControlFlow::Continue(())
+        });
+
+        // Avoid allocating small `Vec`s when they'll be extended right after.
+        if res == ControlFlow::Break(true) && self.found <= required_pushes_before_extension {
+            return;
+        }
+
+        let mut s = if self.lhs_is_let {
             String::from("let ")
         } else {
             String::new()
         };
-        s.push_str(&snippet(cx, self.lhs_span, ".."));
+        if needs_mut {
+            s.push_str("mut ");
+        }
+        s.push_str(self.name.as_str());
+        if let Some(span) = self.let_ty_span {
+            s.push_str(": ");
+            s.push_str(&snippet(cx, span, "_"));
+        }
         s.push_str(" = vec![..];");
 
         span_lint_and_sugg(
@@ -83,60 +156,63 @@ impl<'tcx> LateLintPass<'tcx> for VecInitThenPush {
     }
 
     fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
-        if_chain! {
+        if let Some(init_expr) = local.init
-            if !in_external_macro(cx.sess(), local.span);
+            && let PatKind::Binding(BindingAnnotation::Mutable, id, name, None) = local.pat.kind
-            if let Some(init) = local.init;
+            && !in_external_macro(cx.sess(), local.span)
-            if let PatKind::Binding(BindingAnnotation::Mutable, id, _, None) = local.pat.kind;
+            && let Some(init) = get_vec_init_kind(cx, init_expr)
-            if let Some(init_kind) = get_vec_init_kind(cx, init);
+            && !matches!(init, VecInitKind::WithExprCapacity(_))
-            then {
+        {
-                self.searcher = Some(VecPushSearcher {
+            self.searcher = Some(VecPushSearcher {
-                        local_id: id,
+                local_id: id,
-                        init: init_kind,
+                init,
-                        lhs_is_local: true,
+                lhs_is_let: true,
-                        lhs_span: local.ty.map_or(local.pat.span, |t| local.pat.span.to(t.span)),
+                name: name.name,
-                        err_span: local.span,
+                let_ty_span: local.ty.map(|ty| ty.span),
-                        found: 0,
+                err_span: local.span,
-                    });
+                found: 0,
-            }
+                last_push_expr: init_expr.hir_id,
+            });
         }
     }
 
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if_chain! {
+        if self.searcher.is_none()
-            if self.searcher.is_none();
+            && let ExprKind::Assign(left, right, _) = expr.kind
-            if !in_external_macro(cx.sess(), expr.span);
+            && let ExprKind::Path(QPath::Resolved(None, path)) = left.kind
-            if let ExprKind::Assign(left, right, _) = expr.kind;
+            && let [name] = &path.segments
-            if let Some(id) = path_to_local(left);
+            && let Res::Local(id) = path.res
-            if let Some(init_kind) = get_vec_init_kind(cx, right);
+            && !in_external_macro(cx.sess(), expr.span)
-            then {
+            && let Some(init) = get_vec_init_kind(cx, right)
-                self.searcher = Some(VecPushSearcher {
+            && !matches!(init, VecInitKind::WithExprCapacity(_))
-                    local_id: id,
+        {
-                    init: init_kind,
+            self.searcher = Some(VecPushSearcher {
-                    lhs_is_local: false,
+                local_id: id,
-                    lhs_span: left.span,
+                init,
-                    err_span: expr.span,
+                lhs_is_let: false,
-                    found: 0,
+                let_ty_span: None,
-                });
+                name: name.ident.name,
-            }
+                err_span: expr.span,
+                found: 0,
+                last_push_expr: expr.hir_id,
+            });
         }
     }
 
     fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
         if let Some(searcher) = self.searcher.take() {
-            if_chain! {
+            if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind
-                if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind;
+                && let ExprKind::MethodCall(name, [self_arg, _], _) = expr.kind
-                if let ExprKind::MethodCall(path, [self_arg, _], _) = expr.kind;
+                && path_to_local_id(self_arg, searcher.local_id)
-                if path_to_local_id(self_arg, searcher.local_id);
+                && name.ident.as_str() == "push"
-                if path.ident.name.as_str() == "push";
+            {
-                then {
+                self.searcher = Some(VecPushSearcher {
-                    self.searcher = Some(VecPushSearcher {
+                    found: searcher.found + 1,
-                        found: searcher.found + 1,
+                    err_span: searcher.err_span.to(stmt.span),
-                        err_span: searcher.err_span.to(stmt.span),
+                    last_push_expr: expr.hir_id,
-                        .. searcher
+                    .. searcher
-                    });
+                });
-                } else {
+            } else {
-                    searcher.display_err(cx);
+                searcher.display_err(cx);
-                }
             }
         }
     }

clippy_utils/src/higher.rs

@@ -2,10 +2,11 @@
 
 #![deny(clippy::missing_docs_in_private_items)]
 
+use crate::consts::{constant_simple, Constant};
 use crate::ty::is_type_diagnostic_item;
 use crate::{is_expn_of, match_def_path, paths};
 use if_chain::if_chain;
-use rustc_ast::ast::{self, LitKind};
+use rustc_ast::ast;
 use rustc_hir as hir;
 use rustc_hir::{Arm, Block, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath};
 use rustc_lint::LateContext;
@@ -431,7 +432,7 @@ pub enum VecInitKind {
     /// `Vec::default()` or `Default::default()`
     Default,
     /// `Vec::with_capacity(123)`
-    WithLiteralCapacity(u64),
+    WithConstCapacity(u128),
     /// `Vec::with_capacity(slice.len())`
     WithExprCapacity(HirId),
 }
@@ -449,15 +450,11 @@ pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -
                     return Some(VecInitKind::Default);
                 } else if name.ident.name.as_str() == "with_capacity" {
                     let arg = args.get(0)?;
-                    if_chain! {
+                    return match constant_simple(cx, cx.typeck_results(), arg) {
-                        if let ExprKind::Lit(lit) = &arg.kind;
+                        Some(Constant::Int(num)) => Some(VecInitKind::WithConstCapacity(num)),
-                        if let LitKind::Int(num, _) = lit.node;
+                        _ => Some(VecInitKind::WithExprCapacity(arg.hir_id)),
-                        then {
+                    };
-                            return Some(VecInitKind::WithLiteralCapacity(num.try_into().ok()?));
+                };
-                        }
-                    }
-                    return Some(VecInitKind::WithExprCapacity(arg.hir_id));
-                }
             },
             ExprKind::Path(QPath::Resolved(_, path))
                 if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD)

clippy_utils/src/visitors.rs

@@ -1,4 +1,4 @@
-use crate::path_to_local_id;
+use crate::{get_enclosing_block, path_to_local_id};
 use core::ops::ControlFlow;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
@@ -436,3 +436,61 @@ pub fn for_each_value_source<'tcx, B>(
         _ => f(e),
     }
 }
+
+/// Runs the given function for each path expression referencing the given local which occur after
+/// the given expression.
+pub fn for_each_local_use_after_expr<'tcx, B>(
+    cx: &LateContext<'tcx>,
+    local_id: HirId,
+    expr_id: HirId,
+    f: impl FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B>,
+) -> ControlFlow<B> {
+    struct V<'cx, 'tcx, F, B> {
+        cx: &'cx LateContext<'tcx>,
+        local_id: HirId,
+        expr_id: HirId,
+        found: bool,
+        res: ControlFlow<B>,
+        f: F,
+    }
+    impl<'cx, 'tcx, F: FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B>, B> Visitor<'tcx> for V<'cx, 'tcx, F, B> {
+        type NestedFilter = nested_filter::OnlyBodies;
+        fn nested_visit_map(&mut self) -> Self::Map {
+            self.cx.tcx.hir()
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
+            if !self.found {
+                if e.hir_id == self.expr_id {
+                    self.found = true;
+                } else {
+                    walk_expr(self, e);
+                }
+                return;
+            }
+            if self.res.is_break() {
+                return;
+            }
+            if path_to_local_id(e, self.local_id) {
+                self.res = (self.f)(e);
+            } else {
+                walk_expr(self, e);
+            }
+        }
+    }
+
+    if let Some(b) = get_enclosing_block(cx, local_id) {
+        let mut v = V {
+            cx,
+            local_id,
+            expr_id,
+            found: false,
+            res: ControlFlow::Continue(()),
+            f,
+        };
+        v.visit_block(b);
+        v.res
+    } else {
+        ControlFlow::Continue(())
+    }
+}

tests/ui/vec_init_then_push.rs

@@ -29,6 +29,12 @@ fn main() {
         // no lint
         vec.push(1);
     }
+
+    let mut vec = Vec::with_capacity(5);
+    vec.push(1);
+    vec.push(2);
+    vec.push(3);
+    vec.push(4);
 }
 
 pub fn no_lint() -> Vec<i32> {
@@ -44,3 +50,57 @@ pub fn no_lint() -> Vec<i32> {
         }
     }
 }
+
+fn _from_iter(items: impl Iterator<Item = u32>) -> Vec<u32> {
+    let mut v = Vec::new();
+    v.push(0);
+    v.push(1);
+    v.extend(items);
+    v
+}
+
+fn _cond_push(x: bool) -> Vec<u32> {
+    let mut v = Vec::new();
+    v.push(0);
+    if x {
+        v.push(1);
+    }
+    v.push(2);
+    v
+}
+
+fn _push_then_edit(x: u32) -> Vec<u32> {
+    let mut v = Vec::new();
+    v.push(x);
+    v.push(1);
+    v[0] = v[1] + 5;
+    v
+}
+
+fn _cond_push_with_large_start(x: bool) -> Vec<u32> {
+    let mut v = Vec::new();
+    v.push(0);
+    v.push(1);
+    v.push(0);
+    v.push(1);
+    v.push(0);
+    v.push(0);
+    v.push(1);
+    v.push(0);
+    if x {
+        v.push(1);
+    }
+
+    let mut v2 = Vec::new();
+    v2.push(0);
+    v2.push(1);
+    v2.push(0);
+    v2.push(1);
+    v2.push(0);
+    v2.push(0);
+    v2.push(1);
+    v2.push(0);
+    v2.extend(&v);
+
+    v2
+}

tests/ui/vec_init_then_push.stderr

@@ -3,7 +3,7 @@ error: calls to `push` immediately after creation
    |
 LL | /     let mut def_err: Vec<u32> = Default::default();
 LL | |     def_err.push(0);
-   | |____________________^ help: consider using the `vec![]` macro: `let mut def_err: Vec<u32> = vec![..];`
+   | |____________________^ help: consider using the `vec![]` macro: `let def_err: Vec<u32> = vec![..];`
    |
    = note: `-D clippy::vec-init-then-push` implied by `-D warnings`
 
@@ -30,5 +30,37 @@ LL | /     new_err = Vec::new();
 LL | |     new_err.push(0);
    | |____________________^ help: consider using the `vec![]` macro: `new_err = vec![..];`
 
-error: aborting due to 4 previous errors
+error: calls to `push` immediately after creation
+  --> $DIR/vec_init_then_push.rs:73:5
+   |
+LL | /     let mut v = Vec::new();
+LL | |     v.push(x);
+LL | |     v.push(1);
+   | |______________^ help: consider using the `vec![]` macro: `let mut v = vec![..];`
+
+error: calls to `push` immediately after creation
+  --> $DIR/vec_init_then_push.rs:81:5
+   |
+LL | /     let mut v = Vec::new();
+LL | |     v.push(0);
+LL | |     v.push(1);
+LL | |     v.push(0);
+...  |
+LL | |     v.push(1);
+LL | |     v.push(0);
+   | |______________^ help: consider using the `vec![]` macro: `let mut v = vec![..];`
+
+error: calls to `push` immediately after creation
+  --> $DIR/vec_init_then_push.rs:94:5
+   |
+LL | /     let mut v2 = Vec::new();
+LL | |     v2.push(0);
+LL | |     v2.push(1);
+LL | |     v2.push(0);
+...  |
+LL | |     v2.push(1);
+LL | |     v2.push(0);
+   | |_______________^ help: consider using the `vec![]` macro: `let mut v2 = vec![..];`
+
+error: aborting due to 7 previous errors