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

Add `IntoIterator` impl for arrays by value (`for [T; N]`) #65819

Open
wants to merge 2 commits into
base: master
from

Conversation

@LukasKalbertodt
Copy link
Member

LukasKalbertodt commented Oct 25, 2019

Closes #25725

Initially part of #62959, this PR adds this impl:

impl<T, const N: usize> IntoIterator for [T; N]
where
    [T; N]: LengthAtMost32

Tests and some docs were adjusted. There are probably more places where docs need adjustment, but I postponed doing a proper search until we decided whether we want this. Which brings us to...


TODO

  • Run crater
  • Decide on how to proceed (especially regarding these regressions)
  • Fix root regressions
  • Potentially implement a compatibility lint (see #66017)
  • Adjust all relevant documentation (in this PR)
  • Add tests (in this PR)

The backwards compatibility problem

Adding this impl is not as straight-forward as it seems: there are some backwards compatibility hazards. In particular, due to autoref, this compiles today:

let array = [true; 3];
for x in array.into_iter() {
    let _ = *x; // x is a reference
}

With this change, that code wouldn't compile anymore, as x inside the loop would have the type bool and not &bool (like it does today). One should note that this only happens when using the .method call syntax with .into_iter(). It does not affect .iter() (different method) and it does not affect the for-loop syntax (does not involve autoref).

There has been some discussion in #49000 and in #62959. Most agree that a crater run would be very useful. That's what this PR is for. But the fact that this change didn't break anything in the compiler is already promising. (it did)

Arguments to add this impl despite the potential breakage:

  • It doesn't really make sense to call .into_iter(), as .iter() is shorter and the for loop desugars to into_iter() anyway. So hopefully no one used this in the real world. (people did use that in the real world)
  • RFC 1105 clearly specifies that "implementing any non-fundamental trait" is a "minor change". It also acknowledges that "implementing any existing trait can cause breakage", "However, as before, this kind of breakage is considered 'minor'".
  • @scottmcm wrote a comment that I (and apparently many others) completely agree with:

    My personal opinion: having this impl is so obviously the correct thing that I'd be willing to bend stability guarantees to have it, but we don't even need to because adding a new trait impl is an allowed change, no matter whether it breaks code. And the only code that it breaks, today, is code that was doing a.into_iter() when a.iter() would have done exactly the same thing for less typing, so any workaround needed to not trigger this change will make the code strictly better regardless.


Finally, if we want to add this impl, we need to decide if/what we do to limit the fallout of potential breakage, like adding a clippy lint or making this a built-in rustc warning.

CC @Centril @Mark-Simulacrum @cuviper

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 25, 2019

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Oct 25, 2019

Finally, if we want to add this impl, we need to decide if/what we do to limit the fallout of potential breakage, like adding a clippy lint or making this a built-in rustc warning.

The Clippy lint for this now exists, and I believe there was a crater run done with this impl too. We could also do another Crater run with this PR (the queue is empty right now).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 25, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-10-25T16:08:39.0930075Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-25T16:08:39.1152691Z ##[command]git config gc.auto 0
2019-10-25T16:08:39.1236209Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-25T16:08:39.1301257Z ##[command]git config --get-all http.proxy
2019-10-25T16:08:39.1452944Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65819/merge:refs/remotes/pull/65819/merge
---
2019-10-25T17:08:44.5564012Z .................................................................................................... 1600/9251
2019-10-25T17:08:50.2511175Z .................................................................................................... 1700/9251
2019-10-25T17:09:02.8269650Z ........................................................i...............i........................... 1800/9251
2019-10-25T17:09:10.4313493Z .................................................................................................... 1900/9251
2019-10-25T17:09:25.6518576Z ..............................................iiiii................................................. 2000/9251
2019-10-25T17:09:36.5052054Z .................................................................................................... 2200/9251
2019-10-25T17:09:39.0797158Z .................................................................................................... 2300/9251
2019-10-25T17:09:42.9956997Z .................................................................................................... 2400/9251
2019-10-25T17:10:06.1092397Z .................................................................................................... 2500/9251
---
2019-10-25T17:12:59.6538211Z .................................................i...............i.................................. 4800/9251
2019-10-25T17:13:08.7094825Z .................................................................................................... 4900/9251
2019-10-25T17:13:17.3151600Z .................................................................................................... 5000/9251
2019-10-25T17:13:23.6369622Z .................................................................................................... 5100/9251
2019-10-25T17:13:34.0180013Z ..................................................ii.ii...........i................................. 5200/9251
2019-10-25T17:13:43.8520642Z .................................................................................................... 5400/9251
2019-10-25T17:13:53.1465550Z .................................................................................................... 5500/9251
2019-10-25T17:14:01.0876503Z ....................i............................................................................... 5600/9251
2019-10-25T17:14:06.7785421Z .................................................................................................... 5700/9251
2019-10-25T17:14:06.7785421Z .................................................................................................... 5700/9251
2019-10-25T17:14:19.0161077Z .................................................................................................... 5800/9251
2019-10-25T17:14:30.1941961Z .................ii...i..ii...........i............................................................. 5900/9251
2019-10-25T17:14:52.0963618Z .................................................................................................... 6100/9251
2019-10-25T17:15:00.6647414Z .................................................................................................... 6200/9251
2019-10-25T17:15:00.6647414Z .................................................................................................... 6200/9251
2019-10-25T17:15:18.3466905Z ........................................i..ii....................................................... 6300/9251
2019-10-25T17:15:40.5878966Z .................................................................................................... 6500/9251
2019-10-25T17:15:42.7768238Z ......i............................................................................................. 6600/9251
2019-10-25T17:15:45.0184500Z .................................................................................i.................. 6700/9251
2019-10-25T17:15:47.7478050Z .................................................................................................... 6800/9251
---
2019-10-25T17:20:20.9768317Z  finished in 5.855
2019-10-25T17:20:20.9940752Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:20:21.1555520Z 
2019-10-25T17:20:21.1555748Z running 153 tests
2019-10-25T17:20:24.2934546Z i....iii......iii..iiii...i.............................i..i..................i....i...........ii.i. 100/153
2019-10-25T17:20:26.2404768Z .i.iiii..............i.........iii.i.........ii......
2019-10-25T17:20:26.2406705Z 
2019-10-25T17:20:26.2407929Z  finished in 5.246
2019-10-25T17:20:26.2647884Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:20:27.2336457Z 
---
2019-10-25T17:20:28.4669836Z  finished in 2.206
2019-10-25T17:20:28.4850648Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:20:28.6452250Z 
2019-10-25T17:20:28.6452690Z running 9 tests
2019-10-25T17:20:28.6453805Z iiiiiiiii
2019-10-25T17:20:28.6454336Z 
2019-10-25T17:20:28.6454374Z  finished in 0.160
2019-10-25T17:20:28.6647841Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:20:29.7344058Z 
---
2019-10-25T17:20:47.4021005Z  finished in 18.737
2019-10-25T17:20:47.4224274Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:20:47.5797524Z 
2019-10-25T17:20:47.5798270Z running 123 tests
2019-10-25T17:21:12.2987338Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....ii..........iiii..........i...ii...i.......ii. 100/123
2019-10-25T17:21:17.0330233Z i.i.i......iii.i.....ii
2019-10-25T17:21:17.0332830Z 
2019-10-25T17:21:17.0334358Z  finished in 29.611
2019-10-25T17:21:17.0346431Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:21:17.0347069Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-10-25T17:33:35.5310013Z 
2019-10-25T17:33:35.5320316Z    Doc-tests core
2019-10-25T17:33:40.6045157Z 
2019-10-25T17:33:40.6046164Z running 2407 tests
2019-10-25T17:33:51.8372051Z ......iiiii......................................................................................... 100/2407
2019-10-25T17:34:02.8126534Z ................................................................................ii.................. 200/2407
2019-10-25T17:34:28.6200617Z ..i................................................................................................. 400/2407
2019-10-25T17:34:28.6200617Z ..i................................................................................................. 400/2407
2019-10-25T17:34:39.3266125Z .................................................i..i.................iiii.......................... 500/2407
2019-10-25T17:34:59.8491940Z .................................................................................................... 700/2407
2019-10-25T17:35:10.3956953Z .................................................................................................... 800/2407
2019-10-25T17:35:20.9406938Z .................................................................................................... 900/2407
2019-10-25T17:35:31.4092079Z .................................................................................................... 1000/2407
---
2019-10-25T17:39:34.5794224Z .................................................................................................... 500/763
2019-10-25T17:39:34.5802106Z ....................thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1165:5
2019-10-25T17:39:34.5802829Z ....thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1165:5
2019-10-25T17:39:34.5803328Z thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: "SendError(..)"', src/libcore/result.rs:1165:5
2019-10-25T17:39:34.5804201Z .......thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1165:5
2019-10-25T17:39:34.7625983Z ..........................................thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1165:5
2019-10-25T17:39:34.7656420Z ....thread '..<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs.:.1165:5
2019-10-25T17:39:34.7680978Z ...thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1165:5
2019-10-25T17:39:36.8198603Z ......................thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/mutex.rs:629:13
2019-10-25T17:39:36.8201490Z ....thread '<unnamed>' panicked at 'test panic in inner thread to poison mutex', src/libstd/sync/mutex.rs:584:13
2019-10-25T17:39:36.8211979Z .thread '<unnamed>' panicked at 'test panic in inner thread to poison mutex', src/libstd/sync/mutex.rs:561:13
2019-10-25T17:39:36.8220089Z .thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/mutex.rs:689:13
---
2019-10-25T17:39:46.1412044Z 
2019-10-25T17:39:46.1417111Z running 995 tests
2019-10-25T17:40:07.3540590Z i................................................................................................... 100/995
2019-10-25T17:40:19.3210688Z .................................................................................................... 200/995
2019-10-25T17:40:27.7094277Z ...................iii......i......i...i......i..................................................... 300/995
2019-10-25T17:40:33.4349727Z .................................................................................................... 400/995
2019-10-25T17:40:41.3556019Z ........................................i..i.................................ii..................... 500/995
2019-10-25T17:40:56.4501747Z .................................................................................................... 700/995
2019-10-25T17:40:56.4501747Z .................................................................................................... 700/995
2019-10-25T17:41:04.5584533Z .....................iiii........................................................................... 800/995
2019-10-25T17:41:20.1484174Z .................................................................................................... 900/995
2019-10-25T17:41:27.9344646Z ...........................................iiii................................................
2019-10-25T17:41:27.9345938Z 
2019-10-25T17:41:27.9390118Z  finished in 196.599
2019-10-25T17:41:27.9406845Z Testing term stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-10-25T17:41:28.1581321Z    Compiling term v0.0.0 (/checkout/src/libterm)
---
2019-10-25T17:41:30.3484127Z    Compiling test v0.0.0 (/checkout/src/libtest)
2019-10-25T17:41:30.9927882Z error[E0614]: type `types::TestType` cannot be dereferenced
2019-10-25T17:41:30.9928269Z    --> src/libtest/tests.rs:273:49
2019-10-25T17:41:30.9928501Z     |
2019-10-25T17:41:30.9928872Z 273 |         let result = time_test_failure_template(*test_type);
2019-10-25T17:41:30.9929271Z 
2019-10-25T17:41:30.9974967Z error[E0614]: type `types::TestType` cannot be dereferenced
2019-10-25T17:41:30.9975254Z    --> src/libtest/tests.rs:324:41
2019-10-25T17:41:30.9975478Z     |
2019-10-25T17:41:30.9975478Z     |
2019-10-25T17:41:30.9975738Z 324 |         let test_desc = typed_test_desc(*test_type);
2019-10-25T17:41:30.9976026Z 
2019-10-25T17:41:30.9976269Z error[E0614]: type `u128` cannot be dereferenced
2019-10-25T17:41:30.9976489Z    --> src/libtest/tests.rs:325:40
2019-10-25T17:41:30.9976671Z     |
2019-10-25T17:41:30.9976671Z     |
2019-10-25T17:41:30.9976944Z 325 |         let exec_time = test_exec_time(*time as u64);
2019-10-25T17:41:30.9977931Z 
2019-10-25T17:41:30.9978217Z error[E0614]: type `bool` cannot be dereferenced
2019-10-25T17:41:30.9978480Z    --> src/libtest/tests.rs:327:61
2019-10-25T17:41:30.9979150Z     |
2019-10-25T17:41:30.9979150Z     |
2019-10-25T17:41:30.9979506Z 327 |         assert_eq!(options.is_warn(&test_desc, &exec_time), *expected_warn);
2019-10-25T17:41:30.9979882Z 
2019-10-25T17:41:30.9985454Z error[E0614]: type `bool` cannot be dereferenced
2019-10-25T17:41:30.9985724Z    --> src/libtest/tests.rs:328:65
2019-10-25T17:41:30.9985945Z     |
2019-10-25T17:41:30.9985945Z     |
2019-10-25T17:41:30.9986386Z 328 |         assert_eq!(options.is_critical(&test_desc, &exec_time), *expected_critical);
2019-10-25T17:41:30.9986780Z 
2019-10-25T17:41:31.0347719Z error: aborting due to 5 previous errors
2019-10-25T17:41:31.0347955Z 
2019-10-25T17:41:31.0348284Z For more information about this error, try `rustc --explain E0614`.
---
2019-10-25T17:41:31.0670323Z   local time: Fri Oct 25 17:41:31 UTC 2019
2019-10-25T17:41:31.3586469Z   network time: Fri, 25 Oct 2019 17:41:31 GMT
2019-10-25T17:41:31.3586602Z == end clock drift check ==
2019-10-25T17:41:32.0118257Z 
2019-10-25T17:41:32.0223680Z ##[error]Bash exited with code '1'.
2019-10-25T17:41:32.0260040Z ##[section]Starting: Checkout
2019-10-25T17:41:32.0262235Z ==============================================================================
2019-10-25T17:41:32.0262286Z Task         : Get sources
2019-10-25T17:41:32.0262344Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@LukasKalbertodt

This comment has been minimized.

Copy link
Member Author

LukasKalbertodt commented Oct 25, 2019

And I thought I ran every test locally... Anyway, we now have an example of code that would break due to this change:

rust/src/libtest/tests.rs

Lines 311 to 329 in 23f890f

let test_vector = [
(TestType::UnitTest, unit.warn.as_millis() - 1, false, false),
(TestType::UnitTest, unit.warn.as_millis(), true, false),
(TestType::UnitTest, unit.critical.as_millis(), true, true),
(TestType::IntegrationTest, integration.warn.as_millis() - 1, false, false),
(TestType::IntegrationTest, integration.warn.as_millis(), true, false),
(TestType::IntegrationTest, integration.critical.as_millis(), true, true),
(TestType::DocTest, doc.warn.as_millis() - 1, false, false),
(TestType::DocTest, doc.warn.as_millis(), true, false),
(TestType::DocTest, doc.critical.as_millis(), true, true),
];
for (test_type, time, expected_warn, expected_critical) in test_vector.into_iter() {
let test_desc = typed_test_desc(*test_type);
let exec_time = test_exec_time(*time as u64);
assert_eq!(options.is_warn(&test_desc, &exec_time), *expected_warn);
assert_eq!(options.is_critical(&test_desc, &exec_time), *expected_critical);
}

This piece of code is interesting because we can guess how this code came to be: the variable is called test_vector, so it likely started out as Vec. But at some point the author changed it to a simple array (or they just forgot the vec!). Then some errors were printed by the compiler, which were fixed by adding a bunch of * dereference operations. But the author didn't notice the wrong/strange into_iter().

Now we have to see how often this exists in the real world.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 25, 2019

Well, if the queue's empty let's get a check run to find things to go fix.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 25, 2019

⌛️ Trying commit d4d42a2 with merge b34fa46...

bors added a commit that referenced this pull request Oct 25, 2019
…r=<try>

Add `IntoIterator` impl for arrays by value (`for [T; N]`)

Closes #25725

Initially part of #62959, this PR adds this impl:

```rust
impl<T, const N: usize> IntoIterator for [T; N]
where
    [T; N]: LengthAtMost32
```

Tests and some docs were adjusted. There are probably more places where docs need adjustment, but I postponed doing a proper search until we decided whether we want this. Which brings us to...

---

## The backwards compatibility problem

Adding this impl is not as straight-forward as it seems: there are some backwards compatibility hazards. In particular, due to autoref, this compiles today:

```rust
let array = [true; 3];
for x in array.into_iter() {
    let _ = *x; // x is a reference
}
```

With this change, that code wouldn't compile anymore, as `x` inside the loop would have the type `bool` and not `&bool` (like it does today). One should note that this only happens when using the `.method` call syntax with `.into_iter()`. It does not affect `.iter()` (different method) and it does not affect the `for`-loop syntax (does not involve autoref).

There has been some discussion in #49000 and in #62959. Most agree that a crater run would be very useful. That's what this PR is for. But the fact that this change didn't break anything in the compiler is already promising.

### Arguments to add this `impl` despite the potential breakage:
- It doesn't really make sense to call `.into_iter()`, as `.iter()` is shorter and the `for` loop desugars to `into_iter()` anyway. So hopefully no one used this in the real world.
- [RFC 1105](https://github.com/nox/rust-rfcs/blob/master/text/1105-api-evolution.md) clearly specifies that "implementing any non-fundamental trait" is a "minor change". It also acknowledges that "implementing any existing trait can cause breakage", "However, as before, this kind of breakage is considered 'minor'".
- @scottmcm wrote [a comment](#62959 (comment)) that I (and apparently many others) completely agree with:
  > My personal opinion: having this impl is so obviously the correct thing that I'd be willing to bend stability guarantees to have it, but we don't even need to because adding a new trait impl is an allowed change, no matter whether it breaks code. And the only code that it breaks, today, is code that was doing `a.into_iter()` when `a.iter()` would have done exactly the same thing for less typing, so any workaround needed to not trigger this change will make the code strictly better regardless.

---

Finally, *if* we want to add this impl, we need to decide if/what we do to limit the fallout of potential breakage, like adding a clippy lint or making this a built-in rustc warning.

CC @Centril @Mark-Simulacrum @cuviper
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 26, 2019

☀️ Try build successful - checks-azure
Build commit: b34fa46 (b34fa46cc6fa5d46c6adcb041ebd28c7f7a374f3)

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Oct 26, 2019

@craterbot check

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Oct 26, 2019

👌 Experiment pr-65819 created and queued.
🤖 Automatically detected try build b34fa46
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Oct 28, 2019

🚧 Experiment pr-65819 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Oct 30, 2019

🎉 Experiment pr-65819 is completed!
📊 5360 regressed and 1 fixed (74234 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 30, 2019

Lots of regressions, though as a positive clippy is catching at least the first one I saw multiple times (such as https://crater-reports.s3.amazonaws.com/pr-65819/try%23b34fa46cc6fa5d46c6adcb041ebd28c7f7a374f3/reg/cargo-bundle-0.4.0/log.txt):

error: this .into_iter() call is equivalent to .iter() and will not move the array
   --> src\decoder.rs:235:47
    |
235 |                     for (i, &table) in tables.into_iter().enumerate() {
    |                                               ^^^^^^^^^ help: call directly: `iter`
    |
    = note: `#[deny(clippy::into_iter_on_array)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array

https://github.com/kaksmet/jpeg-decoder/blob/c8e25252/src/decoder.rs#L235
And submitted image-rs/jpeg-decoder#116 to fix it.

@hellow554

This comment has been minimized.

Copy link
Contributor

hellow554 commented Oct 30, 2019

Another issue one can see a lot: (e.g. here)

error[E0308]: mismatched types
  --> /opt/rustwide/cargo-home/registry/src/github.com-1ecc6299db9ec823/lazy_static-1.1.0/build.rs:13:19
   |
13 |         .filter(|&&f| f)
   |                   ^^
   |                   |
   |                   expected bool, found reference
   |                   help: you can probably remove the explicit borrow: `f`
   |
   = note: expected type `bool`
              found type `&_`

error: aborting due to previous error
lzutao added a commit to lzutao/ring that referenced this pull request Oct 30, 2019
See clippy::into_iter lint and rust-lang/rust#65819
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 8, 2019

[...], but disagree with his comment that a new method would be a better alternative.

Just to clarify a bit: I do agree that the best option would be to gradually transition and land this eventually without hacks (but if that's not agreeable, a method is the next best thing in my view, perhaps just as an unstable option for now during the transition, and to be removed later, if people want this...).

@apoelstra

This comment has been minimized.

Copy link
Contributor

apoelstra commented Nov 8, 2019

I think another important observation is that the fix (replace .into_iter() with .iter()) will continue to work with old compilers, and arguably the result is what the code always should have been. It is similar to .clone() sometimes copying references to non-Clone objects. So there is not a risk of forcing users to support either pre-1.42 (or whatever) or post-1.42. Nor is there a risk that users will be forced to write hacky/unergonomic things to retain support for old compilers. I think this makes this sort of breakage much more palatable.

Another observation is that autoref of arrays to slices has always been a (potential) source of unintended API breakage, not only in stdlib. So users' frustration is at least likely to be familiar frustration :).

Having said that, in general I place a very high value on Rust's stability and I'm glad to see that many others in this thread share this view. Comments to the effect that breakage is okay for breakages' sake, or as long as there is a several-month upgrade period, or as long as the crater-testable sources are fixed beforehand, really frustrate me. But looking at this specific breakage independently of these considerations, my judgement tends toward this one being acceptable.

@LukasKalbertodt LukasKalbertodt force-pushed the LukasKalbertodt:add-into-iterator-for-arrays branch from cadbd04 to 7d659c9 Nov 8, 2019
bors bot added a commit to fortanix/rust-sgx that referenced this pull request Nov 12, 2019
Merge #188
188: Use `slice::iter` instead of `into_iter` to avoid future breakage r=jethrogb a=LukasKalbertodt

`an_array.into_iter()` currently just works because of the autoref
feature, which then calls `<[T] as IntoIterator>::into_iter`. But
in the future, arrays will implement `IntoIterator`, too. In order
to avoid problems in the future, the call is replaced by `iter()`
which is shorter and more explicit.

A crater run showed that your crate is affected by a potential future 
change. See rust-lang/rust#65819 for more information.

Co-authored-by: Lukas Kalbertodt <lukas.kalbertodt@gmail.com>
@bstrie

This comment has been minimized.

Copy link
Contributor

bstrie commented Dec 7, 2019

It seems that we at least have a consensus that a rustc lint to flag this behavior would be desirable. Even in the absence of this PR, the problematic code in question is unidiomatic at best or a mild bug at worst. Let's consider that the next step, to be taken ASAP so that it can reach stable before planning for the proposed 2021 edition gets into full swing. Is anyone working on that yet?

@LukasKalbertodt

This comment has been minimized.

Copy link
Member Author

LukasKalbertodt commented Dec 7, 2019

@bstrie The lint is already implemented :)
See the PR and tracking issue.

@Ekleog

This comment has been minimized.

Copy link

Ekleog commented Dec 18, 2019

From what I can read (ie. ignoring the GitHub-hidden comments which make for too long a discussion to reasonably read), the side in favor of just having the impl is agreeing to a several-months forward-compatibility warning.

I am of a similar opinion, except for the time frame: I would say a year would be the minimum time to wait for Rust's stability to start being considered seriously. I've had comments about a Go user who did some Rust, and whose code was broken after six months of not-touching -- I don't know why, whether there were forward-compatibility warnings during that time, how bad was the fix, whether dependencies were involved or similar, but their comment is a fact. Rust, right now, doesn't have a great reputation for stability. And all code written in Rust is not actively maintained to see forward-compatibility warnings, some are just helper tools that are manually built once and then run, forgotten about until a change is needed months or years later.
(However, if having Rust's stability be taken seriously in the outside world is not an objective, then you can completely ignore this message, even though that would surprise me.)

With that said, if other people agree for a one-year forward-compatibility warning period… this brings us to at least early 2021. At which stage… why not just implement the method only in the 2021 edition? This way old editions remain perfectly stable, and people who want that method can have it approximately as soon as if it was implemented for older editions too. Is there really a need to have this method on 2015 & 2018? Is gaining a few months worth breakage for so many?

(Disclaimer: I do think that, apart for things that are blatant bugs, all breaking changes should wait for the next edition -- 3 years is not that many either, even in the worst case of a breaking change being wanted just after an edition. And yes, I think that at some point RFC 1105 will have to be re-litigated to raise the bar at least for the language and libstd -- especially now that we have editions planned for every 3 years)

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Dec 18, 2019

@Ekleog We don't have any way to guard library impls by edition yet, but even still, this idea is part of the discussion already.
(I find 6 hits on this page for "edition" before you, despite 115 hidden items by GitHub.)

@Ekleog

This comment has been minimized.

Copy link

Ekleog commented Dec 18, 2019

These 6 hits are for people saying they want to have the impl to be available on 2015 and 2018, but hidden for method resolution -- I did read all of the non-hidden messages, as mentioned in my message above. What I'm suggesting is to have crates on 2015 and 2018 just not see any into_iter method for arrays.

I don't know if it's more or less easy from a technical point of view than having it hidden (or if it's more or less likely to be reusable for other future trait impls), but wanted to put this option on the table. If it was already discussed somewhere in the hidden comments, then by all means please just dismiss my message above as an approval of that idea that didn't see the raised negative points, along with an anecdote about Rust's stability as viewed by one outsider :)

@Centril Centril modified the milestones: 1.41, 1.42 Dec 20, 2019
@LukasKalbertodt

This comment has been minimized.

Copy link
Member Author

LukasKalbertodt commented Dec 21, 2019

[...] only in the 2021 edition?
What I'm suggesting is to have crates on 2015 and 2018 just not see any into_iter method for arrays.

See this comment on why this would introduce strange cross-crate effects. Creating a language solution that would allow us to only have the impl in some editions seems to be fairly difficult (if not impossible).


General update regarding regressed crates

The two root regressions that caused the most downstream regressions (colored and jpeg-decoder) were fixed and the fix was released as minor version bump. Many more regressions were fixed by all of you who helped. Thanks a bunch for your work! However, many PRs are not accepted yet as some projects seem to be unmaintained unfortunately. Even worse, three of those unmaintained crates cause a whole bunch of downstream regressions. So that's not great. We have to see whether the crates will be maintained in the future again.

I quickly crunched some numbers to get a very rough estimate: I think we should have fixed roughly half of the 5360 regressions by now.

By checking the data, I noticed another mistake I did earlier in this thread. The 141 root regressions are only from the crates.io crates and not from the GitHub projects. As far as I see, 28916 crates from crates.io were tested. Thus, we have 144/28916 = 0.49% root regressions (previously we assumed 0.19%).

I also ran this tool on this crater report. Its report can be found here. Strangely, it comes up with only 60 root regressions (including github projects!).

How to proceed

The lint will reach stable in roughly 6 weeks. I think it should be on stable for several release cycles before we consider merging this. But as far as I understand, we cannot merge this PR without having stabilized const generics anyway. So we have to wait for quite some time regardless. Right?

My suggestion:

  • We close this PR as it's not particularly useful to have PRs linger around for many months or even years, right?
  • I will create a PR that adds an inherent method owned_iter (or so) to arrays so that nightly users can use the unstable iterator more easily.
  • As soon as const generics is stabilized, this PR can be reopened (or a new equivalent one can be created). By then, the lint has been on stable for long enough, presumably.

What do you think?

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 21, 2019

But as far as I understand, we cannot merge this PR without having stabilized const generics anyway.

You can, as long as you use [T; N]: LengthAtMost32.

@LukasKalbertodt

This comment has been minimized.

Copy link
Member Author

LukasKalbertodt commented Dec 21, 2019

@Centril My line of reasoning was: if we add this impl (with LengthAtMost32), we have to stabilize core::array::IntoIter (right?). Even if that iterator type also has the LengthAtMost32 bound, there would be const generics in the public API.

As far as I understand the LengthAtMost32 trick only works because the user cannot distinguish between an impl with LengthAtMost32 bound and 32 separate impls. But for a type in the public API it's different, right? If we were to remove const generics again, what would happen to IntoIter? We couldn't replace it without using const generics, I think.

Edit: also see this comment.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Dec 22, 2019

if we add this impl (with LengthAtMost32), we have to stabilize core::array::IntoIter (right?).

I don't think so -- there are plenty of stable APIs that rely on unstable details, and you can still use them without naming them -- Try for instance. In this case, a stable user couldn't name IntoIter as a type, but they could still create and use one via IntoIterator.

If we were to remove const generics again, what would happen to IntoIter?

We could fallback to an implementation like my #49000, as long as IntoIter is still unstable. But that would be stuck in unstable limbo until we get const-generics back, or until we accept a less satisfactory type signature to stabilize, so that's not great...

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 22, 2019

I’d be really uncomfortable with a stable API that returns an unstable type. It means that programs on the Stable channel can manipulate values of that type, making it effectively stable in some ways.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Dec 22, 2019

We would be saying that the type has to exist with such functionality (almost like impl Trait if it were allowed in this position), but we wouldn't be committing to its exact generic type signature. IMO that's acceptable, but on the other hand I'm still feeling more conservative about inflicting breakage from .into_iter() method resolution.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 23, 2019

If we're going to add inherent methods first, we can use impl trait for those and expose nothing unstable.

Unfortunately there are a lot of possible next steps and the value of each depends on how we weight different things.

  • We can add inherent methods ASAP with impl trait signatures and a different name from into_iter. We don't have a convention for what this name would be (I prefer iter_by_val to iter_owned but w/e).
  • We can wait until a deprecation period has passed and add these inherent methods with into_iter as the name.
  • We can at that point add the trait impls with the 32 limit returning an unstable associated type, possibly not having the inherent methods until then.
  • We can not add the trait impl then and wait for const generics to stable to add it with no bound.

Personally I'm in favor of adding the trait impl ASAP, so that users have the maximum functionality ASAP (I'm also in favor of removing the array limit to 32 sooner than full const generics are stabilized, but thats a separate conversation).

The inherent methods are more of a trade off to me - how many releases will the deprecation period be? If we're talking about say 3 or 4 releases, it seems hard to say its worth adding weird inherent methods for such a short period. But if we're talking about a longer period, it seems more worthwhile.

@goffrie

This comment has been minimized.

Copy link
Contributor

goffrie commented Dec 23, 2019

const generics in the public API

Could array::IntoIter be changed to IntoIter<[T; N]> instead of IntoIter<T, {N}>? Though it would leave a permanent wart in the API.

briansmith added a commit to briansmith/ring that referenced this pull request Dec 23, 2019
See clippy::into_iter lint and rust-lang/rust#65819
bors bot added a commit to jmesmon/rust-systemd that referenced this pull request Dec 23, 2019
Merge #84
84: Use `slice::iter` instead of `into_iter` to avoid future breakage r=jmesmon a=starfys

Use `slice::iter` instead of `into_iter` to avoid future breakage

`an_array.into_iter()` currently just works because of the autoref
feature, which then calls `<[T] as IntoIterator>::into_iter`. But
in the future, arrays will implement `IntoIterator`, too. In order
to avoid problems in the future, the call is replaced by `iter()`
which is shorter and more explicit.

A crater run showed that your crate is affected by a potential future 
change. See rust-lang/rust#65819 for more information.


Co-authored-by: Steven Sheffey <stevensheffey4@gmail.com>
@LukasKalbertodt

This comment has been minimized.

Copy link
Member Author

LukasKalbertodt commented Jan 26, 2020

Next week, the lint linting array.into_iter() will hit stable. Since the initial crater run, we've been busy fixing crates. Most of the crates causing many regressions have released a fixed minor version. So now is a good time to recheck all regressed crates:

@craterbot run mode=check-only crates=https://gist.githubusercontent.com/LukasKalbertodt/1362aeb0eff39e846170b226afa4c0a4/raw/800a2b62f99596c441641b6a4591078325d8a134/regressed-crates.txt

Note that this new run uses the newest versions of all crates and their dependencies. That's why I expect a (hopefully notable) improvement. (also, no bors try as we can simply use the last build)

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 26, 2020

🚨 Error: missing desired crates: {"rust-cgui-render", "rust-cgui", "stinkage", "izanami"}

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@LukasKalbertodt

This comment has been minimized.

Copy link
Member Author

LukasKalbertodt commented Jan 26, 2020

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 26, 2020

👌 Experiment pr-65819 created and queued.
🤖 Automatically detected try build b34fa46
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.