needless_late_init: ignore if let, let mut and significant drops

clippy_lints/src/matches/redundant_pattern_match.rs

@@ -2,17 +2,16 @@ use super::REDUNDANT_PATTERN_MATCHING;
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet;
 use clippy_utils::sugg::Sugg;
-use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, is_type_lang_item, match_type};
+use clippy_utils::ty::needs_ordered_drop;
 use clippy_utils::{higher, match_def_path};
 use clippy_utils::{is_lang_ctor, is_trait_method, paths};
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
-use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{OptionNone, PollPending};
 use rustc_hir::{
     intravisit::{walk_expr, Visitor},
-    Arm, Block, Expr, ExprKind, LangItem, Node, Pat, PatKind, QPath, UnOp,
+    Arm, Block, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp,
 };
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty};
@@ -32,59 +31,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     }
 }
 
-/// Checks if the drop order for a type matters. Some std types implement drop solely to
-/// deallocate memory. For these types, and composites containing them, changing the drop order
-/// won't result in any observable side effects.
-fn type_needs_ordered_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
-    type_needs_ordered_drop_inner(cx, ty, &mut FxHashSet::default())
-}
-
-fn type_needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, seen: &mut FxHashSet<Ty<'tcx>>) -> bool {
-    if !seen.insert(ty) {
-        return false;
-    }
-    if !ty.needs_drop(cx.tcx, cx.param_env) {
-        false
-    } else if !cx
-        .tcx
-        .lang_items()
-        .drop_trait()
-        .map_or(false, |id| implements_trait(cx, ty, id, &[]))
-    {
-        // This type doesn't implement drop, so no side effects here.
-        // Check if any component type has any.
-        match ty.kind() {
-            ty::Tuple(fields) => fields.iter().any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)),
-            ty::Array(ty, _) => type_needs_ordered_drop_inner(cx, *ty, seen),
-            ty::Adt(adt, subs) => adt
-                .all_fields()
-                .map(|f| f.ty(cx.tcx, subs))
-                .any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)),
-            _ => true,
-        }
-    }
-    // Check for std types which implement drop, but only for memory allocation.
-    else if is_type_diagnostic_item(cx, ty, sym::Vec)
-        || is_type_lang_item(cx, ty, LangItem::OwnedBox)
-        || is_type_diagnostic_item(cx, ty, sym::Rc)
-        || is_type_diagnostic_item(cx, ty, sym::Arc)
-        || is_type_diagnostic_item(cx, ty, sym::cstring_type)
-        || is_type_diagnostic_item(cx, ty, sym::BTreeMap)
-        || is_type_diagnostic_item(cx, ty, sym::LinkedList)
-        || match_type(cx, ty, &paths::WEAK_RC)
-        || match_type(cx, ty, &paths::WEAK_ARC)
-    {
-        // Check all of the generic arguments.
-        if let ty::Adt(_, subs) = ty.kind() {
-            subs.types().any(|ty| type_needs_ordered_drop_inner(cx, ty, seen))
-        } else {
-            true
-        }
-    } else {
-        true
-    }
-}
-
 // Extract the generic arguments out of a type
 fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option<Ty<'_>> {
     if_chain! {
@@ -115,7 +61,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
                 // e.g. In `(String::new(), 0).1` the string is a temporary value.
                 ExprKind::AddrOf(_, _, expr) | ExprKind::Field(expr, _) => {
                     if !matches!(expr.kind, ExprKind::Path(_)) {
-                        if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
+                        if needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
                             self.res = true;
                         } else {
                             self.visit_expr(expr);
@@ -126,7 +72,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
                 // e.g. In `(vec![0])[0]` the vector is a temporary value.
                 ExprKind::Index(base, index) => {
                     if !matches!(base.kind, ExprKind::Path(_)) {
-                        if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
+                        if needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
                             self.res = true;
                         } else {
                             self.visit_expr(base);
@@ -143,7 +89,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
                             .typeck_results()
                             .type_dependent_def_id(expr.hir_id)
                             .map_or(false, |id| self.cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref());
-                        if self_by_ref && type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg)) {
+                        if self_by_ref && needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg)) {
                             self.res = true;
                         } else {
                             self.visit_expr(self_arg);
@@ -243,7 +189,7 @@ fn find_sugg_for_if_let<'tcx>(
     // scrutinee would be, so they have to be considered as well.
     // e.g. in `if let Some(x) = foo.lock().unwrap().baz.as_ref() { .. }` the lock will be held
     // for the duration if body.
-    let needs_drop = type_needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);
+    let needs_drop = needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);
 
     // check that `while_let_on_iterator` lint does not trigger
     if_chain! {

clippy_lints/src/needless_late_init.rs

@@ -1,10 +1,14 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::path_to_local;
 use clippy_utils::source::snippet_opt;
-use clippy_utils::visitors::{expr_visitor, is_local_used};
+use clippy_utils::ty::needs_ordered_drop;
+use clippy_utils::visitors::{expr_visitor, expr_visitor_no_bodies, is_local_used};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::Visitor;
-use rustc_hir::{Block, Expr, ExprKind, HirId, Local, LocalSource, MatchSource, Node, Pat, PatKind, Stmt, StmtKind};
+use rustc_hir::{
+    BindingAnnotation, Block, Expr, ExprKind, HirId, Local, LocalSource, MatchSource, Node, Pat, PatKind, Stmt,
+    StmtKind,
+};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::Span;
@@ -73,6 +77,31 @@ fn contains_assign_expr<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) ->
     seen
 }
 
+fn contains_let(cond: &Expr<'_>) -> bool {
+    let mut seen = false;
+    expr_visitor_no_bodies(|expr| {
+        if let ExprKind::Let(_) = expr.kind {
+            seen = true;
+        }
+
+        !seen
+    })
+    .visit_expr(cond);
+
+    seen
+}
+
+fn stmt_needs_ordered_drop(cx: &LateContext<'_>, stmt: &Stmt<'_>) -> bool {
+    let StmtKind::Local(local) = stmt.kind else { return false };
+    !local.pat.walk_short(|pat| {
+        if let PatKind::Binding(.., None) = pat.kind {
+            !needs_ordered_drop(cx, cx.typeck_results().pat_ty(pat))
+        } else {
+            true
+        }
+    })
+}
+
 #[derive(Debug)]
 struct LocalAssign {
     lhs_id: HirId,
@@ -187,11 +216,14 @@ fn first_usage<'tcx>(
     local_stmt_id: HirId,
     block: &'tcx Block<'tcx>,
 ) -> Option<Usage<'tcx>> {
+    let significant_drop = needs_ordered_drop(cx, cx.typeck_results().node_type(binding_id));
+
     block
         .stmts
         .iter()
         .skip_while(|stmt| stmt.hir_id != local_stmt_id)
         .skip(1)
+        .take_while(|stmt| !significant_drop || !stmt_needs_ordered_drop(cx, stmt))
         .find(|&stmt| is_local_used(cx, stmt, binding_id))
         .and_then(|stmt| match stmt.kind {
             StmtKind::Expr(expr) => Some(Usage {
@@ -258,7 +290,7 @@ fn check<'tcx>(
                 },
             );
         },
-        ExprKind::If(_, then_expr, Some(else_expr)) => {
+        ExprKind::If(cond, then_expr, Some(else_expr)) if !contains_let(cond) => {
             let (applicability, suggestions) = assignment_suggestions(cx, binding_id, [then_expr, else_expr])?;
 
             span_lint_and_then(
@@ -338,7 +370,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessLateInit {
             if let Local {
                 init: None,
                 pat: &Pat {
-                    kind: PatKind::Binding(_, binding_id, _, None),
+                    kind: PatKind::Binding(BindingAnnotation::Unannotated, binding_id, _, None),
                     ..
                 },
                 source: LocalSource::Normal,

clippy_utils/src/ty.rs

@@ -3,11 +3,11 @@
 #![allow(clippy::module_name_repetitions)]
 
 use rustc_ast::ast::Mutability;
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_hir as hir;
 use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::def_id::DefId;
-use rustc_hir::{Expr, TyKind, Unsafety};
+use rustc_hir::{Expr, LangItem, TyKind, Unsafety};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::mir::interpret::{ConstValue, Scalar};
@@ -22,7 +22,7 @@ use rustc_trait_selection::infer::InferCtxtExt;
 use rustc_trait_selection::traits::query::normalize::AtExt;
 use std::iter;
 
-use crate::{match_def_path, must_use_attr, path_res};
+use crate::{match_def_path, must_use_attr, path_res, paths};
 
 // Checks if the given type implements copy.
 pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
@@ -83,6 +83,20 @@ pub fn get_associated_type<'tcx>(
         })
 }
 
+/// Get the diagnostic name of a type, e.g. `sym::HashMap`. To check if a type
+/// implements a trait marked with a diagnostic item use [`implements_trait`].
+///
+/// For a further exploitation what diagnostic items are see [diagnostic items] in
+/// rustc-dev-guide.
+///
+/// [Diagnostic Items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html
+pub fn get_type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symbol> {
+    match ty.kind() {
+        ty::Adt(adt, _) => cx.tcx.get_diagnostic_name(adt.did()),
+        _ => None,
+    }
+}
+
 /// Returns true if ty has `iter` or `iter_mut` methods
 pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<Symbol> {
     // FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
@@ -319,6 +333,57 @@ pub fn match_type(cx: &LateContext<'_>, ty: Ty<'_>, path: &[&str]) -> bool {
     }
 }
 
+/// Checks if the drop order for a type matters. Some std types implement drop solely to
+/// deallocate memory. For these types, and composites containing them, changing the drop order
+/// won't result in any observable side effects.
+pub fn needs_ordered_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    fn needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, seen: &mut FxHashSet<Ty<'tcx>>) -> bool {
+        if !seen.insert(ty) {
+            return false;
+        }
+        if !ty.has_significant_drop(cx.tcx, cx.param_env) {
+            false
+        }
+        // Check for std types which implement drop, but only for memory allocation.
+        else if is_type_lang_item(cx, ty, LangItem::OwnedBox)
+            || matches!(
+                get_type_diagnostic_name(cx, ty),
+                Some(sym::HashSet | sym::Rc | sym::Arc | sym::cstring_type)
+            )
+            || match_type(cx, ty, &paths::WEAK_RC)
+            || match_type(cx, ty, &paths::WEAK_ARC)
+        {
+            // Check all of the generic arguments.
+            if let ty::Adt(_, subs) = ty.kind() {
+                subs.types().any(|ty| needs_ordered_drop_inner(cx, ty, seen))
+            } else {
+                true
+            }
+        } else if !cx
+            .tcx
+            .lang_items()
+            .drop_trait()
+            .map_or(false, |id| implements_trait(cx, ty, id, &[]))
+        {
+            // This type doesn't implement drop, so no side effects here.
+            // Check if any component type has any.
+            match ty.kind() {
+                ty::Tuple(fields) => fields.iter().any(|ty| needs_ordered_drop_inner(cx, ty, seen)),
+                ty::Array(ty, _) => needs_ordered_drop_inner(cx, *ty, seen),
+                ty::Adt(adt, subs) => adt
+                    .all_fields()
+                    .map(|f| f.ty(cx.tcx, subs))
+                    .any(|ty| needs_ordered_drop_inner(cx, ty, seen)),
+                _ => true,
+            }
+        } else {
+            true
+        }
+    }
+
+    needs_ordered_drop_inner(cx, ty, &mut FxHashSet::default())
+}
+
 /// Peels off all references on the type. Returns the underlying type and the number of references
 /// removed.
 pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) {

tests/ui/needless_late_init.rs

@@ -1,5 +1,15 @@
-#![allow(unused)]
+#![feature(let_chains)]
-#![allow(clippy::let_unit_value)]
+#![allow(unused, clippy::nonminimal_bool, clippy::let_unit_value)]
+
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::rc::Rc;
+
+struct SignificantDrop;
+impl std::ops::Drop for SignificantDrop {
+    fn drop(&mut self) {
+        println!("dropped");
+    }
+}
 
 fn main() {
     let a;
@@ -18,13 +28,6 @@ fn main() {
         b = "five"
     }
 
-    let c;
-    if let Some(n) = Some(5) {
-        c = n;
-    } else {
-        c = -50;
-    }
-
     let d;
     if true {
         let temp = 5;
@@ -37,7 +40,7 @@ fn main() {
     if true {
         e = format!("{} {}", a, b);
     } else {
-        e = format!("{}", c);
+        e = format!("{}", n);
     }
 
     let f;
@@ -53,7 +56,27 @@ fn main() {
         panic!();
     }
 
-    println!("{}", a);
+    // Drop order only matters if both are significant
+    let x;
+    let y = SignificantDrop;
+    x = 1;
+
+    let x;
+    let y = 1;
+    x = SignificantDrop;
+
+    let x;
+    // types that should be considered insignificant
+    let y = 1;
+    let y = "2";
+    let y = String::new();
+    let y = vec![3.0];
+    let y = HashMap::<usize, usize>::new();
+    let y = BTreeMap::<usize, usize>::new();
+    let y = HashSet::<usize>::new();
+    let y = BTreeSet::<usize>::new();
+    let y = Box::new(4);
+    x = SignificantDrop;
 }
 
 async fn in_async() -> &'static str {
@@ -177,5 +200,32 @@ fn does_not_lint() {
     }
     in_macro!();
 
-    println!("{}", x);
+    // ignore if-lets - https://github.com/rust-lang/rust-clippy/issues/8613
+    let x;
+    if let Some(n) = Some("v") {
+        x = 1;
+    } else {
+        x = 2;
+    }
+
+    let x;
+    if true && let Some(n) = Some("let chains too") {
+        x = 1;
+    } else {
+        x = 2;
+    }
+
+    // ignore mut bindings
+    // https://github.com/shepmaster/twox-hash/blob/b169c16d86eb8ea4a296b0acb9d00ca7e3c3005f/src/sixty_four.rs#L88-L93
+    // https://github.com/dtolnay/thiserror/blob/21c26903e29cb92ba1a7ff11e82ae2001646b60d/tests/test_generics.rs#L91-L100
+    let mut x: usize;
+    x = 1;
+    x = 2;
+    x = 3;
+
+    // should not move the declaration if `x` has a significant drop, and there
+    // is another binding with a significant drop between it and the first usage
+    let x;
+    let y = SignificantDrop;
+    x = SignificantDrop;
 }

tests/ui/needless_late_init.stderr

@@ -1,5 +1,5 @@
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:5:5
+  --> $DIR/needless_late_init.rs:15:5
    |
 LL |     let a;
    |     ^^^^^^
@@ -21,7 +21,7 @@ LL |     };
    |      +
 
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:14:5
+  --> $DIR/needless_late_init.rs:24:5
    |
 LL |     let b;
    |     ^^^^^^
@@ -42,28 +42,7 @@ LL |     };
    |      +
 
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:21:5
+  --> $DIR/needless_late_init.rs:31:5
-   |
-LL |     let c;
-   |     ^^^^^^
-   |
-help: declare `c` here
-   |
-LL |     let c = if let Some(n) = Some(5) {
-   |     +++++++
-help: remove the assignments from the branches
-   |
-LL ~         n
-LL |     } else {
-LL ~         -50
-   |
-help: add a semicolon after the `if` expression
-   |
-LL |     };
-   |      +
-
-error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:28:5
    |
 LL |     let d;
    |     ^^^^^^
@@ -84,7 +63,7 @@ LL |     };
    |      +
 
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:36:5
+  --> $DIR/needless_late_init.rs:39:5
    |
 LL |     let e;
    |     ^^^^^^
@@ -97,7 +76,7 @@ help: remove the assignments from the branches
    |
 LL ~         format!("{} {}", a, b)
 LL |     } else {
-LL ~         format!("{}", c)
+LL ~         format!("{}", n)
    |
 help: add a semicolon after the `if` expression
    |
@@ -105,7 +84,7 @@ LL |     };
    |      +
 
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:43:5
+  --> $DIR/needless_late_init.rs:46:5
    |
 LL |     let f;
    |     ^^^^^^
@@ -121,7 +100,7 @@ LL +         1 => "three",
    | 
 
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:49:5
+  --> $DIR/needless_late_init.rs:52:5
    |
 LL |     let g: usize;
    |     ^^^^^^^^^^^^^
@@ -140,9 +119,42 @@ help: add a semicolon after the `if` expression
 LL |     };
    |      +
 
+error: unneeded late initialization
+  --> $DIR/needless_late_init.rs:60:5
+   |
+LL |     let x;
+   |     ^^^^^^
+   |
+help: declare `x` here
+   |
+LL |     let x = 1;
+   |     ~~~~~
+
 error: unneeded late initialization
   --> $DIR/needless_late_init.rs:64:5
    |
+LL |     let x;
+   |     ^^^^^^
+   |
+help: declare `x` here
+   |
+LL |     let x = SignificantDrop;
+   |     ~~~~~
+
+error: unneeded late initialization
+  --> $DIR/needless_late_init.rs:68:5
+   |
+LL |     let x;
+   |     ^^^^^^
+   |
+help: declare `x` here
+   |
+LL |     let x = SignificantDrop;
+   |     ~~~~~
+
+error: unneeded late initialization
+  --> $DIR/needless_late_init.rs:87:5
+   |
 LL |     let a;
    |     ^^^^^^
    |
@@ -162,7 +174,7 @@ LL |     };
    |      +
 
 error: unneeded late initialization
-  --> $DIR/needless_late_init.rs:81:5
+  --> $DIR/needless_late_init.rs:104:5
    |
 LL |     let a;
    |     ^^^^^^
@@ -182,5 +194,5 @@ help: add a semicolon after the `match` expression
 LL |     };
    |      +
 
-error: aborting due to 9 previous errors
+error: aborting due to 11 previous errors
 

tests/ui/needless_late_init_fixable.fixed

@@ -15,11 +15,5 @@ fn main() {
     let d: usize = 1;
 
     
-    let mut e = 1;
+    let e = format!("{}", d);
-    e = 2;
-
-    
-    let h = format!("{}", e);
-
-    println!("{}", a);
 }

tests/ui/needless_late_init_fixable.rs

@@ -14,12 +14,6 @@ fn main() {
     let d: usize;
     d = 1;
 
-    let mut e;
+    let e;
-    e = 1;
+    e = format!("{}", d);
-    e = 2;
-
-    let h;
-    h = format!("{}", e);
-
-    println!("{}", a);
 }

tests/ui/needless_late_init_fixable.stderr

@@ -46,24 +46,13 @@ LL |     let d: usize = 1;
 error: unneeded late initialization
   --> $DIR/needless_late_init_fixable.rs:17:5
    |
-LL |     let mut e;
+LL |     let e;
-   |     ^^^^^^^^^^
+   |     ^^^^^^
    |
 help: declare `e` here
    |
-LL |     let mut e = 1;
+LL |     let e = format!("{}", d);
-   |     ~~~~~~~~~
-
-error: unneeded late initialization
-  --> $DIR/needless_late_init_fixable.rs:21:5
-   |
-LL |     let h;
-   |     ^^^^^^
-   |
-help: declare `h` here
-   |
-LL |     let h = format!("{}", e);
    |     ~~~~~
 
-error: aborting due to 6 previous errors
+error: aborting due to 5 previous errors