Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix needless_collect's tendency to suggest code requiring multiple mutable borrows of the same value. #7982

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

Blckbrry-Pi
Copy link
Contributor

Fixes error specified in #7975.

changelog: [needless_collect] no longer suggests removal of collect when removal would create code requiring mutably borrowing a value multiple times.

Still needs to generalize to other mixes of let bindings, `map` method calls, etc.
…table borrows.

Also add some more tests to check that it's working.
@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 16, 2021
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already looking quite good. I only have some minor readability nits.

clippy_lints/src/loops/needless_collect.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/needless_collect.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/needless_collect.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/needless_collect.rs Outdated Show resolved Hide resolved
1. Make the lifetime contained in LateContext `'tcx`.
2. Fix `'txc` to `'tcx` because it was a typo.
3. Refactor `IterFunctionVisitor`'s `visit_block` method to be more readable.
4. Replace uses of `rustc_middle::ty::TyKind` with `rustc::middle::ty`, and remove the `#[allow(...)]`.

(Thank you llogiq for all these suggestions!)
@Blckbrry-Pi
Copy link
Contributor Author

I have fixed the suggestions. Thank you for making them!

@llogiq
Copy link
Contributor

llogiq commented Nov 16, 2021

Thank you! @bors r+

@bors
Copy link
Collaborator

bors commented Nov 16, 2021

📌 Commit c52b389 has been approved by llogiq

@bors
Copy link
Collaborator

bors commented Nov 16, 2021

⌛ Testing commit c52b389 with merge 83ad511...

@bors
Copy link
Collaborator

bors commented Nov 16, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 83ad511 to master...

@bors bors merged commit 83ad511 into rust-lang:master Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants