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

Detect when user tries to write Iterator that incorrectly borrows from Self and would need GATs #125337

Closed
estebank opened this issue May 20, 2024 · 5 comments · Fixed by #125407
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators A-lifetimes Area: lifetime related D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

Code

struct Data {
    v: Vec<i32>,
}

impl Iterator for Data {
    type Item = &[i32];
    fn next(&mut self) -> Option<Self::Item> {
        let mut a = 0;
        let mut b = 0;
        Some(&self.v[a..b])
    }
}

fn main() {}

Current output

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

Desired output

error[E0637]: `&` without an explicit lifetime name cannot be used here
 --> src/main.rs:6:17
  |
6 |     type Item = &[i32];
  |                 ^ explicit lifetime name needed here
note: associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type, so `Data` should be declared with a lifetime
note: you can't create an `Iterator` that borrows each `Item` from itself, as it doesn't use Generic Associated Types, but you can instead create a new type that borrows `Data` and implement `Iterator` for that type: <LINK TO EXAMPLE>

Rationale and extra context

No response

Other cases

No response

Rust Version

1.78.0

Anything else?

Taken from https://users.rust-lang.org/t/lifetime-error-from-iterator-with-its-item-as-a-reference/108863

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-iterators Area: Iterators D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels May 20, 2024
@pacak
Copy link
Contributor

pacak commented May 21, 2024

I'll give it a go. So far managed to implement something that detects this condition.

@estebank
Copy link
Contributor Author

@pacak great!

I wonder if we should make the first note something like

note: associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
   |
LL | impl Iterator for Data {
   |      --------     ---- this type doesn't have a lifetime parameter in its definition, but you could change it to have one
   |      |
   |      this trait doesn't have a lifetime parameter in its definition
LL |     type Item = &[i32];
   |          ^^^^ this associated type doesn't have a lifetime parameter in the trait definition

(that can be done by creating a MultiSpan and doing push_span_label on it, before feeding it to the span_note method)

You'd have to check whether Iterator or Data (or both) are local and then extend the label telling people "but you could change it". Furthermore, if both Iterator and Data are not local, maybe we shouldn't even emit the error and instead let the other error be the only one.

@pacak
Copy link
Contributor

pacak commented May 21, 2024

Check I made is basically if there's a problem with an anonymous lifetime and we are writing an impl core::iter::traits::iterator::Iterator for some datatype and we are "not in function" then report this error. trait rules ensures that Data is going to be local.

Basically works right now, remaining issues are cosmetic - need to figure out the easiest way to go from ast Ty to a textual representation, but if I reword things according to your last message - this is no longer a problem.

Will try to make a pull request tomorrow or later today.

@estebank
Copy link
Contributor Author

I think there's an opportunity here for two separate improvements. The one you're making (explicit help for Iterator) and one for the general case (every other trait).

@pacak
Copy link
Contributor

pacak commented May 21, 2024

Yea, I was thinking about a generic version as well - looking up how associated datatype is defined in the trait and complaining. That's going to be later.

@bors bors closed this as completed in 49f9504 Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 5, 2024
Rollup merge of rust-lang#125407 - pacak:no-lending-iterators, r=pnkfelix

Detect when user is trying to create a lending `Iterator` and give a custom explanation

The scope for this diagnostic is to detect lending iterators specifically and it's main goal is to help beginners to understand that what they are trying to implement might not be possible for `Iterator` trait specifically.

I ended up to changing the wording from originally proposed in the ticket because it might be misleading otherwise: `Data` might have a lifetime parameter but it can be unrelated to items user is planning to return.

Fixes rust-lang#125337
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators A-lifetimes Area: lifetime related D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants