From 68e9a60b62a4447f0380c7ee88581b569cb8ae44 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 23 Oct 2024 17:54:51 +0000 Subject: [PATCH] new lint: `unnecessary_box_pin` --- CHANGELOG.md | 1 + README.md | 2 +- book/src/README.md | 2 +- book/src/lint_configuration.md | 1 + clippy_config/src/conf.rs | 1 + clippy_config/src/msrvs.rs | 2 +- clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unnecessary_box_pin.rs | 184 ++++++++++++++++++++++++ tests/ui/unnecessary_box_pin.fixed | 61 ++++++++ tests/ui/unnecessary_box_pin.rs | 61 ++++++++ tests/ui/unnecessary_box_pin.stderr | 62 ++++++++ 12 files changed, 377 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/unnecessary_box_pin.rs create mode 100644 tests/ui/unnecessary_box_pin.fixed create mode 100644 tests/ui/unnecessary_box_pin.rs create mode 100644 tests/ui/unnecessary_box_pin.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bdbc91db939..e2b8cc7ed0cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6053,6 +6053,7 @@ Released 2018-09-13 [`unit_hash`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_hash [`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord [`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints +[`unnecessary_box_pin`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_pin [`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast [`unnecessary_clippy_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_clippy_cfg diff --git a/README.md b/README.md index ec76a6dfb08e..1690e2beb16f 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category. diff --git a/book/src/README.md b/book/src/README.md index 7bdfb97c3acf..23527ba896af 100644 --- a/book/src/README.md +++ b/book/src/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 43b551ae2161..ec2fe3029545 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -726,6 +726,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`type_repetition_in_bounds`](https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds) * [`unchecked_duration_subtraction`](https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction) * [`uninlined_format_args`](https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args) +* [`unnecessary_box_pin`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_pin) * [`unnecessary_lazy_evaluations`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations) * [`unnested_or_patterns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns) * [`unused_trait_names`](https://rust-lang.github.io/rust-clippy/master/index.html#unused_trait_names) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 4757c0b1339a..acc0454df52b 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -586,6 +586,7 @@ define_Conf! { type_repetition_in_bounds, unchecked_duration_subtraction, uninlined_format_args, + unnecessary_box_pin, unnecessary_lazy_evaluations, unnested_or_patterns, unused_trait_names, diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index 2f4da4cba3df..5cf75676c1dd 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -26,7 +26,7 @@ msrv_aliases! { 1,73,0 { MANUAL_DIV_CEIL } 1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE } 1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN } - 1,68,0 { PATH_MAIN_SEPARATOR_STR } + 1,68,0 { PATH_MAIN_SEPARATOR_STR, PIN_MACRO } 1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS } 1,63,0 { CLONE_INTO } 1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE, CONST_EXTERN_C_FN } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index d8918d37afa9..269255555c15 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -739,6 +739,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::unit_types::UNIT_ARG_INFO, crate::unit_types::UNIT_CMP_INFO, crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO, + crate::unnecessary_box_pin::UNNECESSARY_BOX_PIN_INFO, crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO, crate::unnecessary_literal_bound::UNNECESSARY_LITERAL_BOUND_INFO, crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a0ff8316d5cd..7219fd4f204a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -362,6 +362,7 @@ mod uninit_vec; mod unit_return_expecting_ord; mod unit_types; mod unnamed_address; +mod unnecessary_box_pin; mod unnecessary_box_returns; mod unnecessary_literal_bound; mod unnecessary_map_on_constructor; @@ -949,5 +950,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf))); store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp)); store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound)); + store.register_late_pass(move |_| Box::new(unnecessary_box_pin::UnnecessaryBoxPin::new(conf))); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unnecessary_box_pin.rs b/clippy_lints/src/unnecessary_box_pin.rs new file mode 100644 index 000000000000..1c0ec34625ec --- /dev/null +++ b/clippy_lints/src/unnecessary_box_pin.rs @@ -0,0 +1,184 @@ +use clippy_config::msrvs::Msrv; +use clippy_config::{Conf, msrvs}; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::visitors::for_each_local_use_after_expr; +use clippy_utils::{is_from_proc_macro, std_or_core}; +use rustc_errors::Applicability; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::{Expr, ExprKind, LetStmt, Node, PatKind, QPath}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::impl_lint_pass; +use rustc_span::{Span, sym}; +use std::ops::ControlFlow; + +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to `Box::pin` with the `Pin>` not moved and only used in places where a `Pin<&mut _>` + /// suffices, in which case the `pin!` macro can be used. + /// + /// ### Why is this bad? + /// `Box::pin` creates an extra heap allocation for the pointee, while `pin!` creates a local `Pin<&mut T>`, + /// so this saves an extra heap allocation. + /// + /// See the documentation for [`pin!`](https://doc.rust-lang.org/stable/std/pin/macro.pin.html) + /// for a more detailed explanation on how these two differ. + /// + /// ### Known issues + /// Currently the lint is fairly limited and only emits a warning if the pinned box is used through `.as_mut()` + /// to prevent false positives w.r.t. lifetimes + /// (`Pin>` returned by `Box::pin` is `'static`, `Pin<&mut _>` returned by `pin!` is not). + /// + /// The following works with `Box::pin` but not with `pin!`: + /// ``` + /// fn assert_static(_: T) {} + /// assert_static(Box::pin(async {})); + /// ``` + /// + /// Restricting to only lint `.as_mut()` means that we end up with a temporary in both cases, + /// so if it compiled with `.as_mut()`, then it ought to work with `pin!` as well. + /// + /// ### Example + /// ```no_run + /// # #![feature(noop_waker)] + /// # use std::task::{Poll, Waker, Context}; + /// # use std::future::Future; + /// + /// fn now_or_never(fut: F) -> Option { + /// let mut fut = Box::pin(fut); + /// + /// match fut.as_mut().poll(&mut Context::from_waker(Waker::noop())) { + /// Poll::Ready(val) => Some(val), + /// Poll::Pending => None + /// } + /// } + /// ``` + /// Use instead: + /// ```no_run + /// # #![feature(noop_waker)] + /// # use std::task::{Poll, Waker, Context}; + /// # use std::future::Future; + /// + /// fn now_or_never(fut: F) -> Option { + /// let mut fut = std::pin::pin!(fut); + /// + /// match fut.as_mut().poll(&mut Context::from_waker(Waker::noop())) { + /// Poll::Ready(val) => Some(val), + /// Poll::Pending => None + /// } + /// } + /// ``` + #[clippy::version = "1.84.0"] + pub UNNECESSARY_BOX_PIN, + perf, + "using `Box::pin` where `pin!` suffices" +} + +pub struct UnnecessaryBoxPin { + msrv: Msrv, +} + +impl UnnecessaryBoxPin { + pub fn new(conf: &'static Conf) -> Self { + Self { + msrv: conf.msrv.clone(), + } + } +} + +impl_lint_pass!(UnnecessaryBoxPin => [UNNECESSARY_BOX_PIN]); + +impl<'tcx> LateLintPass<'tcx> for UnnecessaryBoxPin { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Call(callee, [_]) = expr.kind + && let ExprKind::Path(QPath::TypeRelative(bx, segment)) = &callee.kind + && cx.typeck_results().node_type(bx.hir_id).is_box() + && segment.ident.name == sym::pin + && let Some(enclosing_body) = cx.enclosing_body + && let Some(std_or_core) = std_or_core(cx) + && self.msrv.meets(msrvs::PIN_MACRO) + && !in_external_macro(cx.sess(), expr.span) + && !is_from_proc_macro(cx, expr) + { + let enclosing_body_def_id = cx.tcx.hir().body_owner_def_id(enclosing_body); + + if let ControlFlow::Continue(as_mut_span) = check_pin_box_use(cx, expr, false, enclosing_body_def_id) { + span_lint_and_then( + cx, + UNNECESSARY_BOX_PIN, + expr.span, + "pinning a value with `Box::pin` when local pinning suffices", + |diag| { + let mut replacements = vec![(callee.span, format!("{std_or_core}::pin::pin!"))]; + replacements.extend(as_mut_span.map(|span| (span, String::new()))); + + diag.multipart_suggestion_verbose( + "use the `pin!` macro", + replacements, + Applicability::MachineApplicable, + ); + }, + ); + } + } + } + + extract_msrv_attr!(LateContext); +} + +/// Checks how a `Pin>` is used. Returns `Continue(span)` if this use is valid with +/// `Box::pin` changed to `pin!`. +/// +/// The span is the `.as_mut()` span that can be safely removed. +/// Note that it's currently only returned if `Box::pin()` is the receiver of it (and not first +/// stored in a binding) to avoid move errors. +/// +/// That is, `as_mut` can be safely removed here: +/// ```ignore +/// - Box::pin(async {}).as_mut().poll(...); +/// + pin!(async {}).poll(...); +/// ``` +/// +/// but not here, as the poll call consumes it and the binding cannot be used again in subsequent +/// iterations: +/// ```ignore +/// - let mut bx = Box::pin(async {}); +/// + let mut bx = pin!(async {}); +/// loop { +/// - bx.as_mut().poll(...); +/// + bx.poll(...); +/// } +/// ``` +fn check_pin_box_use<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + moved: bool, + enclosing_body: LocalDefId, +) -> ControlFlow<(), Option> { + match cx.tcx.parent_hir_node(expr.hir_id) { + Node::Expr(as_mut_expr) + if let ExprKind::MethodCall(segment, recv, [], span) = as_mut_expr.kind + && recv.hir_id == expr.hir_id + && segment.ident.name.as_str() == "as_mut" => + { + ControlFlow::Continue((!moved).then(|| span.with_lo(recv.span.hi()))) + }, + Node::LetStmt(LetStmt { pat, ty: None, .. }) + if let PatKind::Binding(_, local_id, ..) = pat.kind + && !moved => + { + for_each_local_use_after_expr(cx, local_id, expr.hir_id, |expr| { + if check_pin_box_use(cx, expr, true, enclosing_body).is_continue() + // Make sure the `Pin` is not captured by a closure. + && cx.tcx.hir().enclosing_body_owner(expr.hir_id) == enclosing_body + { + ControlFlow::Continue(()) + } else { + ControlFlow::Break(()) + } + })?; + ControlFlow::Continue(None) + }, + _ => ControlFlow::Break(()), + } +} diff --git a/tests/ui/unnecessary_box_pin.fixed b/tests/ui/unnecessary_box_pin.fixed new file mode 100644 index 000000000000..a22480403536 --- /dev/null +++ b/tests/ui/unnecessary_box_pin.fixed @@ -0,0 +1,61 @@ +//@aux-build:proc_macros.rs +#![warn(clippy::unnecessary_box_pin)] + +extern crate proc_macros; + +use std::convert::identity; +use std::future::Future; +use std::pin::Pin; +use std::task::Context; + +async fn fut() {} + +fn accept_unpin_fut(_: impl Future + Unpin) {} + +fn assert_static(_: impl Send + Sync + 'static) {} + +fn test(cx: &mut Context<'_>) { + accept_unpin_fut(std::pin::pin!(fut())); + //~^ unnecessary_box_pin + + let mut bx = std::pin::pin!(fut()); + //~^ unnecessary_box_pin + accept_unpin_fut(bx.as_mut()); + + #[allow(clippy::let_underscore_future)] + let _: Pin<&mut _> = std::pin::pin!(fut()); + //~^ unnecessary_box_pin + + let bx = Box::pin(fut()); + assert_static(|| bx); + assert_static(|| Box::pin(fut())); + + std::pin::pin!(fut()).poll(cx); + //~^ unnecessary_box_pin + + assert_static(identity(Box::pin(async {}))); + + let mut bx = std::pin::pin!(fut()); + //~^ unnecessary_box_pin + loop { + bx.as_mut().poll(cx); + } + + proc_macros::with_span! { + span + let mut bx = Box::pin(fut()); + accept_unpin_fut(bx.as_mut()); + } + proc_macros::external! { + let mut bx = Box::pin(fut()); + accept_unpin_fut(bx.as_mut()); + } +} + +#[clippy::msrv = "1.67.0"] +fn too_low_msrv() { + let mut bx = Box::pin(fut()); + accept_unpin_fut(bx.as_mut()); +} + +fn main() {} diff --git a/tests/ui/unnecessary_box_pin.rs b/tests/ui/unnecessary_box_pin.rs new file mode 100644 index 000000000000..d3395aeae051 --- /dev/null +++ b/tests/ui/unnecessary_box_pin.rs @@ -0,0 +1,61 @@ +//@aux-build:proc_macros.rs +#![warn(clippy::unnecessary_box_pin)] + +extern crate proc_macros; + +use std::convert::identity; +use std::future::Future; +use std::pin::Pin; +use std::task::Context; + +async fn fut() {} + +fn accept_unpin_fut(_: impl Future + Unpin) {} + +fn assert_static(_: impl Send + Sync + 'static) {} + +fn test(cx: &mut Context<'_>) { + accept_unpin_fut(Box::pin(fut()).as_mut()); + //~^ unnecessary_box_pin + + let mut bx = Box::pin(fut()); + //~^ unnecessary_box_pin + accept_unpin_fut(bx.as_mut()); + + #[allow(clippy::let_underscore_future)] + let _: Pin<&mut _> = Box::pin(fut()).as_mut(); + //~^ unnecessary_box_pin + + let bx = Box::pin(fut()); + assert_static(|| bx); + assert_static(|| Box::pin(fut())); + + Box::pin(fut()).as_mut().poll(cx); + //~^ unnecessary_box_pin + + assert_static(identity(Box::pin(async {}))); + + let mut bx = Box::pin(fut()); + //~^ unnecessary_box_pin + loop { + bx.as_mut().poll(cx); + } + + proc_macros::with_span! { + span + let mut bx = Box::pin(fut()); + accept_unpin_fut(bx.as_mut()); + } + proc_macros::external! { + let mut bx = Box::pin(fut()); + accept_unpin_fut(bx.as_mut()); + } +} + +#[clippy::msrv = "1.67.0"] +fn too_low_msrv() { + let mut bx = Box::pin(fut()); + accept_unpin_fut(bx.as_mut()); +} + +fn main() {} diff --git a/tests/ui/unnecessary_box_pin.stderr b/tests/ui/unnecessary_box_pin.stderr new file mode 100644 index 000000000000..9367a25d8605 --- /dev/null +++ b/tests/ui/unnecessary_box_pin.stderr @@ -0,0 +1,62 @@ +error: pinning a value with `Box::pin` when local pinning suffices + --> tests/ui/unnecessary_box_pin.rs:18:22 + | +LL | accept_unpin_fut(Box::pin(fut()).as_mut()); + | ^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unnecessary-box-pin` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_box_pin)]` +help: use the `pin!` macro + | +LL - accept_unpin_fut(Box::pin(fut()).as_mut()); +LL + accept_unpin_fut(std::pin::pin!(fut())); + | + +error: pinning a value with `Box::pin` when local pinning suffices + --> tests/ui/unnecessary_box_pin.rs:21:18 + | +LL | let mut bx = Box::pin(fut()); + | ^^^^^^^^^^^^^^^ + | +help: use the `pin!` macro + | +LL | let mut bx = std::pin::pin!(fut()); + | ~~~~~~~~~~~~~~ + +error: pinning a value with `Box::pin` when local pinning suffices + --> tests/ui/unnecessary_box_pin.rs:26:26 + | +LL | let _: Pin<&mut _> = Box::pin(fut()).as_mut(); + | ^^^^^^^^^^^^^^^ + | +help: use the `pin!` macro + | +LL - let _: Pin<&mut _> = Box::pin(fut()).as_mut(); +LL + let _: Pin<&mut _> = std::pin::pin!(fut()); + | + +error: pinning a value with `Box::pin` when local pinning suffices + --> tests/ui/unnecessary_box_pin.rs:33:5 + | +LL | Box::pin(fut()).as_mut().poll(cx); + | ^^^^^^^^^^^^^^^ + | +help: use the `pin!` macro + | +LL - Box::pin(fut()).as_mut().poll(cx); +LL + std::pin::pin!(fut()).poll(cx); + | + +error: pinning a value with `Box::pin` when local pinning suffices + --> tests/ui/unnecessary_box_pin.rs:38:18 + | +LL | let mut bx = Box::pin(fut()); + | ^^^^^^^^^^^^^^^ + | +help: use the `pin!` macro + | +LL | let mut bx = std::pin::pin!(fut()); + | ~~~~~~~~~~~~~~ + +error: aborting due to 5 previous errors +