issue#3424 improve filter_map lint docs by waynr · Pull Request #3461 · rust-lang/rust-clippy

waynr · 2018-11-27T16:18:39Z

waynr · 2018-11-27T16:33:00Z

I've tested all of these on the rust playground. One thing I'm not certain about is whether or not it's okay to create multiple example headings -- I thought it would be a good idea to create a separate section for each combination.

Also, I have to say that the filter + flat_map and filter_map + flat_map examples that I came up with don't inspire confidence that the suggested approach is actually better. I guess that's why it's marked as pedantic and lints can be pretty easily disabled in one's project...

ghost · 2018-11-27T17:11:01Z

We had discussion about this lint in issue #3188 and concluded that filter(..).map(..) should only be replaced with filter_map if there is some unwrapping going on. IMO, filter(..).flat_map(..) and filter_map(..).flat_map(..) shouldn't be linted at all.

ghost · 2018-11-27T17:15:21Z

Actually linting filter(..).flat_map(..) in is OK in some cases (#3188 has an example), but not filter_map(..).flat_map(..).

waynr · 2018-11-28T19:38:30Z

-}
-
-/// lint use of `filter().flat_map()` for `Iterators`
-fn lint_filter_flat_map<'a, 'tcx>(


@mikerite I added a commit to this PR that removes the linting of filter().flat_map() since I couldn't think of a simpler way to achieve the same behavior based on the example you gave. I'd be happy to add it back and include it in the updated docstring if you have any guidance for me here. Thanks!

bors · 2018-12-12T06:24:14Z

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

flip1995 · 2019-06-19T12:10:37Z

Ping @waynr. I'm going over old PRs, that were abandoned by us reviewers (sorry for that!) or by the authors. Are you still interested in completing this?

waynr · 2019-06-19T16:14:10Z

@flip1995 I am still interested, it just fell off my radar! I'll try to get to it no later than Monday next week (but possibly as early as tonight). Thanks for the ping!

* issue#3188 make the 'filter_map' lint more specific * Remove filter + flat_map lint * Run 'util/dev update_lints' * Update filter_methods.stderr * Address dogfood ci failure * Add FILTER_MAP_MAP to the methods LintPass

waynr · 2019-06-23T16:10:07Z

Okay I've resolved the merge conflicts and I think everything is order again so this should be ready for re-review. Reviewers may want to ensure that I haven't unintentionally changed anything important while rebasing -- I don't remember much about this work and haven't worked with Rust since so I'm missing much of the context I had back in December when originally working on it.

flip1995

Thanks for getting back to this!

cc @mikerite can you also take a look at this please?

flip1995 · 2019-06-24T08:28:25Z

+
 declare_clippy_lint! {
    /// **What it does:** Checks for usage of `_.filter(_).map(_)`,
    /// `_.filter(_).flat_map(_)`, `_.filter_map(_).flat_map(_)` and similar.


This doc needs to be adapted

Co-Authored-By: Philipp Krones <hello@philkrones.com>

flip1995

I need to do a in depth review of this, but probably won't have time for this until next week...

flip1995 · 2019-06-26T09:08:40Z

+    /// println!("{:?}", correct);
+    /// assert_eq!(correct, more_correct);
+    /// ```
+    ///             .iter()


You forgot to remove the old example code 😉

flip1995

I finally got to reviewing this more in depth. Sorry for the long wait time :(

I think removing both filter().flat_map() and filter_map().flat_map() is totally fine, since I can only think of some few edge cases where rewriting this into one function call would maybe make sense.

flip1995 · 2019-07-11T14:45:29Z

+    let mut span = expr.span;
+    if let hir::ExprKind::MethodCall(_, _, ref expr_vec) = expr.node {
+        if let hir::ExprKind::MethodCall(_, ref filter_span, _) = expr_vec[0].node {
+            span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt());


There's already a function for this on Span: https://doc.rust-lang.org/nightly/nightly-rustc/src/syntax_pos/lib.rs.html#217-219

Suggested change

span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt());

span = expr.span.with_lo(filter_span.lo());

flip1995 · 2019-07-11T14:48:54Z


-error: called `filter_map(p).flat_map(q)` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` and filtering by returning an empty Iterator.
-  --> $DIR/filter_methods.rs:13:21
+error: redundant closure found


Can you allow redundant_closure for these tests?

flip1995 · 2019-07-11T14:49:39Z

+        .map(|x| x.unwrap())
        .collect();

+    // validate iterator of options triggers filter + flat_map lint


This was removed in this PR, wasn't it?

flip1995 · 2019-07-11T14:50:45Z

+        .map(|s| format!("{}{}", s, s))
+        .collect();
+
+    // validate iterator of values **does not** trigger filter + flat_map lint


Same as above: Wasn't this removed completely anyway?

bors · 2019-07-17T08:08:44Z

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

phansch · 2020-04-26T10:50:15Z

Hi @waynr, would you still be up to finish this PR? No worries if not, it's been a long time =)

flip1995 · 2020-05-25T15:21:26Z

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

jonboh · 2023-10-10T18:23:17Z

@rustbot label -S-inactive-closed

waynr mentioned this pull request Nov 27, 2018

Make the filter_map lint more specific #3188

Closed

waynr force-pushed the issue#3424 branch from a7c6f0b to eda304a Compare November 28, 2018 19:35

waynr commented Nov 28, 2018

View reviewed changes

waynr force-pushed the issue#3424 branch 2 times, most recently from 6926b28 to c2eb74f Compare December 6, 2018 00:20

phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 6, 2018

waynr added 3 commits June 23, 2019 15:42

issue#3424 improve filter_map lint docs

06f39e1

* issue#3188 make the 'filter_map' lint more specific * Remove filter + flat_map lint * Run 'util/dev update_lints' * Update filter_methods.stderr * Address dogfood ci failure * Add FILTER_MAP_MAP to the methods LintPass

Fix lingering rebase breakages

19d335b

Recreate tests/ui/filter_methods.stderr after rebase

8c64708

waynr force-pushed the issue#3424 branch from c2eb74f to 8c64708 Compare June 23, 2019 16:04

Re-run 'util/dev update-lints'

eeab7db

flip1995 requested changes Jun 24, 2019

View reviewed changes

waynr and others added 4 commits June 24, 2019 20:46

Update clippy_lints/src/methods/mod.rs

f96eea7

Co-Authored-By: Philipp Krones <hello@philkrones.com>

Update filter_methods stderr

9101623

Restore test to validate filter_map + flat_map doesn't trigger a lint

9b45aec

Fix docstring for FILTER_MAP lint

ee9c13d

flip1995 reviewed Jun 26, 2019

View reviewed changes

flip1995 requested changes Jul 11, 2019

View reviewed changes

phansch added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 17, 2019

flip1995 closed this May 25, 2020

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 25, 2020

rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023

	span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt());
	span = expr.span.with_lo(filter_span.lo());

Conversation

waynr commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waynr commented Nov 27, 2018

Uh oh!

ghost commented Nov 27, 2018

Uh oh!

ghost commented Nov 27, 2018

Uh oh!

waynr Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

bors commented Dec 12, 2018

Uh oh!

flip1995 commented Jun 19, 2019

Uh oh!

waynr commented Jun 19, 2019

Uh oh!

waynr commented Jun 23, 2019

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flip1995 Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

bors commented Jul 17, 2019

Uh oh!

phansch commented Apr 26, 2020

Uh oh!

flip1995 commented May 25, 2020

Uh oh!

jonboh commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

waynr commented Nov 27, 2018 •

edited

Loading