Uplift clippy::drop_ref to rustc

Author: Urgau

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}`

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 <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 = 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
+}

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,

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)]

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<T>(val: T) {
+    drop(&val); //~ WARN calls to `std::mem::drop`
+    drop(val);
+}
+
+#[allow(dead_code)]
+fn test_similarly_named_function() {
+    fn drop<T>(_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<bool, ()> {
+    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<u8, ()> {
+    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) -> &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`
+        _ => (),
+    }
+}

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
+