From 8f7cadec0b163f90243b7f8502cb715be7c87a20 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 22 Sep 2025 18:50:01 +0100 Subject: [PATCH 1/2] Add or_else_then_unwrap This is the same as or_then_unwrap but for or_else calls with a closure immediately calling `Some`. --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/mod.rs | 41 ++++++++++ .../src/methods/or_else_then_unwrap.rs | 81 +++++++++++++++++++ tests/ui/or_else_then_unwrap.fixed | 66 +++++++++++++++ tests/ui/or_else_then_unwrap.rs | 69 ++++++++++++++++ tests/ui/or_else_then_unwrap.stderr | 26 ++++++ 7 files changed, 285 insertions(+) create mode 100644 clippy_lints/src/methods/or_else_then_unwrap.rs create mode 100644 tests/ui/or_else_then_unwrap.fixed create mode 100644 tests/ui/or_else_then_unwrap.rs create mode 100644 tests/ui/or_else_then_unwrap.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index c773cf15756b..2db746e13674 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6485,6 +6485,7 @@ Released 2018-09-13 [`option_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or_else [`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option [`option_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_unwrap_used +[`or_else_then_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_else_then_unwrap [`or_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call [`or_then_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_then_unwrap [`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5563b8094f01..07a118f56676 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -444,6 +444,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::OPTION_AS_REF_DEREF_INFO, crate::methods::OPTION_FILTER_MAP_INFO, crate::methods::OPTION_MAP_OR_NONE_INFO, + crate::methods::OR_ELSE_THEN_UNWRAP_INFO, crate::methods::OR_FUN_CALL_INFO, crate::methods::OR_THEN_UNWRAP_INFO, crate::methods::PATH_BUF_PUSH_OVERWRITE_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8679689c8ad4..bd62236397a3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -87,6 +87,7 @@ mod option_as_ref_cloned; mod option_as_ref_deref; mod option_map_or_none; mod option_map_unwrap_or; +mod or_else_then_unwrap; mod or_fun_call; mod or_then_unwrap; mod path_buf_push_overwrite; @@ -916,6 +917,42 @@ declare_clippy_lint! { "using `.chars().next()` to check if a string starts with a char" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `.or_else(…).unwrap()` calls to Options and Results. + /// + /// ### Why is this bad? + /// You should use `.unwrap_or_else(…)` instead for clarity. + /// + /// ### Example + /// ```no_run + /// # let fallback = "fallback"; + /// // Result + /// # type Error = &'static str; + /// # let result: Result, Error> = Err("error"); + /// let value = result.or_else(|_| Ok::, Error>(vec![fallback])).unwrap(); + /// + /// // Option + /// # let option: Option> = None; + /// let value = option.or_else(|| Some(vec![fallback])).unwrap(); + /// ``` + /// Use instead: + /// ```no_run + /// # let fallback = "fallback"; + /// // Result + /// # let result: Result, &str> = Err("error"); + /// let value = result.unwrap_or_else(|_| vec![fallback]); + /// + /// // Option + /// # let option: Option> = None; + /// let value = option.unwrap_or_else(|| vec![fallback]); + /// ``` + #[clippy::version = "1.90.0"] + pub OR_ELSE_THEN_UNWRAP, + complexity, + "checks for `.or_else(…).unwrap()` calls to Options and Results." +} + declare_clippy_lint! { /// ### What it does /// Checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`, @@ -4680,6 +4717,7 @@ impl_lint_pass!(Methods => [ RESULT_MAP_OR_INTO_OPTION, OPTION_MAP_OR_NONE, BIND_INSTEAD_OF_MAP, + OR_ELSE_THEN_UNWRAP, OR_FUN_CALL, OR_THEN_UNWRAP, EXPECT_FUN_CALL, @@ -5534,6 +5572,9 @@ impl Methods { Some((sym::or, recv, [or_arg], or_span, _)) => { or_then_unwrap::check(cx, expr, recv, or_arg, or_span); }, + Some((sym::or_else, recv, [or_arg], or_span, _)) => { + or_else_then_unwrap::check(cx, expr, recv, or_arg, or_span); + }, _ => {}, } unnecessary_literal_unwrap::check(cx, expr, recv, name, args); diff --git a/clippy_lints/src/methods/or_else_then_unwrap.rs b/clippy_lints/src/methods/or_else_then_unwrap.rs new file mode 100644 index 000000000000..b779c4479225 --- /dev/null +++ b/clippy_lints/src/methods/or_else_then_unwrap.rs @@ -0,0 +1,81 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{is_res_lang_ctor, path_res}; +use rustc_errors::Applicability; +use rustc_hir::lang_items::LangItem; +use rustc_hir::{Body, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_span::{Span, sym}; + +use super::OR_ELSE_THEN_UNWRAP; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + unwrap_expr: &Expr<'_>, + recv: &'tcx Expr<'tcx>, + or_else_arg: &'tcx Expr<'_>, + or_span: Span, +) { + let ty = cx.typeck_results().expr_ty(recv); // get type of x (we later check if it's Option or Result) + let title; + let or_else_arg_content: Span; + + if is_type_diagnostic_item(cx, ty, sym::Option) { + title = "found `.or_else(|| Some(…)).unwrap()`"; + if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::OptionSome) { + or_else_arg_content = content; + } else { + return; + } + } else if is_type_diagnostic_item(cx, ty, sym::Result) { + title = "found `.or_else(|| Ok(…)).unwrap()`"; + if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::ResultOk) { + or_else_arg_content = content; + } else { + return; + } + } else { + // Someone has implemented a struct with .or(...).unwrap() chaining, + // but it's not an Option or a Result, so bail + return; + } + + let mut applicability = Applicability::MachineApplicable; + let suggestion = format!( + "unwrap_or_else(|| {})", + snippet_with_applicability(cx, or_else_arg_content, "..", &mut applicability) + ); + + span_lint_and_sugg( + cx, + OR_ELSE_THEN_UNWRAP, + unwrap_expr.span.with_lo(or_span.lo()), + title, + "try", + suggestion, + applicability, + ); +} + +fn get_content_if_ctor_matches_in_closure(cx: &LateContext<'_>, expr: &Expr<'_>, item: LangItem) -> Option { + if let ExprKind::Closure(closure) = expr.kind { + if let Body { + params: [], + value: body, + } = cx.tcx.hir_body(closure.body) + { + if let ExprKind::Call(some_expr, [arg]) = body.kind + && is_res_lang_ctor(cx, path_res(cx, some_expr), item) + { + Some(arg.span.source_callsite()) + } else { + None + } + } else { + None + } + } else { + None + } +} diff --git a/tests/ui/or_else_then_unwrap.fixed b/tests/ui/or_else_then_unwrap.fixed new file mode 100644 index 000000000000..f796fea94121 --- /dev/null +++ b/tests/ui/or_else_then_unwrap.fixed @@ -0,0 +1,66 @@ +#![warn(clippy::or_then_unwrap)] +#![allow(clippy::map_identity, clippy::let_unit_value, clippy::unnecessary_literal_unwrap)] + +struct SomeStruct; +impl SomeStruct { + fn or_else Option>(self, _: F) -> Self { + self + } + fn unwrap(&self) {} +} + +struct SomeOtherStruct; +impl SomeOtherStruct { + fn or_else(self) -> Self { + self + } + fn unwrap(&self) {} +} + +struct Wrapper { + inner: &'static str, +} +impl Wrapper { + fn new(inner: &'static str) -> Self { + Self { inner } + } +} + +fn main() { + let option: Option = None; + let _ = option.unwrap_or_else(|| Wrapper::new("fallback")); // should trigger lint + // + //~^^ or_else_then_unwrap + + // as part of a method chain + let option: Option = None; + let _ = option + .map(|v| v) + .unwrap_or_else(|| Wrapper::new("fallback")) + .inner + .to_string() + .chars(); + + // Call with macro should preserve the macro call rather than expand it + let option: Option> = None; + let _ = option.unwrap_or_else(|| vec!["fallback"]); // should trigger lint + // + //~^^ or_else_then_unwrap + + // Not Option/Result + let instance = SomeStruct {}; + let _ = instance.or_else(|| Some(SomeStruct {})).unwrap(); // should not trigger lint + + // or takes no argument + let instance = SomeOtherStruct {}; + let _ = instance.or_else().unwrap(); // should not trigger lint and should not panic + + // None in or + let option: Option = None; + #[allow(clippy::unnecessary_lazy_evaluations)] + let _ = option.or_else(|| None).unwrap(); // should not trigger lint + + // other function between + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).map(|v| v).unwrap(); // should not trigger lint +} diff --git a/tests/ui/or_else_then_unwrap.rs b/tests/ui/or_else_then_unwrap.rs new file mode 100644 index 000000000000..be6d3f68d4bd --- /dev/null +++ b/tests/ui/or_else_then_unwrap.rs @@ -0,0 +1,69 @@ +#![warn(clippy::or_then_unwrap)] +#![allow(clippy::map_identity, clippy::let_unit_value, clippy::unnecessary_literal_unwrap)] + +struct SomeStruct; +impl SomeStruct { + fn or_else Option>(self, _: F) -> Self { + self + } + fn unwrap(&self) {} +} + +struct SomeOtherStruct; +impl SomeOtherStruct { + fn or_else(self) -> Self { + self + } + fn unwrap(&self) {} +} + +struct Wrapper { + inner: &'static str, +} +impl Wrapper { + fn new(inner: &'static str) -> Self { + Self { inner } + } +} + +fn main() { + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); // should trigger lint + // + //~^^ or_else_then_unwrap + + // as part of a method chain + let option: Option = None; + let _ = option + .map(|v| v) + .or_else(|| Some(Wrapper::new("fallback"))) // should trigger lint + // + //~^^ or_else_then_unwrap + .unwrap() + .inner + .to_string() + .chars(); + + // Call with macro should preserve the macro call rather than expand it + let option: Option> = None; + let _ = option.or_else(|| Some(vec!["fallback"])).unwrap(); // should trigger lint + // + //~^^ or_else_then_unwrap + + // Not Option/Result + let instance = SomeStruct {}; + let _ = instance.or_else(|| Some(SomeStruct {})).unwrap(); // should not trigger lint + + // or takes no argument + let instance = SomeOtherStruct {}; + let _ = instance.or_else().unwrap(); // should not trigger lint and should not panic + + // None in or + let option: Option = None; + #[allow(clippy::unnecessary_lazy_evaluations)] + let _ = option.or_else(|| None).unwrap(); // should not trigger lint + + // other function between + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).map(|v| v).unwrap(); // should not trigger lint +} diff --git a/tests/ui/or_else_then_unwrap.stderr b/tests/ui/or_else_then_unwrap.stderr new file mode 100644 index 000000000000..b062ae4cb19b --- /dev/null +++ b/tests/ui/or_else_then_unwrap.stderr @@ -0,0 +1,26 @@ +error: found `.or_else(|| Some(…)).unwrap()` + --> tests/ui/or_else_then_unwrap.rs:31:20 + | +LL | let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); // should trigger lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| Wrapper::new("fallback"))` + | + = note: `-D clippy::or-else-then-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::or_else_then_unwrap)]` + +error: found `.or_else(|| Some(…)).unwrap()` + --> tests/ui/or_else_then_unwrap.rs:39:10 + | +LL | .or_else(|| Some(Wrapper::new("fallback"))) // should trigger lint + | __________^ +... | +LL | | .unwrap() + | |_________________^ help: try: `unwrap_or_else(|| Wrapper::new("fallback"))` + +error: found `.or_else(|| Some(…)).unwrap()` + --> tests/ui/or_else_then_unwrap.rs:49:20 + | +LL | let _ = option.or_else(|| Some(vec!["fallback"])).unwrap(); // should trigger lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| vec!["fallback"])` + +error: aborting due to 3 previous errors + From b02f978942d46f000eecd6e16ed4948e9d4dcf94 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 25 Sep 2025 22:26:29 +0100 Subject: [PATCH 2/2] Collapse nested `if`s Co-authored-by: Ada Alakbarova <58857108+ada4a@users.noreply.github.com> --- .../src/methods/or_else_then_unwrap.rs | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/methods/or_else_then_unwrap.rs b/clippy_lints/src/methods/or_else_then_unwrap.rs index b779c4479225..3437625ad25c 100644 --- a/clippy_lints/src/methods/or_else_then_unwrap.rs +++ b/clippy_lints/src/methods/or_else_then_unwrap.rs @@ -59,22 +59,15 @@ pub(super) fn check<'tcx>( } fn get_content_if_ctor_matches_in_closure(cx: &LateContext<'_>, expr: &Expr<'_>, item: LangItem) -> Option { - if let ExprKind::Closure(closure) = expr.kind { - if let Body { + if let ExprKind::Closure(closure) = expr.kind + && let Body { params: [], value: body, } = cx.tcx.hir_body(closure.body) - { - if let ExprKind::Call(some_expr, [arg]) = body.kind - && is_res_lang_ctor(cx, path_res(cx, some_expr), item) - { - Some(arg.span.source_callsite()) - } else { - None - } - } else { - None - } + && let ExprKind::Call(some_expr, [arg]) = body.kind + && is_res_lang_ctor(cx, path_res(cx, some_expr), item) + { + Some(arg.span.source_callsite()) } else { None }