Changes to let_unit_value

* View through locals in `let_unit_value` when determining if inference is required
* Don't remove typed let bindings for more functions

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,108 @@ 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()
-}

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);
  }
 

clippy_utils/src/visitors.rs

@@ -494,3 +494,49 @@ pub fn for_each_local_use_after_expr<'tcx, B>(
  ControlFlow::Continue(())
  }
 }
+
+/// 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(())
+ }
+}

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;
+ }
+ }
 }