Open
Conversation
This was referenced Apr 11, 2026
Contributor
Author
This was referenced Apr 11, 2026
fb07c41 to
902d085
Compare
902d085 to
d032aab
Compare
29dd07a to
f45ff82
Compare
5126dba to
78bc151
Compare
78bc151 to
e625532
Compare
e625532 to
21c3ec6
Compare
46ecbb0 to
c0f20be
Compare
21c3ec6 to
4e95dab
Compare
c0f20be to
f1ace3e
Compare
4e95dab to
e0f9299
Compare
f1ace3e to
f5dd688
Compare
e0f9299 to
d7cad3e
Compare
d7cad3e to
32ecd12
Compare
32ecd12 to
b98d19c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data-layout/src/fixed_layout.rs`:
- Around line 1029-1033: The match arm handling a 3-byte length width is dead
code because Capacity::len_width() only returns 1 or 2; remove the unreachable 3
=> branch from the generated match in fixed_layout.rs (the block that constructs
a 4-byte raw array and reads u32::from_le_bytes) or, if you want to preserve it
for future-proofing, replace it with a clear comment referencing
Capacity::len_width() and why it cannot occur; update or remove any
tests/examples that expect a 3-byte path accordingly to keep behavior consistent
with Capacity::len_width().
- Line 41: The variable `datalen_is_min` is declared but never used; either
remove the declaration and any assignments to `datalen_is_min` (including the
`datalen_is_min = true` you added), or implement the intended logic by using
`datalen_is_min` in the surrounding control flow (e.g., check `if datalen_is_min
{ ... }` where the minimum-length behavior should apply) so the flag affects
processing; locate the `datalen_is_min` declaration and the assignment to
`datalen_is_min = true` and either delete both or add the conditional logic that
reads the flag (using the variable name `datalen_is_min`) to enforce the
intended behavior.
- Around line 626-632: The capacity check in the Capacity::Value arm is
off-by-one and uses the wrong threshold and message: replace the current check
"if capacity >= 0xff" with a check that enforces the actual 2-byte max (e.g.,
"if capacity > 0xffff" or "if capacity >= 0x1_0000") so capacity == 0xff (255)
is allowed and anything above 0xFFFF is rejected; also update the syn::Error
message (the one using field.span()) to correctly reference 0xFFFF and that
len_width can be at most 2 bytes; keep the check in the same Capacity::Value
block and ensure consistency with Capacity::len_width().
- Around line 586-608: In gen_view_methods' Self::Vec arm, stop destructuring
the non-existent len_width field and instead call capacity.len_width() where
len_width was used: remove len_width from the pattern (keep elem and capacity),
replace read_len_expr(offset, *len_width) with read_len_expr(offset,
capacity.len_width()), and replace usize_lit(*len_width) with
usize_lit(capacity.len_width()); leave the capacity literal handling as-is (use
capacity for the capacity value).
- Around line 508-534: In gen_field_encode's match arm for Self::Vec, stop
destructuring the non-existent len_width field and derive it from capacity
instead: remove len_width from the pattern Self::Vec { elem, capacity, len_width
} and compute let lw = capacity.len_width(); then replace usages of
usize_lit(*len_width) and len_width_ty with usize_lit(lw) and a len_width_ty
match on lw (1 => quote!(u8), 2 => quote!(u16), _ => unreachable!()). Keep
existing references to elem (elem_size), capacity (usize_lit(*capacity)), and
field_ident as-is and use lw for the byte-slice index/length calculations.
- Around line 460-475: The match arms for FixedFieldKind::Vec in
gen_validate_vec_len, gen_field_encode, and gen_view_methods incorrectly
destructure a non-existent len_width field; change the pattern to only extract
elem and capacity (e.g. FixedFieldKind::Vec { elem, capacity }) and compute let
len_width = capacity.len_width(); then use usize_lit(len_width) (and
len_width_lit) where needed and remove any direct destructuring of len_width
from the variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1787c514-abdc-421f-88f4-2390c4431b4d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomldata-layout/Cargo.tomldata-layout/README.mddata-layout/src/fixed_layout.rsdata-layout/src/lib.rsdata-layout/tests/fixed_layout.rs
b98d19c to
26ff596
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
data-layout/src/fixed_layout.rs (1)
1066-1082: 🧹 Nitpick | 🔵 TrivialDead code: 3-byte length width case is unreachable.
Capacity::len_width()(lines 818-822) only returns 1 or 2, so the 3-byte case here is never executed. Consider removing it or adding a comment explaining why it exists for future-proofing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data-layout/src/fixed_layout.rs` around lines 1066 - 1082, The match arm handling a 3-byte length in read_len_expr is dead because Capacity::len_width() only returns 1 or 2; remove the 3 => { ... } arm (or replace it with a short explanatory comment if you prefer to keep future-proofing) so the match only handles 1 and 2 and leaves the default unreachable branch, and add a brief comment on read_len_expr referencing Capacity::len_width() to explain why 3-byte widths are not expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data-layout/src/fixed_layout.rs`:
- Around line 239-240: The snippet has a formatting nit: add a missing space
after the '=' in the let binding so it reads `let raw: [u8; 2] =
bytes[offset..offset + 2].try_into().expect("validated len");` (locate the
binding around the u16::from_le_bytes(raw) as usize conversion in
fixed_layout.rs and update the spacing).
In `@data-layout/src/lib.rs`:
- Around line 29-35: The docs mention a supported attribute parameter
#[fixed_layout(assert_len = <const>)], but the implementation in fixed_layout.rs
(function parse_args) currently rejects any parameters; either implement
parsing/validation of an optional assert_len parameter in parse_args and
propagate it to whatever codegen/validation uses fixed_layout, or remove the
assert_len mention from the doc comment in lib.rs; to fix, update parse_args in
fixed_layout.rs to accept and parse the assert_len key (validate it is a
compile-time integer constant and return it via the existing attribute parse
result) and add handling where fixed_layout codegen/validation enforces or
records the asserted size, or delete the assert_len line from the doc comment in
lib.rs to match the current "fixed_layout does not support parameters" behavior.
In `@data-layout/tests/fixed_layout.rs`:
- Around line 47-49: Update the incorrect comment describing the
encrypted_data_suffix: change "Vec<8>" to "Vec<u8>" and correct the offset text
from "offset: 125" to "offset: 118" so the comment matches the actual code that
sets bytes[118] = 8 and bytes[119..127].copy_from_slice(...); keep the rest of
the comment unchanged.
In `@data-layout/tests/test_fixed_layout_flexible.rs`:
- Around line 46-48: The inline comment for the `encrypted_data_suffix` field is
incorrect; update it to match the actual code: change "offset: 125" to "offset:
118", fix the type "Vec<8>" to "Vec<u8>", and replace the misleading "capacity =
120" with "capacity = flexible" (or note the `#[capacity = flexible]` attribute)
so the comment accurately describes the bytes writes around bytes[118..128] and
the len_width = 2 usage.
---
Duplicate comments:
In `@data-layout/src/fixed_layout.rs`:
- Around line 1066-1082: The match arm handling a 3-byte length in read_len_expr
is dead because Capacity::len_width() only returns 1 or 2; remove the 3 => { ...
} arm (or replace it with a short explanatory comment if you prefer to keep
future-proofing) so the match only handles 1 and 2 and leaves the default
unreachable branch, and add a brief comment on read_len_expr referencing
Capacity::len_width() to explain why 3-byte widths are not expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4e04b65-1b32-4925-97f5-8d3dfdcd0097
📒 Files selected for processing (4)
data-layout/src/fixed_layout.rsdata-layout/src/lib.rsdata-layout/tests/fixed_layout.rsdata-layout/tests/test_fixed_layout_flexible.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Once this crate works for this repo, we can move this out to its own repo, as it is quite reusable functionality (might use in DLP v2 as well).
fixed_layout allows us to express zero-copy types in fewer lines of code:
which replaces this (existing code):
Summary by CodeRabbit
New Features
Documentation
Tests