Add rebind_fn_arg_as_mut lint

[cauebs]

Closes #1657.

I'm really not sure about the way I organized everything, and I think I might be forgetting some corner cases. Any feedback is welcome.

changelog: none

[rust-highfive]

r? @Manishearth

(rust_highfive has picked a reviewer for you, use r? to override)

[cauebs]

I just re-read the discussion in #1657 and noticed I still have to make it bail out when inside macros, right?

@Manishearth just as in my other PR, can you add the hacktoberfest-accepted label to this one before monday?

[Manishearth]

Approving for hacktoberfest, but the approach is incorrect.

This should not need a visitor IMO, the way to structure the code should be to look for let mut x = x; statements, and then check if the pattern x was defined in a fn arg by checking the source of its def id.

[Manishearth]

And yes, you'll need to introduce a macro check, see how in_external_macro() is used in this repo to understand better.

[cauebs]

Alright, cool! I ended up learning some new things. Thanks for the feedback.

Could you explain the difference between in_external_macro and Span::from_expansion?

[Manishearth]

in_external_macro checks if the span comes from a macro from outside this crate, which is precisely what we want. Expansion spans are part of the puzzle in this check, but in_external_macro is the utility function that does the check.

[Manishearth]

impl seems great, but i want a second opinion on the lint level. Unsure if it should be style or pedantic.

[Manishearth]

@ebroto what do you think of the lint level for this?

[ebroto]

I think this should be a style lint, but IMO we should:

• Check that the scope of the linted binding is the same as the scope of the parameter (so, the whole function body)
• Fix the known problem

If we want to leave these cases for a follow-up, I would add the lint to nursery for the time being.

[cauebs]

I think I can address your first suggestion this weekend.

As to the second, if by fixing you mean generating an applicable suggestion, I only didn't do it because the output looked awful: a suggestion to remove an entire line just prints out a blank that might confuse the user if they don't pay attention to the line number. If it's possible to make it better than that, let me know!

[ebroto]

Sorry for not being precise. I meant to avoid the false positive when the parameter has been shadowed.

[cauebs]

Oh, right. But actually, I think that is fixed by my new impl. I'm comparing the resolution id for the variable (path) with the HIR id of the parameter's pattern. The thing is, I'm starting to think we should expand this lint to cover not only function parameters but any binding. Is there some case in which let mut x = x; is actually desired?

[camsteffen]

I believe that is already covered by the shadow_same lint.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[giraffate]

ping from triage @cauebs. Do you have any questions on how to proceed here?

[giraffate]

ping from triage @cauebs. Thanks for contributing Clippy! Sadly this PR didn't have any update in the last 2 weeks. If you have more time to continue working on this, feel free to reopen.