From 88a57963da0d55f5a27c7b3dfe899d972136659c Mon Sep 17 00:00:00 2001 From: kraktus Date: Sat, 17 Sep 2022 15:33:38 +0200 Subject: [PATCH 1/2] [`drop_copy`]: Do not lint idiomatic in match arm --- clippy_lints/src/drop_forget_ref.rs | 28 ++++++++++++++++++--- tests/ui/drop_forget_copy.rs | 20 +++++++++++++++ tests/ui/drop_forget_copy.stderr | 38 ++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/drop_forget_ref.rs b/clippy_lints/src/drop_forget_ref.rs index b35f0b8ca52da..de94b87cc409f 100644 --- a/clippy_lints/src/drop_forget_ref.rs +++ b/clippy_lints/src/drop_forget_ref.rs @@ -1,7 +1,8 @@ use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_note}; +use clippy_utils::get_parent_node; use clippy_utils::is_must_use_func_call; use clippy_utils::ty::{is_copy, is_must_use_ty, is_type_lang_item}; -use rustc_hir::{Expr, ExprKind, LangItem}; +use rustc_hir::{Arm, Expr, ExprKind, LangItem, Node}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::sym; @@ -202,11 +203,13 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { && let Some(fn_name) = cx.tcx.get_diagnostic_name(def_id) { let arg_ty = cx.typeck_results().expr_ty(arg); + let is_copy = is_copy(cx, arg_ty); + let drop_is_only_expr_in_arm = and_only_expr_in_arm(cx, arg, expr); let (lint, msg) = match fn_name { sym::mem_drop if arg_ty.is_ref() => (DROP_REF, DROP_REF_SUMMARY), sym::mem_forget if arg_ty.is_ref() => (FORGET_REF, FORGET_REF_SUMMARY), - sym::mem_drop if is_copy(cx, arg_ty) => (DROP_COPY, DROP_COPY_SUMMARY), - sym::mem_forget if is_copy(cx, arg_ty) => (FORGET_COPY, FORGET_COPY_SUMMARY), + sym::mem_drop if is_copy && !drop_is_only_expr_in_arm => (DROP_COPY, DROP_COPY_SUMMARY), + sym::mem_forget if is_copy => (FORGET_COPY, FORGET_COPY_SUMMARY), sym::mem_drop if is_type_lang_item(cx, arg_ty, LangItem::ManuallyDrop) => { span_lint_and_help( cx, @@ -221,7 +224,9 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { sym::mem_drop if !(arg_ty.needs_drop(cx.tcx, cx.param_env) || is_must_use_func_call(cx, arg) - || is_must_use_ty(cx, arg_ty)) => + || is_must_use_ty(cx, arg_ty) + || drop_is_only_expr_in_arm + ) => { (DROP_NON_DROP, DROP_NON_DROP_SUMMARY) }, @@ -241,3 +246,18 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { } } } + +// dropping returned value of a function like in the following snippet is considered idiomatic, see +// #9482 for examples match { +// => drop(fn_with_side_effect_and_returning_some_value()), +// .. +// } +fn and_only_expr_in_arm<'tcx>(cx: &LateContext<'tcx>, arg: &'tcx Expr<'_>, drop_expr: &'tcx Expr<'_>) -> bool { + if matches!(arg.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)) { + let parent_node = get_parent_node(cx.tcx, drop_expr.hir_id); + if let Some(Node::Arm(Arm { body, .. })) = &parent_node { + return body.hir_id == drop_expr.hir_id; + } + } + false +} diff --git a/tests/ui/drop_forget_copy.rs b/tests/ui/drop_forget_copy.rs index 7c7a9ecff67f5..a7276dd59f434 100644 --- a/tests/ui/drop_forget_copy.rs +++ b/tests/ui/drop_forget_copy.rs @@ -64,3 +64,23 @@ fn main() { let a5 = a1.clone(); forget(a5); } + +#[allow(unused)] +#[allow(clippy::unit_cmp)] +fn issue9482(x: u8) { + fn println_and(t: T) -> T { + println!("foo"); + t + } + + match x { + 0 => drop(println_and(12)), // Don't lint (copy type), we only care about side-effects + 1 => drop(println_and(String::new())), // Don't lint (no copy type), we only care about side-effects + 2 => { + drop(println_and(13)); // Lint, even if we only care about the side-effect, it's already in a block + }, + 3 if drop(println_and(14)) == () => (), // Lint, idiomatic use is only in body of `Arm` + 4 => drop(2), // Lint, not a fn/method call + _ => (), + } +} diff --git a/tests/ui/drop_forget_copy.stderr b/tests/ui/drop_forget_copy.stderr index 88228afae89c0..ec7dd2f08f1d5 100644 --- a/tests/ui/drop_forget_copy.stderr +++ b/tests/ui/drop_forget_copy.stderr @@ -72,5 +72,41 @@ note: argument has type `SomeStruct` LL | forget(s4); | ^^ -error: aborting due to 6 previous errors +error: calls to `std::mem::drop` with a value that implements `Copy`. Dropping a copy leaves the original intact + --> $DIR/drop_forget_copy.rs:80:13 + | +LL | drop(println_and(13)); // Lint, even if we only care about the side-effect, it's already in a block + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: argument has type `i32` + --> $DIR/drop_forget_copy.rs:80:18 + | +LL | drop(println_and(13)); // Lint, even if we only care about the side-effect, it's already in a block + | ^^^^^^^^^^^^^^^ + +error: calls to `std::mem::drop` with a value that implements `Copy`. Dropping a copy leaves the original intact + --> $DIR/drop_forget_copy.rs:82:14 + | +LL | 3 if drop(println_and(14)) == () => (), // Lint, idiomatic use is only in body of `Arm` + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: argument has type `i32` + --> $DIR/drop_forget_copy.rs:82:19 + | +LL | 3 if drop(println_and(14)) == () => (), // Lint, idiomatic use is only in body of `Arm` + | ^^^^^^^^^^^^^^^ + +error: calls to `std::mem::drop` with a value that implements `Copy`. Dropping a copy leaves the original intact + --> $DIR/drop_forget_copy.rs:83:14 + | +LL | 4 => drop(2), // Lint, not a fn/method call + | ^^^^^^^ + | +note: argument has type `i32` + --> $DIR/drop_forget_copy.rs:83:19 + | +LL | 4 => drop(2), // Lint, not a fn/method call + | ^ + +error: aborting due to 9 previous errors From 187c27e6b0c756db340e4acdb18b9aae7fd1a1df Mon Sep 17 00:00:00 2001 From: kraktus Date: Tue, 27 Sep 2022 21:01:23 +0200 Subject: [PATCH 2/2] rename `and_only_expr_in_arm` -> `is_single_call_in_arm` --- clippy_lints/src/drop_forget_ref.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/drop_forget_ref.rs b/clippy_lints/src/drop_forget_ref.rs index de94b87cc409f..81d04935d8966 100644 --- a/clippy_lints/src/drop_forget_ref.rs +++ b/clippy_lints/src/drop_forget_ref.rs @@ -204,11 +204,11 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { { let arg_ty = cx.typeck_results().expr_ty(arg); let is_copy = is_copy(cx, arg_ty); - let drop_is_only_expr_in_arm = and_only_expr_in_arm(cx, arg, expr); + let drop_is_single_call_in_arm = is_single_call_in_arm(cx, arg, expr); let (lint, msg) = match fn_name { sym::mem_drop if arg_ty.is_ref() => (DROP_REF, DROP_REF_SUMMARY), sym::mem_forget if arg_ty.is_ref() => (FORGET_REF, FORGET_REF_SUMMARY), - sym::mem_drop if is_copy && !drop_is_only_expr_in_arm => (DROP_COPY, DROP_COPY_SUMMARY), + sym::mem_drop if is_copy && !drop_is_single_call_in_arm => (DROP_COPY, DROP_COPY_SUMMARY), sym::mem_forget if is_copy => (FORGET_COPY, FORGET_COPY_SUMMARY), sym::mem_drop if is_type_lang_item(cx, arg_ty, LangItem::ManuallyDrop) => { span_lint_and_help( @@ -225,7 +225,7 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { if !(arg_ty.needs_drop(cx.tcx, cx.param_env) || is_must_use_func_call(cx, arg) || is_must_use_ty(cx, arg_ty) - || drop_is_only_expr_in_arm + || drop_is_single_call_in_arm ) => { (DROP_NON_DROP, DROP_NON_DROP_SUMMARY) @@ -252,7 +252,7 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { // => drop(fn_with_side_effect_and_returning_some_value()), // .. // } -fn and_only_expr_in_arm<'tcx>(cx: &LateContext<'tcx>, arg: &'tcx Expr<'_>, drop_expr: &'tcx Expr<'_>) -> bool { +fn is_single_call_in_arm<'tcx>(cx: &LateContext<'tcx>, arg: &'tcx Expr<'_>, drop_expr: &'tcx Expr<'_>) -> bool { if matches!(arg.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)) { let parent_node = get_parent_node(cx.tcx, drop_expr.hir_id); if let Some(Node::Arm(Arm { body, .. })) = &parent_node {