about summary refs log tree commit diff
diff options
context:
space:
mode:
authorSamuel Tardieu <sam@rfc1149.net>2025-02-17 15:22:46 +0100
committerSamuel Tardieu <sam@rfc1149.net>2025-02-17 15:49:44 +0100
commit66d19d84ae1a83bba0317e2c991cadb2ab28a064 (patch)
tree866ec5eb82739c547bac657e3c5366d09652bd6f
parent735bed7aa5085c14f96a6e887c1d091c5a69ef85 (diff)
downloadrust-66d19d84ae1a83bba0317e2c991cadb2ab28a064.tar.gz
rust-66d19d84ae1a83bba0317e2c991cadb2ab28a064.zip
`manual_ok_err`: blockify the replacement of an `else if …`
If the part being replaced is an `if` expression following an `else`,
the replacement expression must be blockified.
-rw-r--r--clippy_lints/src/matches/manual_ok_err.rs15
-rw-r--r--tests/ui/manual_ok_err.fixed9
-rw-r--r--tests/ui/manual_ok_err.rs11
-rw-r--r--tests/ui/manual_ok_err.stderr20
4 files changed, 52 insertions, 3 deletions
diff --git a/clippy_lints/src/matches/manual_ok_err.rs b/clippy_lints/src/matches/manual_ok_err.rs
index 3deaaf96c1e..576e42a564c 100644
--- a/clippy_lints/src/matches/manual_ok_err.rs
+++ b/clippy_lints/src/matches/manual_ok_err.rs
@@ -1,7 +1,8 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::{indent_of, reindent_multiline};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::option_arg_ty;
-use clippy_utils::{is_res_lang_ctor, path_res, peel_blocks, span_contains_comment};
+use clippy_utils::{get_parent_expr, is_res_lang_ctor, path_res, peel_blocks, span_contains_comment};
 use rustc_ast::BindingMode;
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr};
@@ -132,13 +133,23 @@ fn apply_lint(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>, is_ok
         Applicability::MachineApplicable
     };
     let scrut = Sugg::hir_with_applicability(cx, scrutinee, "..", &mut app).maybe_par();
+    let sugg = format!("{scrut}.{method}()");
+    // If the expression being expanded is the `if …` part of an `else if …`, it must be blockified.
+    let sugg = if let Some(parent_expr) = get_parent_expr(cx, expr)
+        && let ExprKind::If(_, _, Some(else_part)) = parent_expr.kind
+        && else_part.hir_id == expr.hir_id
+    {
+        reindent_multiline(&format!("{{\n    {sugg}\n}}"), true, indent_of(cx, parent_expr.span))
+    } else {
+        sugg
+    };
     span_lint_and_sugg(
         cx,
         MANUAL_OK_ERR,
         expr.span,
         format!("manual implementation of `{method}`"),
         "replace with",
-        format!("{scrut}.{method}()"),
+        sugg,
         app,
     );
 }
diff --git a/tests/ui/manual_ok_err.fixed b/tests/ui/manual_ok_err.fixed
index e7e0464c478..bc169b64be9 100644
--- a/tests/ui/manual_ok_err.fixed
+++ b/tests/ui/manual_ok_err.fixed
@@ -89,3 +89,12 @@ const fn cf(x: Result<u32, &'static str>) -> Option<u32> {
         Err(_) => None,
     }
 }
+
+fn issue14239() {
+    let _ = if false {
+        None
+    } else {
+        "1".parse::<u8>().ok()
+    };
+    //~^^^^^ manual_ok_err
+}
diff --git a/tests/ui/manual_ok_err.rs b/tests/ui/manual_ok_err.rs
index 77300b7af53..03c730d4b4e 100644
--- a/tests/ui/manual_ok_err.rs
+++ b/tests/ui/manual_ok_err.rs
@@ -125,3 +125,14 @@ const fn cf(x: Result<u32, &'static str>) -> Option<u32> {
         Err(_) => None,
     }
 }
+
+fn issue14239() {
+    let _ = if false {
+        None
+    } else if let Ok(n) = "1".parse::<u8>() {
+        Some(n)
+    } else {
+        None
+    };
+    //~^^^^^ manual_ok_err
+}
diff --git a/tests/ui/manual_ok_err.stderr b/tests/ui/manual_ok_err.stderr
index f10f52cc4c9..13fceacda10 100644
--- a/tests/ui/manual_ok_err.stderr
+++ b/tests/ui/manual_ok_err.stderr
@@ -93,5 +93,23 @@ LL | |         _ => None,
 LL | |     };
    | |_____^ help: replace with: `(-S).ok()`
 
-error: aborting due to 8 previous errors
+error: manual implementation of `ok`
+  --> tests/ui/manual_ok_err.rs:132:12
+   |
+LL |       } else if let Ok(n) = "1".parse::<u8>() {
+   |  ____________^
+LL | |         Some(n)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
+   |
+help: replace with
+   |
+LL ~     } else {
+LL +         "1".parse::<u8>().ok()
+LL ~     };
+   |
+
+error: aborting due to 9 previous errors