Add IntoIterator impl for arrays by value (for [T; N])
#65819
Conversation
|
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
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). |
|
The job Click to expand the log.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 |
|
And I thought I ran every test locally... Anyway, we now have an example of code that would break due to this change: Lines 311 to 329 in 23f890f This piece of code is interesting because we can guess how this code came to be: the variable is called Now we have to see how often this exists in the real world. |
|
Well, if the queue's empty let's get a check run to find things to go fix. @bors try |
|
⌛ Trying commit d4d42a228236bcfea946bb8a85a72a9ed6cb7772 with merge b34fa46cc6fa5d46c6adcb041ebd28c7f7a374f3... |
|
☀️ Try build successful - checks-azure |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
|
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_arrayhttps://github.com/kaksmet/jpeg-decoder/blob/c8e25252/src/decoder.rs#L235 |
|
Another issue one can see a lot: (e.g. here) |
See clippy::into_iter lint and rust-lang/rust#65819
The existing lint already offers that fix. I don't know what you mean about different types, because it returns |
|
Oh that's great then. The only case where a blind transformation on eligible |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
I'm not sure how could that be an issue considering |
You're in good company to think it works like that (#84147 (comment)), but no, method resolution tries to use the value behind the dereference first, as this playground shows. I don't think this needs to block #84147, but we could file an issue for 2021 to expand the scope of the |
|
BTW @LukasKalbertodt I did cherry-pick two of your commits from this PR into #84147, but not:
|
|
@xfix Here's an example of the condition I was talking about (playground). Pretend that |
|
🎉 Experiment
|
|
I didn't have a lot of time to analyze this latest report, but from the looks of it, not a ton has changed compared to the previous run.
And just like the report from August 2020, it looks like the vast majority of regressions is caused by a few regressed dependencies. Almost all of which have already released a fix, usually as a minor version bump. Thus, most of these broken projects are just one What this shows, I think, is that there are just a lot of projects that are not actively maintained anymore. E.g. one-time hobby projects. We cannot expect those to ever get updated and it would be a shame if those projects wouldn't compile anymore in the future. That's why I'm really glad we have #84147 now which avoids this breakage altogether. I think it's a bit unfortunate that this kind of method resolution trick was only properly considered/investigated a few weeks ago. If we had known about this earlier (and how comparatively easy it is to implement), this could have saved quite a bit of work. But overall I'm just glad we have this solution now so that we can have the impl without breaking the world. So thanks everyone involved in that! Finally, I guess I can close this PR now? (After 1.5 years :D) |
…matsakis,m-ou-se Cautiously add IntoIterator for arrays by value Add the attribute described in rust-lang#84133, `#[rustc_skip_array_during_method_dispatch]`, which effectively hides a trait from method dispatch when the receiver type is an array. Then cherry-pick `IntoIterator for [T; N]` from rust-lang#65819 and gate it with that attribute. Arrays can now be used as `IntoIterator` normally, but `array.into_iter()` has edition-dependent behavior, returning `slice::Iter` for 2015 and 2018 editions, or `array::IntoIter` for 2021 and later. r? `@nikomatsakis` cc `@LukasKalbertodt` `@rust-lang/libs`
|
@LukasKalbertodt Thanks a lot for all the work and effort you put into this! Thanks to your work, such as the lint you added, the analysis of the crater runs, the implementation of |
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
Point out that behavior might be switched on 2015 and 2018 too one day Reword documentation to make it clear that behaviour can be switched on older editions too, one day in the future. It doesn't *have* to be switched, but I think it's good to have it as an option and re-evaluate it a few months/years down the line when e.g. the crates that showed up in crater were broken by different changes in the language already. cc rust-lang#25725, rust-lang#65819, rust-lang#66145, rust-lang#84147 , and rust-lang#84133 (comment)
|
It seems like there are slight differences between this and the previous This will work: use itertools::Itertools; // 0.10.1
fn main() {
let a = [1u8, 2, 3];
let b: (u64, u64, u64) = std::array::IntoIter::new(a).map(Into::into).next_tuple().unwrap();
}But this won't: use itertools::Itertools; // 0.10.1
fn main() {
let a = [1u8, 2, 3];
let b: (u64, u64, u64) = a.into_iter().map(Into::into).next_tuple().unwrap();
}With error:
it looks like |
|
@mimoo That is very much intentional to avoid significant breakage. It will behave the way you're expecting in Rust 2021, which will be released tomorrow. |
Current status (2020-12-30):
Const generics are stable now, so this PR is unblocked. The main remaining question is how to deal with the regressions caused by this change (see this comment). In the meantime, you can use
IntoIterator::newwhich has been stabilized independently.Status update comments:
IntoIteratorimpl for arrays by value (for [T; N]) #65819 (comment)IntoIteratorimpl for arrays by value (for [T; N]) #65819 (comment)IntoIteratorimpl for arrays by value (for [T; N]) #65819 (comment)Closes #25725
Initially part of #62959, this PR adds this impl:
TODO
array.into_iter()#66017)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:
With this change, that code wouldn't compile anymore, as
xinside the loop would have the typebooland not&bool(like it does today). One should note that this only happens when using the.methodcall syntax with.into_iter(). It does not affect.iter()(different method) and it does not affect thefor-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
impldespite the potential breakage:.into_iter(), as.iter()is shorter and theforloop desugars tointo_iter()anyway.So hopefully no one used this in the real world.(people did use that in the real world)CC @Centril @Mark-Simulacrum @cuviper