new lint: unnecessary_box_pin

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

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.

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

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)

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,

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 }

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,

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`
 }

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<Box<_>>` 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<Box<_>>` 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: '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<F: Future>(fut: F) -> Option<F::Output> {
+    ///     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<F: Future>(fut: F) -> Option<F::Output> {
+    ///     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<Box<_>>` 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<Span>> {
+    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(()),
+    }
+}

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() {}

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() {}