clippy::iter_without_into_iter suggestion does not compile

[lopopolo]

Summary

The suggested implementation of IntoIterator does not compile.

Reproducer

I tried this code:

#![warn(clippy::all)]
#![warn(clippy::pedantic)]

pub struct Foo;

pub struct FooIter<'a>(Option<&'a Foo>);

impl<'a> Iterator for FooIter<'a> {
    type Item = &'a Foo;
    
    fn next(&mut self) -> Option<Self::Item> {
        self.0.take()
    }
}

impl Foo {
    #[must_use]
    pub fn iter(&self) -> FooIter<'_> {
        FooIter(Some(self))
    }
}


fn main() {
    println!("Hello, world!");
}

I expected to see this happen: clippy emits a clippy::iter_without_into_iter and gives a suggested fix that compiles.

Instead, this happened:

    Checking playground v0.0.1 (/playground)
warning: `iter` method without an `IntoIterator` impl for `&Foo`
  --> src/main.rs:18:5
   |
18 | /     pub fn iter(&self) -> FooIter<'_> {
19 | |         FooIter(Some(self))
20 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_without_into_iter
note: the lint level is defined here
  --> src/main.rs:2:9
   |
2  | #![warn(clippy::pedantic)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(clippy::iter_without_into_iter)]` implied by `#[warn(clippy::pedantic)]`
help: consider implementing `IntoIterator` for `&Foo`
   |
16 + 
17 + impl IntoIterator for &Foo {
18 +     type IntoIter = FooIter<'_>;
19 +     type Iter = &Foo;
20 +     fn into_iter() -> Self::IntoIter {
21 +         self.iter()
22 +     }
23 + }
   |

warning: `playground` (bin "playground") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s

Adding the suggestion results in this code:

#![warn(clippy::all)]
#![warn(clippy::pedantic)]

pub struct Foo;

pub struct FooIter<'a>(Option<&'a Foo>);

impl<'a> Iterator for FooIter<'a> {
    type Item = &'a Foo;
    
    fn next(&mut self) -> Option<Self::Item> {
        self.0.take()
    }
}

impl IntoIterator for &Foo {
    type IntoIter = FooIter<'_>;
    type Iter = &Foo;
    fn into_iter() -> Self::IntoIter {
        self.iter()
    }
}

impl Foo {
    #[must_use]
    pub fn iter(&self) -> FooIter<'_> {
        FooIter(Some(self))
    }
}


fn main() {
    println!("Hello, world!");
}

And these compilation errors:

   Compiling playground v0.0.1 (/playground)
error[E0637]: `'_` cannot be used here
  --> src/main.rs:17:29
   |
17 |     type IntoIter = FooIter<'_>;
   |                             ^^ `'_` is a reserved lifetime name

error[E0437]: type `Iter` is not a member of trait `IntoIterator`
  --> src/main.rs:18:5
   |
18 |     type Iter = &Foo;
   |     ^^^^^----^^^^^^^^
   |     |    |
   |     |    help: there is an associated type with a similar name: `Item`
   |     not a member of trait `IntoIterator`

error[E0637]: `&` without an explicit lifetime name cannot be used here
  --> src/main.rs:18:17
   |
18 |     type Iter = &Foo;
   |                 ^ explicit lifetime name needed here

error[E0424]: expected value, found module `self`
  --> src/main.rs:20:9
   |
19 |     fn into_iter() -> Self::IntoIter {
   |        --------- this function doesn't have a `self` parameter
20 |         self.iter()
   |         ^^^^ `self` value is a keyword only available in methods with a `self` parameter
   |
help: add a `self` receiver parameter to make the associated `fn` a method
   |
19 |     fn into_iter(&self) -> Self::IntoIter {
   |                  +++++

error[E0186]: method `into_iter` has a `self` declaration in the trait, but not in the impl
  --> src/main.rs:19:5
   |
19 |     fn into_iter() -> Self::IntoIter {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `self` in impl
   |
   = note: `into_iter` from trait: `fn(Self) -> <Self as IntoIterator>::IntoIter`

error[E0046]: not all trait items implemented, missing: `Item`
  --> src/main.rs:16:1
   |
16 | impl IntoIterator for &Foo {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `Item` in implementation
   |
   = help: implement the missing item: `type Item = /* Type */;`

Some errors have detailed explanations: E0046, E0186, E0424, E0437, E0637.
For more information about an error, try `rustc --explain E0046`.
error: could not compile `playground` (bin "playground") due to 6 previous errors

The suggested code is:

• missing a lifetime generic on the impl
• uses an anonymous lifetime in an impermissible location
• missing a named lifetime on the type Item member
• missing the self parameter in fn into_iter
• defining an Iter associated type that is not present on the IntoIterator trait
• missing an Item associated type

Version

Stable channel
Build using the Stable version: 1.73.0
clippy 0.1.75 (2023-10-19 4578435)

from the playground

Additional Labels

@rustbot label +I-suggestion-causes-error

[lopopolo]

in some non-toy examples in real code, it appears the suggested impl block doesn't define any generics i.e. both types and lifetimes.

[lopopolo]

Additionally, I had this lint trigger on a type in a crate that is not publicly reachable. In that case, the suggestion, which appears to be focused on guiding towards idiomatic public APIs that implement ubiquitous traits from core and std, seems misplaced and only contributing to code bloat and slower compile times.

[y21]

Yeah, the suggestion currently relies on the user adjusting the code a bit, and the applicability is correctly set to Unspecified here for this very reason.
I do agree though that a machine applicable solution would still be nice to have and some of these should have really been included in my PR (eg the self arg), that's my bad. If I find the time tomorrow, I'll look into this.

I will also say though that I was fully aware that the suggestion needs adjustments by the user (lifetimes, for example), and that's not really a problem imo so long as the applicability is correct and rustfix doesn't break your code but, again, a suggestion that compiles is always better, so an issue seems justified

[lopopolo]

thanks @y21. I don't use rust-analyzer, rustfix, or clippy --fix, so the concept of an unspecified applicability suggestion isn't one I'm familiar with.

Looking at the cargo clippy output doesn't make any distinction between a suggestion that is expected to compile vs one that isn't expect to, which is why I opened this issue.

thanks again for taking a look 🙇

[y21]

Looked into- and thought about this for a bit. I think it'd be quite tedious to make this a machine-applicable suggestion that compiles in any case, since our suggestions (except for a few) are built by crafting strings together. We can take snippets of source code from nodes in the HIR (and we're already doing this) and that usually works fine, but in this case it seems very error prone for a few reasons (though you've already mentioned a few of them):

• Lifetime elision doesn't work in associated types, but it does in return position
  • We'd need to translate '_ to some 'a that has the same lifetime as the implementor reference
    • Of course, that shouldn't shadow any lifetime already in scope. In case 'a is already used, this needs to use 'b, etc.
• This needs to take into account that there are multiple possible (lifetime) binders
  • The 'a in FooIter<'a> can come from fn iter<'a> or impl<'a>, both of which have subtly different meanings/implications
  • We'd need to merge the two places where generic types, consts and lifetimes are introduced into one place

I'm not certain that this covers everything, so I'd be more comfortable in just leaving the applicability where it is and continue relying on the user fixing these kinds of errors themselves.
One thing that I think can and should be made (will try to submit a PR for this), is that the self argument should be added and the Iter associated item should be renamed to Item.

I thought about taking a snippet of the relevant places from the Iterator impl, but I don't think we can do that since we only have access to the HIR of the current crate that is being checked by Clippy. So we can't look at Iterator impls for foreign types. That rules this option out.

Additionally, I had this lint trigger on a type in a crate that is not publicly reachable.

I assume you are saying that the type is not publicly reachable (and not that you have some kind of internal crate that isn't reachable from the outside, in which case I would say that it's fine to just #![allow] it in that internal crate).
But yeah, it sounds like a good idea to restrict this lint to only types that are exported.

[lopopolo]

@y21 thanks for your work on this. I wanted to followup that this lint helped me find a spot where I had IntoIterator impl'd on an &Struct thing where Struct had a generic parameter with a default value. This lint found the missing generic. Thank you!