[willwang-io]

Detect iterator chains that call rev before and after map. Although the chain appears to run the mapping function in reverse order, the reversals cancel and the closure runs in the iterator's original order.

Suggest removing both rev calls and explain how to preserve reverse mapping order when that was intended. Restrict the lint to Iterator methods to avoid matching same-named inherent methods.

changelog:[double_rev_map]: warn when map is surrounded by cancelling rev calls.

Fixes: #17587

• Followed [lint naming conventions][lint_naming]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

[CommanderStorm]

I am a bit baffled that people actually do this, but they do..
or at least meta and a bunch of similarly vibed projects do

https://github.com/search?q=%2F%5C.rev%5C%28%5C%29.map%5C%28.%2B%5C%29.rev%5C%28%5C%29%2F&type=code

View changes since this review

[github-actions]

No changes for f5dd6b8

[CommanderStorm]

Community review: LGTM; thanks.

Make sure to squash the changes

View changes since this review

[Gri-ffin]

Can we add a test where the receiver comes from a macro expansion like this:

vec![0, 1, 2]
    .into_iter()
    .rev()
    .map(|x| x + 1)
    .rev()
    .collect();

I think it is reasonable to lint here since all the three method calls share the same context and are written by the user, and only the receiver originates from a macro.

View changes since this review

[Gri-ffin]

This shouldn't lint on the other hand:

macro_rules! reversed {
    ($iter:expr) => {
        $iter.rev()
    };
}

let _: Vec<_> = reversed!(0..3)
    .map(|x| x + 1)
    .rev()
    .collect();

[CommanderStorm]

(make sure to squash the commits into one commit)

[Gri-ffin]

I can't think of a case where it would be incorrect to remove both rev calls, can this be possibly machine applicable or do you have some example in mind?

View changes since the review

[willwang-io]

I originally had this as machine-applicable, but changed it following @CommanderStorm's earlier review: #17686 (comment).

The concern was that removing both reversals preserve the existing iteration behaviour, but might hide a misunderstanding if the author intended reverse-order mapping. MaybeIncorrect leaves that decision to the user. I don't have a counterexample involving iteration order in mind..

[CommanderStorm]

lets add a comment please.

2/2 getting confused on this is a good signal that this needs a comment.

[willwang-io]

Add comment: "Removing both reversals preserves the current behavior, but the author may have intended the mapping closure to run in reverse order."

[Gri-ffin]

Thank you!

View changes since this review

[rustbot]

r? @llogiq

rustbot has assigned @llogiq for the project review.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 9 candidates
• 9 candidates expanded to 9 candidates
• Random selection from 6 candidates

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.