From 0d21ae0e194fd8f7f1f67bf1921910e0ca21a32c Mon Sep 17 00:00:00 2001
From: Tim Nielens <tim.nielens@gmail.com>
Date: Sat, 24 Oct 2020 11:35:05 +0200
Subject: [PATCH] manual-unwrap-or / pr remarks, round 3

---
 clippy_lints/src/manual_unwrap_or.rs | 13 +++----------
 tests/ui/manual_unwrap_or.fixed      | 12 +++++++++++-
 tests/ui/manual_unwrap_or.rs         | 15 ++++++++++++++-
 tests/ui/manual_unwrap_or.stderr     | 19 ++++++++++++++-----
 4 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/clippy_lints/src/manual_unwrap_or.rs b/clippy_lints/src/manual_unwrap_or.rs
index cc9ee28d027..22aa37e41fe 100644
--- a/clippy_lints/src/manual_unwrap_or.rs
+++ b/clippy_lints/src/manual_unwrap_or.rs
@@ -1,5 +1,6 @@
 use crate::consts::constant_simple;
 use crate::utils;
+use crate::utils::sugg;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{def, Arm, Expr, ExprKind, Pat, PatKind, QPath};
@@ -104,28 +105,20 @@ fn lint_manual_unwrap_or<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
             None
         };
         if let Some(or_arm) = applicable_or_arm(match_arms);
-        if let Some(scrutinee_snippet) = utils::snippet_opt(cx, scrutinee.span);
         if let Some(or_body_snippet) = utils::snippet_opt(cx, or_arm.body.span);
         if let Some(indent) = utils::indent_of(cx, expr.span);
         if constant_simple(cx, cx.typeck_results(), or_arm.body).is_some();
         then {
             let reindented_or_body =
                 utils::reindent_multiline(or_body_snippet.into(), true, Some(indent));
-            let wrap_in_parens = !matches!(scrutinee, Expr {
-                kind: ExprKind::Call(..) | ExprKind::Path(_), ..
-            });
-            let l_paren = if wrap_in_parens { "(" } else { "" };
-            let r_paren = if wrap_in_parens { ")" } else { "" };
             utils::span_lint_and_sugg(
                 cx,
                 MANUAL_UNWRAP_OR, expr.span,
                 &format!("this pattern reimplements `{}`", case.unwrap_fn_path()),
                 "replace with",
                 format!(
-                    "{}{}{}.unwrap_or({})",
-                    l_paren,
-                    scrutinee_snippet,
-                    r_paren,
+                    "{}.unwrap_or({})",
+                    sugg::Sugg::hir(cx, scrutinee, "..").maybe_par(),
                     reindented_or_body,
                 ),
                 Applicability::MachineApplicable,
diff --git a/tests/ui/manual_unwrap_or.fixed b/tests/ui/manual_unwrap_or.fixed
index 582f5c6f7a8..5aa5a43cb92 100644
--- a/tests/ui/manual_unwrap_or.fixed
+++ b/tests/ui/manual_unwrap_or.fixed
@@ -74,9 +74,19 @@ fn result_unwrap_or() {
     let a = Ok::<i32, &str>(1);
     a.unwrap_or(42);
 
-    // int case, suggestion must surround with parenthesis
+    // int case, suggestion must surround Result expr with parenthesis
     (Ok(1) as Result<i32, &str>).unwrap_or(42);
 
+    // method call case, suggestion must not surround Result expr `s.method()` with parenthesis
+    struct S {}
+    impl S {
+        fn method(self) -> Option<i32> {
+            Some(42)
+        }
+    }
+    let s = S {};
+    s.method().unwrap_or(42);
+
     // int case reversed
     Ok::<i32, &str>(1).unwrap_or(42);
 
diff --git a/tests/ui/manual_unwrap_or.rs b/tests/ui/manual_unwrap_or.rs
index 0e2b7ecadb3..df534031f54 100644
--- a/tests/ui/manual_unwrap_or.rs
+++ b/tests/ui/manual_unwrap_or.rs
@@ -95,12 +95,25 @@ fn result_unwrap_or() {
         Err(_) => 42,
     };
 
-    // int case, suggestion must surround with parenthesis
+    // int case, suggestion must surround Result expr with parenthesis
     match Ok(1) as Result<i32, &str> {
         Ok(i) => i,
         Err(_) => 42,
     };
 
+    // method call case, suggestion must not surround Result expr `s.method()` with parenthesis
+    struct S {}
+    impl S {
+        fn method(self) -> Option<i32> {
+            Some(42)
+        }
+    }
+    let s = S {};
+    match s.method() {
+        Some(i) => i,
+        None => 42,
+    };
+
     // int case reversed
     match Ok::<i32, &str>(1) {
         Err(_) => 42,
diff --git a/tests/ui/manual_unwrap_or.stderr b/tests/ui/manual_unwrap_or.stderr
index 6603ab43437..fc174c4c270 100644
--- a/tests/ui/manual_unwrap_or.stderr
+++ b/tests/ui/manual_unwrap_or.stderr
@@ -84,8 +84,17 @@ LL | |         Err(_) => 42,
 LL | |     };
    | |_____^ help: replace with: `(Ok(1) as Result<i32, &str>).unwrap_or(42)`
 
+error: this pattern reimplements `Option::unwrap_or`
+  --> $DIR/manual_unwrap_or.rs:112:5
+   |
+LL | /     match s.method() {
+LL | |         Some(i) => i,
+LL | |         None => 42,
+LL | |     };
+   | |_____^ help: replace with: `s.method().unwrap_or(42)`
+
 error: this pattern reimplements `Result::unwrap_or`
-  --> $DIR/manual_unwrap_or.rs:105:5
+  --> $DIR/manual_unwrap_or.rs:118:5
    |
 LL | /     match Ok::<i32, &str>(1) {
 LL | |         Err(_) => 42,
@@ -94,7 +103,7 @@ LL | |     };
    | |_____^ help: replace with: `Ok::<i32, &str>(1).unwrap_or(42)`
 
 error: this pattern reimplements `Result::unwrap_or`
-  --> $DIR/manual_unwrap_or.rs:111:5
+  --> $DIR/manual_unwrap_or.rs:124:5
    |
 LL | /     match Ok::<i32, &str>(1) {
 LL | |         Ok(i) => i,
@@ -103,7 +112,7 @@ LL | |     };
    | |_____^ help: replace with: `Ok::<i32, &str>(1).unwrap_or(1 + 42)`
 
 error: this pattern reimplements `Result::unwrap_or`
-  --> $DIR/manual_unwrap_or.rs:118:5
+  --> $DIR/manual_unwrap_or.rs:131:5
    |
 LL | /     match Ok::<i32, &str>(1) {
 LL | |         Ok(i) => i,
@@ -124,7 +133,7 @@ LL |     });
    |
 
 error: this pattern reimplements `Result::unwrap_or`
-  --> $DIR/manual_unwrap_or.rs:128:5
+  --> $DIR/manual_unwrap_or.rs:141:5
    |
 LL | /     match Ok::<&str, &str>("Bob") {
 LL | |         Ok(i) => i,
@@ -132,5 +141,5 @@ LL | |         Err(_) => "Alice",
 LL | |     };
    | |_____^ help: replace with: `Ok::<&str, &str>("Bob").unwrap_or("Alice")`
 
-error: aborting due to 12 previous errors
+error: aborting due to 13 previous errors