Added detailled suggs for new case

pag4k · pag4k · commit 6165cccf7e43 · 2021-02-17T16:41:50.000-05:00
diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs
@@ -1,13 +1,13 @@
 use crate::utils::{
-    contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_sugg,
-    span_lint_and_then, visitors::find_all_ret_expressions,
+    contains_return, in_macro, match_qpath, paths, return_ty, snippet, span_lint_and_then,
+    visitors::find_all_ret_expressions,
 };
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{Body, ExprKind, FnDecl, HirId, Impl, ItemKind, Node};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::subst::GenericArgKind;
+use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::sym;
 use rustc_span::Span;
@@ -85,46 +85,44 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
             }
         }
 
-        // Check if return type is Option or Result. If neither, abort.
-        let return_ty = return_ty(cx, hir_id);
-        let (return_type_label, path) = if is_type_diagnostic_item(cx, return_ty, sym::option_type) {
-            ("Option", &paths::OPTION_SOME)
-        } else if is_type_diagnostic_item(cx, return_ty, sym::result_type) {
-            ("Result", &paths::RESULT_OK)
-        } else {
-            return;
-        };
-
-        // Take the first inner type of the Option or Result. If can't, abort.
-        let inner_ty = if_chain! {
-            // Skip Option or Result and take the first outermost inner type.
-            if let Some(inner_ty) = return_ty.walk().nth(1);
-            if let GenericArgKind::Type(inner_ty) = inner_ty.unpack();
-            then {
-                inner_ty
+        // Get the wrapper and inner types, if can't, abort.
+        let (return_type_label, path, inner_type) = if let ty::Adt(adt_def, subst) = return_ty(cx, hir_id).kind() {
+            if cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did) {
+                ("Option", &paths::OPTION_SOME, subst.type_at(0))
+            } else if cx.tcx.is_diagnostic_item(sym::result_type, adt_def.did) {
+                ("Result", &paths::RESULT_OK, subst.type_at(0))
             } else {
                 return;
             }
+        } else {
+            return;
         };
 
         // Check if all return expression respect the following condition and collect them.
         let mut suggs = Vec::new();
         let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
             if_chain! {
-                // Abort if in macro.
                 if !in_macro(ret_expr.span);
                 // Check if a function call.
                 if let ExprKind::Call(ref func, ref args) = ret_expr.kind;
                 // Get the Path of the function call.
                 if let ExprKind::Path(ref qpath) = func.kind;
                 // Check if OPTION_SOME or RESULT_OK, depending on return type.
                 if match_qpath(qpath, path);
-                // Make sure the function call has only one argument.
                 if args.len() == 1;
                 // Make sure the function argument does not contain a return expression.
                 if !contains_return(&args[0]);
                 then {
-                    suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
+                    suggs.push(
+                        (
+                            ret_expr.span,
+                            if inner_type.is_unit() {
+                                "".to_string()
+                            } else {
+                                snippet(cx, args[0].span.source_callsite(), "..").to_string()
+                            }
+                        )
+                    );
                     true
                 } else {
                     false
@@ -133,42 +131,36 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
         });
 
         if can_sugg && !suggs.is_empty() {
-            // Issue 6640: If the inner type is Unit, emit lint similar to clippy::unused_unit.
-            if inner_ty.is_unit() {
-                span_lint_and_sugg(
-                    cx,
-                    UNNECESSARY_WRAPS,
-                    fn_decl.output.span(),
-                    "unneeded wrapped unit return type",
-                    format!("remove the `-> {}<[...]>`", return_type_label).as_str(),
-                    String::new(),
-                    Applicability::MaybeIncorrect,
-                );
+            let (lint_msg, return_type_suggestion_msg, return_type_suggestion) = if inner_type.is_unit() {
+                (
+                    "this function's return value is unnecessary".to_string(),
+                    "remove the return type...".to_string(),
+                    snippet(cx, fn_decl.output.span(), "..").to_string(),
+                )
             } else {
-                span_lint_and_then(
-                    cx,
-                    UNNECESSARY_WRAPS,
-                    span,
+                (
                     format!(
                         "this function's return value is unnecessarily wrapped by `{}`",
                         return_type_label
-                    )
-                    .as_str(),
-                    |diag| {
-                        diag.span_suggestion(
-                            fn_decl.output.span(),
-                            format!("remove `{}` from the return type...", return_type_label).as_str(),
-                            inner_ty.to_string(),
-                            Applicability::MaybeIncorrect,
-                        );
-                        diag.multipart_suggestion(
-                            "...and change the returning expressions",
-                            suggs,
-                            Applicability::MaybeIncorrect,
-                        );
-                    },
+                    ),
+                    format!("remove `{}` from the return type...", return_type_label),
+                    inner_type.to_string(),
+                )
+            };
+
+            span_lint_and_then(cx, UNNECESSARY_WRAPS, span, lint_msg.as_str(), |diag| {
+                diag.span_suggestion(
+                    fn_decl.output.span(),
+                    return_type_suggestion_msg.as_str(),
+                    return_type_suggestion,
+                    Applicability::MaybeIncorrect,
                 );
-            }
+                diag.multipart_suggestion(
+                    "...and then change the returning expressions",
+                    suggs,
+                    Applicability::MaybeIncorrect,
+                );
+            });
         }
     }
 }
diff --git a/tests/ui/unnecessary_wraps.stderr b/tests/ui/unnecessary_wraps.stderr
@@ -15,7 +15,7 @@ help: remove `Option` from the return type...
    |
 LL | fn func1(a: bool, b: bool) -> i32 {
    |                               ^^^
-help: ...and change the returning expressions
+help: ...and then change the returning expressions
    |
 LL |         return 42;
 LL |     }
@@ -41,7 +41,7 @@ help: remove `Option` from the return type...
    |
 LL | fn func2(a: bool, b: bool) -> i32 {
    |                               ^^^
-help: ...and change the returning expressions
+help: ...and then change the returning expressions
    |
 LL |         return 10;
 LL |     }
@@ -63,7 +63,7 @@ help: remove `Option` from the return type...
    |
 LL | fn func5() -> i32 {
    |               ^^^
-help: ...and change the returning expressions
+help: ...and then change the returning expressions
    |
 LL |     1
    |
@@ -80,7 +80,7 @@ help: remove `Result` from the return type...
    |
 LL | fn func7() -> i32 {
    |               ^^^
-help: ...and change the returning expressions
+help: ...and then change the returning expressions
    |
 LL |     1
    |
@@ -97,22 +97,62 @@ help: remove `Option` from the return type...
    |
 LL |     fn func12() -> i32 {
    |                    ^^^
-help: ...and change the returning expressions
+help: ...and then change the returning expressions
    |
 LL |         1
    |
 
-error: unneeded wrapped unit return type
-  --> $DIR/unnecessary_wraps.rs:120:38
+error: this function's return value is unnecessary
+  --> $DIR/unnecessary_wraps.rs:120:1
+   |
+LL | / fn issue_6640_1(a: bool, b: bool) -> Option<()> {
+LL | |     if a && b {
+LL | |         return Some(());
+LL | |     }
+...  |
+LL | |     }
+LL | | }
+   | |_^
+   |
+help: remove the return type...
    |
 LL | fn issue_6640_1(a: bool, b: bool) -> Option<()> {
-   |                                      ^^^^^^^^^^ help: remove the `-> Option<[...]>`
+   |                                      ^^^^^^^^^^
+help: ...and then change the returning expressions
+   |
+LL |         return ;
+LL |     }
+LL |     if a {
+LL |         Some(());
+LL |         
+LL |     } else {
+ ...
 
-error: unneeded wrapped unit return type
-  --> $DIR/unnecessary_wraps.rs:133:38
+error: this function's return value is unnecessary
+  --> $DIR/unnecessary_wraps.rs:133:1
+   |
+LL | / fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
+LL | |     if a && b {
+LL | |         return Ok(());
+LL | |     }
+...  |
+LL | |     }
+LL | | }
+   | |_^
+   |
+help: remove the return type...
    |
 LL | fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
-   |                                      ^^^^^^^^^^^^^^^ help: remove the `-> Result<[...]>`
+   |                                      ^^^^^^^^^^^^^^^
+help: ...and then change the returning expressions
+   |
+LL |         return ;
+LL |     }
+LL |     if a {
+LL |         
+LL |     } else {
+LL |         return ;
+   |
 
 error: aborting due to 7 previous errors
 
