Auto merge of rust-lang#109732 - Urgau:uplift_drop_forget_ref_lints, r=davidtwco

Uplift `clippy::{drop,forget}_{ref,copy}` lints

This PR aims at uplifting the `clippy::drop_ref`, `clippy::drop_copy`, `clippy::forget_ref` and `clippy::forget_copy` lints.

Those lints are/were declared in the correctness category of clippy because they lint on useless and most probably is not what the developer wanted.

## `drop_ref` and `forget_ref`

The `drop_ref` and `forget_ref` lint checks for calls to `std::mem::drop` or `std::mem::forget` with a reference instead of an owned value.

### Example

```rust
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();
```

### Explanation

Calling `drop` or `forget` on a reference will only drop the reference itself, which is a no-op. It will not call the `drop` or `forget` method on the underlying referenced value, which is likely what was intended.

## `drop_copy` and `forget_copy`

The `drop_copy` and `forget_copy` lint checks for calls to `std::mem::forget` or `std::mem::drop` with a value that derives the Copy trait.

### Example

```rust
let x: i32 = 42; // i32 implements Copy
std::mem::forget(x) // A copy of x is passed to the function, leaving the
                    // original unaffected
```

### Explanation

Calling `std::mem::forget` [does nothing for types that implement Copy](https://doc.rust-lang.org/std/mem/fn.drop.html) since the value will be copied and moved into the function on invocation.

-----

Followed the instructions for uplift a clippy describe here: rust-lang#99696 (review)

cc `@m-ou-se` (as T-libs-api leader because the uplifting was discussed in a recent meeting)

Author: bors

compiler/rustc_builtin_macros/src/format_foreign.rs

@@ -562,15 +562,13 @@ pub(crate) mod printf {
         }
 
         if let Type = state {
-            drop(c);
             type_ = at.slice_between(next).unwrap();
 
             // Don't use `move_to!` here, as we *can* be at the end of the input.
             at = next;
         }
 
-        drop(c);
-        drop(next);
+        let _ = c; // to avoid never used value
 
         end = at;
         let position = InnerSpan::new(start.at, end.at);

compiler/rustc_infer/src/infer/nll_relate/mod.rs

@@ -828,7 +828,7 @@ where
                 } else {
                     match variables.probe(vid) {
                         TypeVariableValue::Known { value: u } => {
-                            drop(variables);
+                            drop(inner);
                             self.relate(u, u)
                         }
                         TypeVariableValue::Unknown { universe: _universe } => {

compiler/rustc_lint/messages.ftl

@@ -520,3 +520,19 @@ 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 does nothing
+    .label = argument has type `{$arg_ty}`
+    .note = use `let _ = ...` to ignore the expression or result
+
+lint_drop_copy = calls to `std::mem::drop` with a value that implements `Copy` does nothing
+    .label = argument has type `{$arg_ty}`
+    .note = use `let _ = ...` to ignore the expression or result
+
+lint_forget_ref = calls to `std::mem::forget` with a reference instead of an owned value does nothing
+    .label = argument has type `{$arg_ty}`
+    .note = use `let _ = ...` to ignore the expression or result
+
+lint_forget_copy = calls to `std::mem::forget` with a value that implements `Copy` does nothing
+    .label = argument has type `{$arg_ty}`
+    .note = use `let _ = ...` to ignore the expression or result

compiler/rustc_lint/src/drop_forget_useless.rs

@@ -0,0 +1,164 @@
+use rustc_hir::{Arm, Expr, ExprKind, Node};
+use rustc_span::sym;
+
+use crate::{
+    lints::{DropCopyDiag, DropRefDiag, ForgetCopyDiag, ForgetRefDiag},
+    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! {
+    /// The `forget_ref` lint checks for calls to `std::mem::forget` with a reference
+    /// instead of an owned value.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// let x = Box::new(1);
+    /// std::mem::forget(&x); // Should have been forget(x), x will still be dropped
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Calling `forget` on a reference will only forget the
+    /// reference itself, which is a no-op. It will not forget the underlying
+    /// referenced value, which is likely what was intended.
+    pub FORGET_REF,
+    Warn,
+    "calls to `std::mem::forget` with a reference instead of an owned value"
+}
+
+declare_lint! {
+    /// The `drop_copy` lint checks for calls to `std::mem::drop` with a value
+    /// that derives the Copy trait.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// let x: i32 = 42; // i32 implements Copy
+    /// std::mem::drop(x); // A copy of x is passed to the function, leaving the
+    ///                    // original unaffected
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Calling `std::mem::drop` [does nothing for types that
+    /// implement Copy](https://doc.rust-lang.org/std/mem/fn.drop.html), since the
+    /// value will be copied and moved into the function on invocation.
+    pub DROP_COPY,
+    Warn,
+    "calls to `std::mem::drop` with a value that implements Copy"
+}
+
+declare_lint! {
+    /// The `forget_copy` lint checks for calls to `std::mem::forget` with a value
+    /// that derives the Copy trait.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// let x: i32 = 42; // i32 implements Copy
+    /// std::mem::forget(x); // A copy of x is passed to the function, leaving the
+    ///                      // original unaffected
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Calling `std::mem::forget` [does nothing for types that
+    /// implement Copy](https://doc.rust-lang.org/std/mem/fn.drop.html) since the
+    /// value will be copied and moved into the function on invocation.
+    ///
+    /// An alternative, but also valid, explanation is that Copy types do not
+    /// implement the Drop trait, which means they have no destructors. Without a
+    /// destructor, there is nothing for `std::mem::forget` to ignore.
+    pub FORGET_COPY,
+    Warn,
+    "calls to `std::mem::forget` with a value that implements Copy"
+}
+
+declare_lint_pass!(DropForgetUseless => [DROP_REF, FORGET_REF, DROP_COPY, FORGET_COPY]);
+
+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 is_copy = arg_ty.is_copy_modulo_regions(cx.tcx, cx.param_env);
+            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, label: arg.span });
+                },
+                sym::mem_forget if arg_ty.is_ref() => {
+                    cx.emit_spanned_lint(FORGET_REF, expr.span, ForgetRefDiag { arg_ty, label: arg.span });
+                },
+                sym::mem_drop if is_copy && !drop_is_single_call_in_arm => {
+                    cx.emit_spanned_lint(DROP_COPY, expr.span, DropCopyDiag { arg_ty, label: arg.span });
+                }
+                sym::mem_forget if is_copy => {
+                    cx.emit_spanned_lint(FORGET_COPY, expr.span, ForgetCopyDiag { arg_ty, label: 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,43 @@ pub struct ForLoopsOverFalliblesSuggestion<'a> {
     pub end_span: Span,
 }
 
+// drop_ref.rs
+#[derive(LintDiagnostic)]
+#[diag(lint_drop_ref)]
+#[note]
+pub struct DropRefDiag<'a> {
+    pub arg_ty: Ty<'a>,
+    #[label]
+    pub label: Span,
+}
+
+#[derive(LintDiagnostic)]
+#[diag(lint_drop_copy)]
+#[note]
+pub struct DropCopyDiag<'a> {
+    pub arg_ty: Ty<'a>,
+    #[label]
+    pub label: Span,
+}
+
+#[derive(LintDiagnostic)]
+#[diag(lint_forget_ref)]
+#[note]
+pub struct ForgetRefDiag<'a> {
+    pub arg_ty: Ty<'a>,
+    #[label]
+    pub label: Span,
+}
+
+#[derive(LintDiagnostic)]
+#[diag(lint_forget_copy)]
+#[note]
+pub struct ForgetCopyDiag<'a> {
+    pub arg_ty: Ty<'a>,
+    #[label]
+    pub label: Span,
+}
+
 // hidden_unicode_codepoints.rs
 #[derive(LintDiagnostic)]
 #[diag(lint_hidden_unicode_codepoints)]

compiler/rustc_middle/src/mir/visit.rs

@@ -880,12 +880,11 @@ macro_rules! make_mir_visitor {
             ) {
                 let Constant {
                     span,
-                    user_ty,
+                    user_ty: _, // no visit method for this
                     literal,
                 } = constant;
 
                 self.visit_span($(& $mutability)? *span);
-                drop(user_ty); // no visit method for this
                 match literal {
                     ConstantKind::Ty(ct) => self.visit_ty_const($(&$mutability)? *ct, location),
                     ConstantKind::Val(_, ty) => self.visit_ty($(& $mutability)? *ty, TyContext::Location(location)),

library/core/src/mem/mod.rs

@@ -968,6 +968,7 @@ pub const fn replace<T>(dest: &mut T, src: T) -> T {
 /// Integers and other types implementing [`Copy`] are unaffected by `drop`.
 ///
 /// ```
+/// # #![cfg_attr(not(bootstrap), allow(drop_copy))]
 /// #[derive(Copy, Clone)]
 /// struct Foo(u8);
 ///

library/core/src/task/poll.rs

@@ -116,7 +116,7 @@ impl<T> Poll<T> {
     ///     let fut = Pin::new(&mut fut);
     ///
     ///     let num = fut.poll(cx).ready()?;
-    ///     # drop(num);
+    ///     # let _ = num; // to silence unused warning
     ///     // ... use num
     ///
     ///     Poll::Ready(())

library/core/src/task/ready.rs

@@ -22,7 +22,7 @@ use core::task::Poll;
 ///     let fut = Pin::new(&mut fut);
 ///
 ///     let num = ready!(fut.poll(cx));
-///     # drop(num);
+///     # let _ = num;
 ///     // ... use num
 ///
 ///     Poll::Ready(())
@@ -44,7 +44,7 @@ use core::task::Poll;
 ///     Poll::Ready(t) => t,
 ///     Poll::Pending => return Poll::Pending,
 /// };
-///     # drop(num);
+///     # let _ = num; // to silence unused warning
 ///     # // ... use num
 ///     #
 ///     # Poll::Ready(())

library/std/src/panicking.rs

@@ -541,7 +541,7 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
             // Lazily, the first time this gets called, run the actual string formatting.
             self.string.get_or_insert_with(|| {
                 let mut s = String::new();
-                drop(s.write_fmt(*inner));
+                let _err = s.write_fmt(*inner);
                 s
             })
         }

library/std/src/sys/sgx/waitqueue/mod.rs

@@ -202,12 +202,18 @@ impl WaitQueue {
     pub fn notify_one<T>(
         mut guard: SpinMutexGuard<'_, WaitVariable<T>>,
     ) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
+        // SAFETY: lifetime of the pop() return value is limited to the map
+        // closure (The closure return value is 'static). The underlying
+        // stack frame won't be freed until after the WaitGuard created below
+        // is dropped.
         unsafe {
-            if let Some(entry) = guard.queue.inner.pop() {
+            let tcs = guard.queue.inner.pop().map(|entry| -> Tcs {
                 let mut entry_guard = entry.lock();
-                let tcs = entry_guard.tcs;
                 entry_guard.wake = true;
-                drop(entry);
+                entry_guard.tcs
+            });
+
+            if let Some(tcs) = tcs {
                 Ok(WaitGuard { mutex_guard: Some(guard), notified_tcs: NotifiedTcs::Single(tcs) })
             } else {
                 Err(guard)
@@ -223,13 +229,17 @@ impl WaitQueue {
     pub fn notify_all<T>(
         mut guard: SpinMutexGuard<'_, WaitVariable<T>>,
     ) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
+        // SAFETY: lifetime of the pop() return values are limited to the
+        // while loop body. The underlying stack frames won't be freed until
+        // after the WaitGuard created below is dropped.
         unsafe {
             let mut count = 0;
             while let Some(entry) = guard.queue.inner.pop() {
                 count += 1;
                 let mut entry_guard = entry.lock();
                 entry_guard.wake = true;
             }
+
             if let Some(count) = NonZeroUsize::new(count) {
                 Ok(WaitGuard { mutex_guard: Some(guard), notified_tcs: NotifiedTcs::All { count } })
             } else {

library/std/src/sys/unix/fs.rs

@@ -1210,7 +1210,7 @@ impl File {
                 // Redox doesn't appear to support `UTIME_OMIT`.
                 // ESP-IDF and HorizonOS do not support `futimens` at all and the behavior for those OS is therefore
                 // the same as for Redox.
-                drop(times);
+                let _ = times;
                 Err(io::const_io_error!(
                     io::ErrorKind::Unsupported,
                     "setting file times not supported",

library/std/src/thread/tests.rs

@@ -375,7 +375,9 @@ fn test_scoped_threads_nll() {
     // this is mostly a *compilation test* for this exact function:
     fn foo(x: &u8) {
         thread::scope(|s| {
-            s.spawn(|| drop(x));
+            s.spawn(|| match x {
+                _ => (),
+            });
         });
     }
     // let's also run it for good measure