From e9677105bf85a2b0c57e8d67d2ed22a286333033 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Tomasz=20Mi=C4=85sko?= Date: Sun, 2 Aug 2020 00:00:00 +0000 Subject: [PATCH] try_err: Consider Try impl for Poll when generating suggestions There are two different implementation of Try trait for Poll type; Poll> and Poll>>. Take them into account when generating suggestions. For example, for Err(e)? suggest either return Poll::Ready(Err(e)) or return Poll::Ready(Some(Err(e))) as appropriate. --- clippy_lints/src/try_err.rs | 112 +++++++++++++++++++++++++------- clippy_lints/src/utils/paths.rs | 2 +- tests/ui/try_err.fixed | 21 ++++++ tests/ui/try_err.rs | 21 ++++++ tests/ui/try_err.stderr | 30 +++++++-- 5 files changed, 155 insertions(+), 31 deletions(-) diff --git a/clippy_lints/src/try_err.rs b/clippy_lints/src/try_err.rs index d3b351f30ef..3bd73d9f21a 100644 --- a/clippy_lints/src/try_err.rs +++ b/clippy_lints/src/try_err.rs @@ -1,10 +1,13 @@ -use crate::utils::{match_qpath, paths, snippet, snippet_with_macro_callsite, span_lint_and_sugg}; +use crate::utils::{ + is_type_diagnostic_item, match_def_path, match_qpath, paths, snippet, snippet_with_macro_callsite, + span_lint_and_sugg, +}; use if_chain::if_chain; use rustc_errors::Applicability; -use rustc_hir::{Arm, Expr, ExprKind, MatchSource}; +use rustc_hir::{Expr, ExprKind, MatchSource}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::Ty; +use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { @@ -65,19 +68,39 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if let Some(ref err_arg) = err_args.get(0); if let ExprKind::Path(ref err_fun_path) = err_fun.kind; if match_qpath(err_fun_path, &paths::RESULT_ERR); - if let Some(return_type) = find_err_return_type(cx, &expr.kind); - + if let Some(return_ty) = find_return_type(cx, &expr.kind); then { - let err_type = cx.typeck_results().expr_ty(err_arg); + let prefix; + let suffix; + let err_ty; + + if let Some(ty) = result_error_type(cx, return_ty) { + prefix = "Err("; + suffix = ")"; + err_ty = ty; + } else if let Some(ty) = poll_result_error_type(cx, return_ty) { + prefix = "Poll::Ready(Err("; + suffix = "))"; + err_ty = ty; + } else if let Some(ty) = poll_option_result_error_type(cx, return_ty) { + prefix = "Poll::Ready(Some(Err("; + suffix = ")))"; + err_ty = ty; + } else { + return; + }; + + let expr_err_ty = cx.typeck_results().expr_ty(err_arg); + let origin_snippet = if err_arg.span.from_expansion() { snippet_with_macro_callsite(cx, err_arg.span, "_") } else { snippet(cx, err_arg.span, "_") }; - let suggestion = if err_type == return_type { - format!("return Err({})", origin_snippet) + let suggestion = if err_ty == expr_err_ty { + format!("return {}{}{}", prefix, origin_snippet, suffix) } else { - format!("return Err({}.into())", origin_snippet) + format!("return {}{}.into(){}", prefix, origin_snippet, suffix) }; span_lint_and_sugg( @@ -94,27 +117,68 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } } -// In order to determine whether to suggest `.into()` or not, we need to find the error type the -// function returns. To do that, we look for the From::from call (see tree above), and capture -// its output type. -fn find_err_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option> { +/// Finds function return type by examining return expressions in match arms. +fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option> { if let ExprKind::Match(_, ref arms, MatchSource::TryDesugar) = expr { - arms.iter().find_map(|ty| find_err_return_type_arm(cx, ty)) - } else { - None + for arm in arms.iter() { + if let ExprKind::Ret(Some(ref ret)) = arm.body.kind { + return Some(cx.typeck_results().expr_ty(ret)); + } + } } + None } -// Check for From::from in one of the match arms. -fn find_err_return_type_arm<'tcx>(cx: &LateContext<'tcx>, arm: &'tcx Arm<'_>) -> Option> { +/// Extracts the error type from Result. +fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { if_chain! { - if let ExprKind::Ret(Some(ref err_ret)) = arm.body.kind; - if let ExprKind::Call(ref from_error_path, ref from_error_args) = err_ret.kind; - if let ExprKind::Path(ref from_error_fn) = from_error_path.kind; - if match_qpath(from_error_fn, &paths::TRY_FROM_ERROR); - if let Some(from_error_arg) = from_error_args.get(0); + if let ty::Adt(_, subst) = ty.kind; + if is_type_diagnostic_item(cx, ty, sym!(result_type)); + let err_ty = subst.type_at(1); + then { + Some(err_ty) + } else { + None + } + } +} + +/// Extracts the error type from Poll>. +fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(def, subst) = ty.kind; + if match_def_path(cx, def.did, &paths::POLL); + let ready_ty = subst.type_at(0); + + if let ty::Adt(ready_def, ready_subst) = ready_ty.kind; + if cx.tcx.is_diagnostic_item(sym!(result_type), ready_def.did); + let err_ty = ready_subst.type_at(1); + + then { + Some(err_ty) + } else { + None + } + } +} + +/// Extracts the error type from Poll>>. +fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(def, subst) = ty.kind; + if match_def_path(cx, def.did, &paths::POLL); + let ready_ty = subst.type_at(0); + + if let ty::Adt(ready_def, ready_subst) = ready_ty.kind; + if cx.tcx.is_diagnostic_item(sym!(option_type), ready_def.did); + let some_ty = ready_subst.type_at(0); + + if let ty::Adt(some_def, some_subst) = some_ty.kind; + if cx.tcx.is_diagnostic_item(sym!(result_type), some_def.did); + let err_ty = some_subst.type_at(1); + then { - Some(cx.typeck_results().expr_ty(from_error_arg)) + Some(err_ty) } else { None } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index a515ee29c82..923b319d777 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -80,6 +80,7 @@ pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"]; pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"]; pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; +pub const POLL: [&str; 4] = ["core", "task", "poll", "Poll"]; pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"]; pub const PTR_NULL: [&str; 2] = ["ptr", "null"]; pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"]; @@ -129,7 +130,6 @@ pub const TO_STRING_METHOD: [&str; 4] = ["alloc", "string", "ToString", "to_string"]; pub const TRANSMUTE: [&str; 4] = ["core", "intrinsics", "", "transmute"]; pub const TRY_FROM: [&str; 4] = ["core", "convert", "TryFrom", "try_from"]; -pub const TRY_FROM_ERROR: [&str; 4] = ["std", "ops", "Try", "from_error"]; pub const TRY_INTO_RESULT: [&str; 4] = ["std", "ops", "Try", "into_result"]; pub const TRY_INTO_TRAIT: [&str; 3] = ["core", "convert", "TryInto"]; pub const VEC: [&str; 3] = ["alloc", "vec", "Vec"]; diff --git a/tests/ui/try_err.fixed b/tests/ui/try_err.fixed index 29d9139d3e3..9e77dcd8731 100644 --- a/tests/ui/try_err.fixed +++ b/tests/ui/try_err.fixed @@ -6,6 +6,9 @@ #[macro_use] extern crate macro_rules; +use std::io; +use std::task::Poll; + // Tests that a simple case works // Should flag `Err(err)?` pub fn basic_test() -> Result { @@ -104,3 +107,21 @@ pub fn macro_inside(fail: bool) -> Result { } Ok(0) } + +pub fn poll_write(n: usize) -> Poll> { + if n == 0 { + return Poll::Ready(Err(io::ErrorKind::WriteZero.into())) + } else if n == 1 { + return Poll::Ready(Err(io::Error::new(io::ErrorKind::InvalidInput, "error"))) + }; + + Poll::Ready(Ok(n)) +} + +pub fn poll_next(ready: bool) -> Poll>> { + if !ready { + return Poll::Ready(Some(Err(io::ErrorKind::NotFound.into()))) + } + + Poll::Ready(None) +} diff --git a/tests/ui/try_err.rs b/tests/ui/try_err.rs index 5e85d091a2a..41bcb0a189e 100644 --- a/tests/ui/try_err.rs +++ b/tests/ui/try_err.rs @@ -6,6 +6,9 @@ #[macro_use] extern crate macro_rules; +use std::io; +use std::task::Poll; + // Tests that a simple case works // Should flag `Err(err)?` pub fn basic_test() -> Result { @@ -104,3 +107,21 @@ pub fn macro_inside(fail: bool) -> Result { } Ok(0) } + +pub fn poll_write(n: usize) -> Poll> { + if n == 0 { + Err(io::ErrorKind::WriteZero)? + } else if n == 1 { + Err(io::Error::new(io::ErrorKind::InvalidInput, "error"))? + }; + + Poll::Ready(Ok(n)) +} + +pub fn poll_next(ready: bool) -> Poll>> { + if !ready { + Err(io::ErrorKind::NotFound)? + } + + Poll::Ready(None) +} diff --git a/tests/ui/try_err.stderr b/tests/ui/try_err.stderr index 21e9d4048a5..3f1cbc17e72 100644 --- a/tests/ui/try_err.stderr +++ b/tests/ui/try_err.stderr @@ -1,5 +1,5 @@ error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:15:9 + --> $DIR/try_err.rs:18:9 | LL | Err(err)?; | ^^^^^^^^^ help: try this: `return Err(err)` @@ -11,28 +11,46 @@ LL | #![deny(clippy::try_err)] | ^^^^^^^^^^^^^^^ error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:25:9 + --> $DIR/try_err.rs:28:9 | LL | Err(err)?; | ^^^^^^^^^ help: try this: `return Err(err.into())` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:45:17 + --> $DIR/try_err.rs:48:17 | LL | Err(err)?; | ^^^^^^^^^ help: try this: `return Err(err)` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:64:17 + --> $DIR/try_err.rs:67:17 | LL | Err(err)?; | ^^^^^^^^^ help: try this: `return Err(err.into())` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:103:9 + --> $DIR/try_err.rs:106:9 | LL | Err(foo!())?; | ^^^^^^^^^^^^ help: try this: `return Err(foo!())` -error: aborting due to 5 previous errors +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:113:9 + | +LL | Err(io::ErrorKind::WriteZero)? + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `return Poll::Ready(Err(io::ErrorKind::WriteZero.into()))` + +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:115:9 + | +LL | Err(io::Error::new(io::ErrorKind::InvalidInput, "error"))? + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `return Poll::Ready(Err(io::Error::new(io::ErrorKind::InvalidInput, "error")))` + +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:123:9 + | +LL | Err(io::ErrorKind::NotFound)? + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `return Poll::Ready(Some(Err(io::ErrorKind::NotFound.into())))` + +error: aborting due to 8 previous errors -- 2.44.0