From e5ec071db0c2fb5ee9d52e548cdd2e850e73f7c5 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 12 Jul 2026 00:01:11 +0200 Subject: [PATCH] Add `let_else_ok_or` lint New `style` lint that detects `let...else` statements binding the payload of an `Option` whose `else` branch only does `return Err()`, and suggests the more concise `Option::ok_or`/`ok_or_else` with the `?` operator: let Some(value) = opt else { return Err("missing"); }; // -> let value = opt.ok_or("missing")?; `ok_or_else` is suggested when the error value is non-trivial, to preserve the laziness of the original `else` branch. The lint bails on `ref` bindings, comments or `#[cfg]` in the `else` branch, and `return Err(..)` expressions produced by a macro expansion (e.g. `anyhow::bail!`), where the error value cannot be rendered into a sound suggestion. `ok_or`/`ok_or_else` move the error value unconditionally, whereas the `else` branch only runs on `None`. When the error value moves a non-`Copy` local that is still used on the `Some` path (or could be moved again across a loop iteration), the rewrite would reference a moved value and not compile, so the suggestion is downgraded to non-machine-applicable in that case (values that are merely borrowed, e.g. `Err(format!("{x}"))`, stay machine-applicable). The error snippet is rendered through the statement's context so an error value that is itself a macro call (`format!(..)`) is shown as written instead of leaking its expansion internals. --- CHANGELOG.md | 1 + clippy_config/src/types.rs | 6 +- clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/let_else_ok_or.rs | 245 +++++++++++++++++++++++ clippy_lints/src/lib.rs | 4 +- clippy_utils/src/sym.rs | 1 + tests/ui/let_else_ok_or.fixed | 147 ++++++++++++++ tests/ui/let_else_ok_or.rs | 159 +++++++++++++++ tests/ui/let_else_ok_or.stderr | 53 +++++ tests/ui/let_else_ok_or_unfixable.rs | 31 +++ tests/ui/let_else_ok_or_unfixable.stderr | 21 ++ tests/ui/manual_let_else.fixed | 3 + tests/ui/manual_let_else.rs | 3 + tests/ui/manual_let_else.stderr | 70 +++---- 14 files changed, 706 insertions(+), 39 deletions(-) create mode 100644 clippy_lints/src/let_else_ok_or.rs create mode 100644 tests/ui/let_else_ok_or.fixed create mode 100644 tests/ui/let_else_ok_or.rs create mode 100644 tests/ui/let_else_ok_or.stderr create mode 100644 tests/ui/let_else_ok_or_unfixable.rs create mode 100644 tests/ui/let_else_ok_or_unfixable.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 856c229f4679..334db30194d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7067,6 +7067,7 @@ Released 2018-09-13 [`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty [`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero [`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return +[`let_else_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_else_ok_or [`let_underscore_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_drop [`let_underscore_future`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_future [`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index a6f7bd69226b..6a998992136b 100644 --- a/clippy_config/src/types.rs +++ b/clippy_config/src/types.rs @@ -468,9 +468,9 @@ impl<'de> Deserialize<'de> for SourceItemOrderingModuleItemGroupings { let all_items = SourceItemOrderingModuleItemKind::all_variants(); if expected_items.is_empty() && items_total == all_items.len() { - let Some(use_group_index) = lut.get(&SourceItemOrderingModuleItemKind::Use) else { - return Err(de::Error::custom("Error in internal LUT.")); - }; + let use_group_index = lut + .get(&SourceItemOrderingModuleItemKind::Use) + .ok_or_else(|| de::Error::custom("Error in internal LUT."))?; let Some((_, use_group_items)) = groups.get(*use_group_index) else { return Err(de::Error::custom("Error in internal LUT.")); }; diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e77a0ff4a8ab..37c6930ab0f2 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -257,6 +257,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::len_without_is_empty::LEN_WITHOUT_IS_EMPTY_INFO, crate::len_zero::COMPARISON_TO_EMPTY_INFO, crate::len_zero::LEN_ZERO_INFO, + crate::let_else_ok_or::LET_ELSE_OK_OR_INFO, crate::let_if_seq::USELESS_LET_IF_SEQ_INFO, crate::let_underscore::LET_UNDERSCORE_FUTURE_INFO, crate::let_underscore::LET_UNDERSCORE_LOCK_INFO, diff --git a/clippy_lints/src/let_else_ok_or.rs b/clippy_lints/src/let_else_ok_or.rs new file mode 100644 index 000000000000..c10630d630c3 --- /dev/null +++ b/clippy_lints/src/let_else_ok_or.rs @@ -0,0 +1,245 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::eager_or_lazy::switch_to_lazy_eval; +use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath}; +use clippy_utils::source::snippet_with_context; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::is_copy; +use clippy_utils::visitors::{Descend, for_each_expr}; +use clippy_utils::{ + get_enclosing_loop_or_multi_call_closure, is_in_const_context, span_contains_cfg, span_contains_comment, sym, +}; +use core::ops::ControlFlow; +use rustc_errors::Applicability; +use rustc_hir::LangItem::{OptionSome, ResultErr}; +use rustc_hir::{ + BindingMode, Block, ByRef, Expr, ExprKind, HirId, HirIdSet, LetStmt, Mutability, PatKind, Stmt, StmtKind, +}; +use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::mir::FakeReadCause; +use rustc_middle::ty::{BorrowKind, TyCtxt}; +use rustc_session::impl_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for `let...else` statements that bind the contents of an `Option` + /// and whose `else` branch just returns an `Err`. + /// + /// ### Why is this bad? + /// `Option::ok_or` combined with the `?` operator expresses the same thing + /// more concisely. + /// + /// ### Example + /// ```no_run + /// # fn parse(opt: Option) -> Result { + /// let Some(value) = opt else { + /// return Err("missing value".to_string()); + /// }; + /// # Ok(value) + /// # } + /// ``` + /// Use instead: + /// ```no_run + /// # fn parse(opt: Option) -> Result { + /// let value = opt.ok_or_else(|| "missing value".to_string())?; + /// # Ok(value) + /// # } + /// ``` + #[clippy::version = "1.98.0"] + pub LET_ELSE_OK_OR, + style, + "`let...else` returning an `Err` that could be written with `Option::ok_or` and `?`" +} + +impl_lint_pass!(LetElseOkOr => [LET_ELSE_OK_OR]); + +pub struct LetElseOkOr { + /// Whether the features needed to call `Option::ok_or`/`ok_or_else` and `?` in a `const` + /// context (`const_option_ops`, `const_trait_impl` and `const_try`) are all enabled. When + /// they are not, the suggestion would not compile inside a `const` item. + const_ok_or_available: bool, +} + +impl LetElseOkOr { + pub fn new(tcx: TyCtxt<'_>, _conf: &'static Conf) -> Self { + let features = tcx.features(); + Self { + const_ok_or_available: features.enabled(sym::const_option_ops) + && features.enabled(sym::const_trait_impl) + && features.enabled(sym::const_try), + } + } +} + +impl<'tcx> LateLintPass<'tcx> for LetElseOkOr { + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) { + // `Option::ok_or`/`ok_or_else` and `?` are not usable in a `const` context unless the + // relevant unstable features are enabled, so the suggestion would not compile there. + if is_in_const_context(cx) && !self.const_ok_or_available { + return; + } + if let StmtKind::Let(LetStmt { + pat, + init: Some(init), + els: Some(els), + .. + }) = stmt.kind + // Don't fire on code coming from a macro: the suggestion would not be applicable. + && !stmt.span.from_expansion() + // The pattern binds the payload of an `Option`, e.g. `Some(value)`. + && let PatKind::TupleStruct(qpath, [inner_pat], _) = pat.kind + && cx.qpath_res(&qpath, pat.hir_id).ctor_parent(cx).is_lang_item(cx, OptionSome) + // ... to a single, by-value binding (skip `ref`/`mut ref` and nested patterns). + && let PatKind::Binding(BindingMode(ByRef::No, mutability), _, ident, None) = inner_pat.kind + // The initializer is really an owned `Option`, so `ok_or` is callable on it. + && cx.typeck_results().expr_ty(init).is_diag_item(cx, sym::Option) + // The `else` branch unconditionally does `return Err()`. + && let Some(err_arg) = err_return_arg(cx, els) + // Don't drop comments or `#[cfg]`-ed code from the `else` branch. + && !span_contains_comment(cx, els.span) + && !span_contains_cfg(cx, els.span) + { + // `return Err()` lets `` be *coerced* to the function's error type (e.g. an + // unsizing coercion of `&[u8; N]` to `&[u8]`), whereas `?` routes the value through + // `From::from`, which may have no matching impl (`From<&[u8; N]>` for `&[u8]` does not + // exist). When such a type-changing coercion is applied the rewrite might not compile, + // so only keep the suggestion machine-applicable when the error value already has the + // function's error type (an identity `From`). Benign adjustments that don't change the + // type (e.g. a reborrow) are fine, so compare the written type against the adjusted one + // rather than counting them. + let typeck = cx.typeck_results(); + let mut applicability = if typeck.expr_ty(err_arg) == typeck.expr_ty_adjusted(err_arg) + // `ok_or`/`ok_or_else` move the error value unconditionally, whereas the `else` + // branch only runs on `None`. If the error value moves out a place still used on + // the `Some` path, the rewrite would reference a moved value and not compile, so + // keep it out of `--fix`. + && !err_moves_local_used_elsewhere(cx, err_arg) + { + Applicability::MachineApplicable + } else { + Applicability::MaybeIncorrect + }; + let init_sugg = Sugg::hir_with_context(cx, init, stmt.span.ctxt(), "..", &mut applicability).maybe_paren(); + // Use the statement's context so an error value that is itself a macro call (e.g. + // `format!(..)`) is rendered as written instead of leaking its expansion internals. + let err_snippet = snippet_with_context(cx, err_arg.span, stmt.span.ctxt(), "..", &mut applicability).0; + + // `let...else` only evaluates the `else` branch on `None`, so keep that laziness + // by suggesting `ok_or_else` whenever the error is more than a trivial value. + let method = if switch_to_lazy_eval(cx, err_arg) { + format!("ok_or_else(|| {err_snippet})") + } else { + format!("ok_or({err_snippet})") + }; + + let mut_str = if mutability == Mutability::Mut { "mut " } else { "" }; + + span_lint_and_sugg( + cx, + LET_ELSE_OK_OR, + stmt.span, + "this `let...else` may be rewritten with `Option::ok_or` and `?`", + "replace it with", + format!("let {mut_str}{ident} = {init_sugg}.{method}?;"), + applicability, + ); + } + } +} + +/// If `block` unconditionally diverges through `return Err()`, returns that ``. +fn err_return_arg<'tcx>(cx: &LateContext<'_>, block: &'tcx Block<'tcx>) -> Option<&'tcx Expr<'tcx>> { + let ret_expr = match block { + Block { + stmts: [], + expr: Some(expr), + .. + } => expr, + Block { + stmts: [stmt], + expr: None, + .. + } if let StmtKind::Semi(expr) = stmt.kind => expr, + _ => return None, + }; + + if let ExprKind::Ret(Some(ret_value)) = ret_expr.kind + && let ExprKind::Call(err_path, [err_arg]) = ret_value.kind + // A macro-generated `return Err(..)` (e.g. `anyhow::bail!`) would make us reach into the + // macro for the error value and produce a suggestion leaking its internals. + && !ret_value.span.from_expansion() + && err_path.res(cx).ctor_parent(cx).is_lang_item(cx, ResultErr) + { + Some(err_arg) + } else { + None + } +} + +/// Returns `true` if evaluating `err_arg` moves a non-`Copy` local (or argument) that the rewrite +/// would invalidate. +/// +/// The `else` branch only runs on `None`, so the original code never touches the error value on the +/// `Some` path. `ok_or`/`ok_or_else`, by contrast, move the error value unconditionally, so if it +/// moves out a place that is read elsewhere — or that could be moved again on another loop +/// iteration — the rewrite would reference a moved value and fail to compile. Values that are +/// merely *borrowed* (e.g. `Err(format!("{x}"))`) are fine. +fn err_moves_local_used_elsewhere<'tcx>(cx: &LateContext<'tcx>, err_arg: &'tcx Expr<'tcx>) -> bool { + let mut delegate = MovedLocals { + cx, + locals: HirIdSet::default(), + }; + ExprUseVisitor::for_clippy(cx, err_arg.hir_id.owner.def_id, &mut delegate) + .consume_expr(err_arg) + .into_ok(); + if delegate.locals.is_empty() { + return false; + } + + // A move inside a loop (or a closure called more than once) can run repeatedly and move the + // same place twice; the original `else` branch never does, as it only runs on `None`, which + // diverges via `return`. + if get_enclosing_loop_or_multi_call_closure(cx, err_arg).is_some() { + return true; + } + + // Otherwise the move only breaks compilation if one of the moved locals is read somewhere else + // in the body (the `Some` path still relying on a value the rewrite has now moved). Reads inside + // `err_arg` itself are the move we are rewriting, so they don't count. Walking the whole body + // also covers function parameters, whose scope is the entire body rather than a single block. + let body = cx.tcx.hir_body_owned_by(err_arg.hir_id.owner.def_id); + for_each_expr(cx.tcx, body.value, |e| { + if e.hir_id == err_arg.hir_id { + ControlFlow::Continue(Descend::No) + } else if e.res_local_id().is_some_and(|id| delegate.locals.contains(&id)) { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(Descend::Yes) + } + }) + .is_some() +} + +/// Collects the locals that are *moved* (not just borrowed or copied) by the visited expression. +struct MovedLocals<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + locals: HirIdSet, +} + +impl<'tcx> Delegate<'tcx> for MovedLocals<'_, 'tcx> { + fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) { + // Only a move of a non-`Copy` place invalidates a later use; copies leave the original + // readable, so they can never break the rewrite. + if let PlaceBase::Local(local) = place_with_id.place.base + && !is_copy(self.cx, place_with_id.place.ty()) + { + self.locals.insert(local); + } + } + + fn use_cloned(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: BorrowKind) {} + fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {} +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 74e03c9b3859..3a302025fcb2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1,4 +1,4 @@ -#![feature(box_patterns)] +<#![feature(box_patterns)] #![feature(control_flow_into_value)] #![feature(exact_div)] #![feature(f128)] @@ -190,6 +190,7 @@ mod large_stack_frames; mod legacy_numeric_constants; mod len_without_is_empty; mod len_zero; +mod let_else_ok_or; mod let_if_seq; mod let_underscore; mod let_with_type_underscore; @@ -866,6 +867,7 @@ rustc_lint::late_lint_methods!( RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns, RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse, RestWhenDestructuringStruct: rest_when_destructuring_struct::RestWhenDestructuringStruct = rest_when_destructuring_struct::RestWhenDestructuringStruct, + LetElseOkOr: let_else_ok_or::LetElseOkOr = let_else_ok_or::LetElseOkOr::new(tcx, conf), // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 5da94bfda5e6..7d45b4d1808f 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -189,6 +189,7 @@ generate! { collapsible_else_if, collapsible_if, collect, + const_option_ops, const_ptr, contains, convert_identity, diff --git a/tests/ui/let_else_ok_or.fixed b/tests/ui/let_else_ok_or.fixed new file mode 100644 index 000000000000..558f2e114680 --- /dev/null +++ b/tests/ui/let_else_ok_or.fixed @@ -0,0 +1,147 @@ +#![warn(clippy::let_else_ok_or)] +#![allow( + clippy::let_and_return, + clippy::diverging_sub_expression, + clippy::question_mark, + dead_code, + unused +)] + +use std::ffi::OsString; + +// Trivial error value -> `ok_or`. +fn trivial(opt: Option) -> Result { + //~v let_else_ok_or + let value = opt.ok_or("missing")?; + Ok(value) +} + +// Allocating/expensive error value -> `ok_or_else` to preserve laziness. +fn allocating(opt: Option) -> Result { + //~v let_else_ok_or + let value = opt.ok_or_else(|| "missing value".to_string())?; + Ok(value) +} + +// The real-world motivating case (uutils/diffutils#243). +fn parse_params(mut opts: impl Iterator) -> Result { + let executable = opts.next().ok_or("Usage: ".to_string())?; + Ok(executable.to_string_lossy().to_string()) +} + +fn from_call(opts: &mut impl Iterator) -> Result { + //~v let_else_ok_or + let executable = opts.next().ok_or_else(|| "Usage: ".to_string())?; + Ok(executable) +} + +// `mut` binding should be preserved. +fn mut_binding(opt: Option>) -> Result, &'static str> { + //~v let_else_ok_or + let mut value = opt.ok_or("empty")?; + value.push(1); + Ok(value) +} + +// Method-call receiver needs no extra parentheses. +fn method_receiver(s: &str) -> Result { + //~v let_else_ok_or + let first = s.chars().next().ok_or("empty string")?; + Ok(first) +} + +// The error value only *borrows* a local that is used later (it does not move it), so the rewrite +// stays machine-applicable even though `msg` lives on past the statement. +fn borrows_value_used_later(opt: Option, msg: String) -> Result { + //~v let_else_ok_or + let value = opt.ok_or_else(|| format!("missing: {msg}"))?; + println!("{msg}"); + Ok(value) +} + +// +// Cases that must NOT lint. +// + +// `else` does more than return an `Err`. +fn else_has_side_effects(opt: Option) -> Result { + let Some(value) = opt else { + eprintln!("nothing here"); + return Err("missing"); + }; + Ok(value) +} + +// `else` returns `None`, not `Err` (handled by `question_mark`). +fn returns_none(opt: Option) -> Option { + let Some(value) = opt else { + return None; + }; + Some(value) +} + +// `else` returns a non-`Err` value. +fn returns_default(opt: Option) -> i32 { + let Some(value) = opt else { + return 0; + }; + value +} + +// Comments in the `else` branch would be lost. +fn has_comment(opt: Option) -> Result { + let Some(value) = opt else { + // important explanation + return Err("missing"); + }; + Ok(value) +} + +// Binding by reference cannot be rewritten as `ok_or`. +fn by_ref(opt: &Option) -> Result { + let Some(value) = opt else { + return Err("missing"); + }; + Ok(*value) +} + +// The `else` branch is a macro expanding to `return Err(..)`; the error value lives inside the +// macro, so the suggestion must not reach into it (e.g. `anyhow::bail!`). +macro_rules! bail { + ($msg:literal) => { + return Err($msg) + }; +} + +fn else_is_bail_macro(opt: Option) -> Result { + let Some(value) = opt else { + bail!("missing"); + }; + Ok(value) +} + +// Inside a macro the suggestion would not be applicable. +macro_rules! bind_or_err { + ($opt:expr) => {{ + let Some(value) = $opt else { + return Err("missing"); + }; + value + }}; +} + +fn in_macro(opt: Option) -> Result { + let value = bind_or_err!(opt); + Ok(value) +} + +// In a `const` context the suggestion (`opt.ok_or(..)?`) would not compile, as `Option::ok_or` +// and `?` are not yet const-stable, so the lint must stay silent here. +const fn in_const(opt: Option) -> Result { + let Some(value) = opt else { + return Err(()); + }; + Ok(value) +} + +fn main() {} diff --git a/tests/ui/let_else_ok_or.rs b/tests/ui/let_else_ok_or.rs new file mode 100644 index 000000000000..2e80458c3e88 --- /dev/null +++ b/tests/ui/let_else_ok_or.rs @@ -0,0 +1,159 @@ +#![warn(clippy::let_else_ok_or)] +#![allow( + clippy::let_and_return, + clippy::diverging_sub_expression, + clippy::question_mark, + dead_code, + unused +)] + +use std::ffi::OsString; + +// Trivial error value -> `ok_or`. +fn trivial(opt: Option) -> Result { + //~v let_else_ok_or + let Some(value) = opt else { + return Err("missing"); + }; + Ok(value) +} + +// Allocating/expensive error value -> `ok_or_else` to preserve laziness. +fn allocating(opt: Option) -> Result { + //~v let_else_ok_or + let Some(value) = opt else { + return Err("missing value".to_string()); + }; + Ok(value) +} + +// The real-world motivating case (uutils/diffutils#243). +fn parse_params(mut opts: impl Iterator) -> Result { + let executable = opts.next().ok_or("Usage: ".to_string())?; + Ok(executable.to_string_lossy().to_string()) +} + +fn from_call(opts: &mut impl Iterator) -> Result { + //~v let_else_ok_or + let Some(executable) = opts.next() else { + return Err("Usage: ".to_string()); + }; + Ok(executable) +} + +// `mut` binding should be preserved. +fn mut_binding(opt: Option>) -> Result, &'static str> { + //~v let_else_ok_or + let Some(mut value) = opt else { + return Err("empty"); + }; + value.push(1); + Ok(value) +} + +// Method-call receiver needs no extra parentheses. +fn method_receiver(s: &str) -> Result { + //~v let_else_ok_or + let Some(first) = s.chars().next() else { + return Err("empty string"); + }; + Ok(first) +} + +// The error value only *borrows* a local that is used later (it does not move it), so the rewrite +// stays machine-applicable even though `msg` lives on past the statement. +fn borrows_value_used_later(opt: Option, msg: String) -> Result { + //~v let_else_ok_or + let Some(value) = opt else { + return Err(format!("missing: {msg}")); + }; + println!("{msg}"); + Ok(value) +} + +// +// Cases that must NOT lint. +// + +// `else` does more than return an `Err`. +fn else_has_side_effects(opt: Option) -> Result { + let Some(value) = opt else { + eprintln!("nothing here"); + return Err("missing"); + }; + Ok(value) +} + +// `else` returns `None`, not `Err` (handled by `question_mark`). +fn returns_none(opt: Option) -> Option { + let Some(value) = opt else { + return None; + }; + Some(value) +} + +// `else` returns a non-`Err` value. +fn returns_default(opt: Option) -> i32 { + let Some(value) = opt else { + return 0; + }; + value +} + +// Comments in the `else` branch would be lost. +fn has_comment(opt: Option) -> Result { + let Some(value) = opt else { + // important explanation + return Err("missing"); + }; + Ok(value) +} + +// Binding by reference cannot be rewritten as `ok_or`. +fn by_ref(opt: &Option) -> Result { + let Some(value) = opt else { + return Err("missing"); + }; + Ok(*value) +} + +// The `else` branch is a macro expanding to `return Err(..)`; the error value lives inside the +// macro, so the suggestion must not reach into it (e.g. `anyhow::bail!`). +macro_rules! bail { + ($msg:literal) => { + return Err($msg) + }; +} + +fn else_is_bail_macro(opt: Option) -> Result { + let Some(value) = opt else { + bail!("missing"); + }; + Ok(value) +} + +// Inside a macro the suggestion would not be applicable. +macro_rules! bind_or_err { + ($opt:expr) => {{ + let Some(value) = $opt else { + return Err("missing"); + }; + value + }}; +} + +fn in_macro(opt: Option) -> Result { + let value = bind_or_err!(opt); + Ok(value) +} + +// In a `const` context the suggestion (`opt.ok_or(..)?`) would not compile, as `Option::ok_or` +// and `?` are not yet const-stable, so the lint must stay silent here. +const fn in_const(opt: Option) -> Result { + let Some(value) = opt else { + return Err(()); + }; + Ok(value) +} + +fn main() {} diff --git a/tests/ui/let_else_ok_or.stderr b/tests/ui/let_else_ok_or.stderr new file mode 100644 index 000000000000..7729055086cc --- /dev/null +++ b/tests/ui/let_else_ok_or.stderr @@ -0,0 +1,53 @@ +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or.rs:15:5 + | +LL | / let Some(value) = opt else { +LL | | return Err("missing"); +LL | | }; + | |______^ help: replace it with: `let value = opt.ok_or("missing")?;` + | + = note: `-D clippy::let-else-ok-or` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::let_else_ok_or)]` + +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or.rs:24:5 + | +LL | / let Some(value) = opt else { +LL | | return Err("missing value".to_string()); +LL | | }; + | |______^ help: replace it with: `let value = opt.ok_or_else(|| "missing value".to_string())?;` + +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or.rs:38:5 + | +LL | / let Some(executable) = opts.next() else { +LL | | return Err("Usage: ".to_string()); +LL | | }; + | |______^ help: replace it with: `let executable = opts.next().ok_or_else(|| "Usage: ".to_string())?;` + +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or.rs:47:5 + | +LL | / let Some(mut value) = opt else { +LL | | return Err("empty"); +LL | | }; + | |______^ help: replace it with: `let mut value = opt.ok_or("empty")?;` + +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or.rs:57:5 + | +LL | / let Some(first) = s.chars().next() else { +LL | | return Err("empty string"); +LL | | }; + | |______^ help: replace it with: `let first = s.chars().next().ok_or("empty string")?;` + +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or.rs:67:5 + | +LL | / let Some(value) = opt else { +LL | | return Err(format!("missing: {msg}")); +LL | | }; + | |______^ help: replace it with: `let value = opt.ok_or_else(|| format!("missing: {msg}"))?;` + +error: aborting due to 6 previous errors + diff --git a/tests/ui/let_else_ok_or_unfixable.rs b/tests/ui/let_else_ok_or_unfixable.rs new file mode 100644 index 000000000000..c8bc439553df --- /dev/null +++ b/tests/ui/let_else_ok_or_unfixable.rs @@ -0,0 +1,31 @@ +//@no-rustfix: the suggestion is intentionally not machine-applicable here +#![warn(clippy::let_else_ok_or)] +#![allow(clippy::question_mark, dead_code, unused)] + +// The `else` value relies on a coercion to the function's error type that `?` cannot reproduce: the +// `return Err(b"abc")` coerces `&[u8; 3]` to `&[u8]`, but `opt.ok_or(b"abc")?` would need +// `From<&[u8; 3]>` for `&[u8]`, which does not exist, so the rewrite would not compile. The lint +// still fires (the rewrite is usually a one-character annotation away from correct), but the +// suggestion is downgraded to non-machine-applicable so `--fix` leaves the code untouched. +fn coerced_array_to_slice(opt: Option) -> Result { + //~v let_else_ok_or + let Some(value) = opt else { + return Err(b"abc"); + }; + Ok(value) +} + +// The error value *moves* a non-`Copy` local that the `Some` path still uses. `ok_or`/`ok_or_else` +// move the error value unconditionally, so `opt.ok_or(msg)?` would leave the later `msg` +// referencing a moved value and fail to compile. The lint still fires, but the suggestion is +// downgraded to non-machine-applicable so `--fix` leaves it untouched. +fn moves_value_used_later(opt: Option, msg: String) -> Result { + //~v let_else_ok_or + let Some(value) = opt else { + return Err(msg); + }; + println!("{msg}"); + Ok(value) +} + +fn main() {} diff --git a/tests/ui/let_else_ok_or_unfixable.stderr b/tests/ui/let_else_ok_or_unfixable.stderr new file mode 100644 index 000000000000..2b94014627fc --- /dev/null +++ b/tests/ui/let_else_ok_or_unfixable.stderr @@ -0,0 +1,21 @@ +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or_unfixable.rs:12:5 + | +LL | / let Some(value) = opt else { +LL | | return Err(b"abc"); +LL | | }; + | |______^ help: replace it with: `let value = opt.ok_or(b"abc")?;` + | + = note: `-D clippy::let-else-ok-or` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::let_else_ok_or)]` + +error: this `let...else` may be rewritten with `Option::ok_or` and `?` + --> tests/ui/let_else_ok_or_unfixable.rs:24:5 + | +LL | / let Some(value) = opt else { +LL | | return Err(msg); +LL | | }; + | |______^ help: replace it with: `let value = opt.ok_or(msg)?;` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/manual_let_else.fixed b/tests/ui/manual_let_else.fixed index 894379ec577a..c1d854536abe 100644 --- a/tests/ui/manual_let_else.fixed +++ b/tests/ui/manual_let_else.fixed @@ -7,6 +7,9 @@ clippy::never_loop, clippy::unused_unit )] +// The fixed output contains `let Some(_) = _ else { return Err(..) }` shapes that this lint would +// in turn rewrite; that is out of scope for the `manual_let_else` tests. +#![allow(clippy::let_else_ok_or)] #![warn(clippy::manual_let_else)] enum Variant { A(usize, usize), diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 925dfbc6b772..8b4b0a11b6f2 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -7,6 +7,9 @@ clippy::never_loop, clippy::unused_unit )] +// The fixed output contains `let Some(_) = _ else { return Err(..) }` shapes that this lint would +// in turn rewrite; that is out of scope for the `manual_let_else` tests. +#![allow(clippy::let_else_ok_or)] #![warn(clippy::manual_let_else)] enum Variant { A(usize, usize), diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index 77e32eada5c6..76e3f8f11c3d 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -1,5 +1,5 @@ error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:24:5 + --> tests/ui/manual_let_else.rs:27:5 | LL | let v = if let Some(v_some) = g() { v_some } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };` @@ -8,7 +8,7 @@ LL | let v = if let Some(v_some) = g() { v_some } else { return }; = help: to override `-D warnings` add `#[allow(clippy::manual_let_else)]` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:27:5 + --> tests/ui/manual_let_else.rs:30:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -27,7 +27,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:35:5 + --> tests/ui/manual_let_else.rs:38:5 | LL | / let v = if let Some(v) = g() { ... | @@ -45,25 +45,25 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:48:9 + --> tests/ui/manual_let_else.rs:51:9 | LL | let v = if let Some(v_some) = g() { v_some } else { continue }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { continue };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:51:9 + --> tests/ui/manual_let_else.rs:54:9 | LL | let v = if let Some(v_some) = g() { v_some } else { break }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { break };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:56:5 + --> tests/ui/manual_let_else.rs:59:5 | LL | let v = if let Some(v_some) = g() { v_some } else { panic!() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { panic!() };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:60:5 + --> tests/ui/manual_let_else.rs:63:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -82,7 +82,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:69:5 + --> tests/ui/manual_let_else.rs:72:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -101,7 +101,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:78:5 + --> tests/ui/manual_let_else.rs:81:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -121,7 +121,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:88:5 + --> tests/ui/manual_let_else.rs:91:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -143,7 +143,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:100:5 + --> tests/ui/manual_let_else.rs:103:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -162,7 +162,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:109:5 + --> tests/ui/manual_let_else.rs:112:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -183,7 +183,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:120:5 + --> tests/ui/manual_let_else.rs:123:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -204,7 +204,7 @@ LL + } }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:131:5 + --> tests/ui/manual_let_else.rs:134:5 | LL | / let v = if let Some(v_some) = g() { LL | | @@ -232,7 +232,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:150:5 + --> tests/ui/manual_let_else.rs:153:5 | LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { LL | | @@ -251,7 +251,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:159:5 + --> tests/ui/manual_let_else.rs:162:5 | LL | / let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) { LL | | @@ -270,7 +270,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:170:13 + --> tests/ui/manual_let_else.rs:173:13 | LL | let $n = if let Some(v) = $e { v } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some($n) = g() else { return };` @@ -281,19 +281,19 @@ LL | create_binding_if_some!(w, g()); = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info) error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:180:5 + --> tests/ui/manual_let_else.rs:183:5 | LL | let v = if let Variant::A(a, 0) = e() { a } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(v, 0) = e() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:184:5 + --> tests/ui/manual_let_else.rs:187:5 | LL | let mut v = if let Variant::B(b) = e() { b } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(mut v) = e() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:189:5 + --> tests/ui/manual_let_else.rs:192:5 | LL | / let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested { LL | | @@ -312,19 +312,19 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:197:5 + --> tests/ui/manual_let_else.rs:200:5 | LL | let v = if let Variant::A(.., a) = e() { a } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:201:5 + --> tests/ui/manual_let_else.rs:204:5 | LL | let w = if let (Some(v), ()) = (g(), ()) { v } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let (Some(w), ()) = (g(), ()) else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:205:5 + --> tests/ui/manual_let_else.rs:208:5 | LL | / let w = if let Some(S { v: x }) = Some(S { v: 0 }) { LL | | @@ -343,7 +343,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:214:5 + --> tests/ui/manual_let_else.rs:217:5 | LL | / let v = if let Some(S { v: x }) = Some(S { v: 0 }) { LL | | @@ -362,7 +362,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:223:5 + --> tests/ui/manual_let_else.rs:226:5 | LL | / let (x, S { v }, w) = if let Some(U { v, w, x }) = None::>> { LL | | @@ -381,7 +381,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:341:5 + --> tests/ui/manual_let_else.rs:344:5 | LL | / let _ = match ff { LL | | @@ -391,13 +391,13 @@ LL | | }; | |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:418:9 + --> tests/ui/manual_let_else.rs:421:9 | LL | let v = if let Some(v_some) = g() { v_some } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:430:9 + --> tests/ui/manual_let_else.rs:433:9 | LL | / let signature = match value { LL | | @@ -417,7 +417,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:446:9 + --> tests/ui/manual_let_else.rs:449:9 | LL | / let signature = match value { LL | | @@ -437,7 +437,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:459:9 + --> tests/ui/manual_let_else.rs:462:9 | LL | / let value = match foo() { LL | | @@ -447,7 +447,7 @@ LL | | }; | |__________^ help: consider writing: `let Ok(value) = foo() else { return Err("abc") };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:466:9 + --> tests/ui/manual_let_else.rs:469:9 | LL | / let v = match w { LL | | @@ -457,7 +457,7 @@ LL | | }; | |__________^ help: consider writing: `let Some(v) = w else { return Err("abc") };` error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:496:9 + --> tests/ui/manual_let_else.rs:499:9 | LL | / let value = if let Some(value) = unsafe { something_unsafe() } { LL | | @@ -475,7 +475,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:505:9 + --> tests/ui/manual_let_else.rs:508:9 | LL | / let value = if let Some(value) = if some_flag { None } else { Some(3) } { LL | | @@ -493,7 +493,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:516:5 + --> tests/ui/manual_let_else.rs:519:5 | LL | / _ = match i { LL | | Ok(i) => i, @@ -511,7 +511,7 @@ LL ~ } };; | error: this could be rewritten as `let...else` - --> tests/ui/manual_let_else.rs:524:5 + --> tests/ui/manual_let_else.rs:527:5 | LL | / _ = match i { LL | | Ok(i) => i,