WIP: 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.

[y21]

I wonder if it'd be worth moving this function to clippy_utils (as path_to_local_with_projections), and using that one instead of path_to_local. That would make the test case in _unfixable fixable right?

The lint where this is from is also doing the same kind of thing as this PR is, so it seems like this is not too uncommon of an issue that it could justify adding a new util function.

[ada4a]

Sounds like a good idea!

In fact, I was already able to find one more place that could make use of this function:

rust-clippy/clippy_lints/src/transmute/eager_transmute.rs

Lines 63 to 71 in e8185ec

	/// Checks if an expression is a path to a local variable (with optional projections), e.g.
	/// `x.field[0].field2` would return true.
	fn is_local_with_projections(expr: &Expr<'_>) -> bool {
	match expr.kind {
	ExprKind::Path(_) => path_to_local(expr).is_some(),
	ExprKind::Field(expr, _) | ExprKind::Index(expr, ..) => is_local_with_projections(expr),
	_ => false,
	}
	}

could be just path_to_local_with_projections(expr).is_some().

So maybe it would make sense to open a separate PR for creating this function and finding all the places it could be used in?

[ada4a]

I went ahead and opened a PR at #15396, let me know what you think of it