Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint for chaining flatten after map #3203

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
4 participants
@yaahallo
Copy link
Contributor

commented Sep 21, 2018

This change adds a lint to check for instances of map(..).flatten()
that can be trivially shortened to flat_map(..)

Closes #3196

@mati865
Copy link
Contributor

left a comment

Take a look at Travis error:

Please run util/update_lints.py to regenerate lints lists.

if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `map(..).flatten()` on an `Iterator`. \
This is more succinctly expressed by calling `.flat_map(..)`";
span_lint(cx, MAP_FLATTEN, expr.span, msg);

This comment has been minimized.

Copy link
@mati865

mati865 Sep 21, 2018

Contributor

It would be nice if this generated suggestions like here

@yaahallo yaahallo force-pushed the yaahallo:master branch from 9c36399 to 191ea19 Sep 21, 2018

Show resolved Hide resolved clippy_lints/src/methods.rs Outdated
@Manishearth

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

@yaahallo btw when you have a moment could you look at #3100 ?

@yaahallo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

I feel bad because I've been getting and ignoring those emails for ages 😅

@yaahallo yaahallo force-pushed the yaahallo:master branch 2 times, most recently from e1af5f5 to c899376 Sep 24, 2018

@yaahallo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

I'm not sure what to make of that appveyor failure

@flip1995

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

Unrelated: #3118

@yaahallo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

Okay then. As far as I can tell this is ready to go. Let me know if there are still any issues that I missed and/or when you need me to rebase.

fn lint_map_flatten<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
expr: &'tcx hir::Expr,
_map_args: &'tcx [hir::Expr],

This comment has been minimized.

Copy link
@flip1995

flip1995 Sep 24, 2018

Collaborator

Since map_args is used below, you should remove the underscore

@yaahallo yaahallo force-pushed the yaahallo:master branch from c899376 to 7b8a2a4 Sep 24, 2018

cx: &LateContext<'a, 'tcx>,
expr: &'tcx hir::Expr,
map_args: &'tcx [hir::Expr],
_flatten_args: &'tcx [hir::Expr],

This comment has been minimized.

Copy link
@flip1995

flip1995 Sep 24, 2018

Collaborator

_flatten_args is never used in the function body, so it is an unused argument and can be removed. We need a lint for this (unused arguments starting with a _)!

This comment has been minimized.

Copy link
@yaahallo

yaahallo Sep 24, 2018

Author Contributor

Aah, yea I followed the pattern from lint_filter_map which also has those unused args left in, so I assumed you guys left in the args by convention incase its needed later or something. I'll remove it completely.

This comment has been minimized.

Copy link
@flip1995

flip1995 Sep 24, 2018

Collaborator

Indeed. I don't really know why lint_filter_map has these arguments. Maybe it was necessary before and never got removed? 🤔 But this isn't part of this PR.

This comment has been minimized.

Copy link
@yaahallo

yaahallo Sep 24, 2018

Author Contributor

If you want to make a new issue to clean those up I don't mind picking that task up.

This comment has been minimized.

Copy link
@flip1995

flip1995 Sep 25, 2018

Collaborator

That would be great! You can just open a PR. If you're motivated you could also write a lint for this.

@flip1995

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

Everything else LGTM!

Lint for chaining flatten after map
This change adds a lint to check for instances of `map(..).flatten()`
that can be trivially shortened to `flat_map(..)`

Closes #3196

@yaahallo yaahallo force-pushed the yaahallo:master branch from 7b8a2a4 to 14feb36 Sep 24, 2018

@flip1995 flip1995 merged commit e3f7b40 into rust-lang:master Sep 25, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.