-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add unused_async_trait_impl lint #16244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Wassasin
wants to merge
16
commits into
rust-lang:master
Choose a base branch
from
Wassasin:unused_async_trait_impl
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
2c59897
Add unused_async_trait_impl lint
Wassasin 8a8e0ae
Adjusted description and use declare_lint_pass
Wassasin 345a48f
Remove the execution of unused_async_trait_impl tests, added todo!() …
Wassasin 6802082
Simplified by using check_impl_item instead of check_fn
Wassasin ded1b73
Use std_or_core to determine the builtin_crate instead of assuming core
Wassasin 5ba562f
Added no_std version of test
Wassasin 6378a15
Use snippet_with_applicability to ensure the suggestion can be given …
Wassasin 5670f89
Improved lint description
Wassasin 1fa2639
Fix lint span
Wassasin 6bd9e3a
Slightly shorten snippet_with_applicability-line
Wassasin 081977b
Fix indentation for body block
Wassasin c9ee761
Fix whitespace around the removed async keyword
Wassasin 65e9892
Added indented testcase
Wassasin 09816b2
Clarify how the construction of the body snippet works
Wassasin 84b451b
Made example slightly more complicate to demonstrate correct indentation
Wassasin cdff8eb
Expanded on testcases
Wassasin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| use clippy_utils::diagnostics::span_lint_hir_and_then; | ||
| use clippy_utils::is_def_id_trait_method; | ||
| use clippy_utils::source::SpanRangeExt; | ||
| use clippy_utils::usage::is_todo_unimplemented_stub; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::intravisit::{FnKind, Visitor, walk_expr, walk_fn}; | ||
| use rustc_hir::{ | ||
| Body, Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, Defaultness, Expr, ExprKind, FnDecl, IsAsync, Node, | ||
| TraitItem, YieldSource, | ||
| }; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_middle::hir::nested_filter; | ||
| use rustc_session::impl_lint_pass; | ||
| use rustc_span::Span; | ||
| use rustc_span::def_id::LocalDefId; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// Checks for trait function 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 function to return a future, | ||
| /// returning a `core::future::ready` with the result is more efficient. | ||
| /// | ||
| /// ### 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<Output = i64> { | ||
| /// core::future::ready(4) // Chosen by fair dice roll. Guaranteed to be random. | ||
| /// } | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// ### Note | ||
| /// An `async` block generates code that defers execution until the Future is polled. | ||
| /// When using `core::future::ready` the code is executed immediately. | ||
Wassasin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| #[clippy::version = "1.94.0"] | ||
| pub UNUSED_ASYNC_TRAIT_IMPL, | ||
| pedantic, | ||
| "finds async trait impl functions with no await statements" | ||
| } | ||
|
|
||
| pub struct UnusedAsyncTraitImpl; | ||
|
|
||
| impl_lint_pass!(UnusedAsyncTraitImpl => [UNUSED_ASYNC_TRAIT_IMPL]); | ||
Wassasin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| struct AsyncFnVisitor<'a, 'tcx> { | ||
| cx: &'a LateContext<'tcx>, | ||
Wassasin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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_fn( | ||
Wassasin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| &mut self, | ||
| cx: &LateContext<'tcx>, | ||
| fn_kind: FnKind<'tcx>, | ||
| fn_decl: &'tcx FnDecl<'tcx>, | ||
| body: &Body<'tcx>, | ||
| span: Span, | ||
| def_id: LocalDefId, | ||
| ) { | ||
| if let IsAsync::Async(async_span) = fn_kind.asyncness() | ||
| && !span.from_expansion() | ||
| && is_def_id_trait_method(cx, def_id) | ||
| && !is_default_trait_impl(cx, def_id) | ||
| && !async_fn_contains_todo_unimplemented_macro(cx, body) | ||
Wassasin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| let mut visitor = AsyncFnVisitor { | ||
| cx, | ||
| found_await: false, | ||
| async_depth: 0, | ||
| }; | ||
| walk_fn(&mut visitor, fn_kind, fn_decl, body.id(), def_id); | ||
| if !visitor.found_await { | ||
| span_lint_hir_and_then( | ||
| cx, | ||
| UNUSED_ASYNC_TRAIT_IMPL, | ||
| cx.tcx.local_def_id_to_hir_id(def_id), | ||
| span, | ||
| "unused `async` for async trait impl function with no await statements", | ||
| |diag| { | ||
| if let Some(output_src) = fn_decl.output.span().get_source_text(cx) | ||
| && let Some(body_src) = body.value.span.get_source_text(cx) | ||
Wassasin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| let output_str = output_src.as_str(); | ||
| let body_str = body_src.as_str(); | ||
|
|
||
| let sugg = vec![ | ||
| (async_span, String::new()), | ||
| (fn_decl.output.span(), format!("impl Future<Output = {output_str}>")), | ||
| (body.value.span, format!("{{ core::future::ready({body_str}) }}")), | ||
Wassasin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ]; | ||
|
|
||
| diag.help("a Future can be constructed from the return value with `core::future::ready`"); | ||
| diag.multipart_suggestion( | ||
| format!("consider removing the `async` from this function and returning `impl Future<Output = {output_str}>` instead"), | ||
| sugg, | ||
| Applicability::MachineApplicable | ||
| ); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn is_default_trait_impl(cx: &LateContext<'_>, def_id: LocalDefId) -> bool { | ||
| matches!( | ||
| cx.tcx.hir_node_by_def_id(def_id), | ||
| Node::TraitItem(TraitItem { | ||
| defaultness: Defaultness::Default { .. }, | ||
| .. | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| #![warn(clippy::unused_async_trait_impl)] | ||
|
|
||
| trait HasAsyncMethod { | ||
| async fn do_something() -> u32; | ||
| } | ||
|
|
||
| struct Inefficient; | ||
| struct Efficient; | ||
|
|
||
| impl HasAsyncMethod for Inefficient { | ||
| fn do_something() -> impl Future<Output = u32> { core::future::ready({ | ||
| //~^ unused_async_trait_impl | ||
| 1 | ||
| }) } | ||
| } | ||
|
|
||
| impl HasAsyncMethod for Efficient { | ||
| fn do_something() -> impl Future<Output = u32> { | ||
| core::future::ready(1) | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| Inefficient::do_something(); | ||
| Efficient::do_something(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| #![warn(clippy::unused_async_trait_impl)] | ||
|
|
||
| trait HasAsyncMethod { | ||
| async fn do_something() -> u32; | ||
| } | ||
|
|
||
| struct Inefficient; | ||
| struct Efficient; | ||
|
|
||
| impl HasAsyncMethod for Inefficient { | ||
| async fn do_something() -> u32 { | ||
| //~^ unused_async_trait_impl | ||
| 1 | ||
| } | ||
| } | ||
|
|
||
| impl HasAsyncMethod for Efficient { | ||
| fn do_something() -> impl Future<Output = u32> { | ||
| core::future::ready(1) | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| Inefficient::do_something(); | ||
| Efficient::do_something(); | ||
Wassasin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| error: unused `async` for async trait impl function with no await statements | ||
| --> tests/ui/unused_async_trait_impl.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<Output = u32>` instead | ||
| | | ||
| LL ~ fn do_something() -> impl Future<Output = u32> { core::future::ready({ | ||
| LL + | ||
| LL + 1 | ||
| LL + }) } | ||
| | | ||
|
|
||
| error: aborting due to 1 previous error | ||
|
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.