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

Report for loops using iter() and iter_mut() unnecesarily #10683

Closed
jdm opened this issue Apr 18, 2016 · 5 comments
Closed

Report for loops using iter() and iter_mut() unnecesarily #10683

jdm opened this issue Apr 18, 2016 · 5 comments
Labels
A-mach L-python Python is required

Comments

@jdm
Copy link
Member

jdm commented Apr 18, 2016

Rust allows using for foo in &bar instead of for foo in bar.iter(), and for foo in &mut bar instead of for foo in bar.iter_mut(). Let's add a check to check_rust that warns about instances of .iter() and .iter_mut() that could be replaced. There are a lot of existing instances of this pattern that will need replacing, too.

Note: patterns like for foo in bar.iter().enumerate() (ie. places where the return value of iter() is further refined) cannot be replaced and shouldn't be reported.

Code: python/tidy/servo_tidy/tidy.py
Tests: add a test to python/tidy/servo_tidy_tests/test_tidy.py and run it via ./mach test-tidy --self-test

@jdm jdm added E-less-complex Straightforward. Recommended for a new contributor. A-mach labels Apr 18, 2016
@wafflespeanut wafflespeanut added the L-python Python is required label Apr 18, 2016
@aeischeid
Copy link
Contributor

aeischeid commented Apr 19, 2016

Since I was touching the check_rust method earlier I think I have the sense for how to do this.

"places where the return value of iter() is further refined..." - I am only a little bit familiar with Rust syntax. Are there cases where the return val might be further refined but .iter() is followed by a space character?

@jdm
Copy link
Member Author

jdm commented Apr 20, 2016

That shouldn't be possible, no.

@aeischeid
Copy link
Contributor

think I may be close on defining the rule and the test cases, but stuck on why the tests are failing when I don't expect them to. see commit in my branch aeischeid@57f5483

@jdm jdm added the C-assigned There is someone working on resolving the issue label May 10, 2016
@jdm jdm added the C-has-open-pr There is a PR open that resolves the issue label May 25, 2016
@wafflespeanut wafflespeanut removed C-assigned There is someone working on resolving the issue C-has-open-pr There is a PR open that resolves the issue E-less-complex Straightforward. Recommended for a new contributor. labels Jun 5, 2016
@wafflespeanut
Copy link
Contributor

wafflespeanut commented Jun 5, 2016

I'm removing the easy label, since this can't be fixed straightaway (given the conclusion we've arrived at #11116). Maybe we should have a todo list in the issue mentioning which types need the Iterator trait so that we can go about fixing this.

@nox
Copy link
Contributor

nox commented Oct 1, 2017

We have some types that have an iter method but don't even implement IntoIterator. I think that's a job better left to Clippy. Closing.

@nox nox closed this as completed Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mach L-python Python is required
Projects
None yet
Development

No branches or pull requests

4 participants