diff --git a/CHANGELOG.md b/CHANGELOG.md index ff39c6a0a6c..14d822083d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4959,6 +4959,7 @@ Released 2018-09-13 [`manual_string_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_string_new [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap +[`manual_try_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold [`manual_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or [`manual_while_let_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_while_let_some [`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 52bc2010207..9d9ee6ba307 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -364,6 +364,7 @@ crate::methods::MANUAL_SATURATING_ARITHMETIC_INFO, crate::methods::MANUAL_SPLIT_ONCE_INFO, crate::methods::MANUAL_STR_REPEAT_INFO, + crate::methods::MANUAL_TRY_FOLD_INFO, crate::methods::MAP_CLONE_INFO, crate::methods::MAP_COLLECT_RESULT_UNIT_INFO, crate::methods::MAP_ERR_IGNORE_INFO, diff --git a/clippy_lints/src/methods/manual_try_fold.rs b/clippy_lints/src/methods/manual_try_fold.rs new file mode 100644 index 00000000000..9f92524d356 --- /dev/null +++ b/clippy_lints/src/methods/manual_try_fold.rs @@ -0,0 +1,55 @@ +use clippy_utils::{ + diagnostics::span_lint_and_sugg, + is_from_proc_macro, + msrvs::{Msrv, ITERATOR_TRY_FOLD}, + source::snippet_opt, + ty::implements_trait, +}; +use rustc_errors::Applicability; +use rustc_hir::{ + def::{DefKind, Res}, + Expr, ExprKind, +}; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_span::Span; + +use super::MANUAL_TRY_FOLD; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &Expr<'tcx>, + init: &Expr<'_>, + acc: &Expr<'_>, + fold_span: Span, + msrv: &Msrv, +) { + if !in_external_macro(cx.sess(), fold_span) + && msrv.meets(ITERATOR_TRY_FOLD) + && let init_ty = cx.typeck_results().expr_ty(init) + && let Some(try_trait) = cx.tcx.lang_items().try_trait() + && implements_trait(cx, init_ty, try_trait, &[]) + && let ExprKind::Call(path, [first, rest @ ..]) = init.kind + && let ExprKind::Path(qpath) = path.kind + && let Res::Def(DefKind::Ctor(_, _), _) = cx.qpath_res(&qpath, path.hir_id) + && let ExprKind::Closure(closure) = acc.kind + && let Some(args_snip) = closure.fn_arg_span.and_then(|fn_arg_span| snippet_opt(cx, fn_arg_span)) + && !is_from_proc_macro(cx, expr) + { + let init_snip = rest + .is_empty() + .then_some(first.span) + .and_then(|span| snippet_opt(cx, span)) + .unwrap_or("...".to_owned()); + + span_lint_and_sugg( + cx, + MANUAL_TRY_FOLD, + fold_span, + "you seem to be using `Iterator::fold` on a type that implements `Try`", + "use `try_fold` instead", + format!("try_fold({init_snip}, {args_snip}, ...)", ), + Applicability::HasPlaceholders, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b1d3b61aea3..a8d03bb95e6 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -50,6 +50,7 @@ mod manual_ok_or; mod manual_saturating_arithmetic; mod manual_str_repeat; +mod manual_try_fold; mod map_clone; mod map_collect_result_unit; mod map_err_ignore; @@ -3286,6 +3287,30 @@ "calling `.drain(..).collect()` to move all elements into a new collection" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `Iterator::fold` with a type that implements `Try`. + /// + /// ### Why is this bad? + /// This is better represented with `try_fold`, but this has one major difference: It will + /// short-circuit on failure. *This is almost always what you want*. This can also open the door + /// for additional optimizations as well, as rustc can guarantee the function is never + /// called on `None`, `Err`, etc., alleviating otherwise necessary checks. + /// + /// ### Example + /// ```rust + /// vec![1, 2, 3].iter().fold(Some(0i32), |sum, i| sum?.checked_add(*i)); + /// ``` + /// Use instead: + /// ```rust + /// vec![1, 2, 3].iter().try_fold(0i32, |sum, i| sum.checked_add(*i)); + /// ``` + #[clippy::version = "1.72.0"] + pub MANUAL_TRY_FOLD, + perf, + "checks for usage of `Iterator::fold` with a type that implements `Try`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3416,7 +3441,8 @@ pub fn new( CLEAR_WITH_DRAIN, MANUAL_NEXT_BACK, UNNECESSARY_LITERAL_UNWRAP, - DRAIN_COLLECT + DRAIN_COLLECT, + MANUAL_TRY_FOLD, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3709,7 +3735,10 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true), _ => {}, }, - ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span), + ("fold", [init, acc]) => { + manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv); + unnecessary_fold::check(cx, expr, init, acc, span); + }, ("for_each", [_]) => { if let Some(("inspect", _, [_], span2, _)) = method_call(recv) { inspect_for_each::check(cx, expr, span2); diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 1f70f11bd67..3637476c408 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -44,6 +44,7 @@ macro_rules! msrv_aliases { 1,34,0 { TRY_FROM } 1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES } 1,28,0 { FROM_BOOL } + 1,27,0 { ITERATOR_TRY_FOLD } 1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN } 1,24,0 { IS_ASCII_DIGIT } 1,18,0 { HASH_MAP_RETAIN, HASH_SET_RETAIN } diff --git a/tests/ui/manual_try_fold.rs b/tests/ui/manual_try_fold.rs new file mode 100644 index 00000000000..f43519dd44f --- /dev/null +++ b/tests/ui/manual_try_fold.rs @@ -0,0 +1,84 @@ +//@aux-build:proc_macros.rs +#![allow(clippy::unnecessary_fold, unused)] +#![warn(clippy::manual_try_fold)] +#![feature(try_trait_v2)] + +use std::ops::ControlFlow; +use std::ops::FromResidual; +use std::ops::Try; + +#[macro_use] +extern crate proc_macros; + +// Test custom `Try` with more than 1 argument +struct NotOption(i32, i32); + +impl FromResidual for NotOption { + fn from_residual(_: R) -> Self { + todo!() + } +} + +impl Try for NotOption { + type Output = (); + type Residual = (); + + fn from_output(_: Self::Output) -> Self { + todo!() + } + + fn branch(self) -> ControlFlow { + todo!() + } +} + +// Test custom `Try` with only 1 argument +#[derive(Default)] +struct NotOptionButWorse(i32); + +impl FromResidual for NotOptionButWorse { + fn from_residual(_: R) -> Self { + todo!() + } +} + +impl Try for NotOptionButWorse { + type Output = (); + type Residual = (); + + fn from_output(_: Self::Output) -> Self { + todo!() + } + + fn branch(self) -> ControlFlow { + todo!() + } +} + +fn main() { + [1, 2, 3] + .iter() + .fold(Some(0i32), |sum, i| sum?.checked_add(*i)) + .unwrap(); + [1, 2, 3] + .iter() + .fold(NotOption(0i32, 0i32), |sum, i| NotOption(0i32, 0i32)); + [1, 2, 3] + .iter() + .fold(NotOptionButWorse(0i32), |sum, i| NotOptionButWorse(0i32)); + // Do not lint + [1, 2, 3].iter().try_fold(0i32, |sum, i| sum.checked_add(*i)).unwrap(); + [1, 2, 3].iter().fold(0i32, |sum, i| sum + i); + [1, 2, 3] + .iter() + .fold(NotOptionButWorse::default(), |sum, i| NotOptionButWorse::default()); + external! { + [1, 2, 3].iter().fold(Some(0i32), |sum, i| sum?.checked_add(*i)).unwrap(); + [1, 2, 3].iter().try_fold(0i32, |sum, i| sum.checked_add(*i)).unwrap(); + } + with_span! { + span + [1, 2, 3].iter().fold(Some(0i32), |sum, i| sum?.checked_add(*i)).unwrap(); + [1, 2, 3].iter().try_fold(0i32, |sum, i| sum.checked_add(*i)).unwrap(); + } +} diff --git a/tests/ui/manual_try_fold.stderr b/tests/ui/manual_try_fold.stderr new file mode 100644 index 00000000000..1a17435b459 --- /dev/null +++ b/tests/ui/manual_try_fold.stderr @@ -0,0 +1,22 @@ +error: you seem to be using `Iterator::fold` on a type that implements `Try` + --> $DIR/manual_try_fold.rs:61:10 + | +LL | .fold(Some(0i32), |sum, i| sum?.checked_add(*i)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `try_fold` instead: `try_fold(0i32, |sum, i|, ...)` + | + = note: `-D clippy::manual-try-fold` implied by `-D warnings` + +error: you seem to be using `Iterator::fold` on a type that implements `Try` + --> $DIR/manual_try_fold.rs:65:10 + | +LL | .fold(NotOption(0i32, 0i32), |sum, i| NotOption(0i32, 0i32)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `try_fold` instead: `try_fold(..., |sum, i|, ...)` + +error: you seem to be using `Iterator::fold` on a type that implements `Try` + --> $DIR/manual_try_fold.rs:68:10 + | +LL | .fold(NotOptionButWorse(0i32), |sum, i| NotOptionButWorse(0i32)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `try_fold` instead: `try_fold(0i32, |sum, i|, ...)` + +error: aborting due to 3 previous errors +