Use ReadOnly in TryFromBytes::is_bit_valid

[joshlf]

Previously, is_bit_valid had the signature:

fn is_bit_valid<A>(c: Ptr<'_, Self, (A, Unaligned, Initialized)>) -> bool

In this commit, we remove the A aliasing parameter and wrap Self in
ReadOnly, yielding:

fn is_bit_valid(c: Ptr<'_, ReadOnly<Self>, (Shared, Unaligned, Initialized)>) -> bool

This ensures that is_bit_valid's argument is always uconditionally
Immutable regardless of whether Self: Immutable. This solves a
number of open problems:

• Ensures that is_bit_valid can never mutate its referent (TryFromBytes::is_bit_valid should promise not to mutate its referent #1831),
  which is important for custom validators (Support custom validators for TryFromBytes #1330)
• Makes it so that custom validators can be written without needing to
  be generic over aliasing, which in turn means we can support custom
  validators without exposing much of our Ptr machinery in our public
  API
• Allows us to support #[derive(TryFromBytes)] on unions without
  requiring Self: Immutable (Support #[derive(TryFromBytes)] on unions without requiring Self: Immutable #1832)
• Permits T -> U fallible transmutation in more cases (see [pointer] Support read-only aliasing; use in TryFromBytes::is_bit_valid #2336 for
  more details)

Makes progress on #2336

---

• 　 [derive] Allow TryFromBytes on non-Immutable unions #2876
• 👉 Use ReadOnly in TryFromBytes::is_bit_valid #2873

Latest Update: v67 — Compare vs v66

📚 Full Patch History

Links show the diff between the row version and the column version.

Version	v66	v65	v64	v63	v62	v61	v60	v59	v58	v57	v56	v55	v54	v53	v52	v51	v50	v49	v48	v47	v46	v45	v44	v43	v42	v41	v40	v39	v38	v37	v36	v35	v34	v33	v32	v31	v30	v29	v28	v27	v26	v25	v24	v23	v22	v21	v20	v19	v18	v17	v16	v15	v14	v13	v12	v11	v10	v9	v8	v7	v6	v5	v4	v3	v2	v1	Base
v67	v66	v65	v64	v63	v62	v61	v60	v59	v58	v57	v56	v55	v54	v53	v52	v51	v50	v49	v48	v47	v46	v45	v44	v43	v42	v41	v40	v39	v38	v37	v36	v35	v34	v33	v32	v31	v30	v29	v28	v27	v26	v25	v24	v23	v22	v21	v20	v19	v18	v17	v16	v15	v14	v13	v12	v11	v10	v9	v8	v7	v6	v5	v4	v3	v2	v1	Base
v66		v65																																																																	Base
v65			v64																																																																Base
v64				v63																																																															Base
v63					v62																																																														Base
v62						v61																																																													Base
v61							v60																																																												Base
v60								v59																																																											Base
v59									v58																																																										Base
v58										v57																																																									Base
v57											v56																																																								Base
v56												v55																																																							Base
v55													v54																																																						Base
v54														v53																																																					Base
v53															v52																																																				Base
v52																v51																																																			Base
v51																	v50																																																		Base
v50																		v49																																																	Base
v49																			v48																																																Base
v48																				v47																																															Base
v47																					v46																																														Base
v46																						v45																																													Base
v45																							v44																																												Base
v44																								v43																																											Base
v43																									v42																																										Base
v42																										v41																																									Base
v41																											v40																																								Base
v40																												v39																																							Base
v39																													v38																																						Base
v38																														v37																																					Base
v37																															v36																																				Base
v36																																v35																																			Base
v35																																	v34																																		Base
v34																																		v33																																	Base
v33																																			v32																																Base
v32																																				v31																															Base
v31																																					v30																														Base
v30																																						v29																													Base
v29																																							v28																												Base
v28																																								v27																											Base
v27																																									v26																										Base
v26																																										v25																									Base
v25																																											v24																								Base
v24																																												v23																							Base
v23																																													v22																						Base
v22																																														v21																					Base
v21																																															v20																				Base
v20																																																v19																			Base
v19																																																	v18																		Base
v18																																																		v17																	Base
v17																																																			v16																Base
v16																																																				v15															Base
v15																																																					v14														Base
v14																																																						v13													Base
v13																																																							v12												Base
v12																																																								v11											Base
v11																																																									v10										Base
v10																																																										v9									Base
v9																																																											v8								Base
v8																																																												v7							Base
v7																																																													v6						Base
v6																																																														v5					Base
v5																																																															v4				Base
v4																																																																v3			Base
v3																																																																	v2		Base
v2																																																																		v1	Base
v1																																																																			Base

[gemini-code-assist]

Summary of Changes

Hello @joshlf, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a significant refactoring to consistently use ReadOnly views when performing bit validity checks across the codebase. By updating the Maybe type alias and modifying various macros and trait implementations, the change ensures that is_bit_valid functions operate on immutable data, enhancing type safety and correctness. This also streamlines the implementation of validity checks for types like UnsafeCell and atomic types, making the system more robust.

Highlights

• is_bit_valid Refactoring: The is_bit_valid function for UnsafeCell<T> and within the impl_for_transmute_from! macro has been simplified to directly use candidate.transmute() instead of into_exclusive_or_pme() and get_mut().
• Maybe Type Alias Update: The Maybe type alias, which defines the argument type for TryFromBytes::is_bit_valid, now uses ReadOnly<T> instead of T, ensuring that bit validity checks operate on immutable views.
• Macro Enhancements for ReadOnly: The impl_size_eq! and unsafe_impl_for_transparent_wrapper! macros have been extended to correctly handle ReadOnly types, including new SizeEq implementations and casting mechanisms.
• Atomic Type Trait Refinement: The macros for implementing traits for atomic types have been split into impl_layout_traits_for_atomics! and impl_validity_traits_for_atomics!, providing a clearer separation of concerns for layout and validity traits, and incorporating the new impl_size_eq! syntax.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a ReadOnly wrapper to the Maybe type alias, which is used in is_bit_valid checks. This is a significant safety improvement, as it enforces at the type level that these checks do not perform mutations. The changes correctly propagate this new approach through various macros and implementations, and the refactoring of some macros for better clarity is appreciated. My review focuses on cleaning up some leftover commented-out code from this refactoring effort.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 83.33333% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.33%. Comparing base (5fcf372) to head (d207a40).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/wrappers.rs	61.90%	8 Missing ⚠️
src/macros.rs	0.00%	2 Missing ⚠️
src/util/macros.rs	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2873      +/-   ##
==========================================
+ Coverage   91.31%   92.33%   +1.01%     
==========================================
  Files          20       19       -1     
  Lines        5897     5831      -66     
==========================================
- Hits         5385     5384       -1     
+ Misses        512      447      -65     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[joshlf]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the is_bit_valid implementation to leverage the new ReadOnly wrapper type, ensuring that is_bit_valid always operates on immutable, shared references. This change simplifies the API and improves type safety by explicitly handling immutability. Additionally, atomic trait implementations have been modularized, and various internal casting mechanisms have been updated to align with these core changes.

I am having trouble creating individual review comments. Click here to see my feedback.

src/lib.rs (3091)

The call to is_bit_valid now includes c_ptr.reborrow_shared() before forget_aligned(). This is a critical change to ensure that is_bit_valid receives a shared reference to ReadOnly<T>, which is now the expected type due to the Maybe type alias change. Failing to provide a shared reference could lead to soundness issues.

src/wrappers.rs (668-675)

The trait implementations for ReadOnly<T> have been expanded to include TryFromBytes, FromZeros, and FromBytes. The TryFromBytes implementation now uses a complex cast (c.cast::<_, <ReadOnly<T> as SizeEq<ReadOnly<ReadOnly<T>>>>::CastFrom, _>()) to ensure is_bit_valid is called correctly. This is a critical change for ReadOnly to function as intended within the zerocopy framework, as it ensures proper type conversions and validation for immutable references.

src/util/macro_util.rs (689-701)

The try_transmute function has been critically updated to use ReadOnly<Dst> and Ptr::from_ref instead of Ptr::from_mut. It also adds transmute::<_, _, BecauseImmutable>() before forget_aligned(). This ensures that is_bit_valid receives a shared reference to ReadOnly<T>, which is vital for maintaining soundness with the new Maybe type and is_bit_valid signature. Incorrect handling here could lead to severe soundness issues.

src/pointer/ptr.rs (824-831)

The try_into_valid method now includes the ReadOnly<T>: Read<I::Aliasing, R> bound and modifies the call to is_bit_valid to use self.reborrow().transmute::<_, _, _>().reborrow_shared(). This is a critical change to ensure is_bit_valid receives a Ptr to ReadOnly<T> with shared aliasing, which is necessary for correctness given the new Maybe definition. Incorrect handling here could lead to soundness issues.

src/pointer/mod.rs (25-32)

The Maybe type alias has been fundamentally changed to Ptr<'a, ReadOnly<T>, ...>. This is a critical modification that ensures any pointer passed to is_bit_valid is treated as immutable, aligning with the PR's goal of enhancing type safety and correctness for pointer transmutations.

src/macros.rs (1040-1041)

The is_bit_valid function signature for unions in cryptocorrosion_derive_traits has been updated to remove the A: $crate::pointer::invariant::Reference generic parameter, consistent with the trait definition changes.

src/pointer/ptr.rs (326-373)

A new method reborrow_shared has been added to Ptr. This method allows reborrowing a mutable Ptr as a shared Ptr (with Shared aliasing). This is essential for scenarios where an exclusive Ptr needs to be passed to functions expecting a shared Ptr, such as is_bit_valid after the Maybe type change.

src/impls.rs (1528-1530)

The TestIsBitValidShared trait's test_is_bit_valid_shared method has been updated to remove the A: invariant::Reference generic parameter from Maybe<'ptr, T> because Maybe is now implicitly ReadOnly. This simplifies the API and aligns with the new type definition.

src/impls.rs (1591)

The TestAsBytes trait's test_as_bytes method now correctly takes &'t ReadOnly<T> as an argument, aligning with the ReadOnly changes in the zerocopy core.

src/impls.rs (1643-1645)

The test_is_bit_valid_shared method in AutorefWrapper has been updated to remove the A: invariant::Reference generic parameter, reflecting the changes in the Maybe type alias.

src/impls.rs (1703)

The test_as_bytes method in AutorefWrapper now correctly accepts &ReadOnly<$ty>, ensuring consistency with the ReadOnly changes.

src/impls.rs (1753-1754)

The assertion message for is_bit_valid (shared Ptr) has been simplified by removing the val debug print, as the focus is on the type's validity rather than the specific value.

src/impls.rs (1762-1763)

The call to is_bit_valid for the exclusive Ptr now correctly uses c.reborrow_shared(). This is essential because is_bit_valid expects a shared reference to ReadOnly<T>, and reborrow_shared provides this compatibility.

src/impls.rs (1774-1775)

The assertion message for try_ref_from_bytes has been simplified by removing the val debug print, focusing on the method's outcome.

src/impls.rs (1798-1799)

The assertion message for try_mut_from_bytes has been simplified by removing the val debug print, focusing on the method's outcome.

src/impls.rs (1804-1805)

The assertion message for try_read_from_bytes has been simplified by removing the val debug print, focusing on the method's outcome.

src/lib.rs (1670)

The signature of is_bit_valid has been simplified by removing the A: invariant::Reference generic parameter. This is a key change that aligns with the new Maybe type alias, which now implicitly wraps Self in ReadOnly<T>, ensuring that is_bit_valid always receives an immutable, shared reference.

src/lib.rs (3078)

The c_ptr.transmute() call has been replaced with c_ptr.cast::<_, crate::pointer::cast::CastSized, _>() and c_ptr is now mutable. This is a necessary adjustment to the new pointer handling mechanisms, ensuring correct type conversions within the try_read_from function.

src/impls.rs (1379-1381)

The with_passing_test_cases implementation for Option<$tys> now correctly wraps None in ReadOnly::new(None). This ensures consistency with the ReadOnly changes introduced for is_bit_valid.

src/macros.rs (898-899)

The is_bit_valid function signature in cryptocorrosion_derive_traits has been updated to remove the A: $crate::pointer::invariant::Reference generic parameter. This aligns with the simplification of the TryFromBytes::is_bit_valid trait definition, which now implicitly handles immutability through the ReadOnly wrapper.

src/impls.rs (852-853)

The is_bit_valid implementation for UnsafeCell<T> has been simplified by directly using candidate.transmute::<_, _, BecauseImmutable>(). This change correctly reflects that Maybe<'_, Self> now implicitly wraps Self in ReadOnly, making the previous into_exclusive_or_pme() call unnecessary and improving the clarity of the code.

src/impls.rs (1351-1352)

The TryFromBytesTestable trait has been updated to use Box<ReadOnly<Self>> for passing test cases. This is a necessary change to align the testing infrastructure with the new ReadOnly usage in is_bit_valid.

src/impls.rs (1419-1429)

The impl_try_from_bytes_testable macro now correctly handles ReadOnly when creating test cases. The unsafe block for Box::from_raw(raw as *mut _) is justified by the safety comment, ensuring that ReadOnly<T> has the same layout and bit validity as T.

src/pointer/ptr.rs (639-668)

The assume_aliasing and assume_exclusive methods have been removed. These were helper methods for into_exclusive_or_pme, so their removal is consistent with the simplification of the pointer API.

src/impls.rs (1005-1006)

The tuple TryFromBytes implementation now uses project_wrapped instead of project. This is a necessary adaptation to the new projection method designed to work with ReadOnly wrappers, ensuring correct field access and validation.

src/pointer/ptr.rs (960-987)

The new project_wrapped method has been added. This method provides a robust way to project fields from types wrapped in ReadOnly or MaybeUninit, which is essential for correctly navigating the new type system and ensuring proper field access during transmutations.

src/util/macro_util.rs (639-640)

The try_cast_or_pme function now includes the ReadOnly<Dst>: Read<I::Aliasing, R> bound. This is a necessary type bound to correctly handle the ReadOnly wrapper introduced in the Maybe type alias, ensuring type compatibility during pointer transmutations.

src/impls.rs (899-900)

Similar to arrays, the TryFromBytes implementation for slices [T] has been updated to correctly cast the candidate to Ptr<'_, [ReadOnly<T>], _> before calling is_bit_valid. This ensures type compatibility with the new Maybe definition.

src/util/macros.rs (138)

The is_bit_valid function signature within unsafe_impl! has been updated to remove the AA: crate::pointer::invariant::Reference generic parameter. This change aligns with the new TryFromBytes::is_bit_valid trait definition, which now implicitly handles immutability through the ReadOnly wrapper.

src/util/macros.rs (146)

The default is_bit_valid implementation within unsafe_impl! has been updated to remove the AA: crate::pointer::invariant::Reference generic parameter, consistent with the trait definition changes.

src/util/macros.rs (221-222)

The is_bit_valid implementation for UnsafeCell<T> within impl_for_transmute_from! has been simplified. The into_exclusive_or_pme() call is removed, and the transmute now directly uses candidate.transmute::<_, _, (_, _)>(), leveraging the ReadOnly wrapper in Maybe.

src/util/macros.rs (236-239)

The is_bit_valid implementation for types wrapped in <$repr:ty> within impl_for_transmute_from! has been updated. The A: crate::pointer::invariant::Reference generic parameter is removed, and BecauseImmutable is explicitly added to the transmute call, ensuring proper handling of immutability.

src/impls.rs (881-890)

The TryFromBytes implementation for arrays [T; N] has been updated to correctly handle the ReadOnly wrapper. The explicit casting through Ptr<'_, [ReadOnly<T>; N], _>, Ptr<'_, [ReadOnly<T>], _>, and Ptr<'_, ReadOnly<[T]>, _> is crucial for type compatibility with the new Maybe definition, ensuring that is_bit_valid receives the expected immutable reference.

zerocopy-derive/src/derive/try_from_bytes.rs (313-327)

The casting logic for enum variants has been updated to use WrappedProjection and ReadOnly with BecauseRead. This is a necessary adaptation to the new ReadOnly wrapper and projection mechanisms, ensuring correct type handling during enum variant validation.

zerocopy-derive/src/derive/try_from_bytes.rs (366-368)

The is_bit_valid function signature in the generated enum implementation has been updated to remove the ___ZerocopyAliasing generic parameter. This aligns with the simplification of the TryFromBytes::is_bit_valid trait definition, which now implicitly handles immutability through the ReadOnly wrapper.

zerocopy-derive/src/derive/try_from_bytes.rs (637-663)

The is_bit_valid function signature in the generated union implementation has been updated to remove the ___ZerocopyAliasing generic parameter and now uses WrappedProjection in project_transmute_unchecked. This is consistent with the broader changes to use ReadOnly for immutable references and the new projection methods.

zerocopy-derive/src/derive/try_from_bytes.rs (715-717)

The is_bit_valid function signature in try_gen_trivial_is_bit_valid has been updated to remove the ___ZerocopyAliasing generic parameter, aligning with the trait definition changes.

zerocopy-derive/src/derive/try_from_bytes.rs (745-747)

The is_bit_valid function signature in gen_trivial_is_bit_valid_unchecked has been updated to remove the ___ZerocopyAliasing generic parameter, aligning with the trait definition changes.

zerocopy-derive/tests/include.rs (122-130)

The test_trivial_is_bit_valid helper function has been updated to correctly use ReadOnly<MaybeUninit<T>> and ptr.reborrow_shared() when calling is_bit_valid. This is necessary to align the test with the new Maybe type definition and ensure proper handling of immutable references.

zerocopy-derive/tests/include.rs (137-145)

The test_is_bit_valid helper function has been updated to correctly use ReadOnly and bikeshed_recall_initialized_immutable() when preparing the candidate for is_bit_valid. This ensures that the test accurately reflects the new is_bit_valid API, which expects a shared reference to ReadOnly<T>.

zerocopy-derive/tests/struct_try_from_bytes.rs (78-79)

The call to is_bit_valid for Unsized now correctly uses candidate.reborrow_shared(). This is necessary because is_bit_valid expects a shared reference to ReadOnly<T>, and reborrow_shared provides this compatibility from an exclusive Ptr.

[joshlf]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the is_bit_valid implementations across various types and macros to leverage ReadOnly wrappers, simplifying the aliasing logic and making the code more explicit about immutability during validity checks. The changes also introduce new traits and helper functions like project_wrapped and reborrow_shared to support this new approach. Test cases have been updated to reflect these changes in type handling. However, there is a critical safety concern regarding an undocumented unsafe block and a potential missing KnownLayout implementation for AtomicPtr<T>.