From f8790963d9dd9fd4fd22456eef1cbfe2fb8cb12a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Mon, 23 Oct 2023 14:41:10 +0200 Subject: [PATCH 1/2] Add a lint to check needless `Waker` clones --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/mod.rs | 27 ++++++++++++++++ clippy_lints/src/methods/waker_clone_wake.rs | 34 ++++++++++++++++++++ clippy_utils/src/paths.rs | 1 + tests/ui/waker_clone_wake.fixed | 22 +++++++++++++ tests/ui/waker_clone_wake.rs | 22 +++++++++++++ tests/ui/waker_clone_wake.stderr | 11 +++++++ 8 files changed, 119 insertions(+) create mode 100644 clippy_lints/src/methods/waker_clone_wake.rs create mode 100644 tests/ui/waker_clone_wake.fixed create mode 100644 tests/ui/waker_clone_wake.rs create mode 100644 tests/ui/waker_clone_wake.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 26c9877dbff..b28075d8eb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 77438b27f90..9de4a8598b4 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -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, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a935aea5075..9c8d2711c3d 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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; @@ -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, @@ -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. @@ -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); } diff --git a/clippy_lints/src/methods/waker_clone_wake.rs b/clippy_lints/src/methods/waker_clone_wake.rs new file mode 100644 index 00000000000..db13266db80 --- /dev/null +++ b/clippy_lints/src/methods/waker_clone_wake.rs @@ -0,0 +1,34 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::{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(func, waker_ref, &[], _) = recv.kind + && func.ident.name == sym::clone + { + let mut applicability = Applicability::MachineApplicable; + + span_lint_and_sugg( + cx, + WAKER_CLONE_WAKE, + expr.span, + "cloning a `Waker` only to wake it", + "replace with", + format!( + "{}.wake_by_ref()", + snippet_with_applicability(cx, waker_ref.span, "..", &mut applicability) + ), + applicability, + ); + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 4a20399e364..2cf7e2cde96 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -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 diff --git a/tests/ui/waker_clone_wake.fixed b/tests/ui/waker_clone_wake.fixed new file mode 100644 index 00000000000..2df52f57d65 --- /dev/null +++ b/tests/ui/waker_clone_wake.fixed @@ -0,0 +1,22 @@ +#[derive(Clone)] +pub struct Custom; + +impl Custom { + pub fn wake(self) {} +} + +pub fn wake(cx: &mut std::task::Context) { + cx.waker().wake_by_ref(); + + // We don't do that for now + let w = cx.waker().clone(); + w.wake(); + + cx.waker().clone().wake_by_ref(); +} + +pub fn no_lint(c: &Custom) { + c.clone().wake() +} + +fn main() {} diff --git a/tests/ui/waker_clone_wake.rs b/tests/ui/waker_clone_wake.rs new file mode 100644 index 00000000000..4fe354b0ef1 --- /dev/null +++ b/tests/ui/waker_clone_wake.rs @@ -0,0 +1,22 @@ +#[derive(Clone)] +pub struct Custom; + +impl Custom { + pub fn wake(self) {} +} + +pub fn wake(cx: &mut std::task::Context) { + cx.waker().clone().wake(); + + // We don't do that for now + let w = cx.waker().clone(); + w.wake(); + + cx.waker().clone().wake_by_ref(); +} + +pub fn no_lint(c: &Custom) { + c.clone().wake() +} + +fn main() {} diff --git a/tests/ui/waker_clone_wake.stderr b/tests/ui/waker_clone_wake.stderr new file mode 100644 index 00000000000..426a577e620 --- /dev/null +++ b/tests/ui/waker_clone_wake.stderr @@ -0,0 +1,11 @@ +error: cloning a `Waker` only to wake it + --> $DIR/waker_clone_wake.rs:9: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: aborting due to previous error + From ebf6667b571568e88e4b8934013515cb69db74e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Wed, 25 Oct 2023 15:15:29 +0200 Subject: [PATCH 2/2] Apply suggestions --- clippy_lints/src/methods/waker_clone_wake.rs | 12 +++++------- tests/ui/waker_clone_wake.fixed | 17 ++++++++++++----- tests/ui/waker_clone_wake.rs | 17 ++++++++++++----- tests/ui/waker_clone_wake.stderr | 10 ++++++++-- 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/methods/waker_clone_wake.rs b/clippy_lints/src/methods/waker_clone_wake.rs index db13266db80..da66632d55f 100644 --- a/clippy_lints/src/methods/waker_clone_wake.rs +++ b/clippy_lints/src/methods/waker_clone_wake.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::{match_def_path, paths}; +use clippy_utils::{is_trait_method, match_def_path, paths}; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; @@ -13,10 +13,11 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &' if let Some(did) = ty.ty_adt_def() && match_def_path(cx, did.did(), &paths::WAKER) - && let ExprKind::MethodCall(func, waker_ref, &[], _) = recv.kind - && func.ident.name == sym::clone + && 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, @@ -24,10 +25,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &' expr.span, "cloning a `Waker` only to wake it", "replace with", - format!( - "{}.wake_by_ref()", - snippet_with_applicability(cx, waker_ref.span, "..", &mut applicability) - ), + format!("{snippet}.wake_by_ref()"), applicability, ); } diff --git a/tests/ui/waker_clone_wake.fixed b/tests/ui/waker_clone_wake.fixed index 2df52f57d65..9c02b9a90fd 100644 --- a/tests/ui/waker_clone_wake.fixed +++ b/tests/ui/waker_clone_wake.fixed @@ -5,18 +5,25 @@ 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(); - // We don't do that for now + 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(); } -pub fn no_lint(c: &Custom) { - c.clone().wake() -} - fn main() {} diff --git a/tests/ui/waker_clone_wake.rs b/tests/ui/waker_clone_wake.rs index 4fe354b0ef1..edc3bbd8fc0 100644 --- a/tests/ui/waker_clone_wake.rs +++ b/tests/ui/waker_clone_wake.rs @@ -5,18 +5,25 @@ 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(); - // We don't do that for now + 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(); } -pub fn no_lint(c: &Custom) { - c.clone().wake() -} - fn main() {} diff --git a/tests/ui/waker_clone_wake.stderr b/tests/ui/waker_clone_wake.stderr index 426a577e620..f1abf4d9cb0 100644 --- a/tests/ui/waker_clone_wake.stderr +++ b/tests/ui/waker_clone_wake.stderr @@ -1,5 +1,5 @@ error: cloning a `Waker` only to wake it - --> $DIR/waker_clone_wake.rs:9:5 + --> $DIR/waker_clone_wake.rs:15:5 | LL | cx.waker().clone().wake(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `cx.waker().wake_by_ref()` @@ -7,5 +7,11 @@ LL | cx.waker().clone().wake(); = note: `-D clippy::waker-clone-wake` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::waker_clone_wake)]` -error: aborting due to previous error +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