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

…issues
rust-lang · Oct 14, 2020 · d5de6c5 · d5de6c5
1 parent 5cd2689
commit d5de6c5
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 29 deletions.
diff --git a/clippy_lints/src/manual_unwrap_or.rs b/clippy_lints/src/manual_unwrap_or.rs
@@ -33,7 +33,7 @@ declare_clippy_lint! {
     /// ```
     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 lint_option_unwrap_or_case<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tc
         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/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
@@ -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
@@ -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,