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 Box<[T; N]>: IntoIterator without any method dispatch hacks #124108

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Apr 18, 2024

Unlike Box<[T]> (#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. Any breakage would need to be relying specifically on the expr Box::([...]).into_iter() yielding an iterator specifically of type array::IntoIter<T; N> as it currently does.

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 #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.

@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2024

r? @Nilstrieb

rustbot has assigned @Nilstrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 18, 2024
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 18, 2024
@compiler-errors
Copy link
Member Author

why did i rust-timer this -- i plan to use crater lol

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 18, 2024
…=<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.
@bors
Copy link
Contributor

bors commented Apr 18, 2024

⌛ Trying commit 78d9b1e with merge 2e5b773...

@compiler-errors
Copy link
Member Author

Well -- I guess Box<[T; N]> technically dispatches to the &[T] impl on edition < 2021 because we skip the [T; N] impl. Anyways, let's see what crater says.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 18, 2024

☀️ Try build successful - checks-actions
Build commit: 2e5b773 (2e5b7739e5167b731ad5eb628f04a54f932f1fe2)

@rust-timer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-124108 created and queued.
🤖 Automatically detected try build 2e5b773
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 18, 2024
@rust-timer

This comment was marked as off-topic.

@craterbot
Copy link
Collaborator

🚧 Experiment pr-124108 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-124108 is completed!
📊 4 regressed and 2 fixed (439535 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

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 22, 2024
@compiler-errors
Copy link
Member Author

compiler-errors commented Apr 22, 2024

The only regression is derive-visitor 0.3.0:

[INFO] [stdout]  --> tests/test_containers.rs:8:19
[INFO] [stdout]   |
[INFO] [stdout] 8 | #[derive(Default, Drive, DriveMut)]
[INFO] [stdout]   |                   ^^^^^
[INFO] [stdout]   |
[INFO] [stdout]   = note: multiple `impl`s satisfying `Box<[CountMe1; 5]>: Drive` found in the `derive_visitor` crate:
[INFO] [stdout]           - impl<T> Drive for Box<T>
[INFO] [stdout]             where T: Drive;
[INFO] [stdout]           - impl<T> Drive for T
[INFO] [stdout]             where T: 'static, for<'a> &'a T: IntoIterator, for<'a> <&'a T as IntoIterator>::Item: derive_visitor::DerefAndDrive;
[INFO] [stdout]   = note: this error originates in the derive macro `Drive` (in Nightly builds, run with -Z macro-backtrace for more info)

This will not be fixed by adding any method dispatch hacks, so I think we should just land this as-is. I don't think we should block landing this impl on the one breakage.1

Nominating this for T-libs-api for confirmation.

Footnotes

  1. Also, I'm not confident that the set of impls that are present in derive-visitor are actually even coherent/sound. We've been aware of its breakage in the new trait solver for a while: https://github.com/rust-lang/trait-system-refactor-initiative/issues/52

@compiler-errors compiler-errors added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 22, 2024
@compiler-errors compiler-errors changed the title [crate] Add Box<[T; N]>: IntoIterator without any method dispatch hacks Add Box<[T; N]>: IntoIterator without any method dispatch hacks Apr 22, 2024
@scottmcm
Copy link
Member

Hmm, I didn't actually mean a TAIT. Let's see if I can digest it further.

Suppose we had struct alloc::box::IntoIter<…> such that on a Box<[T], A> you get box::IntoIter<[T], A> and on Box<[T; N], A> you get box::IntoIter<[T; N], A>. Then we make vec::IntoIter<T, A> a type alias for box::IntoIter<[T], A>.

The idea would be to think of that [T] vs [T; N] as a tail of the struct, but of course it's not literally a tail of it, so I don't know if it would actually work out. But if you could "unsize" a box::IntoIter<[T; N], A> into a box::IntoIter<[T], A>, that might help people avoid the monomorphization costs of the array if they want, while also letting people avoid the extra usize in the iterator if they want.

(I don't know if that would actually work, though.)

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Apr 26, 2024

@scottmcm what you are describing is actually quite possible: [playground] (finally, my love for unsizing paid off,,,). Basically what you need is just a CoerceUnsized implementation for box::IntoIter which coerces buf: *mut ArrayOrSliceGeneric (so that we can conditionally store length as metadata of the pointer, while allowing unsize coercion) (IIRC there is some work to make this nicer, but you can do that today too) (it currenlty requires a type like IntoIter<[T], T, A>, but I believe this is fixable with a smarter check).

I really dig your idea, this sounds nice :)

(P.S. to be clear I did not mean you meant TAIT, I just meant that it'd be possible to use TAIT)

@compiler-errors
Copy link
Member Author

While that's a clever idea, it also seems like a lot of work to avoid a single usize from the vec::IntoIter impl. Unless someone on T-libs wants this to be implemented, I'd rather we don't, tbh.

@WaffleLapkin
Copy link
Member

I don't think it's that much work, given that this would replace the existing vec::IntoIter with something only a bit more complex (most of the things don't need to change, only signatures and drop is different).

@scottmcm
Copy link
Member

it also seems like a lot of work

I'm hoping it wouldn't be that bad, actually. Looking at vec::IntoIter,

pub struct IntoIter<
T,
#[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global,
> {
pub(super) buf: NonNull<T>,
pub(super) phantom: PhantomData<T>,
pub(super) cap: usize,
// the drop impl reconstructs a RawVec from buf, cap and alloc
// to avoid dropping the allocator twice we need to wrap it into ManuallyDrop
pub(super) alloc: ManuallyDrop<A>,
pub(super) ptr: NonNull<T>,
/// If T is a ZST, this is actually ptr+len. This encoding is picked so that
/// ptr == end is a quick test for the Iterator being empty, that works
/// for both ZST and non-ZST.
/// For non-ZSTs the pointer is treated as `NonNull<T>`
pub(super) end: *const T,
}

if we can "just" change that NonNull<T>+usize to be either NonNull<[T]> or NonNull<[T; N]> accordingly, surprisingly little else would have to change, and it'd save the extra usize. All the iterator stuff working on the ptr+end fields wouldn't have to change at all, for example.

libs-api folks, if the code needed to make it happens turned out to not be too bad, might you be interested? Or is it not worth bothering making a sample PR because you'd not want it anyway?

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 3, 2024
@rfcbot
Copy link

rfcbot commented May 3, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@compiler-errors
Copy link
Member Author

I believe this is ready for review. I don't think it's worth complicating the implementation by adding a new unsizeable iterator just to get rid of the cap: usize field, but regardless, that should be done in a follow-up PR if so imo.

r? libs-api
@rustbot ready

@scottmcm
Copy link
Member

scottmcm commented May 3, 2024

that should be done in a follow-up PR if so imo.

That would be a breaking change to a stable feature, so I don't think it can be a follow-up PR.

Once the trait impl exists returning vec::IntoIter, it's too late to change to a different type.

@scottmcm scottmcm added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 3, 2024
@compiler-errors
Copy link
Member Author

That would be a breaking change to a stable feature, so I don't think it can be a follow-up PR.

It's effectively equally as breaking as introducing this implementation, and we have until the end of the beta bump to decide whether we want this custom implementation in practice. The only code that would break is code that relies on Box<[T; N]>::IntoIter == vec::Iter<T>, and from the crater run we see that's no code currently.

if we can "just" change that NonNull+usize to be either NonNull<[T]> or NonNull<[T; N]> accordingly, surprisingly little else would have to change, and it'd save the extra usize.

I don't think this is possible given the current rules for how CoerceUnsized works. A correct implementation is going to be necessarily as complicated as the example that @WaffleLapkin shared, and I really don't think that is worthwhile. I believe the primary motivation for the Box<[T; N]>: IntoIterator impl existing at all is to avoid a move-out of the heap, and that we're gaining very little by getting rid of that additional cap: usize field.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 10, 2024
@scottmcm
Copy link
Member

I did some explorations of the unsizing version, and unfortunately it looks like the checker for what unsizing is allowed to do doesn't normalize associated types, so even if the source and target types share the same generic parameter as the not-being-unsized field, the compiler still doesn't let you do it.

So the clever version appears not currently feasible, but the simpler version of just having a "static capacity or runtime capacity" would be easy.

@Amanieu
Copy link
Member

Amanieu commented May 14, 2024

We discussed this in the libs-api meeting today. In the end we concluded that we would prefer for Box<[T; N]>'s iterator to be a separate type which include N as a generic parameter. While re-using vec::IntoIter may bring some benefits in compilation time, these are likely to be insignificant considering how little used this type would be. On the other hand the size reduction from omitting the capacity may be useful for some users in performance-critical code.

@bors
Copy link
Contributor

bors commented May 21, 2024

☔ The latest upstream changes (presumably #124097) made this pull request unmergeable. Please resolve the merge conflicts.

@Amanieu Amanieu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2024
@JohnCSimon
Copy link
Member

@compiler-errors

ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

@clarfonthey
Copy link
Contributor

clarfonthey commented Sep 4, 2024

Since compiler-errors took over the larger compiler change, I can take over this one if you need to offload some of the work. Shouldn't be too hard to write up a simple boxed version of array::IntoIter.

(Only if you want, though; just an offer.)

@compiler-errors
Copy link
Member Author

ack, @clarfonthey: I totally missed your message 😭. Go ahead, I probably will not be able to get around to this one... but it also seems to not require any T-compiler involvement.

@clarfonthey
Copy link
Contributor

Right ack at you, will work on my own version of this change.

Not sure whether I want to base it off of vec::IntoIter or array::IntoIter, but I'll figure it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.