From 28cdbc2a641737b291de4e54f9c6cf070d7309be Mon Sep 17 00:00:00 2001 From: Urgau Date: Sun, 26 Mar 2023 12:25:08 +0200 Subject: [PATCH] Uplift clippy::drop_ref to rustc --- compiler/rustc_lint/messages.ftl | 3 + .../rustc_lint/src/drop_forget_useless.rs | 76 +++++++++ compiler/rustc_lint/src/lib.rs | 3 + compiler/rustc_lint/src/lints.rs | 9 ++ tests/ui/lint/drop_ref.rs | 99 ++++++++++++ tests/ui/lint/drop_ref.stderr | 151 ++++++++++++++++++ 6 files changed, 341 insertions(+) create mode 100644 compiler/rustc_lint/src/drop_forget_useless.rs create mode 100644 tests/ui/lint/drop_ref.rs create mode 100644 tests/ui/lint/drop_ref.stderr diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 71cf644eb50..7e6d65db0e8 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -520,3 +520,6 @@ lint_opaque_hidden_inferred_bound = opaque type `{$ty}` does not satisfy its ass .specifically = this associated type bound is unsatisfied for `{$proj_ty}` lint_opaque_hidden_inferred_bound_sugg = add this bound + +lint_drop_ref = calls to `std::mem::drop` with a reference instead of an owned value + .note = argument has type `{$arg_ty}` diff --git a/compiler/rustc_lint/src/drop_forget_useless.rs b/compiler/rustc_lint/src/drop_forget_useless.rs new file mode 100644 index 00000000000..734a43af4a2 --- /dev/null +++ b/compiler/rustc_lint/src/drop_forget_useless.rs @@ -0,0 +1,76 @@ +use rustc_hir::{Arm, Expr, ExprKind, Node}; +use rustc_span::sym; + +use crate::{lints::DropRefDiag, LateContext, LateLintPass, LintContext}; + +declare_lint! { + /// The `drop_ref` lint checks for calls to `std::mem::drop` with a reference + /// instead of an owned value. + /// + /// ### Example + /// + /// ```rust + /// # fn operation_that_requires_mutex_to_be_unlocked() {} // just to make it compile + /// # let mutex = std::sync::Mutex::new(1); // just to make it compile + /// let mut lock_guard = mutex.lock(); + /// std::mem::drop(&lock_guard); // Should have been drop(lock_guard), mutex + /// // still locked + /// operation_that_requires_mutex_to_be_unlocked(); + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Calling `drop` on a reference will only drop the + /// reference itself, which is a no-op. It will not call the `drop` method (from + /// the `Drop` trait implementation) on the underlying referenced value, which + /// is likely what was intended. + pub DROP_REF, + Warn, + "calls to `std::mem::drop` with a reference instead of an owned value" +} + +declare_lint_pass!(DropForgetUseless => [DROP_REF]); + +impl<'tcx> LateLintPass<'tcx> for DropForgetUseless { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Call(path, [arg]) = expr.kind + && let ExprKind::Path(ref qpath) = path.kind + && let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id() + && let Some(fn_name) = cx.tcx.get_diagnostic_name(def_id) + { + let arg_ty = cx.typeck_results().expr_ty(arg); + let drop_is_single_call_in_arm = is_single_call_in_arm(cx, arg, expr); + match fn_name { + sym::mem_drop if arg_ty.is_ref() && !drop_is_single_call_in_arm => { + cx.emit_spanned_lint(DROP_REF, expr.span, DropRefDiag { arg_ty, note: arg.span }); + }, + _ => return, + }; + } + } +} + +// Dropping returned value of a function, as in the following snippet is considered idiomatic, see +// rust-lang/rust-clippy#9482 for examples. +// +// ``` +// match { +// => 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 = cx.tcx.hir().find_parent(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/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 319eb2ea445..5c7016633c2 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -52,6 +52,7 @@ mod array_into_iter; pub mod builtin; mod context; mod deref_into_dyn_supertrait; +mod drop_forget_useless; mod early; mod enum_intrinsics_non_enums; mod errors; @@ -96,6 +97,7 @@ use rustc_span::Span; use array_into_iter::ArrayIntoIter; use builtin::*; use deref_into_dyn_supertrait::*; +use drop_forget_useless::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; use for_loops_over_fallibles::*; use hidden_unicode_codepoints::*; @@ -201,6 +203,7 @@ late_lint_methods!( [ ForLoopsOverFallibles: ForLoopsOverFallibles, DerefIntoDynSupertrait: DerefIntoDynSupertrait, + DropForgetUseless: DropForgetUseless, HardwiredLints: HardwiredLints, ImproperCTypesDeclarations: ImproperCTypesDeclarations, ImproperCTypesDefinitions: ImproperCTypesDefinitions, diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index d7bacc6485f..a8696c81ab6 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/compiler/rustc_lint/src/lints.rs @@ -662,6 +662,15 @@ pub struct ForLoopsOverFalliblesSuggestion<'a> { pub end_span: Span, } +// drop_ref.rs +#[derive(LintDiagnostic)] +#[diag(lint_drop_ref)] +pub struct DropRefDiag<'a> { + pub arg_ty: Ty<'a>, + #[note] + pub note: Span, +} + // hidden_unicode_codepoints.rs #[derive(LintDiagnostic)] #[diag(lint_hidden_unicode_codepoints)] diff --git a/tests/ui/lint/drop_ref.rs b/tests/ui/lint/drop_ref.rs new file mode 100644 index 00000000000..db4f7569f6f --- /dev/null +++ b/tests/ui/lint/drop_ref.rs @@ -0,0 +1,99 @@ +// check-pass + +#![warn(drop_ref)] + +struct SomeStruct; + +fn main() { + drop(&SomeStruct); //~ WARN calls to `std::mem::drop` + + let mut owned1 = SomeStruct; + drop(&owned1); //~ WARN calls to `std::mem::drop` + drop(&&owned1); //~ WARN calls to `std::mem::drop` + drop(&mut owned1); //~ WARN calls to `std::mem::drop` + drop(owned1); + + let reference1 = &SomeStruct; + drop(reference1); //~ WARN calls to `std::mem::drop` + + let reference2 = &mut SomeStruct; + drop(reference2); //~ WARN calls to `std::mem::drop` + + let ref reference3 = SomeStruct; + drop(reference3); //~ WARN calls to `std::mem::drop` +} + +#[allow(dead_code)] +fn test_generic_fn_drop(val: T) { + drop(&val); //~ WARN calls to `std::mem::drop` + drop(val); +} + +#[allow(dead_code)] +fn test_similarly_named_function() { + fn drop(_val: T) {} + drop(&SomeStruct); //OK; call to unrelated function which happens to have the same name + std::mem::drop(&SomeStruct); //~ WARN calls to `std::mem::drop` +} + +#[derive(Copy, Clone)] +pub struct Error; +fn produce_half_owl_error() -> Result<(), Error> { + Ok(()) +} + +fn produce_half_owl_ok() -> Result { + Ok(true) +} + +#[allow(dead_code)] +fn test_owl_result() -> Result<(), ()> { + produce_half_owl_error().map_err(|_| ())?; + produce_half_owl_ok().map(|_| ())?; + // the following should not be linted, + // we should not force users to use toilet closures + // to produce owl results when drop is more convenient + produce_half_owl_error().map_err(drop)?; + produce_half_owl_ok().map_err(drop)?; + Ok(()) +} + +#[allow(dead_code)] +fn test_owl_result_2() -> Result { + produce_half_owl_error().map_err(|_| ())?; + produce_half_owl_ok().map(|_| ())?; + // the following should not be linted, + // we should not force users to use toilet closures + // to produce owl results when drop is more convenient + produce_half_owl_error().map_err(drop)?; + produce_half_owl_ok().map(drop)?; + Ok(1) +} + +#[allow(unused)] +#[allow(clippy::unit_cmp)] +fn issue10122(x: u8) { + // This is a function which returns a reference and has a side-effect, which means + // that calling drop() on the function is considered an idiomatic way of achieving + // the side-effect in a match arm. + fn println_and(t: &T) -> &T { + println!("foo"); + t + } + + match x { + // Don't lint (copy type), we only care about side-effects + 0 => drop(println_and(&12)), + // Don't lint (no copy type), we only care about side-effects + 1 => drop(println_and(&String::new())), + 2 => { + // Lint, even if we only care about the side-effect, it's already in a block + drop(println_and(&13)); //~ WARN calls to `std::mem::drop` + }, + // Lint, idiomatic use is only in body of `Arm` + 3 if drop(println_and(&14)) == () => (), //~ WARN calls to `std::mem::drop` + // Lint, not a fn/method call + 4 => drop(&2), //~ WARN calls to `std::mem::drop` + _ => (), + } +} diff --git a/tests/ui/lint/drop_ref.stderr b/tests/ui/lint/drop_ref.stderr new file mode 100644 index 00000000000..34e4050abcf --- /dev/null +++ b/tests/ui/lint/drop_ref.stderr @@ -0,0 +1,151 @@ +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:8:5 + | +LL | drop(&SomeStruct); + | ^^^^^^^^^^^^^^^^^ + | +note: argument has type `&SomeStruct` + --> $DIR/drop_ref.rs:8:10 + | +LL | drop(&SomeStruct); + | ^^^^^^^^^^^ +note: the lint level is defined here + --> $DIR/drop_ref.rs:3:9 + | +LL | #![warn(drop_ref)] + | ^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:11:5 + | +LL | drop(&owned1); + | ^^^^^^^^^^^^^ + | +note: argument has type `&SomeStruct` + --> $DIR/drop_ref.rs:11:10 + | +LL | drop(&owned1); + | ^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:12:5 + | +LL | drop(&&owned1); + | ^^^^^^^^^^^^^^ + | +note: argument has type `&&SomeStruct` + --> $DIR/drop_ref.rs:12:10 + | +LL | drop(&&owned1); + | ^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:13:5 + | +LL | drop(&mut owned1); + | ^^^^^^^^^^^^^^^^^ + | +note: argument has type `&mut SomeStruct` + --> $DIR/drop_ref.rs:13:10 + | +LL | drop(&mut owned1); + | ^^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:17:5 + | +LL | drop(reference1); + | ^^^^^^^^^^^^^^^^ + | +note: argument has type `&SomeStruct` + --> $DIR/drop_ref.rs:17:10 + | +LL | drop(reference1); + | ^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:20:5 + | +LL | drop(reference2); + | ^^^^^^^^^^^^^^^^ + | +note: argument has type `&mut SomeStruct` + --> $DIR/drop_ref.rs:20:10 + | +LL | drop(reference2); + | ^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:23:5 + | +LL | drop(reference3); + | ^^^^^^^^^^^^^^^^ + | +note: argument has type `&SomeStruct` + --> $DIR/drop_ref.rs:23:10 + | +LL | drop(reference3); + | ^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:28:5 + | +LL | drop(&val); + | ^^^^^^^^^^ + | +note: argument has type `&T` + --> $DIR/drop_ref.rs:28:10 + | +LL | drop(&val); + | ^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:36:5 + | +LL | std::mem::drop(&SomeStruct); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: argument has type `&SomeStruct` + --> $DIR/drop_ref.rs:36:20 + | +LL | std::mem::drop(&SomeStruct); + | ^^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:91:13 + | +LL | drop(println_and(&13)); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: argument has type `&i32` + --> $DIR/drop_ref.rs:91:18 + | +LL | drop(println_and(&13)); + | ^^^^^^^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:94:14 + | +LL | 3 if drop(println_and(&14)) == () => (), + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: argument has type `&i32` + --> $DIR/drop_ref.rs:94:19 + | +LL | 3 if drop(println_and(&14)) == () => (), + | ^^^^^^^^^^^^^^^^ + +warning: calls to `std::mem::drop` with a reference instead of an owned value + --> $DIR/drop_ref.rs:96:14 + | +LL | 4 => drop(&2), + | ^^^^^^^^ + | +note: argument has type `&i32` + --> $DIR/drop_ref.rs:96:19 + | +LL | 4 => drop(&2), + | ^^ + +warning: 12 warnings emitted +