manual-unwrap-or / remove unwrap_or_else suggestion due to ownership issues

diff --git a/clippy_lints/src/manual_unwrap_or.rs b/clippy_lints/src/manual_unwrap_or.rs
index 719a8b91f66905f0b30a4bd90dd3d8ab83526817..ddb8cc25077e177bc3420f09a0e476799ab02e18 100644 (file)
--- a/clippy_lints/src/manual_unwrap_or.rs
+++ b/clippy_lints/src/manual_unwrap_or.rs
@@ -33,7 +33,7 @@
     /// ```
     pub MANUAL_UNWRAP_OR,
     complexity,
-    "finds patterns that can be encoded more concisely with `Option::unwrap_or(_else)`"
+    "finds patterns that can be encoded more concisely with `Option::unwrap_or`"
 }
 
 declare_lint_pass!(ManualUnwrapOr => [MANUAL_UNWRAP_OR]);
@@ -83,26 +83,19 @@ fn applicable_none_arm<'a>(arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> {
         if let Some(scrutinee_snippet) = utils::snippet_opt(cx, scrutinee.span);
         if let Some(none_body_snippet) = utils::snippet_opt(cx, none_arm.body.span);
         if let Some(indent) = utils::indent_of(cx, expr.span);
+        if constant_simple(cx, cx.typeck_results(), none_arm.body).is_some();
         then {
             let reindented_none_body =
                 utils::reindent_multiline(none_body_snippet.into(), true, Some(indent));
-            let eager_eval = constant_simple(cx, cx.typeck_results(), none_arm.body).is_some();
-            let method = if eager_eval {
-                "unwrap_or"
-            } else {
-                "unwrap_or_else"
-            };
             utils::span_lint_and_sugg(
                 cx,
                 MANUAL_UNWRAP_OR, expr.span,
-                &format!("this pattern reimplements `Option::{}`", &method),
+                "this pattern reimplements `Option::unwrap_or`",
                 "replace with",
                 format!(
-                    "{}.{}({}{})",
+                    "{}.unwrap_or({})",
                     scrutinee_snippet,
-                    method,
-                    if eager_eval { "" } else { "|| " },
-                    reindented_none_body
+                    reindented_none_body,
                 ),
                 Applicability::MachineApplicable,
             );

diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index debd3c31d8bffec9d40c6f98756623f70e126e11..6301d623a2b12955c92a29c9afcf13bbb2deff51 100644 (file)
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1183,7 +1183,7 @@
     Lint {
         name: "manual_unwrap_or",
         group: "complexity",
-        desc: "finds patterns that can be encoded more concisely with `Option::unwrap_or(_else)`",
+        desc: "finds patterns that can be encoded more concisely with `Option::unwrap_or`",
         deprecation: None,
         module: "manual_unwrap_or",
     },

diff --git a/tests/ui/manual_unwrap_or.fixed b/tests/ui/manual_unwrap_or.fixed
index a9cc8678c9d17e25d71e541f40893762a3d25190..a8736f1e6efe159c7390fc719585a2a2745369ae 100644 (file)
--- a/tests/ui/manual_unwrap_or.fixed
+++ b/tests/ui/manual_unwrap_or.fixed
@@ -12,10 +12,11 @@ fn unwrap_or() {
     Some(1).unwrap_or(1 + 42);
 
     // multiline case
-    Some(1).unwrap_or_else(|| {
-        let a = 1 + 42;
-        let b = a + 42;
-        b + 42
+    #[rustfmt::skip]
+    Some(1).unwrap_or({
+        42 + 42
+            + 42 + 42 + 42
+            + 42 + 42 + 42
     });
 
     // string case
@@ -40,6 +41,28 @@ fn unwrap_or() {
             None => break,
         };
     }
+
+    // cases where the none arm isn't a constant expression
+    // are not linted due to potential ownership issues
+
+    // ownership issue example, don't lint
+    struct NonCopyable;
+    let mut option: Option<NonCopyable> = None;
+    match option {
+        Some(x) => x,
+        None => {
+            option = Some(NonCopyable);
+            // some more code ...
+            option.unwrap()
+        },
+    };
+
+    // ownership issue example, don't lint
+    let option: Option<&str> = None;
+    match option {
+        Some(s) => s,
+        None => &format!("{} {}!", "hello", "world"),
+    };
 }
 
 fn main() {}

diff --git a/tests/ui/manual_unwrap_or.rs b/tests/ui/manual_unwrap_or.rs
index 5d03d9db16392a2c10f7558441a9d14730e3c306..bede8cffc326ecea9783815694c69bf9141c9352 100644 (file)
--- a/tests/ui/manual_unwrap_or.rs
+++ b/tests/ui/manual_unwrap_or.rs
@@ -21,13 +21,14 @@ fn unwrap_or() {
     };
 
     // multiline case
+    #[rustfmt::skip]
     match Some(1) {
         Some(i) => i,
         None => {
-            let a = 1 + 42;
-            let b = a + 42;
-            b + 42
-        },
+            42 + 42
+                + 42 + 42 + 42
+                + 42 + 42 + 42
+        }
     };
 
     // string case
@@ -55,6 +56,28 @@ fn unwrap_or() {
             None => break,
         };
     }
+
+    // cases where the none arm isn't a constant expression
+    // are not linted due to potential ownership issues
+
+    // ownership issue example, don't lint
+    struct NonCopyable;
+    let mut option: Option<NonCopyable> = None;
+    match option {
+        Some(x) => x,
+        None => {
+            option = Some(NonCopyable);
+            // some more code ...
+            option.unwrap()
+        },
+    };
+
+    // ownership issue example, don't lint
+    let option: Option<&str> = None;
+    match option {
+        Some(s) => s,
+        None => &format!("{} {}!", "hello", "world"),
+    };
 }
 
 fn main() {}

diff --git a/tests/ui/manual_unwrap_or.stderr b/tests/ui/manual_unwrap_or.stderr
index 8f6835ed78d290002a7f7d3e335196d3d828a6c2..674f2952635f6298adeb7d3cbfae994a804e7a10 100644 (file)
--- a/tests/ui/manual_unwrap_or.stderr
+++ b/tests/ui/manual_unwrap_or.stderr
@@ -27,29 +27,29 @@ LL | |         None => 1 + 42,
 LL | |     };
    | |_____^ help: replace with: `Some(1).unwrap_or(1 + 42)`
 
-error: this pattern reimplements `Option::unwrap_or_else`
-  --> $DIR/manual_unwrap_or.rs:24:5
+error: this pattern reimplements `Option::unwrap_or`
+  --> $DIR/manual_unwrap_or.rs:25:5
    |
 LL | /     match Some(1) {
 LL | |         Some(i) => i,
 LL | |         None => {
-LL | |             let a = 1 + 42;
+LL | |             42 + 42
 ...  |
-LL | |         },
+LL | |         }
 LL | |     };
    | |_____^
    |
 help: replace with
    |
-LL |     Some(1).unwrap_or_else(|| {
-LL |         let a = 1 + 42;
-LL |         let b = a + 42;
-LL |         b + 42
+LL |     Some(1).unwrap_or({
+LL |         42 + 42
+LL |             + 42 + 42 + 42
+LL |             + 42 + 42 + 42
 LL |     });
    |
 
 error: this pattern reimplements `Option::unwrap_or`
-  --> $DIR/manual_unwrap_or.rs:34:5
+  --> $DIR/manual_unwrap_or.rs:35:5
    |
 LL | /     match Some("Bob") {
 LL | |         Some(i) => i,