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 more Zeroable and ZeroableInOption impls #158

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Dec 22, 2022

No description provided.

@Lokathor
Copy link
Owner

For the wide pointer/reference types, I'm unclear on the metadata requirements.

Does a null pointer not need a valid length?

and does the None not even need a vtable?

and where specifically is there documentation about this? I'd rather not add the impls if there's no docs about it.

@zachs18
Copy link
Contributor

zachs18 commented Jan 2, 2023

The layout of pointers to unsized types is currently explicitly unspecified in the reference, aside from being Sized and having size and alignment at least that of pointers to sized types. So conservatively (and perhaps pedantically), I'd say the metadata requirements are "unspecified" in stable Rust, and these impls make more assumptions than stable Rust guarantees.

That being said: I do think some reasonable assumptions can be made, especially under the ptr_metadata unstable feature.

For the wide pointer/reference types, I'm unclear on the metadata requirements.
Does a null pointer not need a valid length?

I think it is reasonable to assume that a Zeroable::zeroed() slice raw pointer would be a null pointer with a length of 0 (which is valid; you could get it from safe unsizing, e.g. std::ptr::null::<[T; 0]>() as *const [T]).

and does the None not even need a vtable?

It's not mentioned in the documentation that I can find, but for specifically Option<Box<T>/etc> where T is dyn Trait, I would assume that it cannot require the metadata be a valid vtable, since it may be that no valid vtable even exists, (e.g. None::<&dyn Trait> where Trait has no implementations).

(Not evidence, but under the current implementation: None::<&T> just leaves the "metadata" part of the "pointer" uninitialized, so zeroed is fine.)


I would hesistate to add impls of ZeroableInOption for String and Vec mostly just because they don't feel very "primitive" to me, but also due to lack of layout guarantees (despite Vec's relatively strict guarantees about being a (null-pointer-optimized pointer, length, capacity) triplet (in unspecified order)).

Other than that, I agree that it'd probably be best not to add these until there is stable documentation to support that they are sound, though perhaps they could be added under a cargo feature until then?.

(I also agree with the comment in the code that impls like impl<T: ?Sized> Zeroable for *const T where <T as Pointee>::Metadata: Zeroable would be nice since they would encompass thin T: Sized and extern type pointers, slice pointers, possible future custom DSTs, and structs with one of those as their unsized tail.)

@a1phyr a1phyr force-pushed the zeroable_option_ptr branch from f2feb90 to 66c27c7 Compare January 19, 2023 13:05
@a1phyr
Copy link
Contributor Author

a1phyr commented Jan 19, 2023

I removed the problematic String and Vec impls. Is it enough or do you want me to remove more of them ?

Copy link
Owner

@Lokathor Lokathor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good like this

@Lokathor Lokathor merged commit 2548e46 into Lokathor:main Jan 19, 2023
@Lokathor Lokathor added the semver-patch semver patch change label Jan 20, 2023
leod pushed a commit to leod/bytemuck that referenced this pull request Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch semver patch change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants