Introduce ReadOnly<T> which is unconditionally Immutable#2866
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 new 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new ReadOnly<T> wrapper type. While it's a work in progress, there are two critical issues with the current implementation that need to be addressed. First, ReadOnly<T> incorrectly implements DerefMut, which contradicts its purpose and allows mutation of the wrapped data. Second, it has an unsound unconditional unsafe impl of the Immutable trait, which can lead to undefined behavior by violating one of the core safety invariants of zerocopy. I've provided detailed comments and suggestions for both of these critical issues.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2866 +/- ##
==========================================
- Coverage 91.72% 91.36% -0.36%
==========================================
Files 20 20
Lines 5908 5931 +23
==========================================
Hits 5419 5419
- Misses 489 512 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ba5b262 to
7ad85b6
Compare
3e60851 to
5db7a0c
Compare
7ad85b6 to
6192561
Compare
5db7a0c to
155e82d
Compare
6192561 to
3d9d244
Compare
155e82d to
52ce959
Compare
4ca0e13 to
be7520a
Compare
2dd1cbd to
e933a0e
Compare
4c19d75 to
ad978ce
Compare
e933a0e to
27689d2
Compare
ad978ce to
ee75641
Compare
27689d2 to
d9c3cc0
Compare
6539cee to
e2f1bc3
Compare
d9c3cc0 to
28de514
Compare
e2f1bc3 to
45ff87c
Compare
56473d2 to
374d785
Compare
f29bc13 to
5dac13c
Compare
374d785 to
2443118
Compare
5dac13c to
ade58d5
Compare
2443118 to
ab5e943
Compare
ade58d5 to
26e76d5
Compare
5718b7a to
3c44f90
Compare
26e76d5 to
41f96c8
Compare
3c44f90 to
f6911ab
Compare
8d752d9 to
fe97d59
Compare
20452c3 to
fdf2d74
Compare
fe97d59 to
c76e21a
Compare
fdf2d74 to
fc38019
Compare
c76e21a to
4e211b1
Compare
fc38019 to
c2d0307
Compare
4e211b1 to
e686b08
Compare
c2d0307 to
acc181c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Immutable trait's safety invariant to focus on preventing interior mutation rather than strictly banning UnsafeCells. This change is reflected in updated documentation and the introduction of a new ReadOnly<T> wrapper type. The ReadOnly<T> wrapper ensures that a &ReadOnly<T> is genuinely read-only, making it unconditionally Immutable regardless of whether the wrapped T is. The changes also include renaming derive_no_cell to derive_immutable for consistency.
In order to make this sound, we change the safety invariant on `Immutable` to narrowly ban interior mutation. In other words, the presence of `UnsafeCell`s is acceptable so long as no interior mutation is performed in practice. While we're here, remove the final remaining references to `Immutable`'s old name, `NoCell`. Makes progress on #2336 Closes #1760 gherrit-pr-id: Gbe8d7edd150d80731c79815685c596ed88460ae7
In order to make this sound, we change the safety invariant on
Immutableto narrowly ban interior mutation. In other words, thepresence of
UnsafeCells is acceptable so long as no interior mutationis performed in practice.
While we're here, remove the final remaining references to
Immutable'sold name,
NoCell.Makes progress on #2336
Closes #1760
TryFromByteson non-Immutableunions #2876ReadOnlyinTryFromBytes::is_bit_valid#2873ReadOnly<T>which is unconditionallyImmutable#2866Latest Update: v64 — Compare vs v63
📚 Full Patch History
Links show the diff between the row version and the column version.