WIP: IntoIterator for Box<[T]> + method dispatch mitigation for editions < 2024 #116607
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
clarfonthey
changed the title
|
It appears that Clippy is explicitly testing for this, so, I'll have to take a look at that. But it should at least be ready for a Crater run if we're looking to just judge the impact of the initial impl. |
|
@bors try |
IntoIterator for Box<[T]> ACP: rust-lang/libs-team#263 Recommendation per ACP: this should receive a crater run to gauge impact. If there's no impact, it can be merged as-is, but otherwise it will need a similar edition-based workaround to the array implementation. In addition to what was proposed by the ACP, this also adds `IntoIterator for &Box<[T]>` and `IntoIterator for &mut Box<[T]>` to ensure that those work as expected. I also already had to change at least one line in the compiler to account for this change, which isn't a good sign toward whether edition-specific mitigations may be needed, but we'll see.
| } | ||
|
|
||
| #[stable(feature = "boxed_slice_into_iter", since = "CURRENT_RUSTC_VERSION")] | ||
| impl<'a, I, A: Allocator> !Iterator for &'a Box<[I], A> {} |
There was a problem hiding this comment.
why is this needed? is the !Iterator impl for [_] not enough?
There was a problem hiding this comment.
Unfortunately not. :(
There was a problem hiding this comment.
A comment here for why they're needed would be very 🆒
|
☀️ Try build successful - checks-actions |
clarfonthey
force-pushed
the
box-into-iter
branch
from
b4c7843
to
f31acaa
Compare
This comment has been minimized.
This comment has been minimized.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
I'm not 100% clear on the results (it looks like more than 9 root crates are actually failing), but it does seem to me like some sort of edition mechanism would be good to include. Is that the kind of thing that should have an RFC, or would it be reasonable to say that the ACP is enough? |
Mark-Simulacrum
added
S-waiting-on-author
|
After taking a look at what was done for 2021 edition, it appears that there wasn't any particular RFC for it, just, a PR was made and that was the solution. So, I'm going to follow suit and start work on the override, hopefully not having to tread too deeply in compiler internals. After a quick search, it shouldn't be too difficult to modify the The only major questions I have here are:
EDIT: I'm going to push the code I have right now even though I know it's not done, to give an idea of what I've changed so far. Will mark the PR accordingly once it's finished. |
clarfonthey
changed the title
clarfonthey
force-pushed
the
box-into-iter
branch
from
f31acaa
to
7768ce5
Compare
|
Will try and return to this PR this week. I would like to see this in the 2024 edition. |
clarfonthey
force-pushed
the
box-into-iter
branch
from
7768ce5
to
761974c
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
(did a very crude rebase, and the previous code wasn't even working, so. gonna poke at this more later) |
| // so those calls will still resolve to the slice implementation, by reference. | ||
| #[stable(feature = "boxed_slice_into_iter", since = "CURRENT_RUSTC_VERSION")] | ||
| impl<I, A: Allocator> IntoIterator for Box<[I], A> { | ||
| type IntoIter = vec::IntoIter<I, A>; |
There was a problem hiding this comment.
I wonder, should this really be publically the same type? (can't be changed later)
There was a problem hiding this comment.
It's hard to imagine why we would want something different.
vec::IntoIter is
- ptr + cap + alloc (needed to free allocation)
- ptr to first not yielded element
- ptr one past the last not yielded element
You need all the same things for a box. Since the size of initialized elements changes when iterating, there is no difference with a vec -- you need to save the allocation and you need to save the initialized range in it.
There was a problem hiding this comment.
Also worth pointing out that not making them the same type is also something we can't revert after the fact, so, it feels like we should just pick the logical option here and make them the same.
|
Should we also do this for |
|
I feel like That one I feel like we could potentially copy |
the8472
added
the
T-lang
|
We discussed this in today's libs-api meeting. We'd be happy to see this in the next edition. We didn't think it is worth making separate IntoIter types, and would prefer simply reusing Vec's IntoIter (for both box-of-slice and box-of-array). Keeping the |
|
Marking as T-lang since it affects method dispatch. |
|
I also have been meaning to get back to this but kept running into unrelated life issues. Will hopefully be able to do a proper dive soon-- the deadline to get an initial implementation in two weeks seems pretty doable; my plan, as demonstrated by the existing mess, is to just piggyback off the 2021 edition implementation. |
tmandry
added
T-libs-api
|
Okay, I thought about it and I simply cannot justify putting in this much work with this little help given the other things I have going on in life. Would love to see this in edition 2024, and I'm sure there are folks knowledgeable in this code who could pick up the slack, but if it's pushed to a future edition, that's fine too. I don't want this enough to abandon other priorities at the moment, especially as I'm unemployed and this effort means nothing to any job I apply to. Apologies but also, not sorry for my decision. Will close this and folks can decide what to do. |
|
I'm sorry to hear that @clarfonthey. I'll make sure to pick this up for the edition, and make sure to keep the commits with you as the author, unless you don't want to -- ping me on Zulip if so. Thanks for all the effort you put into this up til now! |
|
Whether you keep me on the commits doesn't really matter much, since these records still exist, although I do appreciate the gesture. |
…=<try> [crate] Add `Box<[T; N]>: IntoIterator` without any method dispatch hacks **Unlike** `Box<[T]>` (rust-lang#116607 (comment)), there's a much higher chance that this will not conflict with existing usages since it produces an iterator with the same type before/after this change, but let's test that theory with crater. Ideally we have fewer migrations that are tangled up in hacks like `rustc_skip_during_method_dispatch`, so if this crater comes back clean, I'd strongly suggest landing this as-is. As for the rationale for having this impl at all, I agree (as `@clarfonthey` pointed out in rust-lang#124097 (comment)) that it is generally better for any user to not require moving the array *out* of the box just to turn it into an iterator.
…fleLapkin Add `IntoIterator` for `Box<[T]>` + edition 2024-specific lints * Adds a similar method probe opt-out mechanism to the `[T;N]: IntoIterator` implementation for edition 2021. * Adjusts the relevant lints (shadowed `.into_iter()` calls, new source of method ambiguity). * Adds some tests. * Took the liberty to rework the logic in the `ARRAY_INTO_ITER` lint, since it was kind of confusing. Based mostly off of rust-lang#116607. ACP: rust-lang/libs-team#263 References rust-lang#59878 Tracking for Rust 2024: rust-lang#123759 Crater run was done here: rust-lang#116607 (comment) Consensus afaict was that there is too much breakage, so let's do this in an edition-dependent way much like `[T; N]: IntoIterator`.
Add `IntoIterator` for `Box<[T]>` + edition 2024-specific lints * Adds a similar method probe opt-out mechanism to the `[T;N]: IntoIterator` implementation for edition 2021. * Adjusts the relevant lints (shadowed `.into_iter()` calls, new source of method ambiguity). * Adds some tests. * Took the liberty to rework the logic in the `ARRAY_INTO_ITER` lint, since it was kind of confusing. Based mostly off of #116607. ACP: rust-lang/libs-team#263 References #59878 Tracking for Rust 2024: rust-lang/rust#123759 Crater run was done here: rust-lang/rust#116607 (comment) Consensus afaict was that there is too much breakage, so let's do this in an edition-dependent way much like `[T; N]: IntoIterator`.
Add `IntoIterator` for `Box<[T]>` + edition 2024-specific lints * Adds a similar method probe opt-out mechanism to the `[T;N]: IntoIterator` implementation for edition 2021. * Adjusts the relevant lints (shadowed `.into_iter()` calls, new source of method ambiguity). * Adds some tests. * Took the liberty to rework the logic in the `ARRAY_INTO_ITER` lint, since it was kind of confusing. Based mostly off of #116607. ACP: rust-lang/libs-team#263 References #59878 Tracking for Rust 2024: rust-lang/rust#123759 Crater run was done here: rust-lang/rust#116607 (comment) Consensus afaict was that there is too much breakage, so let's do this in an edition-dependent way much like `[T; N]: IntoIterator`.
ACP: rust-lang/libs-team#263
References #59878
Recommendation per ACP: this should receive a crater run to gauge impact. If there's no impact, it can be merged as-is, but otherwise it will need a similar edition-based workaround to the array implementation.
In addition to what was proposed by the ACP, this also adds
IntoIterator for &Box<[T]>andIntoIterator for &mut Box<[T]>to ensure that those work as expected. I also already had to change at least one line in the compiler to account for this change, which isn't a good sign toward whether edition-specific mitigations may be needed, but we'll see.