From a81ac09c64412011c210dbbd6234a00e29555797 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 4 Apr 2022 21:54:34 -0400 Subject: [PATCH] Allow more complex expressions in `let_unit_value` --- clippy_lints/src/unit_types/let_unit_value.rs | 10 +++++- clippy_utils/src/visitors.rs | 34 +++++++++++++++++++ tests/ui/let_unit.fixed | 25 ++++++++++++++ tests/ui/let_unit.rs | 25 ++++++++++++++ tests/ui/let_unit.stderr | 31 ++++++++++++++++- tests/ui/panicking_macros.rs | 2 +- 6 files changed, 124 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs index 39352b3ee476..96858fc7f274 100644 --- a/clippy_lints/src/unit_types/let_unit_value.rs +++ b/clippy_lints/src/unit_types/let_unit_value.rs @@ -1,5 +1,6 @@ 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::{Expr, ExprKind, PatKind, Stmt, StmtKind}; @@ -11,11 +12,18 @@ use super::LET_UNIT_VALUE; pub(super) fn check(cx: &LateContext<'_>, stmt: &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() { - if local.init.map_or(false, |e| needs_inferred_result_ty(cx, e)) { + 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, diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs index 40451b17a9c6..198c55502bdc 100644 --- a/clippy_utils/src/visitors.rs +++ b/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}; @@ -370,3 +371,36 @@ pub fn is_expr_unsafe<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { v.visit_expr(e); v.is_unsafe } + +/// Runs the given function for each sub expression producing the final value consumed 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, +) -> ControlFlow { + 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), + } +} diff --git a/tests/ui/let_unit.fixed b/tests/ui/let_unit.fixed index 5fee742b1924..e72b74623255 100644 --- a/tests/ui/let_unit.fixed +++ b/tests/ui/let_unit.fixed @@ -87,4 +87,29 @@ fn _returns_generic() { f4(vec![()]); // Lint f4(vec![()]); // Lint + + // Ok + let _: () = { + let x = 5; + f2(x) + }; + + let _: () = if true { f() } else { f2(0) }; // Ok + let _: () = if true { f() } else { f2(0) }; // Lint + + // Ok + let _: () = match Some(0) { + None => f2(1), + Some(0) => f(), + Some(1) => f2(3), + Some(_) => f2('x'), + }; + + // Lint + match Some(0) { + None => f2(1), + Some(0) => f(), + Some(1) => f2(3), + Some(_) => (), + }; } diff --git a/tests/ui/let_unit.rs b/tests/ui/let_unit.rs index 505e4a7d8fdd..47ee0a767247 100644 --- a/tests/ui/let_unit.rs +++ b/tests/ui/let_unit.rs @@ -87,4 +87,29 @@ fn _returns_generic() { let _: () = f4(vec![()]); // Lint let x: () = f4(vec![()]); // Lint + + // Ok + let _: () = { + let x = 5; + f2(x) + }; + + let _: () = if true { f() } else { f2(0) }; // Ok + let x: () = if true { f() } else { f2(0) }; // Lint + + // Ok + let _: () = match Some(0) { + None => f2(1), + Some(0) => f(), + Some(1) => f2(3), + Some(_) => f2('x'), + }; + + // Lint + let _: () = match Some(0) { + None => f2(1), + Some(0) => f(), + Some(1) => f2(3), + Some(_) => (), + }; } diff --git a/tests/ui/let_unit.stderr b/tests/ui/let_unit.stderr index c4a766bb9744..13ec11a6d33e 100644 --- a/tests/ui/let_unit.stderr +++ b/tests/ui/let_unit.stderr @@ -74,5 +74,34 @@ error: this let-binding has unit value LL | let x: () = f4(vec![()]); // Lint | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f4(vec![()]);` -error: aborting due to 9 previous errors +error: this let-binding has unit value + --> $DIR/let_unit.rs:98:5 + | +LL | let x: () = if true { f() } else { f2(0) }; // Lint + | ^^^^-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | help: use a wild (`_`) binding: `_` + +error: this let-binding has unit value + --> $DIR/let_unit.rs:109:5 + | +LL | / let _: () = match Some(0) { +LL | | None => f2(1), +LL | | Some(0) => f(), +LL | | Some(1) => f2(3), +LL | | Some(_) => (), +LL | | }; + | |______^ + | +help: omit the `let` binding + | +LL ~ match Some(0) { +LL + None => f2(1), +LL + Some(0) => f(), +LL + Some(1) => f2(3), +LL + Some(_) => (), +LL + }; + | + +error: aborting due to 11 previous errors diff --git a/tests/ui/panicking_macros.rs b/tests/ui/panicking_macros.rs index 12a0c776ae2f..041ef17fa683 100644 --- a/tests/ui/panicking_macros.rs +++ b/tests/ui/panicking_macros.rs @@ -1,4 +1,4 @@ -#![allow(clippy::assertions_on_constants, clippy::eq_op)] +#![allow(clippy::assertions_on_constants, clippy::eq_op, clippy::let_unit_value)] #![feature(inline_const)] #![warn(clippy::unimplemented, clippy::unreachable, clippy::todo, clippy::panic)]