New lint for all/any after mapping to bool

[tamaroning]

fixes #7339

This new lint [map_then_identity_transformer] finds _.map(_).<transformer>(_) where the map is collapsible into <transformer>. (<transformer> is any of all, any, find, find_map, flat_map, filter_map, fold, position, position, and map.)
If the transformer is the form of <transformer>(Closure), this lint decides if or not the map is collapsible by checking if a paramters is referenced one time in the body of the Closure.

changelog:

3/6

• Do not lint the following form of code

_.map(
//  param of `map` has more than one line
).<transformer>(_)

• Do not lint .map().find() bacause only find takes a closure like |&x| x
• Different suggestion messages depends on the form of code
  • case1 .map(Closure).<transformer>(Closure)
  • case2 .map(Path).<transformer>(Closure)
  • case3 .map(Closure or Path).<transformer>(Path)
• To avoid conflicting lints, checks if another lint is triggered or not (as @flip1995 suggested below)
  • I confirmed that [map_then_identity_transformer] is not prioritized in all cases
• Modify tests
• Update the explanation

[rust-highfive]

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

[tamaroning]

@rustbot label -S-waiting-on-review

[tamaroning]

@rustbot label +S-waiting-on-review

I am not a native English speaker so could you please improve the documentation?

[flip1995]

It decides if or not the map is collapsible by checking if a parameters is referenced one time in both map and the following transformer.

Couldn't it ignore the count of the parameter in the first map and only check the following transformer, whether the parameter appears only once?

So from your tests, .map(|x| x * x).<transformer>(|y| Some(y)) could be simplified to .<transformer>(|x| Some(x * x)).

To put it another way: wouldn't it make sense that it only depends on the count of the transformer?

With that, I have a few concerns that should also be tested:

• Single (one-line) expression in map: IMO it might make sense to limit this lint on one-line expressions in map. I haven't looked at the code, whether this is already checked, but there should be tests for that. In my experience with those map-chain lints, there is a possibility to make the code harder to read. Heuristics like this can prevent this.
• interaction with other lints: I see you allowed a few in the test. This might have too many conflicts right now.
• Dogfood: Instead of allowing the lint in Clippy, you should apply it. (If there is no obvious way how to do it, it is a good sign that the lint probably needs some more thought)

I am not a native English speaker so could you please improve the documentation?

Yes sure!

[bors]

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

[flip1995]

I see rebasing has gone horribly wrong here. In cases like this git reflog is sometimes your friend. Run this command, then find the commit (the ref) before you started merging/rebasing. After that you take the hash displayed at the beginning of that line and reset to that state: git reset --hard <sha>.

[tamaroning]

Thanks!
I mistakenly closed this PR so reopened it.

[tamaroning]

@flip1995

Single (one-line) expression in map: IMO it might make sense to limit this lint on one-line expressions in map. I haven't looked at the code, whether this is already checked, but there should be tests for that. In my experience with those map-chain lints, there is a possibility to make the code harder to read. Heuristics like this can prevent this.

For example, it is better not to lint the following code (from Clippy source). If the size of the term of a body of closures can be measured, using the size as heuristics may be good.

.map(|imp_did| cx.tcx.hir().expect_item(imp_did.expect_local()))
.any(|imp| has_unsafe(cx, imp));

interaction with other lints: I see you allowed a few in the test. This might have too many conflicts right now.

Do you mean that it is not good to have more than one lint triggered at the same time?

[flip1995]

Do you mean that it is not good to have more than one lint triggered at the same time?

On the same part of code, this should be avoided, because it can lead to confusing suggestions. If 2 lints trigger on the same expression with different suggestions on how to fix it, it is more confusing than helpful.

This can be prevented by checking if another lint triggered. This is down by returning bool from the check function of the lint and only check for the conflictng lint, if the first lint didn't trigger. Since this lint conflicts with many other lints, figuring out precedence is a bit of a task that has to be done on a case by case basis.

Note: if applying one lint suggestion, results in another lint triggering when re-running Clippy, it is not that bad.

[bors]

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

[tamaroning]

@flip1995
Sorry for late reply and thanks for your comments!
I fixed code based on your advices. Please see the top comment.

@rustbot label -S-waiting-on-author +S-waiting-on-review

[bors]

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

[blyxyas]

@tamaroning I'm going to close this PR, if you want to continue development on it, I'd advice completely re-starting on another branch with the new PR process. Although, if you really want to just develop further in this (very outdated) branch, feel free to re-open it.