Separate SplitAt's bounds checking and overlap checking
#2473
Conversation
|
cc @kupiakos |
src/split_at.rs
Outdated
| /// use zerocopy::{SplitAt, FromBytes}; | ||
| /// # use zerocopy_derive::*; | ||
| /// | ||
| /// #[derive(SplitAt, FromBytes, KnownLayout, IntoBytes)] |
There was a problem hiding this comment.
Presumably you don't want to derive IntoBytes? As a consequence, you presumably don't need Packet to be generic over B?
There was a problem hiding this comment.
It's required by FromBytes::mut_from_bytes.
There was a problem hiding this comment.
Ack. I wonder if this is a hint that users will ~never need this API lol. In particular, since slice DSTs are unsized, you can't ever construct one by value, and so the only way to construct a &mut MySliceDst is via reference transmute, but all of our support for reference transmutes requires Self: IntoBytes. I suppose you could have constructed one yourself (e.g. via unsized coercion), but it's certainly niche. Oh well.
There was a problem hiding this comment.
A bunch of these look a little silly when situated right next to FromByte::ref_from_bytes (which requires Immutable) and FromBytes::mut_from_bytes (which requires IntoBytes), but I can see these being useful in generic contexts.
There was a problem hiding this comment.
Let's say you have a generic function that needs to split a dst that gets passed in. This function is generic over & and &mut references to a DST. Maybe you have some &mut DSTs that aren't Immutable, and some & DSTs that aren't IntoBytes. In such a case, a T: Unaligned bound might be the common denominator that lets you write this generic function.
There was a problem hiding this comment.
I see SplitAt as extending the toolkit of DST manipulation that's dependent on zerocopy traits and expertise to get right, towards a goal of working with them nearly as easily as sized structs. So I do generally support whatever flexible trait bounds are possible for converting, even though I don't have a "key use case" example either.
you can't ever construct one by value
I've been thinking that Box<Dst> construction could be done with a macro and struct init syntax. This'd fit well in zerocopy with the knowledge from KnownLayout/SplitAt.
There was a problem hiding this comment.
In such a case, a
T: Unalignedbound might be the common denominator that lets you write this generic function.
Sure, but this comment thread is about via_runtime_check, not about via_unaligned. My point is that, if you always need to have an IntoBytes bound anyway, then you may never actually need via_runtime_check.
To be clear, I still think we should keep via_runtime_check for completeness. I just think it's an interesting observation that we should think more about at some point.
There was a problem hiding this comment.
I've been thinking that
Box<Dst>construction could be done with a macro and struct init syntax. This'd fit well in zerocopy with the knowledge fromKnownLayout/SplitAt.
Oh this could be really cool! If you have a prototype at some point, let us know! I could see adding this to zerocopy if it's a simple enough API (which I imagine it would be).
b8695e1 to
b342e51
Compare
fd5f6b5 to
9562f5e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2473 +/- ##
==========================================
- Coverage 90.68% 90.36% -0.33%
==========================================
Files 19 19
Lines 7712 7808 +96
==========================================
+ Hits 6994 7056 +62
- Misses 718 752 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1bc06d1 to
57acb73
Compare
708346e to
c8a657d
Compare
d0d7cd0 to
b21d06c
Compare
There was a problem hiding this comment.
Copilot reviewed 6 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (3)
- zerocopy-derive/tests/ui-msrv/struct.stderr: Language not supported
- zerocopy-derive/tests/ui-nightly/struct.stderr: Language not supported
- zerocopy-derive/tests/ui-stable/struct.stderr: Language not supported
By doing so, dynamic overlap checking can be avoided entirely in many cases. Makes progress towards #1290.
Previously,
SplitAt's methods returnedOption<(&(mut) Self, &(mut) Self::Trailing)>, whereNonereflects an index that's either out of bounds, or results in the split parts overlapping in a problematic manner. This design conflated two failure conditions into one and, worse, presented an overly expensive and conservative API: dynamically checking for overlap is expensive, and there are many cases in which overlapping parts are not problematic, and this API could not scale to accommodate them all.SplitAt's methods now returnOption<Split<&(mut) Self>>, whereNonereflects an out-of-bounds index, andSplit<&(mut) Self>encodes the possibly-overlapping parts.Split<&(mut) Self>provides methods for producing references to the split parts via both dynamic overlap checking and via static bounds (e.g.,Self: Immutable). This API is more both cheaper and more expressive.This API is, in fact, slightly more expressive than is possible to demonstrate within zerocopy's current capabilities.
Split::via_runtime_checkis overly conservative whenT: Immutable, but it is not currently possible to construct a DST which would have overlapping parts withoutT: Immutable. We intend to follow up with mechanisms for DST construction that don't have this limitation.Makes progress towards #1290.