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

Add lint: from_iter_instead_of_collect #6101

Merged
merged 13 commits into from
Nov 3, 2020
Merged

Add lint: from_iter_instead_of_collect #6101

merged 13 commits into from
Nov 3, 2020

Conversation

pitiK3U
Copy link
Contributor

@pitiK3U pitiK3U commented Oct 1, 2020

Fixes #5679

This implements lint for ::from_iter() from #5679 not the general issue (std::ops::Add::add, etc.).
This lint checks if expression is function call with from_iter name and if it's implementation of the std::iter::FromIterator trait.

changelog: Introduce from_iter_instead_of_collect lint

@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 @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

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 Oct 1, 2020
@pitiK3U
Copy link
Contributor Author

pitiK3U commented Oct 1, 2020

I am not sure how to resolve the get_unwrap failing test.
If I am supposed to update .stderr of the test, change the test code, ignore this lint in the test or change category of this lint.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Some comments for cleanup and to get it aligned with the rest of the codebase.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
tests/ui/from_iter_instead_of_collect.rs Outdated Show resolved Hide resolved
tests/ui/from_iter_instead_of_collect.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 1, 2020
@flip1995
Copy link
Member

flip1995 commented Oct 1, 2020

About the get_unwrap test: Just allow the new lint in the get_unwrap test file. You may have to update the get_unwrap.stderr file, because line numbers may change.


Please also remove the empty stdout file.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Last NIT, after that it's ready to get merged 🚀

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

flip1995 commented Oct 1, 2020

The lint you implemented detected this problem in Clippy itself:

error: usage of `FromIterator::from_iter`
   --> clippy_lints/src/lifetimes.rs:217:24
    |
217 |           .intersection(&FxHashSet::from_iter(
    |  ________________________^
218 | |             input_visitor
219 | |                 .nested_elision_site_lts
220 | |                 .iter()
...   |
223 | |                 .filter(|v| matches!(v, RefLt::Named(_))),
224 | |         ))
    | |_________^
    |
    = note: `-D clippy::from-iter-instead-of-collect` implied by `-D clippy::all`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
help: use `.collect()` instead of `::from_iter()`
    |
217 |         .intersection(&input_visitor
218 |                 .nested_elision_site_lts
219 |                 .iter()
220 |                 .chain(output_visitor.nested_elision_site_lts.iter())
221 |                 .cloned()
222 |                 .filter(|v| matches!(v, RefLt::Named(_))).collect())
    |

@flip1995
Copy link
Member

ping from triage @pitiK3U. There's only a small fix left to be done. Do you have any questions on how to proceed here?

@pitiK3U
Copy link
Contributor Author

pitiK3U commented Oct 11, 2020

ping from triage @pitiK3U. There's only a small fix left to be done. Do you have any questions on how to proceed here?

Sorry for taking me too long to finish this. My college had started and I hadn't had time to finish it. Will try to do so asap.

bors added a commit that referenced this pull request Oct 12, 2020
Remove the generated files by `update-references.sh` if they are empty

An empty file may be generated by `update-references.sh` and committed as is when creating a patch like #6101 (comment) and #6079 (review). So, I think it would be helpful to add documentation, and automatically remove the generated file if it's empty.

changelog: none
@flip1995
Copy link
Member

Thanks! Only the dogfood error (#6101 (comment)) is now left to fix

@bors
Copy link
Contributor

bors commented Oct 29, 2020

☔ The latest upstream changes (presumably #6227) 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

@flip1995
Copy link
Member

flip1995 commented Nov 3, 2020

@bors r+

Thanks! I rebased it for you, so I can directly r+ it, so that there won't be another rebase required.

@bors
Copy link
Contributor

bors commented Nov 3, 2020

📌 Commit ddf23d6 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Nov 3, 2020

⌛ Testing commit ddf23d6 with merge a2bf404...

@bors
Copy link
Contributor

bors commented Nov 3, 2020

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

@bors bors merged commit a2bf404 into rust-lang:master Nov 3, 2020
@pitiK3U pitiK3U deleted the from_iter_instead_of_collect branch November 3, 2020 16:25
@pitiK3U
Copy link
Contributor Author

pitiK3U commented Nov 3, 2020

@flip1995 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint suggestion: use .collect() instead of ::from_iter()
5 participants