Auto merge of rust-lang#11012 - Centri3:manual_try_fold, r=blyxyas,xF…

…rednet New lint [`manual_try_fold`] Closes rust-lang#10208 --- changelog: New lint [`manual_try_fold`] [rust-lang#11012](rust-lang/rust-clippy#11012)
flip1995 · Jul 1, 2023 · 37f4c17 · 37f4c17
2 parents c7bf05c + cb5d7e3
commit 37f4c17
Show file tree

Hide file tree

Showing 9 changed files with 224 additions and 3 deletions.
diff --git 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/book/src/lint_configuration.md b/book/src/lint_configuration.md
@@ -150,6 +150,7 @@ The minimum rust version that the project supports
 * [`manual_retain`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_retain)
 * [`type_repetition_in_bounds`](https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds)
 * [`tuple_array_conversions`](https://rust-lang.github.io/rust-clippy/master/index.html#tuple_array_conversions)
+* [`manual_try_fold`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold)
 
 
 ## `cognitive-complexity-threshold`

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -364,6 +364,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     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
@@ -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
+        && !is_from_proc_macro(cx, expr)
+        && let Some(args_snip) = closure.fn_arg_span.and_then(|fn_arg_span| snippet_opt(cx, fn_arg_span))
+    {
+        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,
+            "usage of `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
@@ -50,6 +50,7 @@ mod manual_next_back;
 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,35 @@ declare_clippy_lint! {
     "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?
+    /// The code should use `try_fold` instead, which short-circuits on failure, thus opening the
+    /// door for additional optimizations not possible with `fold` as rustc can guarantee the
+    /// function is never called on `None`, `Err`, etc., alleviating otherwise necessary checks. It's
+    /// also slightly more idiomatic.
+    ///
+    /// ### Known issues
+    /// This lint doesn't take into account whether a function does something on the failure case,
+    /// i.e., whether short-circuiting will affect behavior. Refactoring to `try_fold` is not
+    /// desirable in those cases.
+    ///
+    /// ### 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 +3446,8 @@ impl_lint_pass!(Methods => [
     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 +3740,10 @@ impl Methods {
                     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_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -294,7 +294,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),

diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs
@@ -44,6 +44,7 @@ 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
@@ -0,0 +1,100 @@
+//@aux-build:proc_macros.rs:proc-macro
+#![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<R> FromResidual<R> 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<Self::Residual, Self::Output> {
+        todo!()
+    }
+}
+
+// Test custom `Try` with only 1 argument
+#[derive(Default)]
+struct NotOptionButWorse(i32);
+
+impl<R> FromResidual<R> 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<Self::Residual, Self::Output> {
+        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();
+    }
+}
+
+#[clippy::msrv = "1.26.0"]
+fn msrv_too_low() {
+    [1, 2, 3]
+        .iter()
+        .fold(Some(0i32), |sum, i| sum?.checked_add(*i))
+        .unwrap();
+}
+
+#[clippy::msrv = "1.27.0"]
+fn msrv_juust_right() {
+    [1, 2, 3]
+        .iter()
+        .fold(Some(0i32), |sum, i| sum?.checked_add(*i))
+        .unwrap();
+}
diff --git a/tests/ui/manual_try_fold.stderr b/tests/ui/manual_try_fold.stderr
@@ -0,0 +1,28 @@
+error: usage of `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: usage of `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: usage of `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: usage of `Iterator::fold` on a type that implements `Try`
+  --> $DIR/manual_try_fold.rs:98:10
+   |
+LL |         .fold(Some(0i32), |sum, i| sum?.checked_add(*i))
+   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `try_fold` instead: `try_fold(0i32, |sum, i| ...)`
+
+error: aborting due to 4 previous errors
+