Fix for issue 6640

3 files changed, 93 insertions, 136 deletions

diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs
index 8ac5dd696b7..eec76ce8b03 100644
--- a/clippy_lints/src/unnecessary_wraps.rs
+++ b/clippy_lints/src/unnecessary_wraps.rs
@@ -1,6 +1,6 @@
 use crate::utils::{
-    contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
-    visitors::find_all_ret_expressions,
+    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,
 };
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -64,6 +64,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
         span: Span,
         hir_id: HirId,
     ) {
+        // Abort if public function/method or closure.
         match fn_kind {
             FnKind::ItemFn(.., visibility, _) | FnKind::Method(.., Some(visibility), _) => {
                 if visibility.node.is_pub() {
@@ -74,6 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
             _ => (),
         }
 
+        // Abort if the method is implementing a trait or of it a trait method.
         if let Some(Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) {
             if matches!(
                 item.kind,
@@ -83,22 +85,43 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
             }
         }
 
-        let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::option_type) {
+        // 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(cx, hir_id), sym::result_type) {
+        } 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
+            } 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()));
@@ -110,39 +133,42 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
         });
 
         if can_sugg && !suggs.is_empty() {
-            span_lint_and_then(
-                cx,
-                UNNECESSARY_WRAPS,
-                span,
-                format!(
-                    "this function's return value is unnecessarily wrapped by `{}`",
-                    return_type
-                )
-                .as_str(),
-                |diag| {
-                    let inner_ty = return_ty(cx, hir_id)
-                        .walk()
-                        .skip(1) // skip `std::option::Option` or `std::result::Result`
-                        .take(1) // take the first outermost inner type
-                        .filter_map(|inner| match inner.unpack() {
-                            GenericArgKind::Type(inner_ty) => Some(inner_ty.to_string()),
-                            _ => None,
-                        });
-                    inner_ty.for_each(|inner_ty| {
+            // 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,
+                );
+            } 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).as_str(),
-                            inner_ty,
+                            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,
-                    );
-                },
-            );
+                        diag.multipart_suggestion(
+                            "...and change the returning expressions",
+                            suggs,
+                            Applicability::MaybeIncorrect,
+                        );
+                    },
+                );
+            }
         }
     }
 }
diff --git a/tests/ui/unnecessary_wraps.rs b/tests/ui/unnecessary_wraps.rs
index a4570098d71..2d6aedc97ef 100644
--- a/tests/ui/unnecessary_wraps.rs
+++ b/tests/ui/unnecessary_wraps.rs
@@ -116,8 +116,36 @@ fn issue_6384(s: &str) -> Option<&str> {
     })
 }
 
+// should be linted
+fn issue_6640_1(a: bool, b: bool) -> Option<()> {
+    if a && b {
+        return Some(());
+    }
+    if a {
+        Some(());
+        Some(())
+    } else {
+        return Some(());
+    }
+}
+
+// should be linted
+fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
+    if a && b {
+        return Ok(());
+    }
+    if a {
+        Ok(());
+        Ok(())
+    } else {
+        return Ok(());
+    }
+}
+
 fn main() {
     // method calls are not linted
     func1(true, true);
     func2(true, true);
+    issue_6640_1(true, true);
+    issue_6640_2(true, true);
 }
diff --git a/tests/ui/unnecessary_wraps.stderr b/tests/ui/unnecessary_wraps.stderr
index 410f054b8ef..3ca5a8d1702 100644
--- a/tests/ui/unnecessary_wraps.stderr
+++ b/tests/ui/unnecessary_wraps.stderr
@@ -1,106 +1,9 @@
-error: this function's return value is unnecessarily wrapped by `Option`
-  --> $DIR/unnecessary_wraps.rs:8:1
-   |
-LL | / fn func1(a: bool, b: bool) -> Option<i32> {
-LL | |     if a && b {
-LL | |         return Some(42);
-LL | |     }
-...  |
-LL | |     }
-LL | | }
-   | |_^
-   |
-   = note: `-D clippy::unnecessary-wraps` implied by `-D warnings`
-help: remove `Option` from the return type...
-   |
-LL | fn func1(a: bool, b: bool) -> i32 {
-   |                               ^^^
-help: ...and change the returning expressions
-   |
-LL |         return 42;
-LL |     }
-LL |     if a {
-LL |         Some(-1);
-LL |         2
-LL |     } else {
- ...
-
-error: this function's return value is unnecessarily wrapped by `Option`
-  --> $DIR/unnecessary_wraps.rs:21:1
-   |
-LL | / fn func2(a: bool, b: bool) -> Option<i32> {
-LL | |     if a && b {
-LL | |         return Some(10);
-LL | |     }
-...  |
-LL | |     }
-LL | | }
-   | |_^
-   |
-help: remove `Option` from the return type...
-   |
-LL | fn func2(a: bool, b: bool) -> i32 {
-   |                               ^^^
-help: ...and change the returning expressions
-   |
-LL |         return 10;
-LL |     }
-LL |     if a {
-LL |         20
-LL |     } else {
-LL |         30
-   |
-
-error: this function's return value is unnecessarily wrapped by `Option`
-  --> $DIR/unnecessary_wraps.rs:51:1
-   |
-LL | / fn func5() -> Option<i32> {
-LL | |     Some(1)
-LL | | }
-   | |_^
-   |
-help: remove `Option` from the return type...
-   |
-LL | fn func5() -> i32 {
-   |               ^^^
-help: ...and change the returning expressions
-   |
-LL |     1
-   |
-
-error: this function's return value is unnecessarily wrapped by `Result`
-  --> $DIR/unnecessary_wraps.rs:61:1
-   |
-LL | / fn func7() -> Result<i32, ()> {
-LL | |     Ok(1)
-LL | | }
-   | |_^
-   |
-help: remove `Result` from the return type...
-   |
-LL | fn func7() -> i32 {
-   |               ^^^
-help: ...and change the returning expressions
-   |
-LL |     1
-   |
-
-error: this function's return value is unnecessarily wrapped by `Option`
-  --> $DIR/unnecessary_wraps.rs:93:5
-   |
-LL | /     fn func12() -> Option<i32> {
-LL | |         Some(1)
-LL | |     }
-   | |_____^
-   |
-help: remove `Option` from the return type...
-   |
-LL |     fn func12() -> i32 {
-   |                    ^^^
-help: ...and change the returning expressions
-   |
-LL |         1
+error[E0282]: type annotations needed
+  --> $DIR/unnecessary_wraps.rs:138:9
    |
+LL |         Ok(());
+   |         ^^ cannot infer type for type parameter `E` declared on the enum `Result`
 
-error: aborting due to 5 previous errors
+error: aborting due to previous error
 
+For more information about this error, try `rustc --explain E0282`.