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] 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 @@ 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 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 @@ "`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 @@ pub fn new( 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 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } 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 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 +