Auto merge of #8563 - Jarcho:let_unit_1502, r=Jarcho

Don't lint `let_unit_value` when needed for type inferenece

fixes: #1502

Pinging `@dtolnay.` I think this is enough to fix the issue. Do you have a good list crates to test this on?

changelog: Don't lint `let_unit_value` when needed for type inference

Author: bors

clippy_lints/src/lib.register_all.rs

@@ -308,6 +308,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(uninit_vec::UNINIT_VEC),
     LintId::of(unit_hash::UNIT_HASH),
     LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD),
+    LintId::of(unit_types::LET_UNIT_VALUE),
     LintId::of(unit_types::UNIT_ARG),
     LintId::of(unit_types::UNIT_CMP),
     LintId::of(unnamed_address::FN_ADDRESS_COMPARISONS),

clippy_lints/src/lib.register_pedantic.rs

@@ -89,7 +89,6 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     LintId::of(types::LINKEDLIST),
     LintId::of(types::OPTION_OPTION),
     LintId::of(unicode::UNICODE_NOT_NFC),
-    LintId::of(unit_types::LET_UNIT_VALUE),
     LintId::of(unnecessary_wraps::UNNECESSARY_WRAPS),
     LintId::of(unnested_or_patterns::UNNESTED_OR_PATTERNS),
     LintId::of(unused_async::UNUSED_ASYNC),

clippy_lints/src/lib.register_style.rs

@@ -107,6 +107,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
     LintId::of(single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
     LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS),
     LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME),
+    LintId::of(unit_types::LET_UNIT_VALUE),
     LintId::of(unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS),
     LintId::of(unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
     LintId::of(unused_unit::UNUSED_UNIT),

clippy_lints/src/unit_types/let_unit_value.rs

@@ -1,18 +1,46 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet_with_macro_callsite;
+use clippy_utils::visitors::for_each_value_source;
+use core::ops::ControlFlow;
 use rustc_errors::Applicability;
-use rustc_hir::{Stmt, StmtKind};
+use rustc_hir::{Expr, ExprKind, PatKind, Stmt, StmtKind};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::{self, Ty, TypeFoldable, TypeVisitor};
 
 use super::LET_UNIT_VALUE;
 
 pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
-    if let StmtKind::Local(local) = stmt.kind {
-        if cx.typeck_results().pat_ty(local.pat).is_unit() {
-            if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() {
-                return;
+    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 !matches!(local.pat.kind, PatKind::Wild) {
+                span_lint_and_then(
+                    cx,
+                    LET_UNIT_VALUE,
+                    stmt.span,
+                    "this let-binding has unit value",
+                    |diag| {
+                            diag.span_suggestion(
+                                local.pat.span,
+                                "use a wild (`_`) binding",
+                                "_".into(),
+                                Applicability::MaybeIncorrect, // snippet
+                            );
+                    },
+                );
             }
+        } else {
             span_lint_and_then(
                 cx,
                 LET_UNIT_VALUE,
@@ -33,3 +61,45 @@ pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
         }
     }
 }
+
+fn needs_inferred_result_ty(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
+    let id = match e.kind {
+        ExprKind::Call(
+            Expr {
+                kind: ExprKind::Path(ref path),
+                hir_id,
+                ..
+            },
+            _,
+        ) => cx.qpath_res(path, *hir_id).opt_def_id(),
+        ExprKind::MethodCall(..) => cx.typeck_results().type_dependent_def_id(e.hir_id),
+        _ => return false,
+    };
+    if let Some(id) = id
+        && let sig = cx.tcx.fn_sig(id).skip_binder()
+        && let ty::Param(output_ty) = *sig.output().kind()
+    {
+        sig.inputs().iter().all(|&ty| !ty_contains_param(ty, output_ty.index))
+    } 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

@@ -23,7 +23,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "pre 1.29.0"]
     pub LET_UNIT_VALUE,
-    pedantic,
+    style,
     "creating a `let` binding to a value of unit type, which usually can't be used afterwards"
 }
 

clippy_utils/src/visitors.rs

@@ -1,4 +1,5 @@
 use crate::path_to_local_id;
+use core::ops::ControlFlow;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::intravisit::{self, walk_block, walk_expr, Visitor};
@@ -402,3 +403,36 @@ pub fn contains_unsafe_block<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>)
     v.visit_expr(e);
     v.found_unsafe
 }
+
+/// Runs the given function for each sub-expression producing the final value consumed by the parent
+/// of the give expression.
+///
+/// e.g. for the following expression
+/// ```rust,ignore
+/// if foo {
+///     f(0)
+/// } else {
+///     1 + 1
+/// }
+/// ```
+/// this will pass both `f(0)` and `1+1` to the given function.
+pub fn for_each_value_source<'tcx, B>(
+    e: &'tcx Expr<'tcx>,
+    f: &mut impl FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B>,
+) -> ControlFlow<B> {
+    match e.kind {
+        ExprKind::Block(Block { expr: Some(e), .. }, _) => for_each_value_source(e, f),
+        ExprKind::Match(_, arms, _) => {
+            for arm in arms {
+                for_each_value_source(arm.body, f)?;
+            }
+            ControlFlow::Continue(())
+        },
+        ExprKind::If(_, if_expr, Some(else_expr)) => {
+            for_each_value_source(if_expr, f)?;
+            for_each_value_source(else_expr, f)
+        },
+        ExprKind::DropTemps(e) => for_each_value_source(e, f),
+        _ => f(e),
+    }
+}

tests/ui-internal/interning_defined_symbol.fixed

@@ -1,6 +1,6 @@
 // run-rustfix
 #![deny(clippy::internal)]
-#![allow(clippy::missing_clippy_version_attribute)]
+#![allow(clippy::missing_clippy_version_attribute, clippy::let_unit_value)]
 #![feature(rustc_private)]
 
 extern crate rustc_span;

tests/ui-internal/interning_defined_symbol.rs

@@ -1,6 +1,6 @@
 // run-rustfix
 #![deny(clippy::internal)]
-#![allow(clippy::missing_clippy_version_attribute)]
+#![allow(clippy::missing_clippy_version_attribute, clippy::let_unit_value)]
 #![feature(rustc_private)]
 
 extern crate rustc_span;

tests/ui-toml/functions_maxlines/test.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::too_many_lines)]
+#![allow(clippy::let_unit_value)]
 
 // This function should be considered one line.
 fn many_comments_but_one_line_of_code() {

tests/ui-toml/functions_maxlines/test.stderr

@@ -1,5 +1,5 @@
 error: this function has too many lines (2/1)
-  --> $DIR/test.rs:18:1
+  --> $DIR/test.rs:19:1
    |
 LL | / fn too_many_lines() {
 LL | |     println!("This is bad.");
@@ -10,7 +10,7 @@ LL | | }
    = note: `-D clippy::too-many-lines` implied by `-D warnings`
 
 error: this function has too many lines (4/1)
-  --> $DIR/test.rs:24:1
+  --> $DIR/test.rs:25:1
    |
 LL | / async fn async_too_many_lines() {
 LL | |     println!("This is bad.");
@@ -19,7 +19,7 @@ LL | | }
    | |_^
 
 error: this function has too many lines (4/1)
-  --> $DIR/test.rs:30:1
+  --> $DIR/test.rs:31:1
    |
 LL | / fn closure_too_many_lines() {
 LL | |     let _ = {
@@ -30,7 +30,7 @@ LL | | }
    | |_^
 
 error: this function has too many lines (2/1)
-  --> $DIR/test.rs:52:1
+  --> $DIR/test.rs:53:1
    |
 LL | / fn comment_before_code() {
 LL | |     let _ = "test";