Skip to content

Remove Iterator #[rustc_on_unimplemented]s that no longer apply. #85689

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 27, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented May 25, 2021

Now that IntoIterator is implemented for arrays, all the rustc_on_unimplemented for arrays of ranges (e.g. for _ in [1..3] {}) no longer apply, since they are now valid Rust.

Separated these from #85670, because we should discuss a potential new (clippy?) lint for these.

Until Rust 1.52, for _ in [1..3] {} produced:

error[E0277]: `[std::ops::Range<{integer}>; 1]` is not an iterator
 --> src/main.rs:2:14
  |
2 |     for _ in [1..3] {}
  |              ^^^^^^ if you meant to iterate between two values, remove the square brackets
  |
  = help: the trait `std::iter::Iterator` is not implemented for `[std::ops::Range<{integer}>; 1]`
  = note: `[start..end]` is an array of one `Range`; you might have meant to have a `Range` without the brackets: `start..end`
  = note: required by `std::iter::IntoIterator::into_iter`

But in Rust 1.53 and later, it compiles fine. It iterates over the array by value, for one iteration with the element 1..3.

This is probably a mistake, which is no longer caught. Should we have a lint for it? Should Clippy have a lint for it?

cc @estebank @flip1995

cc #84513

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2021
@m-ou-se m-ou-se added A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 25, 2021
@flip1995
Copy link
Member

This looks like a Clippy lint to me. I can't see many situations, where you can accidentally use the range inside the for loop without getting a type error if you meant to use an integer/the range item.

@CDirkx
Copy link
Contributor

CDirkx commented May 25, 2021

I can't see many situations, where you can accidentally use the range inside the for loop without getting a type error if you meant to use an integer/the range item.

The result doesn't have to be used, iterating over an integer range without using the value is a pretty common way to do fixed sized loops:

fn do_x_16_times() {
    for _ in [0..16] {
        x(); // will actually do x() only once
    }
}

@estebank
Copy link
Contributor

Don't we need to keep this for 201x editions?


Yes, I think we should have a lint in clippy, if not in rustc because of how problematic this mistake could be.

@jyn514
Copy link
Member

jyn514 commented May 26, 2021

Don't we need to keep this for 201x editions?

@estebank arrays implement IntoIterator on all editions. 2021 only affects the method resolution of the into_iter() call. So this is dead code no matter what.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 26, 2021

📌 Commit caf6faf has been approved by estebank

@bors
Copy link
Collaborator

bors commented May 26, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 26, 2021
@cuviper
Copy link
Member

cuviper commented May 26, 2021

This is probably a mistake, which is no longer caught. Should we have a lint for it? Should Clippy have a lint for it?

See rust-lang/rust-clippy#7125

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 26, 2021
Remove Iterator #[rustc_on_unimplemented]s that no longer apply.

Now that `IntoIterator` is implemented for arrays, all the `rustc_on_unimplemented` for arrays of ranges (e.g. `for _ in [1..3] {}`) no longer apply, since they are now valid Rust.

Separated these from rust-lang#85670, because we should discuss a potential new (clippy?) lint for these.

Until Rust 1.52, `for _ in [1..3] {}` produced:

```
error[E0277]: `[std::ops::Range<{integer}>; 1]` is not an iterator
 --> src/main.rs:2:14
  |
2 |     for _ in [1..3] {}
  |              ^^^^^^ if you meant to iterate between two values, remove the square brackets
  |
  = help: the trait `std::iter::Iterator` is not implemented for `[std::ops::Range<{integer}>; 1]`
  = note: `[start..end]` is an array of one `Range`; you might have meant to have a `Range` without the brackets: `start..end`
  = note: required by `std::iter::IntoIterator::into_iter`
```

But in Rust 1.53 and later, it compiles fine. It iterates over the array by value, for one iteration with the element `1..3`.

This is probably a mistake, which is no longer caught. Should we have a lint for it? Should Clippy have a lint for it?

cc `@estebank` `@flip1995`

cc rust-lang#84513
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 27, 2021
Remove Iterator #[rustc_on_unimplemented]s that no longer apply.

Now that `IntoIterator` is implemented for arrays, all the `rustc_on_unimplemented` for arrays of ranges (e.g. `for _ in [1..3] {}`) no longer apply, since they are now valid Rust.

Separated these from rust-lang#85670, because we should discuss a potential new (clippy?) lint for these.

Until Rust 1.52, `for _ in [1..3] {}` produced:

```
error[E0277]: `[std::ops::Range<{integer}>; 1]` is not an iterator
 --> src/main.rs:2:14
  |
2 |     for _ in [1..3] {}
  |              ^^^^^^ if you meant to iterate between two values, remove the square brackets
  |
  = help: the trait `std::iter::Iterator` is not implemented for `[std::ops::Range<{integer}>; 1]`
  = note: `[start..end]` is an array of one `Range`; you might have meant to have a `Range` without the brackets: `start..end`
  = note: required by `std::iter::IntoIterator::into_iter`
```

But in Rust 1.53 and later, it compiles fine. It iterates over the array by value, for one iteration with the element `1..3`.

This is probably a mistake, which is no longer caught. Should we have a lint for it? Should Clippy have a lint for it?

cc ``@estebank`` ``@flip1995``

cc rust-lang#84513
This was referenced May 27, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request May 27, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#84221 (E0599 suggestions and elision of generic argument if no canditate is found)
 - rust-lang#84701 (stabilize member constraints)
 - rust-lang#85564 ( readd capture disjoint fields gate)
 - rust-lang#85583 (Get rid of PreviousDepGraph.)
 - rust-lang#85649 (Update cc)
 - rust-lang#85689 (Remove Iterator #[rustc_on_unimplemented]s that no longer apply.)
 - rust-lang#85719 (Add inline attr to CString::into_inner so it can optimize out NonNull checks)
 - rust-lang#85725 (Remove unneeded workaround)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit de1d7db into rust-lang:master May 27, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 27, 2021
@m-ou-se m-ou-se deleted the array-intoiter-3 branch May 27, 2021 18:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants