try_err: Consider Try impl for Poll when generating suggestions

tmiasko · tmiasko · commit e9677105bf85 · 2020-08-03T20:48:18.000+02:00
There are two different implementation of Try trait for Poll type;
Poll&lt;Result&lt;T, E&gt;&gt; and Poll&lt;Option&lt;Result&lt;T, E&gt;&gt;&gt;. 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.
diff --git 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 @@ impl<'tcx> LateLintPass<'tcx> for TryErr {
             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 @@ impl<'tcx> LateLintPass<'tcx> for TryErr {
     }
 }
 
-// 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<Ty<'tcx>> {
+/// Finds function return type by examining return expressions in match arms.
+fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option<Ty<'tcx>> {
     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<Ty<'tcx>> {
+/// Extracts the error type from Result<T, E>.
+fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
     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<Result<T, E>>.
+fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
+    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<Option<Result<T, E>>>.
+fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
+    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
@@ -80,6 +80,7 @@ pub const PATH: [&str; 3] = ["std", "path", "Path"];
 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: [&str; 3] = ["alloc", "string", "ToString"];
 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
@@ -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<i32, i32> {
@@ -104,3 +107,21 @@ pub fn macro_inside(fail: bool) -> Result<i32, String> {
     }
     Ok(0)
 }
+
+pub fn poll_write(n: usize) -> Poll<io::Result<usize>> {
+    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<Option<io::Result<()>>> {
+    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
@@ -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<i32, i32> {
@@ -104,3 +107,21 @@ pub fn macro_inside(fail: bool) -> Result<i32, String> {
     }
     Ok(0)
 }
+
+pub fn poll_write(n: usize) -> Poll<io::Result<usize>> {
+    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<Option<io::Result<()>>> {
+    if !ready {
+        Err(io::ErrorKind::NotFound)?
+    }
+
+    Poll::Ready(None)
+}
diff --git 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
 
