feat(ok_expect): add autofix (#15867)

changelog: [`ok_expect`]: add autofix

Author: llogiq

clippy_lints/src/methods/err_expect.rs

@@ -1,7 +1,6 @@
 use super::ERR_EXPECT;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::res::MaybeDef;
 use clippy_utils::ty::has_debug_impl;
 use rustc_errors::Applicability;
 use rustc_lint::LateContext;
@@ -17,12 +16,10 @@ pub(super) fn check(
     err_span: Span,
     msrv: Msrv,
 ) {
-    if cx.typeck_results().expr_ty(recv).is_diag_item(cx, sym::Result)
-        // 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
-        && let Some(data_type) = get_data_type(cx, result_type)
-        // Tests if the T type in a `Result<T, E>` implements debug
+    let result_ty = cx.typeck_results().expr_ty(recv);
+    // Grabs the `Result<T, E>` type
+    if let Some(data_type) = get_data_type(cx, result_ty)
+        // Tests if the T type in a `Result<T, E>` implements Debug
         && has_debug_impl(cx, data_type)
         && msrv.meets(cx, msrvs::EXPECT_ERR)
     {
@@ -41,7 +38,7 @@ pub(super) fn check(
 /// 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(_, args) if ty.is_diag_item(cx, sym::Result) => args.types().next(),
+        ty::Adt(adt, args) if cx.tcx.is_diagnostic_item(sym::Result, adt.did()) => args.types().next(),
         _ => None,
     }
 }

clippy_lints/src/methods/mod.rs

@@ -5167,7 +5167,7 @@ impl Methods {
                 },
                 (sym::expect, [_]) => {
                     match method_call(recv) {
-                        Some((sym::ok, recv, [], _, _)) => ok_expect::check(cx, expr, recv),
+                        Some((sym::ok, recv_inner, [], _, _)) => ok_expect::check(cx, expr, recv, recv_inner),
                         Some((sym::err, recv, [], err_span, _)) => {
                             err_expect::check(cx, expr, recv, span, err_span, self.msrv);
                         },

clippy_lints/src/methods/ok_expect.rs

@@ -1,36 +1,43 @@
-use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::res::MaybeDef;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::ty::has_debug_impl;
+use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_lint::LateContext;
+use rustc_lint::{LateContext, LintContext};
 use rustc_middle::ty::{self, Ty};
 use rustc_span::sym;
 
 use super::OK_EXPECT;
 
 /// lint use of `ok().expect()` for `Result`s
-pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>) {
-    if cx.typeck_results().expr_ty(recv).is_diag_item(cx, sym::Result)
-        // lint if the caller of `ok()` is a `Result`
-        && let result_type = cx.typeck_results().expr_ty(recv)
-        && let Some(error_type) = get_error_type(cx, result_type)
+pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, recv_inner: &hir::Expr<'_>) {
+    let result_ty = cx.typeck_results().expr_ty(recv_inner);
+    // lint if the caller of `ok()` is a `Result`
+    if let Some(error_type) = get_error_type(cx, result_ty)
         && has_debug_impl(cx, error_type)
+        && let Some(span) = recv.span.trim_start(recv_inner.span)
     {
-        span_lint_and_help(
+        span_lint_and_then(
             cx,
             OK_EXPECT,
             expr.span,
             "called `ok().expect()` on a `Result` value",
-            None,
-            "you can call `expect()` directly on the `Result`",
+            |diag| {
+                let span = cx.sess().source_map().span_extend_while_whitespace(span);
+                diag.span_suggestion_verbose(
+                    span,
+                    "call `expect()` directly on the `Result`",
+                    String::new(),
+                    Applicability::MachineApplicable,
+                );
+            },
         );
     }
 }
 
 /// Given a `Result<T, E>` type, return its error type (`E`).
 fn get_error_type<'a>(cx: &LateContext<'_>, ty: Ty<'a>) -> Option<Ty<'a>> {
     match ty.kind() {
-        ty::Adt(_, args) if ty.is_diag_item(cx, sym::Result) => args.types().nth(1),
+        ty::Adt(adt, args) if cx.tcx.is_diagnostic_item(sym::Result, adt.did()) => args.types().nth(1),
         _ => None,
     }
 }

tests/ui/ok_expect.fixed

@@ -0,0 +1,51 @@
+#![allow(clippy::unnecessary_literal_unwrap)]
+
+use std::io;
+
+struct MyError(()); // doesn't implement Debug
+
+#[derive(Debug)]
+struct MyErrorWithParam<T> {
+    x: T,
+}
+
+fn main() {
+    let res: Result<i32, ()> = Ok(0);
+    let _ = res.unwrap();
+
+    res.expect("disaster!");
+    //~^ ok_expect
+
+    res.expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+    //~^^ ok_expect
+
+    let resres = res;
+    resres.expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+    //~^^^ ok_expect
+
+    // this one has a suboptimal suggestion, but oh well
+    std::process::Command::new("rustc")
+        .arg("-vV")
+        .output().expect("failed to get rustc version");
+    //~^^^^^ ok_expect
+
+    // the following should not warn, since `expect` isn't implemented unless
+    // the error type implements `Debug`
+    let res2: Result<i32, MyError> = Ok(0);
+    res2.ok().expect("oh noes!");
+    let res3: Result<u32, MyErrorWithParam<u8>> = Ok(0);
+    res3.expect("whoof");
+    //~^ ok_expect
+
+    let res4: Result<u32, io::Error> = Ok(0);
+    res4.expect("argh");
+    //~^ ok_expect
+
+    let res5: io::Result<u32> = Ok(0);
+    res5.expect("oops");
+    //~^ ok_expect
+
+    let res6: Result<u32, &str> = Ok(0);
+    res6.expect("meh");
+    //~^ ok_expect
+}

tests/ui/ok_expect.rs

@@ -16,6 +16,24 @@ fn main() {
     res.ok().expect("disaster!");
     //~^ ok_expect
 
+    res.ok()
+        .expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+    //~^^ ok_expect
+
+    let resres = res;
+    resres
+        .ok()
+        .expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+    //~^^^ ok_expect
+
+    // this one has a suboptimal suggestion, but oh well
+    std::process::Command::new("rustc")
+        .arg("-vV")
+        .output()
+        .ok()
+        .expect("failed to get rustc version");
+    //~^^^^^ ok_expect
+
     // the following should not warn, since `expect` isn't implemented unless
     // the error type implements `Debug`
     let res2: Result<i32, MyError> = Ok(0);

tests/ui/ok_expect.stderr

@@ -4,41 +4,109 @@ error: called `ok().expect()` on a `Result` value
 LL |     res.ok().expect("disaster!");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: you can call `expect()` directly on the `Result`
    = note: `-D clippy::ok-expect` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::ok_expect)]`
+help: call `expect()` directly on the `Result`
+   |
+LL -     res.ok().expect("disaster!");
+LL +     res.expect("disaster!");
+   |
+
+error: called `ok().expect()` on a `Result` value
+  --> tests/ui/ok_expect.rs:19:5
+   |
+LL | /     res.ok()
+LL | |         .expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+   | |___________________________________________________________________________^
+   |
+help: call `expect()` directly on the `Result`
+   |
+LL -     res.ok()
+LL -         .expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+LL +     res.expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+   |
 
 error: called `ok().expect()` on a `Result` value
   --> tests/ui/ok_expect.rs:24:5
    |
+LL | /     resres
+LL | |         .ok()
+LL | |         .expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+   | |___________________________________________________________________________^
+   |
+help: call `expect()` directly on the `Result`
+   |
+LL -     resres
+LL -         .ok()
+LL -         .expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+LL +     resres.expect("longlonglonglonglonglonglonglonglonglonglonglonglonglong");
+   |
+
+error: called `ok().expect()` on a `Result` value
+  --> tests/ui/ok_expect.rs:30:5
+   |
+LL | /     std::process::Command::new("rustc")
+LL | |         .arg("-vV")
+LL | |         .output()
+LL | |         .ok()
+LL | |         .expect("failed to get rustc version");
+   | |______________________________________________^
+   |
+help: call `expect()` directly on the `Result`
+   |
+LL -         .output()
+LL -         .ok()
+LL -         .expect("failed to get rustc version");
+LL +         .output().expect("failed to get rustc version");
+   |
+
+error: called `ok().expect()` on a `Result` value
+  --> tests/ui/ok_expect.rs:42:5
+   |
 LL |     res3.ok().expect("whoof");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: you can call `expect()` directly on the `Result`
+help: call `expect()` directly on the `Result`
+   |
+LL -     res3.ok().expect("whoof");
+LL +     res3.expect("whoof");
+   |
 
 error: called `ok().expect()` on a `Result` value
-  --> tests/ui/ok_expect.rs:28:5
+  --> tests/ui/ok_expect.rs:46:5
    |
 LL |     res4.ok().expect("argh");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: you can call `expect()` directly on the `Result`
+help: call `expect()` directly on the `Result`
+   |
+LL -     res4.ok().expect("argh");
+LL +     res4.expect("argh");
+   |
 
 error: called `ok().expect()` on a `Result` value
-  --> tests/ui/ok_expect.rs:32:5
+  --> tests/ui/ok_expect.rs:50:5
    |
 LL |     res5.ok().expect("oops");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: you can call `expect()` directly on the `Result`
+help: call `expect()` directly on the `Result`
+   |
+LL -     res5.ok().expect("oops");
+LL +     res5.expect("oops");
+   |
 
 error: called `ok().expect()` on a `Result` value
-  --> tests/ui/ok_expect.rs:36:5
+  --> tests/ui/ok_expect.rs:54:5
    |
 LL |     res6.ok().expect("meh");
    |     ^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: you can call `expect()` directly on the `Result`
+help: call `expect()` directly on the `Result`
+   |
+LL -     res6.ok().expect("meh");
+LL +     res6.expect("meh");
+   |
 
-error: aborting due to 5 previous errors
+error: aborting due to 8 previous errors