Use ReadOnly in TryFromBytes::is_bit_valid#2873
Conversation
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! 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 Highlights
🧠 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 AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
81ada6e to
a353963
Compare
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
ad978ce to
ee75641
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
3 times, most recently
from
09a6d35 to
6ac888a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2873 +/- ##
==========================================
- Coverage 92.33% 92.30% -0.03%
==========================================
Files 19 19
Lines 5843 5849 +6
==========================================
+ Hits 5395 5399 +4
- Misses 448 450 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
ee75641 to
6539cee
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
6ac888a to
1b4834d
Compare
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
6539cee to
e2f1bc3
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
2 times, most recently
from
c83b71f to
db5ec29
Compare
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
e2f1bc3 to
45ff87c
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
db5ec29 to
845c5e7
Compare
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
45ff87c to
d1f62db
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
3 times, most recently
from
b6642a7 to
d0f57ea
Compare
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
d1f62db to
65f15b8
Compare
joshlf
force-pushed
the
Gbe8d7edd150d80731c79815685c596ed88460ae7
branch
from
65f15b8 to
318b671
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
d0f57ea to
6cbe1ca
Compare
joshlf
force-pushed
the
G06bb22c59364654d5c0b44cdcb9b70ad58fe8092
branch
from
f5944c4 to
f0bff5b
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
2 times, most recently
from
7d3d321 to
f83a061
Compare
joshlf
force-pushed
the
G06bb22c59364654d5c0b44cdcb9b70ad58fe8092
branch
from
f0bff5b to
55cbce3
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
f83a061 to
dad9c64
Compare
joshlf
force-pushed
the
G06bb22c59364654d5c0b44cdcb9b70ad58fe8092
branch
from
55cbce3 to
b0b3dd7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and welcome refactoring of the TryFromBytes::is_bit_valid method, changing its signature to use ReadOnly<Self>. This improves safety by preventing mutation within validation logic and simplifies the implementation of custom validators. The changes are extensive, touching core traits, macros, and numerous implementations. While the overall direction is excellent, I've identified a few critical issues, primarily related to type mismatches at the new is_bit_valid call sites which will likely cause compilation failures. Additionally, there's a potential unintentional breaking change regarding SizeEq implementations for atomic types that should be addressed.
I am having trouble creating individual review comments. Click here to see my feedback.
src/pointer/ptr.rs (776)
This line seems to have a compile error. reborrow_shared is a method on &mut Ptr, but it's being called on a temporary Ptr value returned by transmute. A similar issue exists in try_read_from in src/lib.rs (line 3082), where c_ptr.reborrow_shared() is called, but its return type Ptr<'_, Wrapping<T>, _> doesn't match the expected Ptr<'_, ReadOnly<Wrapping<T>>, _> for is_bit_valid. These call sites might need to be adjusted to correctly construct the Maybe<...> argument for is_bit_valid.
src/impls.rs (432-435)
The SizeEq<$atomic> for $prim and SizeEq<$prim> for $atomic implementations have been removed. While the new SizeEq<ReadOnly<$atomic>> for ReadOnly<$prim> is necessary for the changes to is_bit_valid, the removed impls seem to still be required for other operations like transmute_ref!. For example, transmute_ref!(&AtomicU8) to &u8 would now fail to compile. Was this removal intentional? If so, it seems like a potentially breaking change. If not, these impls should probably be restored.
zerocopy-derive/tests/struct_try_from_bytes.rs (79)
There seems to be a type mismatch here. is_bit_valid expects a Maybe<'_, Unsized>, which is Ptr<'_, ReadOnly<Unsized>, _>, but candidate.reborrow_shared() returns a Ptr<'_, Unsized, _>. This test will likely fail to compile.
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
2 times, most recently
from
beb77bf to
54942f9
Compare
joshlf
force-pushed
the
G06bb22c59364654d5c0b44cdcb9b70ad58fe8092
branch
2 times, most recently
from
e6fb649 to
a4d2ae8
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
54942f9 to
4a46118
Compare
joshlf
force-pushed
the
G06bb22c59364654d5c0b44cdcb9b70ad58fe8092
branch
from
a4d2ae8 to
a98e1d0
Compare
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
2 times, most recently
from
2096433 to
9477a95
Compare
Previously, `is_bit_valid` had the signature: ```rust 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: ```rust 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 (#1831), which is important for custom validators (#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` (#1832) - Permits `T -> U` fallible transmutation in more cases (see #2336 for more details) Makes progress on #2336 gherrit-pr-id: G7691845b6b02e9f3d9578435d732bacfa6ca674f
joshlf
force-pushed
the
G7691845b6b02e9f3d9578435d732bacfa6ca674f
branch
from
9477a95 to
0c202f7
Compare
3 participants
Previously,
is_bit_validhad the signature:In this commit, we remove the
Aaliasing parameter and wrapSelfinReadOnly, yielding:This ensures that
is_bit_valid's argument is always uconditionallyImmutableregardless of whetherSelf: Immutable. This solves anumber of open problems:
is_bit_validcan never mutate its referent (TryFromBytes::is_bit_validshould promise not to mutate its referent #1831),which is important for custom validators (Support custom validators for
TryFromBytes#1330)be generic over aliasing, which in turn means we can support custom
validators without exposing much of our
Ptrmachinery in our publicAPI
#[derive(TryFromBytes)]on unions withoutrequiring
Self: Immutable(Support#[derive(TryFromBytes)]on unions without requiringSelf: Immutable#1832)T -> Ufallible transmutation in more cases (see [pointer] Support read-only aliasing; use inTryFromBytes::is_bit_valid#2336 formore details)
Makes progress on #2336
try_transmute!(and friends) internals #2938TryFromByteson non-Immutableunions #2876ReadOnlyinTryFromBytes::is_bit_valid#2873Latest Update: v93 — Compare vs v92
📚 Full Patch History
Links show the diff between the row version and the column version.