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

Added lints into_iter_on_ref and into_iter_on_array. #3344

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Oct 20, 2018

Fixes #1565.

into_iter_on_array is a correctness lint against into_iter() on [T; n] and PathBuf (please provide a concise noun that covers both types 🙃).

into_iter_on_ref is a style lint against into_iter() on &C where C is [T], Vec<T>, BTreeSet<T> etc.

Both suggests replacing the into_iter() with iter() or iter_mut().

into_iter_on_array is a correctness lint since it is very likely the standard library would provide an into_iter() method for [T; n] (rust-lang/rust#25725) and existing type inference of [a, b, c].into_iter() will be broken. PathBuf is also placed under this lint since PathBuf::into_iter currently doesn't exist and it makes some sense to implement IntoIterator on it yielding OsStrings.

///
/// **Why is this bad?** Arrays and `PathBuf` do not yet have an `into_iter` method which move out
/// their content into an iterator. Calling `into_iter` instead just forwards to `iter` or
/// `iter_mut` due to auto-referencing, of which only yield references. Furthermore, when the
Copy link
Contributor

Choose a reason for hiding this comment

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

"of which only yield"

Copy link
Member Author

Choose a reason for hiding this comment

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

@oli-obk sorry I'm not sure what do you mean

Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence doesn't parse for me. I'm not sure what you're trying to say.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC iter_mut doesn't directly apply, because it will always try and succeed with the immutable reference first, matching only iter.

Perhaps something like: "Auto-referencing resolves the into_iter call onto a container reference instead, like <&[T; N] as IntoIterator>::into_iter, which just iterates over item references like calling iter would."

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Thanks! After reading the new text I understand what the old text was trying to say ;)

@phansch
Copy link
Member

phansch commented Nov 2, 2018

Sorry for not getting around to merge this faster, could you rebase this for the README.md change? @kennytm

@kennytm
Copy link
Member Author

kennytm commented Nov 2, 2018

@phansch Rebased, thanks!

@phansch
Copy link
Member

phansch commented Nov 2, 2018

bors r+

before another rebase is needed 😅

thanks!

bors bot added a commit that referenced this pull request Nov 2, 2018
3344: Added lints `into_iter_on_ref` and `into_iter_on_array`. r=phansch a=kennytm

Fixes #1565.

`into_iter_on_array` is a correctness lint against `into_iter()` on `[T; n]` and `PathBuf` (please provide a concise noun that covers both types 🙃).

`into_iter_on_ref` is a style lint against `into_iter()` on `&C` where `C` is `[T]`, `Vec<T>`, `BTreeSet<T>` etc.

Both suggests replacing the `into_iter()` with `iter()` or `iter_mut()`.

`into_iter_on_array` is a correctness lint since it is very likely the standard library would provide an `into_iter()` method for `[T; n]` (rust-lang/rust#25725) and existing type inference of `[a, b, c].into_iter()` will be broken. `PathBuf` is also placed under this lint since `PathBuf::into_iter` currently doesn't exist and it makes some sense to implement `IntoIterator` on it yielding `OsString`s.

Co-authored-by: kennytm <kennytm@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 2, 2018

@bors bors bot merged commit 5563bd6 into rust-lang:master Nov 2, 2018
@kennytm kennytm deleted the into-iter-on-array branch November 2, 2018 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants