split_array: remove runtime panics

[Xiretza]

This is an update to #90091 implementing #90091 (comment), which removes runtime panics by:

• making slice-splitting methods return an Option
• asserting the index bounds of array-splitting methods using a const assert.

The latter is a temporary measure needed because generic const expressions are not yet advanced enough to represent this restriction at the type level. As such it is not supposed to be stabilized as-is. It is however an improvement over the previous behaviour, which was an unconditional runtime panic instead of a compile-time error.

[rustbot]

r? @joshtriplett

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

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[Xiretza]

Manged to make everything except array::split_array() const, maybe there's a better way to implement that one that doesn't rely on ManuallyDrop::deref()?

[BoxyUwU]

generic_const_exprs isn't allowed to be used in core/std as its too unstable/broken

[Xiretza]

Alright, at least we've got an answer to that question now.

[Kixunil]

maybe there's a better way to implement that one that doesn't rely on ManuallyDrop::deref()?

I think you can just call mem::forget on the original array after copying. Since the reads don't panic this should be sound.

[WaffleLapkin]

Does it make sense to instead have split_array_ref (panicking) and try_split_array_ref (fallible)?

[Xiretza]

That would blow up the method count to 8; I don't think adding unwrap()/except() is that big of a hurdle.

[Xiretza]

I think you can just call mem::forget on the original array after copying

Thanks, it's const now!

[bors]

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

[bors]

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

[Xiretza]

I've trimmed this down to remove the need for #![feature(generic_const_exprs)], the array methods also return Option<(&[T; M], &[T])> instead of (&[T; M], &[T; N-M]) for now. This at least removes the panicking, making invalid split indices on arrays a compile-time error is future work.

@rustbot ready

[faern]

the array methods also return Option<(&[T; M], &[T])> instead of (&[T; M], &[T; N-M]) for now. This at least removes the panicking, making invalid split indices on arrays a compile-time error is future work.

This makes the function signature wrong. I'd argue that's worse than the panic. And you don't remove the panic, you just move the responsibility of doing that to the caller. I'd argue that it would be better if the function signature was at least correct now, even if the exact desired behavior can't be had yet.

[Xiretza]

This makes the function signature wrong.

The function signature for the array methods were already wrong, now they're just wrong in a slightly different way. They cannot be made correct yet, that's what #![feature(generic_const_exprs)] was for.

And you don't remove the panic, you just move the responsibility of doing that to the caller.

I guess the array methods could use a static_assert!() and impossible-to-fail internal .unwrap() instead to approximate the intended final behaviour, I'll look into that.

[Xiretza]

Updated to @faern's suggestions - the array method signatures are not changed anymore now, but the implementations contain a const assert that at least turns index errors from runtime panics into post-mono errors.

[workingjubilee]

Would you please fully change the message on the title and PR to describe what's actually the case right now? In this case, technically these functions are in fact "panicking", but now at compile-time. The bors merge commit will include that message, so including a bunch of Markdown-annotated strikethrough is liable to be confusing.

[workingjubilee]

I am not really aware of any other code in the standard library that deliberately relies on user-facing "post-monomorphization errors". I believe this may be the first? I believe this should be discussed by the relevant teams before accepting this PR. It's low-risk since this is user-facing, but it is a significant change.

The alternative is essentially "wait for GCE to get better before allowing these functions to develop further".

[Xiretza]

Updated the title and description. Note that the design of the array methods as implemented here is not intended to be final, only an improvement over the current implementation.

[dtolnay]

We discussed this PR in today's library API meeting. Thank you for the PR!

There are 2 changes here:

• Methods that changed from runtime panic on out of bounds, to const assert. We are not on board with this. This is in the same category as Make <[T]>::array_* methods fail to compile on 0 len arrays #99471 (comment).

  Instead we'd recommend that this be integrated into the existing unconditional_panic lint which applies to things like array[100] out of bounds.

• Methods that changed from runtime panic to Option return type. We are also not on board with this. We preferred that the split_array-related APIs be viewed similarly to str::split_at (https://doc.rust-lang.org/1.71.0/std/primitive.str.html#method.split_at), or slice::split_at, which panic.

  Yes, making the user transform an Option into a panic is easier for them than the reverse, but we expect that the dominant use cases will be such that the user knows this index is in bounds, and proliferation of unwrap() in such code is unpleasant.

  When the index is not known to be in bounds at the call site, going through the iteration API to pull off a fixed sized chunk (Tracking Issue for slice::array_chunks #74985) seems like a suitable fit, and the documentation could guide this.

[Xiretza]

@dtolnay This is incredibly frustrating.

I proposed this in the tracking issue over a year ago. Judging by the fact that it has over half as many 👍 reactions as the tracking issue itself, it seemed to be rather well received by the community, i.e. the actual prospective users of the API.

People kept bringing up that the methods should be fallible, so after almost a year of nobody with an actual say in things showing up to say whether it would be acceptable, I finally opened this PR half a year ago. Got some initial reviews that answered a question that was also asked on the tracking issue half a year prior without any real answers there, but at least then I knew and could fix my implementation.

There were crickets for another couple months, when out of the blue libs-api shows up to shoot it down entirely. Meanwhile there has still not been a single official comment on the tracking issue - am I just misunderstanding what tracking issues are for?

This is frustrating not only because of the time I wasted, but also because it feels like I have a need for the fallible API just about every month, but have not once wished for the panicking API. array_chunks is also not an alternative, as it produces chunks that are all the same size.

[c410-f3r]

Well the lack of official feedback is understandable because the majority of the responsible personal are volunteers AFAITC. The visible preference for the current API is also understandable because that is what people have been using for years in the sense they get used to it, not to mention that a bunch of companies/projects are using Rust, which means that change should be handled with care.

That said, I think the situation is more like a feature than a bug. Perhaps everything will improve in the future with better team management and team growth, who knows 🤷

[faern]

We preferred that the split_array-related APIs be viewed similarly to str::split_at (https://doc.rust-lang.org/1.71.0/std/primitive.str.html#method.split_at), or slice::split_at, which panic.

I think this is a mistake. I personally really don't like that split_at panics. Every time I reach for that method I wish it was fallible

but we expect that the dominant use cases will be such that the user knows this index is in bounds, and proliferation of unwrap() in such code is unpleasant.

I definitely don't. Because the split index can be program input that I don't control at compile time. I can't remember a single time where I wanted it to be panicking. Working with byte buffers in memory has so many papercuts in Rust still, and good fallible slice/array subdivision in various kinds is a major part of those.

[Kixunil]

Exactly, split_array_ref has a special use case in parsers where we want to get a known length array. Example:

let (value, input) = input.split_arrray_ref::<4>().ok_or(Error::UnexpectedEof)?;
let value = u32::from_be_byte(value);

With panic this turns into DoS vulnerability hazard.