Add .err().expect() lint

Author: b-NC

clippy_lints/src/methods/err_expect.rs

@@ -0,0 +1,59 @@
+use super::ERR_EXPECT;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::ty::implements_trait;
+use clippy_utils::{meets_msrv, msrvs, ty::is_type_diagnostic_item};
+use rustc_errors::Applicability;
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use rustc_middle::ty::Ty;
+use rustc_semver::RustcVersion;
+use rustc_span::{sym, Span};
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    _expr: &rustc_hir::Expr<'_>,
+    recv: &rustc_hir::Expr<'_>,
+    msrv: Option<&RustcVersion>,
+    expect_span: Span,
+    err_span: Span,
+) {
+    if_chain! {
+        if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result);
+        // Test the version to make sure the lint can be showed (expect_err has been introduced in rust 1.17.0 : https://github.com/rust-lang/rust/pull/38982)
+        if meets_msrv(msrv, &msrvs::EXPECT_ERR);
+
+        // Grabs the `Result<T, E>` type
+        let result_type = cx.typeck_results().expr_ty(recv);
+        // Tests if the T type in a `Result<T, E>` is not None
+        if let Some(data_type) = get_data_type(cx, result_type);
+        // Tests if the T type in a `Result<T, E>` implements debug
+        if has_debug_impl(data_type, cx);
+
+        then {
+            span_lint_and_sugg(
+                cx,
+                ERR_EXPECT,
+                err_span.to(expect_span),
+                "called `.err().expect()` on a `Result` value",
+                "try",
+                "expect_err".to_string(),
+                Applicability::MachineApplicable
+        );
+        }
+    };
+}
+
+/// Given a `Result<T, E>` type, return its data (`T`).
+fn get_data_type<'a>(cx: &LateContext<'_>, ty: Ty<'a>) -> Option<Ty<'a>> {
+    match ty.kind() {
+        ty::Adt(_, substs) if is_type_diagnostic_item(cx, ty, sym::Result) => substs.types().next(),
+        _ => None,
+    }
+}
+
+/// Given a type, very if the Debug trait has been impl'd
+fn has_debug_impl<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
+    cx.tcx
+        .get_diagnostic_item(sym::Debug)
+        .map_or(false, |debug| implements_trait(cx, ty, debug, &[]))
+}

clippy_lints/src/methods/mod.rs

@@ -9,6 +9,7 @@ mod chars_next_cmp_with_unwrap;
 mod clone_on_copy;
 mod clone_on_ref_ptr;
 mod cloned_instead_of_copied;
+mod err_expect;
 mod expect_fun_call;
 mod expect_used;
 mod extend_with_drain;
@@ -362,6 +363,29 @@ declare_clippy_lint! {
     "using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `.err().expect()` calls on the `Result` type.
+    ///
+    /// ### Why is this bad?
+    /// `.expect_err()` can be called directly to avoid the extra type conversion from `ok()`.
+    ///
+    /// ### Example
+    /// ```should_panic
+    /// let x: Result<u32, &str> = Ok(10);
+    /// x.err().expect("Testing err().expect()");
+    /// ```
+    /// Use instead:
+    /// ```should_panic
+    /// let x: Result<u32, &str> = Ok(10);
+    /// x.expect_err("Testing expect_err");
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub ERR_EXPECT,
+    style,
+    r#"using `.err().expect("")` when `.expect_err("")` can be used"#
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usages of `_.unwrap_or_else(Default::default)` on `Option` and
@@ -2165,6 +2189,7 @@ impl_lint_pass!(Methods => [
     NEEDLESS_SPLITN,
     UNNECESSARY_TO_OWNED,
     UNNECESSARY_JOIN,
+    ERR_EXPECT,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -2428,8 +2453,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
             },
             ("expect", [_]) => match method_call(recv) {
                 Some(("ok", [recv], _)) => ok_expect::check(cx, expr, recv),
+                Some(("err", [recv], err_span)) => err_expect::check(cx, expr, recv, msrv, span, err_span),
                 _ => expect_used::check(cx, expr, recv),
             },
+
             ("extend", [arg]) => {
                 string_extend_chars::check(cx, expr, recv, arg);
                 extend_with_drain::check(cx, expr, recv, arg);
@@ -2571,6 +2598,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                     unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
                 },
             },
+
             _ => {},
         }
     }

clippy_lints/src/utils/conf.rs

@@ -156,7 +156,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, EXPECT_ERR.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),

clippy_utils/src/msrvs.rs

@@ -30,6 +30,6 @@ msrv_aliases! {
     1,34,0 { TRY_FROM }
     1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
     1,28,0 { FROM_BOOL }
-    1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST }
+    1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR }
     1,16,0 { STR_REPEAT }
 }

tests/ui/err_expect.fixed

@@ -0,0 +1,14 @@
+// run-rustfix
+
+struct MyTypeNonDebug;
+
+#[derive(Debug)]
+struct MyTypeDebug;
+
+fn main() {
+    let test_debug: Result<MyTypeDebug, u32> = Ok(MyTypeDebug);
+    test_debug.expect_err("Testing debug type");
+
+    let test_non_debug: Result<MyTypeNonDebug, u32> = Ok(MyTypeNonDebug);
+    test_non_debug.err().expect("Testing non debug type");
+}

tests/ui/err_expect.rs

@@ -0,0 +1,14 @@
+// run-rustfix
+
+struct MyTypeNonDebug;
+
+#[derive(Debug)]
+struct MyTypeDebug;
+
+fn main() {
+    let test_debug: Result<MyTypeDebug, u32> = Ok(MyTypeDebug);
+    test_debug.err().expect("Testing debug type");
+
+    let test_non_debug: Result<MyTypeNonDebug, u32> = Ok(MyTypeNonDebug);
+    test_non_debug.err().expect("Testing non debug type");
+}

tests/ui/err_expect.stderr

@@ -0,0 +1,10 @@
+error: called `.err().expect()` on a `Result` value
+  --> $DIR/err_expect.rs:10:16
+   |
+LL |     test_debug.err().expect("Testing debug type");
+   |                ^^^^^^^^^^^^ help: try: `expect_err`
+   |
+   = note: `-D clippy::err-expect` implied by `-D warnings`
+
+error: aborting due to previous error
+

tests/ui/min_rust_version_attr.rs

@@ -145,6 +145,11 @@ fn int_from_bool() -> u8 {
     true as u8
 }
 
+fn err_expect() {
+    let x: Result<u32, &str> = Ok(10);
+    x.err().expect("Testing expect_err");
+}
+
 fn main() {
     filter_map_next();
     checked_conversion();
@@ -162,6 +167,7 @@ fn main() {
     missing_const_for_fn();
     unnest_or_patterns();
     int_from_bool();
+    err_expect();
 }
 
 mod just_under_msrv {

tests/ui/min_rust_version_attr.stderr

@@ -1,12 +1,12 @@
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:186:24
+  --> $DIR/min_rust_version_attr.rs:192:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::manual-strip` implied by `-D warnings`
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:185:9
+  --> $DIR/min_rust_version_attr.rs:191:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -17,13 +17,13 @@ LL ~             assert_eq!(<stripped>.to_uppercase(), "WORLD!");
    |
 
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:198:24
+  --> $DIR/min_rust_version_attr.rs:204:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:197:9
+  --> $DIR/min_rust_version_attr.rs:203:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^