Auto merge of #4222 - jfrikker:try_err, r=flip1995

Adding try_err lint

changelog: Adds the "try_err" lint, which catches instances of the following: Err(x)?
fixes #4212

CHANGELOG.md

@@ -1134,6 +1134,7 @@ Released 2018-09-13
 [`transmuting_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmuting_null
 [`trivial_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_regex
 [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref
+[`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err
 [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
 [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented

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 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 306 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -263,6 +263,7 @@ pub mod temporary_assignment;
 pub mod transmute;
 pub mod transmuting_null;
 pub mod trivially_copy_pass_by_ref;
+pub mod try_err;
 pub mod types;
 pub mod unicode;
 pub mod unsafe_removed_from_name;
@@ -546,6 +547,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_early_lint_pass(box literal_representation::DecimalLiteralRepresentation::new(
             conf.literal_representation_threshold
     ));
+    reg.register_late_lint_pass(box try_err::TryErr);
     reg.register_late_lint_pass(box use_self::UseSelf);
     reg.register_late_lint_pass(box bytecount::ByteCount);
     reg.register_late_lint_pass(box infinite_iter::InfiniteIter);
@@ -861,6 +863,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         transmute::WRONG_TRANSMUTE,
         transmuting_null::TRANSMUTING_NULL,
         trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF,
+        try_err::TRY_ERR,
         types::ABSURD_EXTREME_COMPARISONS,
         types::BORROWED_BOX,
         types::BOX_VEC,
@@ -963,6 +966,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         returns::NEEDLESS_RETURN,
         returns::UNUSED_UNIT,
         strings::STRING_LIT_AS_BYTES,
+        try_err::TRY_ERR,
         types::FN_TO_NUMERIC_CAST,
         types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION,
         types::IMPLICIT_HASHER,

clippy_lints/src/try_err.rs

@@ -0,0 +1,115 @@
+use crate::utils::{match_qpath, paths, snippet, span_lint_and_sugg};
+use if_chain::if_chain;
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::ty::Ty;
+use rustc::{declare_lint_pass, declare_tool_lint};
+use rustc_errors::Applicability;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usages of `Err(x)?`.
+    ///
+    /// **Why is this bad?** The `?` operator is designed to allow calls that
+    /// can fail to be easily chained. For example, `foo()?.bar()` or
+    /// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will
+    /// always return), it is more clear to write `return Err(x)`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// fn foo(fail: bool) -> Result<i32, String> {
+    ///     if fail {
+    ///       Err("failed")?;
+    ///     }
+    ///     Ok(0)
+    /// }
+    /// ```
+    /// Could be written:
+    ///
+    /// ```rust
+    /// fn foo(fail: bool) -> Result<i32, String> {
+    ///     if fail {
+    ///       return Err("failed".into());
+    ///     }
+    ///     Ok(0)
+    /// }
+    /// ```
+    pub TRY_ERR,
+    style,
+    "return errors explicitly rather than hiding them behind a `?`"
+}
+
+declare_lint_pass!(TryErr => [TRY_ERR]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TryErr {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        // Looks for a structure like this:
+        // match ::std::ops::Try::into_result(Err(5)) {
+        //     ::std::result::Result::Err(err) =>
+        //         #[allow(unreachable_code)]
+        //         return ::std::ops::Try::from_error(::std::convert::From::from(err)),
+        //     ::std::result::Result::Ok(val) =>
+        //         #[allow(unreachable_code)]
+        //         val,
+        // };
+        if_chain! {
+            if let ExprKind::Match(ref match_arg, _, MatchSource::TryDesugar) = expr.node;
+            if let ExprKind::Call(ref match_fun, ref try_args) = match_arg.node;
+            if let ExprKind::Path(ref match_fun_path) = match_fun.node;
+            if match_qpath(match_fun_path, &paths::TRY_INTO_RESULT);
+            if let Some(ref try_arg) = try_args.get(0);
+            if let ExprKind::Call(ref err_fun, ref err_args) = try_arg.node;
+            if let Some(ref err_arg) = err_args.get(0);
+            if let ExprKind::Path(ref err_fun_path) = err_fun.node;
+            if match_qpath(err_fun_path, &paths::RESULT_ERR);
+            if let Some(return_type) = find_err_return_type(cx, &expr.node);
+
+            then {
+                let err_type = cx.tables.expr_ty(err_arg);
+                let suggestion = if err_type == return_type {
+                    format!("return Err({})", snippet(cx, err_arg.span, "_"))
+                } else {
+                    format!("return Err({}.into())", snippet(cx, err_arg.span, "_"))
+                };
+
+                span_lint_and_sugg(
+                    cx,
+                    TRY_ERR,
+                    expr.span,
+                    "returning an `Err(_)` with the `?` operator",
+                    "try this",
+                    suggestion,
+                    Applicability::MachineApplicable
+                );
+            }
+        }
+    }
+}
+
+// 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<'a, 'tcx>(cx: &LateContext<'a, '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
+    }
+}
+
+// Check for From::from in one of the match arms.
+fn find_err_return_type_arm<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arm: &'tcx Arm) -> Option<Ty<'tcx>> {
+    if_chain! {
+        if let ExprKind::Ret(Some(ref err_ret)) = arm.body.node;
+        if let ExprKind::Call(ref from_error_path, ref from_error_args) = err_ret.node;
+        if let ExprKind::Path(ref from_error_fn) = from_error_path.node;
+        if match_qpath(from_error_fn, &paths::TRY_FROM_ERROR);
+        if let Some(from_error_arg) = from_error_args.get(0);
+        then {
+            Some(cx.tables.expr_ty(from_error_arg))
+        } else {
+            None
+        }
+    }
+}

clippy_lints/src/utils/paths.rs

@@ -107,6 +107,7 @@ pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"
 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_ERROR: [&str; 4] = ["std", "ops", "Try", "from_error"];
 pub const TRY_INTO_RESULT: [&str; 4] = ["std", "ops", "Try", "into_result"];
 pub const UNINIT: [&str; 4] = ["core", "intrinsics", "", "uninit"];
 pub const VEC: [&str; 3] = ["alloc", "vec", "Vec"];

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 305] = [
+pub const ALL_LINTS: [Lint; 306] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1820,6 +1820,13 @@ pub const ALL_LINTS: [Lint; 305] = [
         deprecation: None,
         module: "trivially_copy_pass_by_ref",
     },
+    Lint {
+        name: "try_err",
+        group: "style",
+        desc: "return errors explicitly rather than hiding them behind a `?`",
+        deprecation: None,
+        module: "try_err",
+    },
     Lint {
         name: "type_complexity",
         group: "complexity",

tests/ui/try_err.fixed

@@ -0,0 +1,80 @@
+// run-rustfix
+
+#![deny(clippy::try_err)]
+
+// Tests that a simple case works
+// Should flag `Err(err)?`
+pub fn basic_test() -> Result<i32, i32> {
+    let err: i32 = 1;
+    // To avoid warnings during rustfix
+    if true {
+        return Err(err);
+    }
+    Ok(0)
+}
+
+// Tests that `.into()` is added when appropriate
+pub fn into_test() -> Result<i32, i32> {
+    let err: u8 = 1;
+    // To avoid warnings during rustfix
+    if true {
+        return Err(err.into());
+    }
+    Ok(0)
+}
+
+// Tests that tries in general don't trigger the error
+pub fn negative_test() -> Result<i32, i32> {
+    Ok(nested_error()? + 1)
+}
+
+// Tests that `.into()` isn't added when the error type
+// matches the surrounding closure's return type, even
+// when it doesn't match the surrounding function's.
+pub fn closure_matches_test() -> Result<i32, i32> {
+    let res: Result<i32, i8> = Some(1)
+        .into_iter()
+        .map(|i| {
+            let err: i8 = 1;
+            // To avoid warnings during rustfix
+            if true {
+                return Err(err);
+            }
+            Ok(i)
+        })
+        .next()
+        .unwrap();
+
+    Ok(res?)
+}
+
+// Tests that `.into()` isn't added when the error type
+// doesn't match the surrounding closure's return type.
+pub fn closure_into_test() -> Result<i32, i32> {
+    let res: Result<i32, i16> = Some(1)
+        .into_iter()
+        .map(|i| {
+            let err: i8 = 1;
+            // To avoid warnings during rustfix
+            if true {
+                return Err(err.into());
+            }
+            Ok(i)
+        })
+        .next()
+        .unwrap();
+
+    Ok(res?)
+}
+
+fn nested_error() -> Result<i32, i32> {
+    Ok(1)
+}
+
+fn main() {
+    basic_test().unwrap();
+    into_test().unwrap();
+    negative_test().unwrap();
+    closure_matches_test().unwrap();
+    closure_into_test().unwrap();
+}

tests/ui/try_err.rs

@@ -0,0 +1,80 @@
+// run-rustfix
+
+#![deny(clippy::try_err)]
+
+// Tests that a simple case works
+// Should flag `Err(err)?`
+pub fn basic_test() -> Result<i32, i32> {
+    let err: i32 = 1;
+    // To avoid warnings during rustfix
+    if true {
+        Err(err)?;
+    }
+    Ok(0)
+}
+
+// Tests that `.into()` is added when appropriate
+pub fn into_test() -> Result<i32, i32> {
+    let err: u8 = 1;
+    // To avoid warnings during rustfix
+    if true {
+        Err(err)?;
+    }
+    Ok(0)
+}
+
+// Tests that tries in general don't trigger the error
+pub fn negative_test() -> Result<i32, i32> {
+    Ok(nested_error()? + 1)
+}
+
+// Tests that `.into()` isn't added when the error type
+// matches the surrounding closure's return type, even
+// when it doesn't match the surrounding function's.
+pub fn closure_matches_test() -> Result<i32, i32> {
+    let res: Result<i32, i8> = Some(1)
+        .into_iter()
+        .map(|i| {
+            let err: i8 = 1;
+            // To avoid warnings during rustfix
+            if true {
+                Err(err)?;
+            }
+            Ok(i)
+        })
+        .next()
+        .unwrap();
+
+    Ok(res?)
+}
+
+// Tests that `.into()` isn't added when the error type
+// doesn't match the surrounding closure's return type.
+pub fn closure_into_test() -> Result<i32, i32> {
+    let res: Result<i32, i16> = Some(1)
+        .into_iter()
+        .map(|i| {
+            let err: i8 = 1;
+            // To avoid warnings during rustfix
+            if true {
+                Err(err)?;
+            }
+            Ok(i)
+        })
+        .next()
+        .unwrap();
+
+    Ok(res?)
+}
+
+fn nested_error() -> Result<i32, i32> {
+    Ok(1)
+}
+
+fn main() {
+    basic_test().unwrap();
+    into_test().unwrap();
+    negative_test().unwrap();
+    closure_matches_test().unwrap();
+    closure_into_test().unwrap();
+}

tests/ui/try_err.stderr

@@ -0,0 +1,32 @@
+error: returning an `Err(_)` with the `?` operator
+  --> $DIR/try_err.rs:11:9
+   |
+LL |         Err(err)?;
+   |         ^^^^^^^^^ help: try this: `return Err(err)`
+   |
+note: lint level defined here
+  --> $DIR/try_err.rs:3:9
+   |
+LL | #![deny(clippy::try_err)]
+   |         ^^^^^^^^^^^^^^^
+
+error: returning an `Err(_)` with the `?` operator
+  --> $DIR/try_err.rs:21:9
+   |
+LL |         Err(err)?;
+   |         ^^^^^^^^^ help: try this: `return Err(err.into())`
+
+error: returning an `Err(_)` with the `?` operator
+  --> $DIR/try_err.rs:41:17
+   |
+LL |                 Err(err)?;
+   |                 ^^^^^^^^^ help: try this: `return Err(err)`
+
+error: returning an `Err(_)` with the `?` operator
+  --> $DIR/try_err.rs:60:17
+   |
+LL |                 Err(err)?;
+   |                 ^^^^^^^^^ help: try this: `return Err(err.into())`
+
+error: aborting due to 4 previous errors
+