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

Lint .into_iter() if that only forwards to .iter() #1565

Closed
oli-obk opened this issue Feb 21, 2017 · 16 comments
Closed

Lint .into_iter() if that only forwards to .iter() #1565

oli-obk opened this issue Feb 21, 2017 · 16 comments
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Feb 21, 2017

cc @bluss

Types like [T; N] don't implement IntoIter themselves, but instead provide it through their Deref<Target=[T]> impl, which is simply a wrapper around calling .iter().

.into_iter() suggests that some ownership transfer is going on, if that is not the case .iter() should be used.

@oli-obk oli-obk added L-unnecessary Lint: Warn about unnecessary code E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages T-middle Type: Probably requires verifiying types labels Feb 21, 2017
@clarfonthey
Copy link
Contributor

So to clarify this, would this mean calling .into_iter() on a reference? Because that's what &[T]'s into_iter does.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 21, 2017

Yea. Some care needs to be taken to ensure that the for loop desugaring is not linted

@bluss
Copy link
Member

bluss commented Feb 21, 2017

That should be simple, the for loop uses IntoIterator::into_iter(<expr>) and this lint should apply exactly to where <expr>.into_iter() does not compile if it were replaced with the former (because of method resolution difference).

@clarfonthey
Copy link
Contributor

rust-lang/rust#49000 also makes it clear that this lint will also help us detect incorrect usages of into_iter() on slices, which currently forwards to iter() but would forward to a dedicated into_iter() if it were provided.

@cuviper
Copy link
Member

cuviper commented Apr 9, 2018

There are actually two ways that array.into_iter() can resolve to slice iteration:

  • using impl<'a, T> IntoIterator for &'a [T; $N], for $N in 0..=32 (possibly with autoref)
  • unsize to [T], then autoref using impl<'a, T> IntoIterator for &'a [T]
    • note: this is an explicit final step in probe.rs -- arrays don't actually implement Deref!

I think we only need to lint cases that would be intercepted by IntoIterator for [T; N]. Something like array_ref.into_iter() should have no problem to continue using IntoIterator for &'a [T; N]. But it should also be fine to suggest array_ref.iter() in that case anyway.

I was investigating how I might implement such a lint in the compiler, but a library-specific lint like this is probably not a great idea to hard-code. Doing it in clippy seems safer, even though it might not get as much reach. For rustc I was thinking that src/librustc_typeck/check/method/confirm.rs might be the right place to check the method resolution. Can you suggest where I might do similar in clippy?

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 9, 2018

I think the best place would be https://github.com/rust-lang-nursery/rust-clippy/blob/c5559c1648b0810e5dff9f94935fba52e9a9e833/clippy_lints/src/methods.rs#L691 which contains all our method call lints

@tamird
Copy link
Contributor

tamird commented Jun 6, 2018

@cuviper any plans to return to this? rust-lang/rust#49000 is quite useful, and seems blocked on this.

@cuviper
Copy link
Member

cuviper commented Jun 9, 2018

I do, yes. I managed to prototype the changes already -- I'll try to get back to clean that up and post a PR for comment soon.

@kennytm
Copy link
Member

kennytm commented Jun 14, 2018

I'm not sure if this has been taken into account by #1565 (comment)

If it is not too difficult to do, I suggest we also lint (&mut [1, 2, 3]).into_iter() to call [1, 2, 3].iter_mut().

Complete list
Type (&x).into_iter()x.iter() (&mut x).into_iter()x.iter_mut()
[T; n]
[T]
Option<T>
Result<T, E>
Vec<T>
BTreeSet<T>
BTreeMap<K, V>
VecDeque<T>
LinkedList<T>
LinkedList<T>
BinaryHeap<T>
HashMap<K, V>
HashSet<T>
PathBuf
Path
std::sync::mpsc::Receiver
std::os::unix::net::UnixListener incoming()

If we disregard the chance of false positive, we could even check:

  1. Explicit calls of x.into_iter() where into_iter comes from the trait IntoIterator, and

  2. the adjusted type of x is a reference (&T or &mut T or &&&&&T), and

  3. one of:

    • the method T::iter_mut exists and the adjusted type is &mut T (or &mut &mut &mut T), or
    • the method T::iter exists,

    and

  4. the return type of T::iter_mut/T::iter is equivalent to <&T as IntoIterator>::IntoIter, then

  5. We emit a lint suggesting to call x.iter_mut()/x.iter() instead.

@cuviper
Copy link
Member

cuviper commented Jul 10, 2018

Sorry I haven't found the time for this, but I just rebased my branch and pushed it as-is, in case somebody else wants to take over. I haven't gone through to see if I covered all the cases @kennytm mentioned. I'll try to be responsive about questions of what I was thinking, since I didn't get around to writing many comments either.
https://github.com/cuviper/rust-clippy/tree/array-into_iter

@cuviper
Copy link
Member

cuviper commented Jul 11, 2018

One thing in particular -- I did not cover so many collection types, just arrays and slices. My motivation was primarily linting to avoid collisions with a future impl IntoIterator for [T; N].

@kennytm
Copy link
Member

kennytm commented Jul 11, 2018

The generic procedure at the end should be able to cover all collections except std::os::unix::net::UnixListener.

@ishitatsuyuki
Copy link
Contributor

@kennytm I think your suggestion is different from the impl IntoIterator for array case.

Your suggestion is more like a style lint (which is like the lints on for loop), and none of them involves deref coercion or unsizing. The array part, however, involves coercion and thus is harder to implement than the one that doesn't.

I guess I'll only implement for the array case for now.

@kennytm
Copy link
Member

kennytm commented Jul 13, 2018

@ishitatsuyuki "adjusted type" means coercion + auto-reference/deref. Check the compiler docs for "adjustment".

@ishitatsuyuki
Copy link
Contributor

@kennytm I mean that the motivation/purpose of these two types of lints are different.

@kennytm
Copy link
Member

kennytm commented Jul 13, 2018

@ishitatsuyuki Yes, and I mean if you need to cover the collection types, it is better to generalize the lint than cover each one individually.

kennytm added a commit to kennytm/rust-clippy that referenced this issue Oct 20, 2018
kennytm added a commit to kennytm/rust-clippy that referenced this issue Oct 21, 2018
kennytm added a commit to kennytm/rust-clippy that referenced this issue Nov 2, 2018
kennytm added a commit to kennytm/rust-clippy that referenced this issue Nov 2, 2018
bors bot added a commit that referenced this issue 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 bors bot closed this as completed in #3344 Nov 2, 2018
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 5, 2020
Should fix clippy toolstat.

Changes:

````
rustup rust-lang/rust#55330
Update stderr
Rename test files
Also lint cfg_attr(.., rustfmt::skip)
Add tests from rustfmt::skip test file
Run update_lints.py script
Add test for non-crate-level inner attributes
Differ between inner and outer attributes
Add tests
Add cfg_attr(rustfmt) lint
Addressed comments.
Fix dogfood error.
Added lints `into_iter_on_ref` and `into_iter_on_array`. Fix rust-lang#1565.
Allow single_match_else
Update stderr
Add copyright statement©
Fix typos
Fix dogfood error
Fix typo and indentation
run update_lints script
Add tests for unknwon_clippy_lints lint
Add new lint: unknwon_clippy_lintsg
clippy: fix pedantic warnings and run clippy::pedantic lints on the codebase.
Fix a false-positive of needless_borrow
UI test cleanup: Extract match_overlapping_arm tests
UI test cleanup: Extract expect_fun_call tests
Add missing code of conduct file
Use slice patterns instead of padding
Fix dogfood and pedantic lints
ci: when installing rust-toolchain-installer-master, install it in debug mode to save some time in ci.
RIIR update lints: Generate deprecated lints
Replace big if/else expression with match
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

7 participants