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

Add waker_clone_and_wake lint to check needless Waker clones #11698

Merged
merged 2 commits into from
Oct 26, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5589,6 +5589,7 @@ Released 2018-09-13
[`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask
[`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads
[`vtable_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons
[`waker_clone_wake`]: https://rust-lang.github.io/rust-clippy/master/index.html#waker_clone_wake
[`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition
[`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop
[`while_let_on_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_on_iterator
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::USELESS_ASREF_INFO,
crate::methods::VEC_RESIZE_TO_ZERO_INFO,
crate::methods::VERBOSE_FILE_READS_INFO,
crate::methods::WAKER_CLONE_WAKE_INFO,
crate::methods::WRONG_SELF_CONVENTION_INFO,
crate::methods::ZST_OFFSET_INFO,
crate::min_ident_chars::MIN_IDENT_CHARS_INFO,
Expand Down
27 changes: 27 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ mod useless_asref;
mod utils;
mod vec_resize_to_zero;
mod verbose_file_reads;
mod waker_clone_wake;
mod wrong_self_convention;
mod zst_offset;

Expand Down Expand Up @@ -3632,6 +3633,28 @@ declare_clippy_lint! {
"`as_str` used to call a method on `str` that is also available on `String`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `waker.clone().wake()`
///
/// ### Why is this bad?
/// Cloning the waker is not necessary, `wake_by_ref()` enables the same operation
/// without extra cloning/dropping.
///
/// ### Example
/// ```rust,ignore
/// waker.clone().wake();
/// ```
/// Should be written
/// ```rust,ignore
/// waker.wake_by_ref();
/// ```
#[clippy::version = "1.75.0"]
pub WAKER_CLONE_WAKE,
perf,
"cloning a `Waker` only to wake it"
}

pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
Expand Down Expand Up @@ -3777,6 +3800,7 @@ impl_lint_pass!(Methods => [
ITER_OUT_OF_BOUNDS,
PATH_ENDS_WITH_EXT,
REDUNDANT_AS_STR,
WAKER_CLONE_WAKE,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -4365,6 +4389,9 @@ impl Methods {
}
unnecessary_literal_unwrap::check(cx, expr, recv, name, args);
},
("wake", []) => {
waker_clone_wake::check(cx, expr, recv);
}
("write", []) => {
readonly_write_lock::check(cx, expr, recv);
}
Expand Down
32 changes: 32 additions & 0 deletions clippy_lints/src/methods/waker_clone_wake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{is_trait_method, match_def_path, paths};
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::LateContext;
use rustc_span::sym;

use super::WAKER_CLONE_WAKE;

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) {
let ty = cx.typeck_results().expr_ty(recv);

if let Some(did) = ty.ty_adt_def()
&& match_def_path(cx, did.did(), &paths::WAKER)
&& let ExprKind::MethodCall(_, waker_ref, &[], _) = recv.kind
&& is_trait_method(cx, recv, sym::Clone)
{
let mut applicability = Applicability::MachineApplicable;
let snippet = snippet_with_applicability(cx, waker_ref.span.source_callsite(), "..", &mut applicability);

span_lint_and_sugg(
cx,
WAKER_CLONE_WAKE,
expr.span,
"cloning a `Waker` only to wake it",
"replace with",
format!("{snippet}.wake_by_ref()"),
applicability,
);
}
}
1 change: 1 addition & 0 deletions clippy_utils/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"];
pub const INSTANT_NOW: [&str; 4] = ["std", "time", "Instant", "now"];
pub const VEC_IS_EMPTY: [&str; 4] = ["alloc", "vec", "Vec", "is_empty"];
pub const VEC_POP: [&str; 4] = ["alloc", "vec", "Vec", "pop"];
pub const WAKER: [&str; 4] = ["core", "task", "wake", "Waker"];
pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"];
#[expect(clippy::invalid_paths)] // not sure why it thinks this, it works so
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/waker_clone_wake.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#[derive(Clone)]
pub struct Custom;

impl Custom {
pub fn wake(self) {}
}

macro_rules! mac {
($cx:ident) => {
$cx.waker()
};
}

pub fn wake(cx: &mut std::task::Context) {
cx.waker().wake_by_ref();

mac!(cx).wake_by_ref();
}

pub fn no_lint(cx: &mut std::task::Context, c: &Custom) {
c.clone().wake();

let w = cx.waker().clone();
w.wake();

cx.waker().clone().wake_by_ref();
}

fn main() {}
29 changes: 29 additions & 0 deletions tests/ui/waker_clone_wake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#[derive(Clone)]
pub struct Custom;

impl Custom {
pub fn wake(self) {}
}

macro_rules! mac {
($cx:ident) => {
$cx.waker()
};
}

pub fn wake(cx: &mut std::task::Context) {
cx.waker().clone().wake();

mac!(cx).clone().wake();
}

pub fn no_lint(cx: &mut std::task::Context, c: &Custom) {
c.clone().wake();

let w = cx.waker().clone();
w.wake();

cx.waker().clone().wake_by_ref();
a1phyr marked this conversation as resolved.
Show resolved Hide resolved
}

fn main() {}
17 changes: 17 additions & 0 deletions tests/ui/waker_clone_wake.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
error: cloning a `Waker` only to wake it
--> $DIR/waker_clone_wake.rs:15:5
|
LL | cx.waker().clone().wake();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `cx.waker().wake_by_ref()`
|
= note: `-D clippy::waker-clone-wake` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::waker_clone_wake)]`

error: cloning a `Waker` only to wake it
--> $DIR/waker_clone_wake.rs:17:5
|
LL | mac!(cx).clone().wake();
| ^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `mac!(cx).wake_by_ref()`

error: aborting due to 2 previous errors

Loading