Implement flat_map lint #4231

jeremystucki · 2019-06-24T13:12:45Z

Fixes #4224

changelog: New Lint flat_map_identity to detect unnecessary calls to flat_map

Manishearth · 2019-06-24T14:42:46Z

PR body needs a changelog entry

Also, not sure if this lint name is appropriate

flip1995 · 2019-06-24T16:21:31Z

Maybe FLAT_MAP_IDENTITY?

lopopolo · 2019-06-27T12:37:44Z

Does this handle .flat_map(std::convert::identity) too? 😄

bors · 2019-07-17T08:09:30Z

☔ The latest upstream changes (presumably #4259) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995 · 2019-08-01T16:25:33Z

Ping from triage @jeremystucki. Do you want to continue working on this?

jeremystucki · 2019-08-05T07:21:42Z

Ah sorry totally forgot about that one, I've been really busy.
I only got time in a week or so. Would you like to continue working on this in the meantime?

flip1995 · 2019-08-05T07:25:41Z

No worries. Just return to this once you have more time. I labeled it with help-wanted, in case someone wants to take over in the mean time.

flip1995 · 2019-08-11T18:40:44Z

clippy_lints/src/methods/mod.rs

+        if match_trait_method(cx, expr, &paths::ITERATOR);
+
+        if flat_map_args.len() == 2;
+        if let hir::ExprKind::Closure(_, _, body_id, _, _) = flat_map_args[1].node;


You could insert the then of the if_chain! here and then split up the two if chains for |x| x and convert::identity, like this:

Suggested change

if let hir::ExprKind::Closure(_, _, body_id, _, _) = flat_map_args[1].node;

then {

let arg = flat_map_args[1];

if_chain! {

if let hir::ExprKind::Closure(_, _, body_id, _, _) = arg.node;

// check for `|x| x`

}

if_chain! {

if let hir::ExprKind::Path(ref qpath) = arg.node;

// check for `convert::identity`

}

}

Co-authored-by: Philipp Krones <hello@philkrones.com>

jeremystucki · 2019-08-12T06:36:07Z

r? @flip1995

flip1995 · 2019-08-12T07:49:08Z

clippy_lints/src/methods/mod.rs

+    /// iter.flatten();
+    /// ```
+    pub FLAT_MAP_IDENTITY,
+    pedantic,


This should be a style or complexity lint IMO.

Everything else LGTM

flip1995

You have to run update_lints

Also both lints can be easily implemented as MachineApplicable lints. Steps to do this:

swap the span_lint function with the span_lint_and_sugg function
As the span, only use the span of the call to flat_map(..)
Add the sugg-message: "try" (or something similar)
Add as the sugg: "flatten()"
In the second if_chain!: don't shadow expr, but name it arg or something else. (This binding can also be shared between both inner if_chain!s)
Remove "This can be simplified by calling flatten()" from the lint message.
Add // run-rustfix to the top of the test file
Update the .stderr file.

jeremystucki · 2019-08-12T19:45:49Z

I can't quite figure out how to get the correct span. Can anyone help me?

This is the current diff:

 error: called `flat_map(|x| x)` on an `Iterator`
   --> $DIR/unnecessary_flat_map.rs:9:14
    |
 LL |     iterator.flat_map(|x| x);
-   |              ^^^^^^^^^^^^^^^ help: try: `flatten()`
+   |              ^^^^^^^^ help: try: `flatten()`
    |
    = note: `-D clippy::flat-map-identity` implied by `-D warnings`
 
 error: called `flat_map(std::convert::identity)` on an `Iterator`
   --> $DIR/unnecessary_flat_map.rs:12:14
    |
 LL |     iterator.flat_map(convert::identity);
-   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`
+   |              ^^^^^^^^ help: try: `flatten()`
 
 error: aborting due to 2 previous errors

flip1995 · 2019-08-13T07:19:47Z

The refactor looks really good!

It seems, that you have to construct the span (In the apply_lints closure):

span.with_hi(expr.span.hi())

This should give you the correct span.

flip1995 · 2019-08-14T08:00:34Z

@bors r+

Thanks!

bors · 2019-08-14T08:00:35Z

📌 Commit 2bfcf89 has been approved by flip1995

bors · 2019-08-14T08:41:34Z

⌛ Testing commit 2bfcf89 with merge 2cf7ee5...

Implement flat_map lint Fixes #4224 changelog: New Lint `flat_map_identity` to detect unnecessary calls to `flat_map`

bors · 2019-08-14T08:54:05Z

💔 Test failed - status-appveyor

phansch · 2019-08-14T09:28:17Z

@bors retry (should work after #4378)

bors · 2019-08-14T09:36:22Z

⌛ Testing commit 2bfcf89 with merge 4f8bdf3...

Implement flat_map lint Fixes #4224 changelog: New Lint `flat_map_identity` to detect unnecessary calls to `flat_map`

bors · 2019-08-14T10:17:51Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 4f8bdf3 to master...

This commit fixes the file names of the `flat_map_identity` test. Previously, their names were started with `unnecessary_flat_map` even though the lint rule name is `flat_map_identity`. This inconsistency happened probably because the rule name was changed during the discussion in the PR where this rule was introduced. ref: rust-lang#4231

Fix file names of flat_map_identity test This patch fixes the file names of the `flat_map_identity` test. Previously, their names were started with `unnecessary_flat_map` even though the lint rule name is `flat_map_identity`. This inconsistency happened probably because the rule name was changed during the discussion in the PR where this rule was introduced. ref: #4231 changelog: none

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jul 3, 2019

jeremystucki added 2 commits July 8, 2019 17:11

Add test for unnecessary_flat_map

1fd617d

Implement flat_map lint

c7da4c2

flip1995 added the E-help-wanted Call for participation: Help is requested to fix this issue. label Aug 5, 2019

jeremystucki added 3 commits August 11, 2019 19:22

Merge branch 'origin/master' into flat-map

05d9f88

Handle calls with 'std::convert::identity'

f0ce04f

Rename 'flat_map' → 'flat_map_identity'

b651f19

flip1995 reviewed Aug 11, 2019

View reviewed changes

jeremystucki and others added 3 commits August 11, 2019 21:02

Refactor if_chain

5fd7d44

Co-authored-by: Philipp Krones <hello@philkrones.com>

Update rustdoc

df90630

Run 'update_lints'

6ddf8c3

flip1995 reviewed Aug 12, 2019

View reviewed changes

Change lint type from 'pedantic' → 'complexity'

09e9568

flip1995 requested changes Aug 12, 2019

View reviewed changes

jeremystucki added 2 commits August 12, 2019 21:29

Minor refactoring

3a65e4e

Use 'span_lint_and_sugg'

d51136d

jeremystucki and others added 2 commits August 12, 2019 21:47

Run 'update_lints'

4275d7b

Add 'unnecessary_flat_map.fixed'

6a263c0

flip1995 approved these changes Aug 13, 2019

View reviewed changes

Use correct span

d578c43

jeremystucki added 2 commits August 13, 2019 21:50

Update test

2fe5e2c

Merge branch 'origin/master' into flat-map

2bfcf89

bors added a commit that referenced this pull request Aug 14, 2019

Auto merge of #4231 - jeremystucki:flat-map, r=flip1995

2cf7ee5

Implement flat_map lint Fixes #4224 changelog: New Lint `flat_map_identity` to detect unnecessary calls to `flat_map`

bors added a commit that referenced this pull request Aug 14, 2019

Auto merge of #4231 - jeremystucki:flat-map, r=flip1995

4f8bdf3

Implement flat_map lint Fixes #4224 changelog: New Lint `flat_map_identity` to detect unnecessary calls to `flat_map`

bors merged commit 2bfcf89 into rust-lang:master Aug 14, 2019

jeremystucki deleted the flat-map branch August 14, 2019 11:42

magurotuna mentioned this pull request Feb 3, 2021

Fix file names of flat_map_identity test #6669

Merged

-        if let hir::ExprKind::Closure(_, _, body_id, _, _) = flat_map_args[1].node;
+        then {
+            let arg = flat_map_args[1];
+            if_chain! {
+                if let hir::ExprKind::Closure(_, _, body_id, _, _) = arg.node;
+                // check for `|x| x`
+            }
+            if_chain! {
+                if let hir::ExprKind::Path(ref qpath) = arg.node;
+                // check for `convert::identity`
+            }
+        }

Implement flat_map lint #4231

Implement flat_map lint #4231

Uh oh!

Conversation

jeremystucki commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Manishearth commented Jun 24, 2019

Uh oh!

flip1995 commented Jun 24, 2019

Uh oh!

lopopolo commented Jun 27, 2019

Uh oh!

bors commented Jul 17, 2019

Uh oh!

flip1995 commented Aug 1, 2019

Uh oh!

jeremystucki commented Aug 5, 2019

Uh oh!

flip1995 commented Aug 5, 2019

Uh oh!

flip1995 Aug 11, 2019

Choose a reason for hiding this comment

Uh oh!

jeremystucki commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

jeremystucki commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 commented Aug 14, 2019

Uh oh!

bors commented Aug 14, 2019

Uh oh!

bors commented Aug 14, 2019

Uh oh!

bors commented Aug 14, 2019

Uh oh!

phansch commented Aug 14, 2019

Uh oh!

bors commented Aug 14, 2019

Uh oh!

bors commented Aug 14, 2019

Uh oh!

Uh oh!

jeremystucki commented Jun 24, 2019 •

edited

Loading

jeremystucki commented Aug 12, 2019 •

edited

Loading

jeremystucki commented Aug 12, 2019 •

edited

Loading

flip1995 commented Aug 13, 2019 •

edited

Loading