[WIP] ReadOnly + TryFromBytes::is_bit_valid #2863

joshlf · 2025-12-30T20:04:46Z

Latest Update: v7 — Compare vs v6

📚 Full Patch History

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

Version	v6	v5	v4	v3	v2	v1	Base
v7	vs v6	vs v5	vs v4	vs v3	vs v2	vs v1	vs Base
v6		vs v5	vs v4	vs v3	vs v2	vs v1	vs Base
v5			vs v4	vs v3	vs v2	vs v1	vs Base
v4				vs v3	vs v2	vs v1	vs Base
v3					vs v2	vs v1	vs Base
v2						vs v1	vs Base
v1							vs Base

gemini-code-assist · 2025-12-30T20:05:07Z

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 new ReadOnly wrapper type in Rust, aimed at enhancing type safety by explicitly marking data as immutable and facilitating robust byte-level validity checks. The ReadOnly wrapper is integrated into the Maybe pointer type, ensuring that any data referenced by Maybe pointers benefits from these new immutability guarantees. This change also includes extensive trait implementations for ReadOnly, allowing it to seamlessly interact with existing byte-oriented safety mechanisms and provide consistent behavior with its underlying type.

Highlights

New ReadOnly Wrapper Introduced: A new ReadOnly<T> transparent wrapper struct has been added, designed to enforce immutability and provide a safe abstraction for byte-level operations.
TryFromBytes::is_bit_valid Implementation: The ReadOnly<T> wrapper now correctly implements TryFromBytes::is_bit_valid by delegating the validity check to its inner type T, ensuring consistent byte validity semantics.
Maybe Type Alias Update: The Maybe type alias in src/pointer/mod.rs has been updated to wrap its generic type T with ReadOnly<T>, indicating that pointers to potentially uninitialized data will now be treated as read-only.
Comprehensive Trait Implementations: ReadOnly<T> now implements a suite of traits including KnownLayout, Unaligned, FromZeros, FromBytes, IntoBytes, Immutable, Deref, DerefMut, and Debug, ensuring its compatibility and correct behavior within the system.
Casting and Transmutation Utilities: Utilities for safe casting and transmutation between ReadOnly<T> and T have been added, leveraging SizeEq and TransmuteFrom traits to facilitate seamless conversions.

🧠 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.

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<T> wrapper, presumably to enable validity checks on types with interior mutability. While the goal is valid, the current implementation has several critical soundness and API design issues. The Immutable implementation for ReadOnly<T> is unsound, which can lead to undefined behavior. The DerefMut implementation on a type named ReadOnly is highly misleading and dangerous. There are also incomplete implementations (todo!()) and broken call sites resulting from the changes. I've provided detailed comments on these critical issues, which must be addressed for this PR to proceed.

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+const _: () = unsafe {
+    unsafe_impl!(T: ?Sized => Immutable for ReadOnly<T>);
+};


The unsafe implementation of Immutable for ReadOnly<T> is unsound. ReadOnly<T> is a #[repr(transparent)] wrapper around T. If T contains an UnsafeCell (and is thus not Immutable), then ReadOnly<T> will also contain an UnsafeCell at the same offset. This implementation unconditionally promises that ReadOnly<T> is Immutable, which violates the safety contract of the Immutable trait (that it does not contain any UnsafeCells). This can lead to undefined behavior, for example by allowing a data race if is_bit_valid reads from a !Immutable type while it's being concurrently mutated.

This seems to be an attempt to allow calling is_bit_valid on types with interior mutability. However, lying to the type system with an unsound impl is not the correct approach. The Immutable trait is a strong guarantee about the type's structure, not a temporary property of a reference.

A different design approach is needed to safely handle validity checks on types with interior mutability. A possible sound implementation would be to make this conditional on T: Immutable, but that would defeat the purpose of this wrapper.

impl_or_verify!(T: ?Sized + Immutable => Immutable for ReadOnly<T>);

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+impl<T: ?Sized + Immutable> DerefMut for ReadOnly<T> {
+    #[inline(always)]
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}


The DerefMut implementation for ReadOnly<T> is highly misleading and potentially dangerous. A type named ReadOnly should not provide mutable access. This violates the principle of least astonishment and creates a significant potential for misuse. This implementation should be removed to maintain the read-only contract of the type's name.

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+impl<'a, T: ?Sized + Immutable> From<&'a T> for &'a ReadOnly<T> {
+    #[inline(always)]
+    fn from(t: &'a T) -> &'a ReadOnly<T> {
+        todo!()
+    }
+}


The From<&'a T> for &'a ReadOnly<T> implementation is incomplete and currently panics due to todo!(). This should be implemented correctly. Since ReadOnly<T> is a #[repr(transparent)] wrapper around T, a safe transmutation can be performed.

The T: ?Sized + Immutable bound on this impl is also questionable. If the Immutable implementation for ReadOnly<T> is made sound (i.e., conditional on T: Immutable), this From impl becomes less useful. This points back to the fundamental soundness issue with the Immutable impl for ReadOnly<T>.

fn from(t: &'a T) -> &'a ReadOnly<T> { // SAFETY: `ReadOnly<T>` is a `#[repr(transparent)]` wrapper over `T`, // so a `&T` can be safely transmuted to a `&ReadOnly<T>`. unsafe { core::mem::transmute(t) } }

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+unsafe impl<T: ?Sized> TransmuteFrom<T, Valid, Valid> for ReadOnly<T> {}
+unsafe impl<T: ?Sized> TransmuteFrom<ReadOnly<T>, Valid, Valid> for T {}


The TransmuteFrom implementations for ReadOnly<T> and T are restricted to Valid validity. Since ReadOnly<T> is a transparent wrapper around T and they share the same bit validity, this transmutation should be sound for any validity V. Generalizing this would also fix a compilation issue in try_into_valid which needs to transmute a Ptr with Initialized validity.

unsafe impl<T: ?Sized, V: Validity> TransmuteFrom<T, V, V> for ReadOnly<T> {} unsafe impl<T: ?Sized, V: Validity> TransmuteFrom<ReadOnly<T>, V, V> for T {}

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+impl<T: ?Sized + Immutable> Deref for ReadOnly<T> {
+    type Target = T;
+
+    #[inline(always)]
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}


The Deref implementation for ReadOnly<T> is constrained by T: Immutable. This seems to defeat the purpose of the ReadOnly wrapper, which is presumably to treat a type T (which may not be Immutable) as immutable. If T is not Immutable, this Deref implementation would be unsound as it could expose interior mutability through a shared reference to what is supposed to be an immutable wrapper. If ReadOnly<T> is to provide an immutable view of a potentially mutable T, it should not implement Deref to T.

Consider removing this Deref implementation. If access to the inner value is needed, it should be through explicit methods that are designed with the appropriate safety considerations.

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+    // SAFETY: TODO
+    define_cast!(unsafe { pub CastFromReadOnly<T: ?Sized> = ReadOnly<T> => T});
+    // SAFETY: TODO
+    define_cast!(unsafe { pub CastToReadOnly<T: ?Sized> = T => ReadOnly<T>});


The SAFETY: TODO comments for these define_cast! macros should be filled in. Since ReadOnly<T> is a #[repr(transparent)] wrapper around T, they have the same layout, and casting between them is sound. The safety comment should state this.

// SAFETY: `ReadOnly<T>` is a `#[repr(transparent)]` wrapper around `T`, // so they have the same layout and a cast is sound. define_cast!(unsafe { pub CastFromReadOnly<T: ?Sized> = ReadOnly<T> => T}); // SAFETY: `ReadOnly<T>` is a `#[repr(transparent)]` wrapper around `T`, // so they have the same layout and a cast is sound. define_cast!(unsafe { pub CastToReadOnly<T: ?Sized> = T => ReadOnly<T>});

gemini-code-assist · 2025-12-30T20:07:53Z

src/wrappers.rs

+impl<T: ?Sized + Immutable + Debug> Debug for ReadOnly<T> {
+    #[inline(always)]
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        self.deref().fmt(f)
+    }
+}


The Debug implementation for ReadOnly<T> has a T: ?Sized + Immutable + Debug bound. The Immutable bound is likely inherited from the Deref implementation it uses. If Deref is removed as suggested in another comment, this Debug impl should be updated to call self.0.fmt(f) directly and only require a T: Debug bound.

impl<T: ?Sized + Debug> Debug for ReadOnly<T> { #[inline(always)] fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { self.0.fmt(f) } }

gherrit-pr-id: Gb15e30e716a041285c1bf7434ce3163f594526a7

This was referenced Dec 30, 2025

[pointer] Add generic projection/cast framework #2860

Open

[WIP] #2864

Open

joshlf force-pushed the Gb15e30e716a041285c1bf7434ce3163f594526a7 branch from ed42ba1 to f2813bb Compare December 30, 2025 20:06

joshlf force-pushed the Gdeb4f5a282b778a82175703218739a93074d0cc4 branch from 021e25d to d116f27 Compare December 30, 2025 20:06

gemini-code-assist bot reviewed Dec 30, 2025

View reviewed changes

joshlf force-pushed the Gdeb4f5a282b778a82175703218739a93074d0cc4 branch from d116f27 to 3d0b918 Compare December 30, 2025 20:41

joshlf force-pushed the Gb15e30e716a041285c1bf7434ce3163f594526a7 branch 2 times, most recently from 7a5cf19 to f7db04e Compare December 30, 2025 20:44

joshlf force-pushed the Gdeb4f5a282b778a82175703218739a93074d0cc4 branch 2 times, most recently from 4e33171 to ba1a532 Compare December 30, 2025 23:58

joshlf force-pushed the Gb15e30e716a041285c1bf7434ce3163f594526a7 branch 2 times, most recently from 622633b to 6ab7850 Compare December 31, 2025 00:30

joshlf force-pushed the Gdeb4f5a282b778a82175703218739a93074d0cc4 branch from ba1a532 to 05f8ad1 Compare December 31, 2025 00:30

[WIP] ReadOnly + TryFromBytes::is_bit_valid

f08dbdd

gherrit-pr-id: Gb15e30e716a041285c1bf7434ce3163f594526a7

joshlf force-pushed the Gb15e30e716a041285c1bf7434ce3163f594526a7 branch from 6ab7850 to f08dbdd Compare January 2, 2026 18:12

joshlf force-pushed the Gdeb4f5a282b778a82175703218739a93074d0cc4 branch from 05f8ad1 to 6414d5d Compare January 2, 2026 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] ReadOnly + TryFromBytes::is_bit_valid #2863

[WIP] ReadOnly + TryFromBytes::is_bit_valid #2863

Uh oh!

joshlf commented Dec 30, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot commented Dec 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

gemini-code-assist bot Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		unsafe impl<T: ?Sized> TransmuteFrom<T, Valid, Valid> for ReadOnly<T> {}
		unsafe impl<T: ?Sized> TransmuteFrom<ReadOnly<T>, Valid, Valid> for T {}

[WIP] ReadOnly + TryFromBytes::is_bit_valid #2863

Are you sure you want to change the base?

[WIP] ReadOnly + TryFromBytes::is_bit_valid #2863

Uh oh!

Conversation

joshlf commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Dec 30, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

joshlf commented Dec 30, 2025 •

edited

Loading