map_identity: suggest making the variable mutable when necessary

[ada4a]

While working on #15229, I noticed a test case in map_identity which avoids linting in the case where removing the .map() would require the iterator variable to be made mutable. But then I saw #14140, and thought I'd try to adapt its approach, by suggesting both removing the .map() and making the variable mutable.

This is WIP only because I'm not sure about the very last diag.span_note -- I think having a method chained immediately after the .map() is the only case which requires adding mut, so it should be safe to say that method_requiring_mut is always Some. I'd like to do a lintcheck run to see if that's actually true.

@samueltardieu do you think this is a good approach? Maybe there are more lints where we could lint + suggest making the variable mut, instead of not linting?

changelog:map_identity: suggest making the variable mutable when necessary

[rustbot]

r? @y21

rustbot has assigned @y21.
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

[ada4a]

@y21 friendly ping

[y21]

Looks good to me, I have one suggestion that could possibly make one more case autofixable

I think having a method chained immediately after the .map() is the only case which requires adding mut, so it should be safe to say that method_requiring_mut is always Some. I'd like to do a lintcheck run to see if that's actually true.

&mut iter.map(...) and let ref mut x = iter.map(..) are some other cases that would need it, though IMO not really worth exhaustively handling all other arbitrary ways since this is just for a specialized note. So that note seems fine to me.

[ada4a]

I've added some intermediary variables, which will hopefully make the logic a bit more clear, but made them lazy-evaluated just in case. Do let me know if the latter (or the former, for that matter) is unnecessary

[ada4a]

Okay so I did a bigger refactor. Everything's a bit more wordy now, but I think that's necessary to represent all the complexity

[ada4a]

..or maybe not. I've made another branch where I just conditionally modify msg to include the "please make this mutable" part, and it works out relatively fine?

branch: https://github.com/ada4a/rust-clippy/tree/map-identity-sugg-on-mut-2
commit: ada4a@20e4bb9

Let me know which of these approaches you prefer. Sorry for being a bit hectic^^

[y21]

Having thought a little more about this, I realized that there is technically a behavioral change here for Copy iterators, where iter.map(|x| x).next() would mutate a temporary copy of the iterator and changing it to iter.next() mutates iter directly, so one could argue that { iter }.next() could be a more correct suggestion for those specific types as that also forces a copy.

That said though, Copy iterators are an anti pattern and there also aren't any such iterators in the standard library, so not sure it's really worth complicating the lint with this. It is very easy to check if a type is Copy (clippy_utils::ty::is_copy), so it might be enough to simply downgrade the suggestion away from MachineApplicable. What do you think about that?

commit: ada4a@20e4bb9

This does look like a slight improvement to me, so I'd be fine with going with that.

[ada4a]

I agree that copy iterators are an antipattern. Downgrading the suggestion does sound like a good option.

[ada4a]

simply downgrade the suggestion away from MachineApplicable

There's one problem with that: currently, applicability is also used to decide whether we should emit the note about making the local mutable (in case we can't make an automatic suggestion). So if:

1. the binding didn't need to be made mutable in the first place (which currently leaves us with Applicability::MachineApplicable, avoiding emitting the note)
2. but the iterator is Copy

then we would now downgrade to Applicability::Unspecified again, and thus get the mutability note, which we don't want. Therefore, we'll either need to create another binding for applicability, modify that, and use that for the suggestion.. or just that all inline in the suggestion itself.

I also tried to do it in the verbose version (from this PR), and it did work out a bit more nicely imo?

I'll push commits to both branches, see which of them you like more

[ada4a]

error: failed to download file error=Reqwest(reqwest::Error { kind: Request, url: "https://static.rust-lang.org/dist/channel-rust-nightly.toml", source: TimedOut })

huh!

[y21]

This version looks all good to me now, can you squash the commits? Thanks!

[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.

[ada4a]

Thank you for all the help:) Squashed (and rebased)