From 70f7c624e442c3049d440cd20f8d77b66580105c 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/cast_alignment.rs | 6 ++-- tests/ui/let_unit.fixed | 25 ++++++++++++++ tests/ui/let_unit.rs | 25 ++++++++++++++ tests/ui/let_unit.stderr | 31 ++++++++++++++++- tests/ui/or_then_unwrap.fixed | 2 +- tests/ui/or_then_unwrap.rs | 2 +- tests/ui/panicking_macros.rs | 2 +- 9 files changed, 129 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs index 39352b3ee47..96858fc7f27 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 @@ 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 3db64b25353..c00bc2bd213 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}; @@ -402,3 +403,36 @@ fn visit_block(&mut self, b: &'tcx Block<'_>) { 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, +) -> 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/cast_alignment.rs b/tests/ui/cast_alignment.rs index e4e7290a30e..95bb883df1b 100644 --- a/tests/ui/cast_alignment.rs +++ b/tests/ui/cast_alignment.rs @@ -44,8 +44,8 @@ fn main() { let _ = core::ptr::read_unaligned(ptr as *const u16); let _ = core::intrinsics::unaligned_volatile_load(ptr as *const u16); let ptr = &mut data as *mut [u8; 2] as *mut u8; - let _ = (ptr as *mut u16).write_unaligned(0); - let _ = core::ptr::write_unaligned(ptr as *mut u16, 0); - let _ = core::intrinsics::unaligned_volatile_store(ptr as *mut u16, 0); + (ptr as *mut u16).write_unaligned(0); + core::ptr::write_unaligned(ptr as *mut u16, 0); + core::intrinsics::unaligned_volatile_store(ptr as *mut u16, 0); } } diff --git a/tests/ui/let_unit.fixed b/tests/ui/let_unit.fixed index 5fee742b192..e72b7462325 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 505e4a7d8fd..47ee0a76724 100644 --- a/tests/ui/let_unit.rs +++ b/tests/ui/let_unit.rs @@ -87,4 +87,29 @@ fn f4(mut x: Vec) -> T { 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 c4a766bb974..13ec11a6d33 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/or_then_unwrap.fixed b/tests/ui/or_then_unwrap.fixed index 6e0d5a87f68..844cc4b7a09 100644 --- a/tests/ui/or_then_unwrap.fixed +++ b/tests/ui/or_then_unwrap.fixed @@ -1,7 +1,7 @@ // run-rustfix #![warn(clippy::or_then_unwrap)] -#![allow(clippy::map_identity)] +#![allow(clippy::map_identity, clippy::let_unit_value)] struct SomeStruct; impl SomeStruct { diff --git a/tests/ui/or_then_unwrap.rs b/tests/ui/or_then_unwrap.rs index e406a71d46d..1528ef9be96 100644 --- a/tests/ui/or_then_unwrap.rs +++ b/tests/ui/or_then_unwrap.rs @@ -1,7 +1,7 @@ // run-rustfix #![warn(clippy::or_then_unwrap)] -#![allow(clippy::map_identity)] +#![allow(clippy::map_identity, clippy::let_unit_value)] struct SomeStruct; impl SomeStruct { diff --git a/tests/ui/panicking_macros.rs b/tests/ui/panicking_macros.rs index 12a0c776ae2..041ef17fa68 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)]