Changes to let_unit_value

Jarcho · Jarcho · commit b903126294b7 · 2022-06-30T09:05:26.000-04:00
* View through locals in `let_unit_value` when determining if inference is required
* Don't remove typed let bindings for more functions
diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs
@@ -1,30 +1,25 @@
 use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::get_parent_node;
 use clippy_utils::source::snippet_with_macro_callsite;
-use clippy_utils::visitors::for_each_value_source;
+use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::{Expr, ExprKind, PatKind, Stmt, StmtKind};
+use rustc_hir::{Expr, ExprKind, HirId, HirIdSet, Node, PatKind, QPath, Stmt, StmtKind, TyKind};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, Ty, TypeFoldable, TypeSuperFoldable, TypeVisitor};
+use rustc_middle::ty;
 
 use super::LET_UNIT_VALUE;
 
-pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
     if let StmtKind::Local(local) = stmt.kind
         && let Some(init) = local.init
         && !local.pat.span.from_expansion()
         && !in_external_macro(cx.sess(), stmt.span)
         && cx.typeck_results().pat_ty(local.pat).is_unit()
     {
-        let needs_inferred = for_each_value_source(init, &mut |e| if needs_inferred_result_ty(cx, e) {
-            ControlFlow::Continue(())
-        } else {
-            ControlFlow::Break(())
-        }).is_continue();
-
-        if needs_inferred {
+        if local.ty.is_some() && expr_needs_inferred_result(cx, init) {
             if !matches!(local.pat.kind, PatKind::Wild) {
                 span_lint_and_then(
                     cx,
@@ -63,48 +58,106 @@ pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
     }
 }
 
-fn needs_inferred_result_ty(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
-    let id = match e.kind {
+/// Checks sub-expressions which create the value returned by the given expression for whether
+/// return value inference is needed. This checks through locals to see if they also need inference
+/// at this point.
+///
+/// e.g.
+/// ```rust,ignore
+/// let bar = foo();
+/// let x: u32 = if true { baz() } else { bar };
+/// ```
+/// Here the sources of the value assigned to `x` would be `baz()`, and `foo()` via the
+/// initialization of `bar`. If both `foo` and `baz` have a return type which require type
+/// inference then this function would return `true`.
+fn expr_needs_inferred_result<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
+    // The locals used for initialization which have yet to be checked.
+    let mut locals_to_check = Vec::new();
+    // All the locals which have been added to `locals_to_check`. Needed to prevent cycles.
+    let mut seen_locals = HirIdSet::default();
+    if !each_value_source_needs_inference(cx, e, &mut locals_to_check, &mut seen_locals) {
+        return false;
+    }
+    while let Some(id) = locals_to_check.pop() {
+        if let Some(Node::Local(l)) = get_parent_node(cx.tcx, id) {
+            if !l.ty.map_or(true, |ty| matches!(ty.kind, TyKind::Infer)) {
+                return false;
+            }
+            if let Some(e) = l.init {
+                if !each_value_source_needs_inference(cx, e, &mut locals_to_check, &mut seen_locals) {
+                    return false;
+                }
+            } else if for_each_local_assignment(cx, id, |e| {
+                if each_value_source_needs_inference(cx, e, &mut locals_to_check, &mut seen_locals) {
+                    ControlFlow::Continue(())
+                } else {
+                    ControlFlow::Break(())
+                }
+            })
+            .is_break()
+            {
+                return false;
+            }
+        }
+    }
+
+    true
+}
+
+fn each_value_source_needs_inference(
+    cx: &LateContext<'_>,
+    e: &Expr<'_>,
+    locals_to_check: &mut Vec<HirId>,
+    seen_locals: &mut HirIdSet,
+) -> bool {
+    for_each_value_source(e, &mut |e| {
+        if needs_inferred_result_ty(cx, e, locals_to_check, seen_locals) {
+            ControlFlow::Continue(())
+        } else {
+            ControlFlow::Break(())
+        }
+    })
+    .is_continue()
+}
+
+fn needs_inferred_result_ty(
+    cx: &LateContext<'_>,
+    e: &Expr<'_>,
+    locals_to_check: &mut Vec<HirId>,
+    seen_locals: &mut HirIdSet,
+) -> bool {
+    let (id, args) = match e.kind {
         ExprKind::Call(
             Expr {
                 kind: ExprKind::Path(ref path),
                 hir_id,
                 ..
             },
-            _,
+            args,
         ) => match cx.qpath_res(path, *hir_id) {
-            Res::Def(DefKind::AssocFn | DefKind::Fn, id) => id,
+            Res::Def(DefKind::AssocFn | DefKind::Fn, id) => (id, args),
             _ => return false,
         },
-        ExprKind::MethodCall(..) => match cx.typeck_results().type_dependent_def_id(e.hir_id) {
-            Some(id) => id,
+        ExprKind::MethodCall(_, args, _) => match cx.typeck_results().type_dependent_def_id(e.hir_id) {
+            Some(id) => (id, args),
             None => return false,
         },
+        ExprKind::Path(QPath::Resolved(None, path)) => {
+            if let Res::Local(id) = path.res
+                && seen_locals.insert(id)
+            {
+                locals_to_check.push(id);
+            }
+            return true;
+        },
         _ => return false,
     };
     let sig = cx.tcx.fn_sig(id).skip_binder();
     if let ty::Param(output_ty) = *sig.output().kind() {
-        sig.inputs().iter().all(|&ty| !ty_contains_param(ty, output_ty.index))
+        sig.inputs().iter().zip(args).all(|(&ty, arg)| {
+            !ty.is_param(output_ty.index) || each_value_source_needs_inference(cx, arg, locals_to_check, seen_locals)
+        })
     } else {
         false
     }
 }
-
-fn ty_contains_param(ty: Ty<'_>, index: u32) -> bool {
-    struct Visitor(u32);
-    impl<'tcx> TypeVisitor<'tcx> for Visitor {
-        type BreakTy = ();
-        fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
-            if let ty::Param(ty) = *ty.kind() {
-                if ty.index == self.0 {
-                    ControlFlow::BREAK
-                } else {
-                    ControlFlow::CONTINUE
-                }
-            } else {
-                ty.super_visit_with(self)
-            }
-        }
-    }
-    ty.visit_with(&mut Visitor(index)).is_break()
-}
diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs
@@ -98,8 +98,8 @@ declare_clippy_lint! {
 
 declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);
 
-impl LateLintPass<'_> for UnitTypes {
-    fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) {
+impl<'tcx> LateLintPass<'tcx> for UnitTypes {
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
         let_unit_value::check(cx, stmt);
     }
 
diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs
@@ -617,3 +617,49 @@ pub fn any_temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, e: &'tcx
     })
     .is_break()
 }
+
+/// Runs the given function for each path expression referencing the given local which occur after
+/// the given expression.
+pub fn for_each_local_assignment<'tcx, B>(
+    cx: &LateContext<'tcx>,
+    local_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,
+        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 let ExprKind::Assign(lhs, rhs, _) = e.kind
+                && self.res.is_continue()
+                && path_to_local_id(lhs, self.local_id)
+            {
+                self.res = (self.f)(rhs);
+                self.visit_expr(rhs);
+            } else {
+                walk_expr(self, e);
+            }
+        }
+    }
+
+    if let Some(b) = get_enclosing_block(cx, local_id) {
+        let mut v = V {
+            cx,
+            local_id,
+            res: ControlFlow::Continue(()),
+            f,
+        };
+        v.visit_block(b);
+        v.res
+    } else {
+        ControlFlow::Continue(())
+    }
+}
diff --git a/tests/ui/let_unit.fixed b/tests/ui/let_unit.fixed
@@ -1,8 +1,7 @@
 // run-rustfix
 
 #![warn(clippy::let_unit_value)]
-#![allow(clippy::no_effect)]
-#![allow(unused_variables)]
+#![allow(unused_variables, clippy::no_effect, clippy::needless_late_init, path_statements)]
 
 macro_rules! let_and_return {
     ($n:expr) => {{
@@ -72,8 +71,8 @@ fn _returns_generic() {
     fn f3<T>(x: T) -> T {
         x
     }
-    fn f4<T>(mut x: Vec<T>) -> T {
-        x.pop().unwrap()
+    fn f5<T: Default>(x: bool) -> Option<T> {
+        x.then(|| T::default())
     }
 
     let _: () = f(); // Ok
@@ -85,8 +84,12 @@ fn _returns_generic() {
     f3(()); // Lint
     f3(()); // Lint
 
-    f4(vec![()]); // Lint
-    f4(vec![()]); // Lint
+    // Should lint:
+    // fn f4<T>(mut x: Vec<T>) -> T {
+    //    x.pop().unwrap()
+    // }
+    // let _: () = f4(vec![()]);
+    // let x: () = f4(vec![()]);
 
     // Ok
     let _: () = {
@@ -112,4 +115,51 @@ fn _returns_generic() {
         Some(1) => f2(3),
         Some(_) => (),
     };
+
+    let _: () = f5(true).unwrap();
+
+    #[allow(clippy::let_unit_value)]
+    {
+        let x = f();
+        let y;
+        let z;
+        match 0 {
+            0 => {
+                y = f();
+                z = f();
+            },
+            1 => {
+                println!("test");
+                y = f();
+                z = f3(());
+            },
+            _ => panic!(),
+        }
+
+        let x1;
+        let x2;
+        if true {
+            x1 = f();
+            x2 = x1;
+        } else {
+            x2 = f();
+            x1 = x2;
+        }
+
+        let opt;
+        match f5(true) {
+            Some(x) => opt = x,
+            None => panic!(),
+        };
+
+        #[warn(clippy::let_unit_value)]
+        {
+            let _: () = x;
+            let _: () = y;
+            z;
+            let _: () = x1;
+            let _: () = x2;
+            let _: () = opt;
+        }
+    }
 }
diff --git a/tests/ui/let_unit.rs b/tests/ui/let_unit.rs
diff --git a/tests/ui/let_unit.stderr b/tests/ui/let_unit.stderr

Original file line number	Diff line number	Diff line change
`@@ -98,8 +98,8 @@ declare_clippy_lint! {`
`98`	`98`
`99`	`99`	`declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);`
`100`	`100`
`101`		`-impl LateLintPass<'_> for UnitTypes {`
`102`		`- fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) {`
	`101`	`+impl<'tcx> LateLintPass<'tcx> for UnitTypes {`
	`102`	`+ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {`
`103`	`103`	`let_unit_value::check(cx, stmt);`
`104`	`104`	`}`
`105`	`105`