From 84fb6b165176d4349377862a1587c522a3ac5ae8 Mon Sep 17 00:00:00 2001 From: Aaron Ang <67321817+aaron-ang@users.noreply.github.com> Date: Sun, 26 Jan 2025 15:19:10 -0800 Subject: [PATCH] Add new lint `return_and_then` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/mod.rs | 55 +++++++++- clippy_lints/src/methods/return_and_then.rs | 67 ++++++++++++ .../src/methods/unnecessary_lazy_eval.rs | 6 +- tests/ui/return_and_then.fixed | 67 ++++++++++++ tests/ui/return_and_then.rs | 63 +++++++++++ tests/ui/return_and_then.stderr | 101 ++++++++++++++++++ 8 files changed, 356 insertions(+), 5 deletions(-) create mode 100644 clippy_lints/src/methods/return_and_then.rs create mode 100644 tests/ui/return_and_then.fixed create mode 100644 tests/ui/return_and_then.rs create mode 100644 tests/ui/return_and_then.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index bc42c07224e1..cb149ccfeead 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6026,6 +6026,7 @@ Released 2018-09-13 [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else [`result_unit_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err [`result_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unwrap_used +[`return_and_then`]: https://rust-lang.github.io/rust-clippy/master/index.html#return_and_then [`return_self_not_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#return_self_not_must_use [`reverse_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#reverse_range_loop [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 86bcf8edd578..1ce6f1256177 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -463,6 +463,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::methods::REPEAT_ONCE_INFO, crate::methods::RESULT_FILTER_MAP_INFO, crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO, + crate::methods::RETURN_AND_THEN_INFO, crate::methods::SEARCH_IS_SOME_INFO, crate::methods::SEEK_FROM_CURRENT_INFO, crate::methods::SEEK_TO_START_INSTEAD_OF_REWIND_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 00be6b81e100..6dbfbc2ca3b4 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -95,6 +95,7 @@ mod readonly_write_lock; mod redundant_as_str; mod repeat_once; mod result_map_or_else_none; +mod return_and_then; mod search_is_some; mod seek_from_current; mod seek_to_start_instead_of_rewind; @@ -4392,6 +4393,46 @@ declare_clippy_lint! { "slicing a string and immediately calling as_bytes is less efficient and can lead to panics" } +declare_clippy_lint! { + /// ### What it does + /// + /// Detect functions that end with `Option::and_then` or `Result::and_then`, and suggest using a question mark (`?`) instead. + /// + /// ### Why is this bad? + /// + /// The `and_then` method is used to chain a computation that returns an `Option` or a `Result`. + /// This can be replaced with the `?` operator, which is more concise and idiomatic. + /// + /// ### Example + /// + /// ```no_run + /// fn test(opt: Option) -> Option { + /// opt.and_then(|n| { + /// if n > 1 { + /// Some(n + 1) + /// } else { + /// None + /// } + /// }) + /// } + /// ``` + /// Use instead: + /// ```no_run + /// fn test(opt: Option) -> Option { + /// let n = opt?; + /// if n > 1 { + /// Some(n + 1) + /// } else { + /// None + /// } + /// } + /// ``` + #[clippy::version = "1.86.0"] + pub RETURN_AND_THEN, + restriction, + "using `Option::and_then` or `Result::and_then` to chain a computation that returns an `Option` or a `Result`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -4561,6 +4602,7 @@ impl_lint_pass!(Methods => [ USELESS_NONZERO_NEW_UNCHECKED, MANUAL_REPEAT_N, SLICED_STRING_AS_BYTES, + RETURN_AND_THEN, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4790,7 +4832,10 @@ impl Methods { let biom_option_linted = bind_instead_of_map::check_and_then_some(cx, expr, recv, arg); let biom_result_linted = bind_instead_of_map::check_and_then_ok(cx, expr, recv, arg); if !biom_option_linted && !biom_result_linted { - unnecessary_lazy_eval::check(cx, expr, recv, arg, "and"); + let ule_and_linted = unnecessary_lazy_eval::check(cx, expr, recv, arg, "and"); + if !ule_and_linted { + return_and_then::check(cx, expr, recv, arg); + } } }, ("any", [arg]) => { @@ -5004,7 +5049,9 @@ impl Methods { get_first::check(cx, expr, recv, arg); get_last_with_len::check(cx, expr, recv, arg); }, - ("get_or_insert_with", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "get_or_insert"), + ("get_or_insert_with", [arg]) => { + unnecessary_lazy_eval::check(cx, expr, recv, arg, "get_or_insert"); + }, ("hash", [arg]) => { unit_hash::check(cx, expr, recv, arg); }, @@ -5145,7 +5192,9 @@ impl Methods { }, _ => iter_nth_zero::check(cx, expr, recv, n_arg), }, - ("ok_or_else", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"), + ("ok_or_else", [arg]) => { + unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"); + }, ("open", [_]) => { open_options::check(cx, expr, recv); }, diff --git a/clippy_lints/src/methods/return_and_then.rs b/clippy_lints/src/methods/return_and_then.rs new file mode 100644 index 000000000000..7b1199ad1e2d --- /dev/null +++ b/clippy_lints/src/methods/return_and_then.rs @@ -0,0 +1,67 @@ +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, GenericArg, Ty}; +use rustc_span::sym; +use std::ops::ControlFlow; + +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_applicability}; +use clippy_utils::ty::get_type_diagnostic_name; +use clippy_utils::visitors::for_each_unconsumed_temporary; +use clippy_utils::{is_expr_final_block_expr, peel_blocks}; + +use super::RETURN_AND_THEN; + +/// lint if `and_then` is the last expression in a block, and +/// there are no references or temporaries in the receiver +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &hir::Expr<'_>, + recv: &'tcx hir::Expr<'tcx>, + arg: &'tcx hir::Expr<'_>, +) { + if !is_expr_final_block_expr(cx.tcx, expr) { + return; + } + + let recv_type = cx.typeck_results().expr_ty(recv); + if !matches!(get_type_diagnostic_name(cx, recv_type), Some(sym::Option | sym::Result)) { + return; + } + + let has_ref_type = matches!(recv_type.kind(), ty::Adt(_, args) if args + .first() + .and_then(|arg0: &GenericArg<'tcx>| GenericArg::as_type(*arg0)) + .is_some_and(Ty::is_ref)); + let has_temporaries = for_each_unconsumed_temporary(cx, recv, |_| ControlFlow::Break(())).is_break(); + if has_ref_type && has_temporaries { + return; + } + + let hir::ExprKind::Closure(&hir::Closure { body, fn_decl, .. }) = arg.kind else { + return; + }; + + let closure_arg = fn_decl.inputs[0]; + let closure_expr = peel_blocks(cx.tcx.hir().body(body).value); + + let mut applicability = Applicability::MachineApplicable; + let arg_snip = snippet_with_applicability(cx, closure_arg.span, "_", &mut applicability); + let recv_snip = snippet_with_applicability(cx, recv.span, "_", &mut applicability); + let body_snip = snippet_with_applicability(cx, closure_expr.span, "..", &mut applicability); + let inner = match body_snip.strip_prefix('{').and_then(|s| s.strip_suffix('}')) { + Some(s) => s.trim_start_matches('\n').trim_end(), + None => &body_snip, + }; + + let msg = "use the question mark operator instead of an `and_then` call"; + let sugg = format!( + "let {} = {}?;\n{}", + arg_snip, + recv_snip, + reindent_multiline(inner.into(), false, indent_of(cx, expr.span)) + ); + + span_lint_and_sugg(cx, RETURN_AND_THEN, expr.span, msg, "try", sugg, applicability); +} diff --git a/clippy_lints/src/methods/unnecessary_lazy_eval.rs b/clippy_lints/src/methods/unnecessary_lazy_eval.rs index 1673a6f8b3a4..7af550fa7c68 100644 --- a/clippy_lints/src/methods/unnecessary_lazy_eval.rs +++ b/clippy_lints/src/methods/unnecessary_lazy_eval.rs @@ -18,7 +18,7 @@ pub(super) fn check<'tcx>( recv: &'tcx hir::Expr<'_>, arg: &'tcx hir::Expr<'_>, simplify_using: &str, -) { +) -> bool { let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option); let is_result = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result); let is_bool = cx.typeck_results().expr_ty(recv).is_bool(); @@ -29,7 +29,7 @@ pub(super) fn check<'tcx>( let body_expr = &body.value; if usage::BindingUsageFinder::are_params_used(cx, body) || is_from_proc_macro(cx, expr) { - return; + return false; } if eager_or_lazy::switch_to_eager_eval(cx, body_expr) { @@ -71,8 +71,10 @@ pub(super) fn check<'tcx>( applicability, ); }); + return true; } } } } + false } diff --git a/tests/ui/return_and_then.fixed b/tests/ui/return_and_then.fixed new file mode 100644 index 000000000000..9736a51ac868 --- /dev/null +++ b/tests/ui/return_and_then.fixed @@ -0,0 +1,67 @@ +#![warn(clippy::return_and_then)] + +fn main() { + fn test_opt_block(opt: Option) -> Option { + let n = opt?; + let mut ret = n + 1; + ret += n; + if n > 1 { Some(ret) } else { None } + } + + fn test_opt_func(opt: Option) -> Option { + let n = opt?; + test_opt_block(Some(n)) + } + + fn test_call_chain() -> Option { + let n = gen_option(1)?; + test_opt_block(Some(n)) + } + + fn test_res_block(opt: Result) -> Result { + let n = opt?; + if n > 1 { Ok(n + 1) } else { Err(n) } + } + + fn test_res_func(opt: Result) -> Result { + let n = opt?; + test_res_block(Ok(n)) + } + + fn test_ref_only() -> Option { + // ref: empty string + let x = Some("")?; + if x.len() > 2 { Some(3) } else { None } + } + + fn test_tmp_only() -> Option { + // unused temporary: vec![1, 2, 4] + let x = Some(match (vec![1, 2, 3], vec![1, 2, 4]) { + (a, _) if a.len() > 1 => a, + (_, b) => b, + })?; + if x.len() > 2 { Some(3) } else { None } + } + + // should not lint + fn test_tmp_ref() -> Option { + String::from("") + .strip_prefix("<") + .and_then(|s| s.strip_suffix(">").map(String::from)) + } + + // should not lint + fn test_unconsumed_tmp() -> Option { + [1, 2, 3] + .iter() + .map(|x| x + 1) + .collect::>() // temporary Vec created here + .as_slice() // creates temporary slice + .first() // creates temporary reference + .and_then(|x| test_opt_block(Some(*x))) + } +} + +fn gen_option(n: i32) -> Option { + Some(n) +} diff --git a/tests/ui/return_and_then.rs b/tests/ui/return_and_then.rs new file mode 100644 index 000000000000..8bcbdfc3a632 --- /dev/null +++ b/tests/ui/return_and_then.rs @@ -0,0 +1,63 @@ +#![warn(clippy::return_and_then)] + +fn main() { + fn test_opt_block(opt: Option) -> Option { + opt.and_then(|n| { + let mut ret = n + 1; + ret += n; + if n > 1 { Some(ret) } else { None } + }) + } + + fn test_opt_func(opt: Option) -> Option { + opt.and_then(|n| test_opt_block(Some(n))) + } + + fn test_call_chain() -> Option { + gen_option(1).and_then(|n| test_opt_block(Some(n))) + } + + fn test_res_block(opt: Result) -> Result { + opt.and_then(|n| if n > 1 { Ok(n + 1) } else { Err(n) }) + } + + fn test_res_func(opt: Result) -> Result { + opt.and_then(|n| test_res_block(Ok(n))) + } + + fn test_ref_only() -> Option { + // ref: empty string + Some("").and_then(|x| if x.len() > 2 { Some(3) } else { None }) + } + + fn test_tmp_only() -> Option { + // unused temporary: vec![1, 2, 4] + Some(match (vec![1, 2, 3], vec![1, 2, 4]) { + (a, _) if a.len() > 1 => a, + (_, b) => b, + }) + .and_then(|x| if x.len() > 2 { Some(3) } else { None }) + } + + // should not lint + fn test_tmp_ref() -> Option { + String::from("") + .strip_prefix("<") + .and_then(|s| s.strip_suffix(">").map(String::from)) + } + + // should not lint + fn test_unconsumed_tmp() -> Option { + [1, 2, 3] + .iter() + .map(|x| x + 1) + .collect::>() // temporary Vec created here + .as_slice() // creates temporary slice + .first() // creates temporary reference + .and_then(|x| test_opt_block(Some(*x))) + } +} + +fn gen_option(n: i32) -> Option { + Some(n) +} diff --git a/tests/ui/return_and_then.stderr b/tests/ui/return_and_then.stderr new file mode 100644 index 000000000000..b2e8bf2ca45a --- /dev/null +++ b/tests/ui/return_and_then.stderr @@ -0,0 +1,101 @@ +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:5:9 + | +LL | / opt.and_then(|n| { +LL | | let mut ret = n + 1; +LL | | ret += n; +LL | | if n > 1 { Some(ret) } else { None } +LL | | }) + | |__________^ + | + = note: `-D clippy::return-and-then` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::return_and_then)]` +help: try + | +LL ~ let n = opt?; +LL + let mut ret = n + 1; +LL + ret += n; +LL + if n > 1 { Some(ret) } else { None } + | + +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:13:9 + | +LL | opt.and_then(|n| test_opt_block(Some(n))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try + | +LL ~ let n = opt?; +LL + test_opt_block(Some(n)) + | + +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:17:9 + | +LL | gen_option(1).and_then(|n| test_opt_block(Some(n))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try + | +LL ~ let n = gen_option(1)?; +LL + test_opt_block(Some(n)) + | + +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:21:9 + | +LL | opt.and_then(|n| if n > 1 { Ok(n + 1) } else { Err(n) }) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try + | +LL ~ let n = opt?; +LL + if n > 1 { Ok(n + 1) } else { Err(n) } + | + +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:25:9 + | +LL | opt.and_then(|n| test_res_block(Ok(n))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try + | +LL ~ let n = opt?; +LL + test_res_block(Ok(n)) + | + +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:30:9 + | +LL | Some("").and_then(|x| if x.len() > 2 { Some(3) } else { None }) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try + | +LL ~ let x = Some("")?; +LL + if x.len() > 2 { Some(3) } else { None } + | + +error: use the question mark operator instead of an `and_then` call + --> tests/ui/return_and_then.rs:35:9 + | +LL | / Some(match (vec![1, 2, 3], vec![1, 2, 4]) { +LL | | (a, _) if a.len() > 1 => a, +LL | | (_, b) => b, +LL | | }) +LL | | .and_then(|x| if x.len() > 2 { Some(3) } else { None }) + | |_______________________________________________________________^ + | +help: try + | +LL ~ let x = Some(match (vec![1, 2, 3], vec![1, 2, 4]) { +LL + (a, _) if a.len() > 1 => a, +LL + (_, b) => b, +LL + })?; +LL + if x.len() > 2 { Some(3) } else { None } + | + +error: aborting due to 7 previous errors +