new lint: redundant_locals

[max-niederman]

This lint checks for code like the following:

let x = 1;
let x = x;

It checks (afaik) all cases where a binding is shadowed by its own value in the same block, including function parameters. This has no effect and is almost certainly accidental, so it's in the correctness category like self_assignment.

This also lays the groundwork for a more generalized version of #5102.

changelog: new lint: [redundant_local]

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[max-niederman]

Okay, so I'm really confused on how the the redundant_locals lint is still being emitted for the popular-crates test even with the Span::from_expansion check. Anybody know why this could happen?

[Centri3]

Okay, so I'm really confused on how the the redundant_locals lint is still being emitted for the popular-crates test even with the Span::from_expansion check. Anybody know why this could happen?

This is likely because clap will itself redefine locals in its generated code (it does this in Args::augment_args, iirc), therefore you'll need to call is_from_proc_macro (which I think will require you to use a LateLintPass lint). afaik Span::from_expansion isn't enough to figure out if something's from a proc macro or not

[max-niederman]

@rustbot review

[dswij]

@max-niederman thanks for the PR!

We follow a no merge-commit policy. Can you help to resolve the conflict by rebasing this to master branch and drop the merge-commit?

@Centri3 Do you want to help review this PR since you've looked at it?

[Centri3]

@Centri3 Do you want to help review this PR since you've looked at it?

I was actually just doing that :D

[Centri3]

Looks pretty good so far, but it misses some redundant locals.

[llogiq]

Please add a test case with a type-ascribed local (e.g. let x: u8 = x;). Those should not be linted.

[Alexendoo]

Similarly some test cases for let ref x = x; and let ref mut x = x; would be good

We might need to add a needs_ordered_drop check as removing the let statement can change the drop order, e.g.

let guard = mutex.lock().unwrap();

// ..

{
    let guard = guard;
    // ..
}

// mutex is now unlocked

[max-niederman]

@Centri3 @Alexendoo I intentionally wrote the lint to only check redefinitions in the same block in order to prevent changes to the drop order, but I definitely think it would be better to check all redefinitions and use needs_ordered_drop now that I know that's an option. I'm going to rewrite as a late-pass lint and do that.

[max-niederman]

I'll also disable linting on type-ascribed locals. Totally forgot about coercion 🤦.

[Centri3]

Looks pretty good, I've left some comments on what can be improved.

[bors]

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

[max-niederman]

I finally got around to working on this again and I'm pretty sure it's in a good state. It would be good to check for multiple-binding local statements, but that's way harder and definitely suffers from diminishing returns in terms of applicability.

[bors]

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

[Centri3]

This looks good, thanks! I may do the (x, y) idea as a followup if that's fine with you ^^

@dswij, can you take a look as well?

[Centri3]

I think this is about ready to be merged :D Just two minor nits, thanks!

[bors]

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

[bors]

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

[Centri3]

Once you rebase and add the crate type this is ready :)

[Centri3]

Sorry, also please squash.

[Centri3]

Thanks! @bors r+

[bors]

📌 Commit 008ba2b has been approved by Centri3

It is now in the queue for this repository.

[bors]

⌛ Testing commit 008ba2b with merge a44dcf8...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing a44dcf8 to master...