Lint for using hand-writing a fold with the same behaviour as any #2350

theotherphil · 2018-01-14T08:31:49Z

Some things that need to be addressed before this can be merged. (@Manishearth @oli-obk)

Do I need to make any changes to the lint name, documentation, description or suggestions?
See DONOTMERGE comment at the start of lint_fold_any - how can I check that the fold being called is actually the trait method on Iterator?
Decide whether I need to handle my TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f) (and if so, work out how to do this) (Resolution: won't bother)
Reduce the span of the error to only include .fold(...) instead the caller as well (see comments from @killercup about enabling rustfix support)

~~I'll create a new issue for the sum and product cases and handle them in another PR.~~

…tial fold value is false. Remove some unwraps

killercup · 2018-01-14T16:47:12Z

clippy_lints/src/methods.rs

@@ -653,7 +670,8 @@ impl LintPass for Pass {
            GET_UNWRAP,
            STRING_EXTEND_CHARS,
            ITER_CLONED_COLLECT,
-            USELESS_ASREF
+            USELESS_ASREF,
+            FOLD_ANY


Nit: Does this macro support trailing commas? If so, please add one and the next diff is only one line :)

It appears not:

error: unexpected end of macro invocation --> clippy_lints/src/methods.rs:674:21 | 674 | FOLD_ANY, | ^

Ugh, that's too bad. Someone should make a pull request to change this :)

I've created an issue #2352. I'll do this next weekend if it's still open.

Oh, this is a macro defined in the compiler, so that issue is probably more useful over at https://github.com/rust-lang/rust :)

Nuts. rust-lang/rust#47428

killercup · 2018-01-14T16:50:02Z

clippy_lints/src/methods.rs

@@ -1105,6 +1125,50 @@ fn lint_iter_cloned_collect(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir
    }
 }

+fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) {
+    // DONOTMERGE: What if this is just some other method called fold?


Not sure if applicable in this context but other parts of clippy use match_trait_method(cx, expr, &paths::ITERATOR)

Thanks. I tried this (well, actually, I implemented the equivalent by hand as I missed this method), but this doesn't quite do what I want - it's not enough to know that the caller is an Iterator - this could be an inherent method rather than the trait method. Maybe this is rare/ill-advised enough that it doesn't matter?

Edit: actually, I think I misread and this does do what I want after all.

killercup · 2018-01-14T16:55:01Z

clippy_lints/src/methods.rs

+                expr.span,
+                // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f)
+                &format!(
+                    ".fold(false, |{f}, {s}| {f} || {r})) is more succinctly expressed as .any(|{s}| {r})",


Can you use span_lint_and_sugg here to get output like this:

error: This `.fold` can be more succinctly expressed as `.any` --> $DIR/methods.rs:390:13 | 390 | let _ = (0..3).fold(false, |acc, x| acc || x > 2); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` | = note: `-D fold-any` implied by `-D warnings`

If you make sure the spans are right and exactly cover the .fold(…), this lint should work great with rustfix :)

Sure, I'll just need to work out what it does first! (There's no doc comment at the moment)

Edit: no need to elaborate - I've got this working now

So, Rust's diagnostic messages (aka "fancy errors") allow you to add annotations to parts of the code. One special style of annotation is a "replacement suggestion". It looks just like the "help:" thing I posted above to a human, but rustfix can parse it to automatically fix your code.

The mystical span_lint_and_sugg is a shortcut to create a new lint message and at the same time also add a suggestion. It's basically the same as span_lint, but you pass two parameters, help: &str, which is the "try: " in the example above, and sugg: String, which is the suggested replacement (something like format!(".any(|{s}| {r})", …) in this case here).

Edit: too late :)

Edit 2: Feel free to add the above as doc comment ;)

Thanks anyway. I'll add a doc comment.

I've actually not quite got this working, as I've got a span pointing at the whole expression, including the range. What's the best way of restricting this to the .fold(...) part?

killercup · 2018-01-14T16:56:56Z

tests/ui/methods.rs

+/// Checks implementation of the `FOLD_ANY` lint
+fn fold_any_ignore_initial_value_of_true() {
+    let _ = (0..3).fold(true, |acc, x| acc || x > 2);
+}


You might also want to add another test that doesn't trigger the lint because it's not using a boolean, e.g. .fold(0, |acc, x| acc += if x > 2 { 1 } else { 0 })

theotherphil · 2018-01-14T18:19:49Z

@killercup I've addressed all of your comments so far, except for reducing the span to only include .fold(...). Although adding the doc comment created a new issue - now I get a wall of warnings about doc rendering differing between old and new renderers!

killercup · 2018-01-14T18:24:02Z

reducing the span to only include .fold(...)

Not sure how to best do this, but I'm sure someone else can help us out :)

Although adding the doc comment created a new issue - now I get a wall of warnings about doc rendering differing between old and new renderers!

Yeah, rustdoc is switching to the CommonMark standard soon. What's the errors telling you? I assume something about the <pre>? If so, try putting the code block in triple-backticks or ident it by 4 spaces. (Just the usual Markdown ways to mark code blocks.)

clarfonthey · 2018-01-14T18:35:24Z

I think that this should be extended to all of the similar methods on Iterator; see the original issue for some of my examples.

I don't mind just starting with any for now, but perhaps the lint should have a more generic name like unnecessary_fold.

theotherphil · 2018-01-14T19:20:26Z

I agree that this should be extended to cover your other examples - I was planning on merging this case first and then adding the rest. I'm happy to change the name. Not sure if we want one lint covering all cases or one lint for each case.

theotherphil · 2018-01-14T19:58:15Z

I've added tests locally to check that match_trait_methoddoes what I want, and discovered that this lint (and presumably the other methods lints) doesn't work if you call fold via Iterator::fold(...)

clarfonthey · 2018-01-14T20:26:38Z

@oli-obk wouldn't this apply to the method-chain lints as well?

oli-obk · 2018-01-16T07:27:22Z

Jup, the method chain lints don't work on universal method calls, just on actual method calls with dot notation

oli-obk · 2018-01-16T07:28:46Z

clippy_lints/src/methods.rs

+                cx,
+                FOLD_ANY,
+                expr.span,
+                // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f)


That's fine, we do have another lint for that ;)

If you really want to, you can extract the code from there into a separate function so you can call it from here and there and not duplicate any logic.

oli-obk · 2018-01-16T07:38:47Z

clippy_lints/src/methods.rs

+            span_lint_and_sugg(
+                cx,
+                FOLD_ANY,
+                expr.span,


You can probably use fold_args[0].span.next_point().with_hi(fold_args[1].span.hi()) to produce the span you want.

This works (with small tweaks). Thanks!

theotherphil · 2018-01-16T07:57:04Z

Thanks for the tips @oli-obk - I'll finish this PR off this weekend.

mzimnn · 2018-01-16T11:41:06Z

clippy_lints/src/utils/mod.rs

+        PatKind::Ref(ref subpat, _) => get_arg_name(subpat),
+        _ => None,
+    }
+}


nit: missing newline

Fixed, thanks

theotherphil · 2018-01-17T07:46:22Z

Please don't merge just yet - I've decided to rename to unnecessary_fold and handle the other cases as part of this PR (I'm not going to worry about producing eta-reducible replacement suggestions for now).

theotherphil · 2018-01-17T20:27:24Z

@oli-obk I think this is now in a mergeable state

killercup · 2018-01-17T20:30:20Z

heads up: you'll most likely need to rebase on master to make this compile on the latest nightly

theotherphil · 2018-01-17T20:50:17Z

Do you know which of the AppVeyor jobs have any hope of succeeding? The first couple of failures appear to have nothing to do with my changes.

killercup · 2018-01-17T20:45:57Z

clippy_lints/src/methods.rs

+/// ```rust
+/// let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+/// ```
+/// This could be written as:


I'm pretty sure it's good style to put an empty line above and below code blocks in markdown.

The existing lints in this file don't, for whatever that's worth

killercup · 2018-01-17T20:46:43Z

clippy_lints/src/methods.rs

+    }
+
+    assert!(fold_args.len() == 3,
+        "Expected fold_args to have three entries - the receiver, the initial value and the closure");


Should this actually panic? I imagine a debug! and return; suffices

Well, it's a logic bug, so I would have thought it should panic. I don't feel strongly though - I can change if it you'd prefer.

killercup · 2018-01-17T20:46:59Z

clippy_lints/src/methods.rs

+                        ".{replacement}(|{s}| {r})",
+                        replacement = replacement_method_name,
+                        s = second_arg_ident,
+                        r = snippet(cx, right_expr.span, "EXPR")


trailing commas are good

here, and everywhere below (and above)

killercup · 2018-01-17T20:47:42Z

clippy_lints/src/utils/mod.rs

+///     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)`
+///     |
+///     = note: `-D fold-any` implied by `-D warnings`
+/// </pre>


replace <pre> with triple back ticks or 4-space indent

killercup · 2018-01-17T20:49:58Z

clippy_lints/src/methods.rs

+                    cx,
+                    UNNECESSARY_FOLD,
+                    fold_span,
+                    // TODO: don't suggest e.g. .any(|x| f(x)) if we can suggest .any(f)


Maybe open an issue for this and link it here?

Also, Clippy has a lint that will suggest using UFCS instead of closures, so a second run will likely trigger (and fix) this anyway.

goodmanjonathan · 2018-01-17T21:51:40Z

clippy_lints/src/methods.rs

+declare_lint! {
+    pub UNNECESSARY_FOLD,
+    Warn,
+    "using `fold` when a more succint alternative exists"


s/succint/succinct

goodmanjonathan · 2018-01-17T21:56:30Z

clippy_lints/src/methods.rs

@@ -623,6 +623,29 @@ declare_lint! {
    "using `as_ref` where the types before and after the call are the same"
 }

+
+/// **What it does:** Checks for using `fold` when a more succint alternative exists.


s/succint/succinct

51: Fix test edge cases r=oli-obk a=killercup Noticed this while adding test case for ~~fun~~ rust-lang/rust-clippy#2350

…_any

oli-obk · 2018-01-19T12:31:10Z

Thanks!

First pass at linting for .any expressed as a .fold

f6e56d2

theotherphil changed the title ~~WIP issue 2237~~ WIP https://github.com/rust-lang-nursery/rust-clippy/issues/2237 Jan 14, 2018

theotherphil changed the title ~~WIP https://github.com/rust-lang-nursery/rust-clippy/issues/2237~~ WIP issue 2237 Jan 14, 2018

theotherphil added 3 commits January 14, 2018 09:30

Tidy using if_chain and snippet function. Actually check that the ini…

1feb9fd

…tial fold value is false. Remove some unwraps

Move get_arg_name into utils

528be23

Add description

7e833ea

theotherphil changed the title ~~WIP issue 2237~~ Lint for using hand-writing a fold with the same behaviour as any Jan 14, 2018

Fix name

360f235

killercup reviewed Jan 14, 2018

View reviewed changes

This was referenced Jan 14, 2018

Support trailing commas in lint_array #2352

Closed

Support trailing commas in lint_array rust-lang/rust#47428

Closed

Address some review comments

70a5535

Check that we're calling Iterator::fold

ad16493

oli-obk suggested changes Jan 16, 2018

View reviewed changes

mzimnn reviewed Jan 16, 2018

View reviewed changes

Fix error span to play nicely with rustfix

a64d19c

oli-obk approved these changes Jan 17, 2018

View reviewed changes

theotherphil added 3 commits January 17, 2018 19:12

Lint on folds implementing .all, .sum and .product

1cac693

Fix bug. Don't expect lint when acc is on rhs

29a2dd4

Rename lint, improve documentation

9806b31

killercup mentioned this pull request Jan 17, 2018

Fix test edge cases rust-lang/rustfix#51

Merged

killercup reviewed Jan 17, 2018

View reviewed changes

Add some reviewer comments

b73efad

goodmanjonathan reviewed Jan 17, 2018

View reviewed changes

Fix typos

a324a2b

goodmanjonathan reviewed Jan 17, 2018

View reviewed changes

bors bot added a commit to rust-lang/rustfix that referenced this pull request Jan 18, 2018

Merge #51

e5505c5

51: Fix test edge cases r=oli-obk a=killercup Noticed this while adding test case for ~~fun~~ rust-lang/rust-clippy#2350

bors bot added a commit to rust-lang/rustfix that referenced this pull request Jan 18, 2018

Merge #51

ac543e7

51: Fix test edge cases r=oli-obk a=killercup Noticed this while adding test case for ~~fun~~ rust-lang/rust-clippy#2350

oli-obk added 2 commits January 19, 2018 12:10

Merge branch 'master' of github.com:Manishearth/rust-clippy into fold…

7030259

…_any

Update error count

71abd81

oli-obk merged commit e642887 into rust-lang:master Jan 19, 2018

theotherphil deleted the fold_any branch January 19, 2018 22:16

camsteffen mentioned this pull request Dec 16, 2020

lint Iterator::folds that can be one of the special methods #2237

Closed

Lint for using hand-writing a fold with the same behaviour as any #2350

Lint for using hand-writing a fold with the same behaviour as any #2350

Conversation

theotherphil commented Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherphil Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

killercup Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

theotherphil Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

killercup Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherphil commented Jan 14, 2018

killercup commented Jan 14, 2018

clarfonthey commented Jan 14, 2018

theotherphil commented Jan 14, 2018

theotherphil commented Jan 14, 2018

clarfonthey commented Jan 14, 2018

oli-obk commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherphil commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherphil commented Jan 17, 2018

theotherphil commented Jan 17, 2018

killercup commented Jan 17, 2018

theotherphil commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

theotherphil Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jan 19, 2018

theotherphil commented Jan 14, 2018 •

edited

Loading

killercup Jan 14, 2018 •

edited

Loading

theotherphil Jan 14, 2018 •

edited

Loading

killercup Jan 14, 2018 •

edited

Loading

theotherphil Jan 14, 2018 •

edited

Loading

killercup Jan 14, 2018 •

edited

Loading

theotherphil commented Jan 17, 2018 •

edited

Loading

theotherphil Jan 17, 2018 •

edited

Loading