Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[drop_copy]: Do not lint idiomatic in match arm #9491

Merged
merged 2 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions clippy_lints/src/drop_forget_ref.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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_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(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_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(
cx,
Expand All @@ -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_single_call_in_arm
) =>
{
(DROP_NON_DROP, DROP_NON_DROP_SUMMARY)
},
Expand All @@ -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 <var> {
// <pat> => drop(fn_with_side_effect_and_returning_some_value()),
// ..
// }
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 {
return body.hir_id == drop_expr.hir_id;
}
}
false
}
20 changes: 20 additions & 0 deletions tests/ui/drop_forget_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) -> 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
_ => (),
}
}
38 changes: 37 additions & 1 deletion tests/ui/drop_forget_copy.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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