Skip to content

Commit

Permalink
Auto merge of #109732 - Urgau:uplift_drop_forget_ref_lints, r=davidtwco
Browse files Browse the repository at this point in the history
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/rust#99696 (review)

cc `@m-ou-se` (as T-libs-api leader because the uplifting was discussed in a recent meeting)
  • Loading branch information
bors committed May 12, 2023
2 parents 54a0e18 + a15064d commit 83b7d71
Show file tree
Hide file tree
Showing 7 changed files with 22 additions and 9 deletions.
1 change: 1 addition & 0 deletions core/src/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
///
Expand Down
2 changes: 1 addition & 1 deletion core/src/task/poll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down
4 changes: 2 additions & 2 deletions core/src/task/ready.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand All @@ -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(())
Expand Down
2 changes: 1 addition & 1 deletion std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
}
Expand Down
16 changes: 13 additions & 3 deletions std/src/sys/sgx/waitqueue/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion std/src/sys/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
4 changes: 3 additions & 1 deletion std/src/thread/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 83b7d71

Please sign in to comment.