diff options
| -rw-r--r-- | clippy_lints/src/manual_unwrap_or.rs | 17 | ||||
| -rw-r--r-- | src/lintlist/mod.rs | 2 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or.fixed | 31 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or.rs | 31 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or.stderr | 18 |
5 files changed, 69 insertions, 30 deletions
diff --git a/clippy_lints/src/manual_unwrap_or.rs b/clippy_lints/src/manual_unwrap_or.rs index 719a8b91f66..ddb8cc25077 100644 --- 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/src/lintlist/mod.rs b/src/lintlist/mod.rs index debd3c31d8b..6301d623a2b 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1183,7 +1183,7 @@ vec![ 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 a9cc8678c9d..a8736f1e6ef 100644 --- 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 5d03d9db163..bede8cffc32 100644 --- 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 8f6835ed78d..674f2952635 100644 --- 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, |