diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cda8003b05c..8211445f0acf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7091,6 +7091,7 @@ Released 2018-09-13 [`unstable_as_mut_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_mut_slice [`unstable_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_slice [`unused_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_async +[`unused_async_trait_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_async_trait_impl [`unused_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_collect [`unused_enumerate_index`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_enumerate_index [`unused_format_specs`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_format_specs diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ef9da84c4407..511586b9b7b2 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -770,6 +770,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::unnested_or_patterns::UNNESTED_OR_PATTERNS_INFO, crate::unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME_INFO, crate::unused_async::UNUSED_ASYNC_INFO, + crate::unused_async_trait_impl::UNUSED_ASYNC_TRAIT_IMPL_INFO, crate::unused_io_amount::UNUSED_IO_AMOUNT_INFO, crate::unused_peekable::UNUSED_PEEKABLE_INFO, crate::unused_result_ok::UNUSED_RESULT_OK_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ef2461f8b097..e12a08cb3356 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -379,6 +379,7 @@ mod unneeded_struct_pattern; mod unnested_or_patterns; mod unsafe_removed_from_name; mod unused_async; +mod unused_async_trait_impl; mod unused_io_amount; mod unused_peekable; mod unused_result_ok; @@ -861,6 +862,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(move |_| Box::new(manual_ilog2::ManualIlog2::new(conf))), Box::new(|_| Box::new(same_length_and_capacity::SameLengthAndCapacity)), Box::new(move |tcx| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(tcx, conf))), + Box::new(|_| Box::new(unused_async_trait_impl::UnusedAsyncTraitImpl)), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/unused_async_trait_impl.rs b/clippy_lints/src/unused_async_trait_impl.rs new file mode 100644 index 000000000000..09c9bd48f92a --- /dev/null +++ b/clippy_lints/src/unused_async_trait_impl.rs @@ -0,0 +1,198 @@ +use clippy_utils::diagnostics::span_lint_hir_and_then; +use clippy_utils::source::{ + HasSession, indent_of, reindent_multiline, snippet_block_with_applicability, snippet_with_applicability, +}; +use clippy_utils::usage::is_todo_unimplemented_stub; +use rustc_errors::Applicability; +use rustc_hir::intravisit::{Visitor, walk_expr}; +use rustc_hir::{ + Body, Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, Expr, ExprKind, ImplItem, ImplItemKind, IsAsync, + YieldSource, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::nested_filter; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for trait method implementations that are declared `async` but have no `.await`s inside of them. + /// + /// ### Why is this bad? + /// Async functions with no async code create computational overhead. + /// Even though the trait requires the method to return a future, + /// returning a `core::future::ready` with the result is more efficient + /// as it reduces the number of states in the Future state machine by at least one. + /// + /// Note that the behaviour is slightly different when using `core::future::ready`, + /// as the value is computed immediately and stored in a future for later retrieval at the first (and only valid) call to `poll`. + /// An `async` block generates code that completely defers the computation of this value until the Future is polled. + /// + /// ### Example + /// ```no_run + /// trait AsyncTrait { + /// async fn get_random_number() -> i64; + /// } + /// + /// impl AsyncTrait for () { + /// async fn get_random_number() -> i64 { + /// 4 // Chosen by fair dice roll. Guaranteed to be random. + /// } + /// } + /// ``` + /// + /// Use instead: + /// ```no_run + /// trait AsyncTrait { + /// async fn get_random_number() -> i64; + /// } + /// + /// impl AsyncTrait for () { + /// fn get_random_number() -> impl Future { + /// core::future::ready(4) // Chosen by fair dice roll. Guaranteed to be random. + /// } + /// } + /// ``` + #[clippy::version = "1.94.0"] + pub UNUSED_ASYNC_TRAIT_IMPL, + pedantic, + "finds async trait impl functions with no await statements" +} + +declare_lint_pass!(UnusedAsyncTraitImpl => [UNUSED_ASYNC_TRAIT_IMPL]); + +struct AsyncFnVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + found_await: bool, + async_depth: usize, +} + +impl<'tcx> Visitor<'tcx> for AsyncFnVisitor<'_, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { + if let ExprKind::Yield(_, YieldSource::Await { .. }) = ex.kind + && self.async_depth == 1 + { + self.found_await = true; + } + + let is_async_block = matches!( + ex.kind, + ExprKind::Closure(Closure { + kind: ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)), + .. + }) + ); + + if is_async_block { + self.async_depth += 1; + } + + walk_expr(self, ex); + + if is_async_block { + self.async_depth -= 1; + } + } + + fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt { + self.cx.tcx + } +} + +impl<'tcx> LateLintPass<'tcx> for UnusedAsyncTraitImpl { + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { + if let ImplItemKind::Fn(ref sig, body_id) = impl_item.kind + && let IsAsync::Async(async_span) = sig.header.asyncness + && let body = cx.tcx.hir_body(body_id) + && !async_fn_contains_todo_unimplemented_macro(cx, body) + { + let mut visitor = AsyncFnVisitor { + cx, + found_await: false, + async_depth: 0, + }; + visitor.visit_nested_body(body_id); + + if !visitor.found_await + && let Some(builtin_crate) = clippy_utils::std_or_core(cx) + { + span_lint_hir_and_then( + cx, + UNUSED_ASYNC_TRAIT_IMPL, + cx.tcx.local_def_id_to_hir_id(impl_item.owner_id.def_id), + impl_item.span, + "unused `async` for async trait impl function with no await statements", + |diag| { + let mut app = Applicability::MachineApplicable; + + let async_span = cx.sess().source_map().span_extend_while_whitespace(async_span); + + let signature_snippet = snippet_with_applicability(cx, sig.decl.output.span(), "_", &mut app); + + // Fetch body snippet and truncate excess indentation. Like this: + // { + // 4 + // } + let body_snippet = snippet_block_with_applicability(cx, body.value.span, "_", None, &mut app); + // Wrap body snippet in `std::future::ready(...)` and indent everything by one level, like this: + // core::future::ready({ + // 4 + // }) + let new_body_inner_snippet: String = reindent_multiline( + &format!("{builtin_crate}::future::ready({body_snippet})"), + false, + Some(4), + ); + + let sugg = vec![ + (async_span, String::new()), + ( + sig.decl.output.span(), + format!("impl Future"), + ), + ( + body.value.span, + // Wrap the entire snippet in fresh curly braces and indent everything except the first + // line by the indentation level of the original body snippet, like this: + // { + // core::future::ready({ + // 4 + // } + // } + reindent_multiline( + &format!("{{\n{new_body_inner_snippet}\n}}"), + true, + indent_of(cx, body.value.span), + ), + ), + ]; + diag.help(format!( + "a Future can be constructed from the return value with `{builtin_crate}::future::ready`" + )); + diag.multipart_suggestion( + format!("consider removing the `async` from this function and returning `impl Future` instead"), + sugg, + app + ); + }, + ); + } + } + } +} + +fn async_fn_contains_todo_unimplemented_macro(cx: &LateContext<'_>, body: &Body<'_>) -> bool { + if let ExprKind::Closure(closure) = body.value.kind + && let ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)) = closure.kind + && let body = cx.tcx.hir_body(closure.body) + && let ExprKind::Block(block, _) = body.value.kind + && block.stmts.is_empty() + && let Some(expr) = block.expr + && let ExprKind::DropTemps(inner) = expr.kind + { + return is_todo_unimplemented_stub(cx, inner); + } + + false +} diff --git a/tests/ui/unused_async_trait_impl.fixed b/tests/ui/unused_async_trait_impl.fixed new file mode 100644 index 000000000000..044fe8dec2c2 --- /dev/null +++ b/tests/ui/unused_async_trait_impl.fixed @@ -0,0 +1,81 @@ +#![warn(clippy::unused_async_trait_impl)] + +trait HasAsyncMethod { + async fn do_something() -> u32; +} + +struct Inefficient; +struct Efficient; +struct Stub; + +impl HasAsyncMethod for Inefficient { + fn do_something() -> impl Future { + std::future::ready({ + //~^ unused_async_trait_impl + 1 + }) + } +} + +impl HasAsyncMethod for Efficient { + fn do_something() -> impl Future { + std::future::ready(1) + } +} + +impl HasAsyncMethod for Stub { + async fn do_something() -> u32 { + todo!() // Do not emit the lint in this case. + } +} + +// Test to check if the identation of the various snippets goes as intended. +mod indented { + struct Indented; + + impl crate::HasAsyncMethod for Indented { + fn do_something() -> impl Future { + std::future::ready({ + //~^ unused_async_trait_impl + let mut x = 0; + for y in 0..64 { + x = (x + 1) * y; + } + + let fake_fut = async { + if x == 0 { + panic!("Fake example"); + } + }; + + x + }) + } + } + + struct Complex(std::marker::PhantomData); + + impl crate::HasAsyncMethod for Complex + where + T: Sized, + { + fn do_something() -> impl Future { + std::future::ready({ + //~^ unused_async_trait_impl + 5 + }) + } + } +} + +trait HasDefaultAsyncMethod { + // The lint should not suggest a change for trait fn's as changing that decl + // implies a less restrictive Future type. + async fn do_something() -> u32 { + 0 + } +} + +impl HasDefaultAsyncMethod for Stub { + // Nothing! +} diff --git a/tests/ui/unused_async_trait_impl.rs b/tests/ui/unused_async_trait_impl.rs new file mode 100644 index 000000000000..6fa23b302463 --- /dev/null +++ b/tests/ui/unused_async_trait_impl.rs @@ -0,0 +1,75 @@ +#![warn(clippy::unused_async_trait_impl)] + +trait HasAsyncMethod { + async fn do_something() -> u32; +} + +struct Inefficient; +struct Efficient; +struct Stub; + +impl HasAsyncMethod for Inefficient { + async fn do_something() -> u32 { + //~^ unused_async_trait_impl + 1 + } +} + +impl HasAsyncMethod for Efficient { + fn do_something() -> impl Future { + std::future::ready(1) + } +} + +impl HasAsyncMethod for Stub { + async fn do_something() -> u32 { + todo!() // Do not emit the lint in this case. + } +} + +// Test to check if the identation of the various snippets goes as intended. +mod indented { + struct Indented; + + impl crate::HasAsyncMethod for Indented { + async fn do_something() -> u32 { + //~^ unused_async_trait_impl + let mut x = 0; + for y in 0..64 { + x = (x + 1) * y; + } + + let fake_fut = async { + if x == 0 { + panic!("Fake example"); + } + }; + + x + } + } + + struct Complex(std::marker::PhantomData); + + impl crate::HasAsyncMethod for Complex + where + T: Sized, + { + async fn do_something() -> u32 { + //~^ unused_async_trait_impl + 5 + } + } +} + +trait HasDefaultAsyncMethod { + // The lint should not suggest a change for trait fn's as changing that decl + // implies a less restrictive Future type. + async fn do_something() -> u32 { + 0 + } +} + +impl HasDefaultAsyncMethod for Stub { + // Nothing! +} diff --git a/tests/ui/unused_async_trait_impl.stderr b/tests/ui/unused_async_trait_impl.stderr new file mode 100644 index 000000000000..4ee51b2918a7 --- /dev/null +++ b/tests/ui/unused_async_trait_impl.stderr @@ -0,0 +1,78 @@ +error: unused `async` for async trait impl function with no await statements + --> tests/ui/unused_async_trait_impl.rs:12:5 + | +LL | / async fn do_something() -> u32 { +LL | | +LL | | 1 +LL | | } + | |_____^ + | + = help: a Future can be constructed from the return value with `std::future::ready` + = note: `-D clippy::unused-async-trait-impl` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unused_async_trait_impl)]` +help: consider removing the `async` from this function and returning `impl Future` instead + | +LL ~ fn do_something() -> impl Future { +LL + std::future::ready({ +LL + +LL + 1 +LL + }) +LL + } + | + +error: unused `async` for async trait impl function with no await statements + --> tests/ui/unused_async_trait_impl.rs:35:9 + | +LL | / async fn do_something() -> u32 { +LL | | +LL | | let mut x = 0; +LL | | for y in 0..64 { +... | +LL | | x +LL | | } + | |_________^ + | + = help: a Future can be constructed from the return value with `std::future::ready` +help: consider removing the `async` from this function and returning `impl Future` instead + | +LL ~ fn do_something() -> impl Future { +LL + std::future::ready({ +LL + +LL + let mut x = 0; +LL + for y in 0..64 { +LL + x = (x + 1) * y; +LL + } +LL + +LL + let fake_fut = async { +LL + if x == 0 { +LL + panic!("Fake example"); +LL + } +LL + }; +LL + +LL + x +LL + }) +LL + } + | + +error: unused `async` for async trait impl function with no await statements + --> tests/ui/unused_async_trait_impl.rs:58:9 + | +LL | / async fn do_something() -> u32 { +LL | | +LL | | 5 +LL | | } + | |_________^ + | + = help: a Future can be constructed from the return value with `std::future::ready` +help: consider removing the `async` from this function and returning `impl Future` instead + | +LL ~ fn do_something() -> impl Future { +LL + std::future::ready({ +LL + +LL + 5 +LL + }) +LL + } + | + +error: aborting due to 3 previous errors + diff --git a/tests/ui/unused_async_trait_impl_no_std.fixed b/tests/ui/unused_async_trait_impl_no_std.fixed new file mode 100644 index 000000000000..ecde299633ca --- /dev/null +++ b/tests/ui/unused_async_trait_impl_no_std.fixed @@ -0,0 +1,17 @@ +#![no_std] +#![warn(clippy::unused_async_trait_impl)] + +trait HasAsyncMethod { + async fn do_something() -> u32; +} + +struct Inefficient; + +impl HasAsyncMethod for Inefficient { + fn do_something() -> impl Future { + core::future::ready({ + //~^ unused_async_trait_impl + 1 + }) + } +} diff --git a/tests/ui/unused_async_trait_impl_no_std.rs b/tests/ui/unused_async_trait_impl_no_std.rs new file mode 100644 index 000000000000..ab6f1f3716a7 --- /dev/null +++ b/tests/ui/unused_async_trait_impl_no_std.rs @@ -0,0 +1,15 @@ +#![no_std] +#![warn(clippy::unused_async_trait_impl)] + +trait HasAsyncMethod { + async fn do_something() -> u32; +} + +struct Inefficient; + +impl HasAsyncMethod for Inefficient { + async fn do_something() -> u32 { + //~^ unused_async_trait_impl + 1 + } +} diff --git a/tests/ui/unused_async_trait_impl_no_std.stderr b/tests/ui/unused_async_trait_impl_no_std.stderr new file mode 100644 index 000000000000..45d0ad677b08 --- /dev/null +++ b/tests/ui/unused_async_trait_impl_no_std.stderr @@ -0,0 +1,24 @@ +error: unused `async` for async trait impl function with no await statements + --> tests/ui/unused_async_trait_impl_no_std.rs:11:5 + | +LL | / async fn do_something() -> u32 { +LL | | +LL | | 1 +LL | | } + | |_____^ + | + = help: a Future can be constructed from the return value with `core::future::ready` + = note: `-D clippy::unused-async-trait-impl` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unused_async_trait_impl)]` +help: consider removing the `async` from this function and returning `impl Future` instead + | +LL ~ fn do_something() -> impl Future { +LL + core::future::ready({ +LL + +LL + 1 +LL + }) +LL + } + | + +error: aborting due to 1 previous error +