Flutterdec dart serwalker#74
Open
kalixtez wants to merge 29 commits into
Open
Conversation
added 14 commits
April 2, 2026 23:12
…ct for fixed-alloc data types, added the ClassId enum to uniquely distinguish all class ids
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new workspace crate 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (6)
crates/flutterdec-serwalker/src/utils.rs (1)
6-7: Use snake_case for the macro identifier.Idiomatic Rust macros (and the established convention enforced by
clippy::upper_case_acronyms/community style) are lower_snake_case (vec!,println!,declare_fixed_length_cluster!). Renaming early avoids a churn-y rename later when the macro is invoked from more places.♻️ Suggested rename
-macro_rules! DECLARE_FIXED_LENGTH_CLUSTER +macro_rules! declare_fixed_length_cluster…with corresponding updates at the import site (
use crate::DECLARE_FIXED_LENGTH_CLUSTER;) and call site incrates/flutterdec-serwalker/src/cluster/mod.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/utils.rs` around lines 6 - 7, Rename the macro DECLARE_FIXED_LENGTH_CLUSTER to snake_case (e.g., declare_fixed_length_cluster) and update all references accordingly: change the macro definition identifier in utils.rs, update any import usages like use crate::DECLARE_FIXED_LENGTH_CLUSTER to use crate::declare_fixed_length_cluster, and update call sites (for example in crates/flutterdec-serwalker/src/cluster/mod.rs) to invoke declare_fixed_length_cluster! instead of DECLARE_FIXED_LENGTH_CLUSTER! so the naming follows Rust/Clippy conventions.crates/flutterdec-serwalker/src/snapshot.rs (1)
32-53:DataSnapshot/SnapshotKindare notpub— unusable outside the crate.These types model the public output of the deserializer, but neither
enum SnapshotKind(line 5) norstruct DataSnapshot(line 32) carries a visibility modifier. As a result they're module-private and the surrounding crate can't expose snapshot parsing to consumers (seelib.rs, where modules are also non-pub). Mark thempub(and consider derivingDebug) once the surface stabilizes.♻️ Quick fix
-enum SnapshotKind // pulled straight out of the C++ def +#[derive(Debug)] +pub enum SnapshotKind // pulled straight out of the C++ def ... -struct DataSnapshot +pub struct DataSnapshot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/snapshot.rs` around lines 32 - 53, DataSnapshot and SnapshotKind are currently module-private; make them public so external crates can use the deserializer output by adding the pub visibility to the struct and enum declarations (i.e., change struct DataSnapshot and enum SnapshotKind to pub struct DataSnapshot and pub enum SnapshotKind) and consider adding derives such as Debug (e.g., #[derive(Debug, ...)]) to both types once the API surface stabilizes; update any related items (fields or constructors) that must be pub for external use as needed.crates/flutterdec-serwalker/src/raw_object/mod.rs (1)
229-244: Placeholder structs needDefault(and likelyPhantomDatainitializer) before the commented-out macro invocations can be enabled.The
DECLARE_FIXED_LENGTH_CLUSTER!macro inutils.rscallsBox::<$name>::default()(line 36). The currently-activeOneByteStringderivesDefault, but the upcoming invocations queued incluster/mod.rs(Type,TypeArguments,TypeParameter, …) reference these placeholder types whose definitions here lack anyDefaultimpl. Adding#[derive(Default)]to the placeholders now will avoid a chain of compile errors when those lines are uncommented.♻️ Suggested fix
-pub struct Array<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct Object<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct AbstractType<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct FunctionType<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct Script<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct Closure<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct Instance<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct WeakArray<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct TypedDataBase<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct TypedData<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct TypedDataView<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct GrowableObjectArray<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct Code<'a> { _marker: std::marker::PhantomData<&'a ()> } -pub struct LoadingUnit<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct Array<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct Object<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct AbstractType<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct FunctionType<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct Script<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct Closure<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct Instance<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct WeakArray<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct TypedDataBase<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct TypedData<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct TypedDataView<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct GrowableObjectArray<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct Code<'a> { _marker: std::marker::PhantomData<&'a ()> } +#[derive(Default)] pub struct LoadingUnit<'a> { _marker: std::marker::PhantomData<&'a ()> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/raw_object/mod.rs` around lines 229 - 244, The placeholder structs (Array, Object, AbstractType, FunctionType, Script, Closure, Instance, WeakArray, TypedDataBase, TypedData, TypedDataView, GrowableObjectArray, Code, LoadingUnit and similar) need a Default implementation so macros like DECLARE_FIXED_LENGTH_CLUSTER! that call Box::<$name>::default() compile; add #[derive(Default)] to each of these placeholder struct definitions (the ones using std::marker::PhantomData) so PhantomData is initialized by Default and the upcoming cluster/mod.rs invocations can be enabled without further errors.crates/flutterdec-serwalker/src/lib.rs (1)
1-8: Crate currently exposes no public API.All modules are private and items inside (
DataSnapshot,Cluster,read_and_decompress_smi,ClassId, …) are either non-pubor onlypubwithin their private module. Nothing from this crate is reachable to consumers yet. Plan to re-export the parsing entry points (e.g.pub use snapshot::DataSnapshot;) and markmod snapshot;/mod cluster;pub(orpub(crate)) once you're ready for outside consumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/lib.rs` around lines 1 - 8, The crate currently exposes no public API because modules are private; make the intended consumer-facing types and functions public by marking modules and/or re-exporting their symbols: change module visibility for snapshot and cluster (e.g., make mod snapshot; and mod cluster; public via pub mod or pub(crate) as appropriate) and add pub re-exports such as pub use snapshot::DataSnapshot; pub use cluster::Cluster; pub use raw_object::ClassId; and pub use stream::read_and_decompress_smi; ensure the actual item declarations (DataSnapshot, Cluster, ClassId, read_and_decompress_smi) are declared pub so they become reachable to downstream crates.crates/flutterdec-serwalker/src/constants.rs (1)
29-30: Add#[repr(u32)]and standard derives toClassIdfor stability and ergonomics.The
ClassIdenum (lines 29–206) lacks#[repr(...)]and key derives. This causes two issues:
Discriminant stability: Without
#[repr(u32)], casting variantsas u32relies on unspecified layout. This is unsafe for FFI, deserialization, or any integer mapping. Dart's serialized class IDs expect a stable u32 representation.Ergonomics:
decide_clusterincluster/mod.rstakesClassIdby value and matches on it; the enum gets moved repeatedly withoutCopy. Downstream code will need to log, compare, and deserialize these IDs, which all requirePartialEq,Eq,Debug, and ideallyCopy+Clone.Add the repr and derives shown below, and consider a
TryFrom<u32> for ClassIdimpl (mirroringSnapshotKind's pattern) to decode the on-disk integer back to the enum.Suggested diff
-pub enum ClassId { +#[repr(u32)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ClassId { IllegalCid = 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/constants.rs` around lines 29 - 30, The ClassId enum lacks a stable representation and common derives; update the enum declaration `ClassId` to add `#[repr(u32)]` and derive `Debug, Copy, Clone, PartialEq, Eq` (and any other project-standard derives) so casting variants with `as u32` is deterministic and values are cheaply copyable, then add a `TryFrom<u32> for ClassId` implementation (mirroring `SnapshotKind`'s pattern) to safely decode persisted integers back into `ClassId`; update call sites like `decide_cluster` in `cluster/mod.rs` if necessary to take `ClassId` by value and use the new `TryFrom` for deserialization.crates/flutterdec-serwalker/src/stream.rs (1)
64-90: Slicing without bounds check will panic on truncated input.
read_u64/read_u32slice[curr_stream_offset..curr_stream_offset + N]directly; if the cursor is near the end (e.g., due to a malformed/truncated snapshot), this panics before reachingtry_into. For a parser that ingests untrusted binaries this is worth turning into a recoverable error (e.g., returnResult<u64, ParseError>or have callers checkremaining()first). Acceptable for now if the design is to fail fast, but please make that policy explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/stream.rs` around lines 64 - 90, read_u64 and read_u32 currently slice byte_stream with [curr_stream_offset..curr_stream_offset + N] which will panic on truncated input; change both to return Result (e.g., Result<u64, ParseError> / Result<u32, ParseError>) and perform a safe bounds check before slicing: check self.remaining() >= u64_size/u32_size (or use byte_stream.get(curr_stream_offset..curr_stream_offset + size) and return a ParseError like UnexpectedEof/InsufficientData if None), only then convert with try_into, call self.advance_pos(size) on success, and return Ok(value); reference the functions read_u64, read_u32, self.byte_stream, self.curr_stream_offset, self.advance_pos, and add/extend ParseError to represent truncated input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/flutterdec-serwalker/Cargo.toml`:
- Line 8: Remove the unused goblin dependency entry from this crate's
Cargo.toml: locate the 'goblin' workspace/dependency declaration (the line
containing "goblin.workspace = true" / the 'goblin' dependency) and delete that
entry so the crate no longer declares goblin until it's actually used; then run
cargo check to verify there are no missing references.
In `@crates/flutterdec-serwalker/src/cluster/mod.rs`:
- Around line 15-23: The functions read_cluster_alloc and read_cluster_fill
currently declare an unused local curr_ref_id causing clippy unused_variable
errors; remove the unused binding or rename it to _curr_ref_id (or prefix with
underscore) in both functions, and do the same for any similar unused bindings
in parse_version_and_features callers, so the compiler no longer treats them as
warnings/errors while you implement the real logic.
- Around line 45-47: The placeholder block for the DECLARE_FIXED_LENGTH_CLUSTER
expansion for OneByteString currently evaluates to `1` (silently returning a
consumed-bytes value and leaving macro parameters like `read_fill`,
`last_ref_id`, and `stream` unused); replace the `{ 1 // to-do }` body with a
`todo!()` (or otherwise panic) so callers fail loudly and Clippy doesn’t error
on unused variables, and when you implement `read_fill` later, use the
`last_ref_id` and `stream` parameters inside the OneByteString implementation to
return the correct consumed byte count.
- Around line 32-38: decide_cluster currently always returns Err, ignores the
clusters parameter, and uses an elided &str lifetime; change it to a proper
unimplemented placeholder (e.g., call todo!() or leave a clear TODO) so callers
know it's a stub, and update the signature to either return a mutable reference
into the provided array (Result<&mut Box<dyn Cluster>, &'static str>) if the
intent is to look up/return an existing cluster by ClassId, or change the
clusters parameter to an index or owned value if you intend to take ownership;
also replace the error type &str with an explicit &'static str (or a real error
enum) to avoid incorrect borrowed lifetimes. Ensure references to
decide_cluster, Cluster, constants::MAX_CLUSTER_NUM, ClassId, and IllegalCid are
updated accordingly.
- Line 6: The Smi alias is inconsistent: cluster/mod.rs defines type Smi = i32
while raw_object/mod.rs uses type Smi = i64, causing silent truncation; fix by
centralizing a single pub type Smi = i64 (either in a new constants.rs or make
it pub in raw_object/mod.rs), remove the local type alias in cluster/mod.rs,
import that pub Smi where needed, and change the public function signature of
read_and_decompress_smi (and any other uses) to return the centralized Smi type
so all modules use i64 consistently.
- Around line 1-4: The file currently imports SIGNED_M (unused) and glob-imports
everything from crate::raw_object, which causes unused-import warnings and
namespace pollution; remove SIGNED_M from the constants import list and replace
use crate::raw_object::*; with explicit imports for only the types used by
DECLARE_FIXED_LENGTH_CLUSTER! (inspect the macro invocations to identify which
placeholder/object type names are referenced) so the module only imports those
symbols; if/when read_and_decompress_smi is updated to require SIGNED_M, re-add
it then.
- Around line 25-30: Update the misleading comment in read_and_decompress_smi to
state that SMIs are encoded with unsigned LEB128 (UNSIGNED_M), not signed; then
fix the type-width bug by making Smi match the target platform width (use i64
like raw_object::Smi in raw_object/mod.rs), have read_and_decompress_smi call
read_modified_leb128() into a u64, perform the right-shift by
constants::SMI_SHIFT at full width, and only then cast the shifted value to Smi
(i64) to avoid truncation on 64-bit platforms.
In `@crates/flutterdec-serwalker/src/snapshot.rs`:
- Around line 79-83: Change the read_clusters signature to take a mutable stream
reference (fn read_clusters(&mut self, stream: &mut Stream)) so cluster parsing
can advance the cursor and match the mutability of stream.read_* methods; also
remove the unused binding curr_ref_id (or rename it to _curr_ref_id / initialize
it where used) to silence clippy warnings, and apply the same unused-variable
fix pattern used in cluster/mod.rs::read_cluster_alloc and read_cluster_fill.
- Around line 10-28: Fix two typos in snapshot.rs: change the truncated comment
near the FullAOT enum variant to read "Full + AOT code, this is the one we care
about, as the" (or rephrase to a complete sentence) and correct the trailing
inline comment in the TryFrom<u64> match's default arm from "Handle invalid
snapshot kidjns" to "Handle invalid snapshot kinds" (the code regions to edit
are the SnapshotKind enum declaration around FullAOT and the default match arm
in impl TryFrom<u64> for SnapshotKind).
- Around line 71-77: In parse_version_and_features, avoid panicking from
String::split_off by validating the C string returned by Stream::read_c_string()
before splitting: check that its byte length is at least
constants::VERSION_HASH_LENGTH and that the split index is on a UTF-8 char
boundary (so splitting won't panic); if validation fails, return or propagate an
error (instead of panicking) and do not assign to
self.features/self.version_hash. Locate the logic in parse_version_and_features,
the call to Stream::read_c_string(), and the use of
constants::VERSION_HASH_LENGTH, and replace the unconditional split_off with a
guarded branch that safely sets self.features and self.version_hash only on
valid input.
- Around line 56-69: Change parse_header to return a Result<(), E> (use the
workspace's Error type or anyhow::Result) and replace the panic!/expect calls
with early-return Err variants: check magic_bytes read from Stream against
MAGIC_BYTES and return a descriptive error on mismatch, replace
SnapshotKind::try_from(...).expect(...) with mapping the Err into an Err result,
and propagate any Stream read errors likewise; keep calling
parse_version_and_features(stream) but have it also return a Result and
propagate its error (use ?). Update signatures and call sites to handle the
Result accordingly.
In `@crates/flutterdec-serwalker/src/stream.rs`:
- Around line 10-13: The seek implementation in fn seek(&mut self, pos: usize)
incorrectly rejects pos == 0 and silently ignores out-of-range positions; change
seek to accept the full range 0..=self.byte_stream.len() and return a Result so
callers get notified on bad input. Specifically, update fn seek(&mut self, pos:
usize) to return Result<(), SeekError> (or a suitable error type), set
self.curr_stream_offset = pos when pos <= self.byte_stream.len(), and return
Err(...) when pos > self.byte_stream.len(); keep references to
self.byte_stream.len() and self.curr_stream_offset so the check and assignment
are obvious in the implementation.
- Around line 99-110: read_c_string currently searches from the start of
self.byte_stream, slices with an inclusive end that can be out-of-bounds, and
returns a string that includes the NUL; fix it by searching from
self.curr_stream_offset (e.g. let hay =
&self.byte_stream[self.curr_stream_offset..]), find position of 0x00 in that
subslice (using position or iter().enumerate()), then compute absolute indices:
if a NUL is found set text_end = curr_stream_offset + pos and take the returned
bytes as &self.byte_stream[self.curr_stream_offset..text_end] (exclusive of the
NUL) and advance_pos by pos + 1 to skip the NUL; if no NUL is found use the rest
of the buffer (&self.byte_stream[self.curr_stream_offset..]), advance_pos by
that length, and convert the sliced bytes to String with proper UTF-8 handling
(e.g. from_utf8 with a clear error message) — update references to
self.byte_stream, self.curr_stream_offset, read_c_string, and advance_pos
accordingly.
- Around line 32-62: The read_modified_leb128 function has three fixes: replace
direct indexing of self.byte_stream[self.curr_stream_offset + idx as usize] with
safe .get(...) checks and return a typed parse error (e.g.,
Result::Err(ParseError::TruncatedStream)) when .get returns None; enforce a max
index bound (cap idx at 9) and check before any left-shift using (idx as usize *
DATA_BITS_PER_BYTE) to return a ParseError::Overflow if exceeded to avoid
>=64-bit shifts; and remove the redundant initial byte re-read by either
initializing read_num from first_byte and starting idx at 1 or by folding the
single-byte case into the main loop so the code only reads each byte once
(update calls to advance_pos and uses of UNSIGNED_MAX_DATA_PER_BYTE,
UNSIGNED_END_OF_DATA_BYTE, DATA_BITS_PER_BYTE, advance_pos, curr_stream_offset
accordingly).
- Around line 2-6: Stream is not constructible and read_c_string/seek contain
logic bugs; add a public constructor (e.g., pub fn new(byte_stream: &'a [u8]) ->
Stream<'a>) that initializes byte_stream and curr_stream_offset = 0 so callers
can create Stream, fix read_c_string to search starting at
self.curr_stream_offset (use
self.byte_stream[self.curr_stream_offset..].iter().position(|&b| b==0)), handle
the case when no null is found without panicking (treat as end-of-buffer or
return the slice to end), return the string slice excluding the null byte, and
advance curr_stream_offset by the relative length + 1 only when a null was found
(or by the remaining length if none); finally, change seek to accept zero by
allowing pos == 0 and validate pos <= self.byte_stream.len() before setting
self.curr_stream_offset = pos.
In `@crates/flutterdec-serwalker/src/utils.rs`:
- Around line 27-50: The generated read_alloc implementation fails to
read/assign the tags field and doesn't set
start_of_fill/end_of_fill/end_of_alloc, and read_fill's parameters can be unused
causing -D warnings; fix by (1) updating read_alloc to consume the tag word
before reading obj_count and assign self.tags (matching CLUSTER_TAGS_SZ/expected
on-disk layout) or remove the tags field/doc if tags aren't present, (2) ensure
start_of_fill, end_of_fill and end_of_alloc are written at the appropriate
points (e.g., set start_of_alloc at the start, set end_of_alloc after allocation
read or set start_of_fill/end_of_fill inside read_fill), and (3) avoid
unused-parameter warnings in fn read_fill by renaming parameters to _last_ref_id
and _stream in the signature or adding a discard like let _ = (&last_ref_id,
&stream); before $fill_impl; adjust the OneByteString invocation accordingly so
last_ref_id/stream use is explicit if intended.
---
Nitpick comments:
In `@crates/flutterdec-serwalker/src/constants.rs`:
- Around line 29-30: The ClassId enum lacks a stable representation and common
derives; update the enum declaration `ClassId` to add `#[repr(u32)]` and derive
`Debug, Copy, Clone, PartialEq, Eq` (and any other project-standard derives) so
casting variants with `as u32` is deterministic and values are cheaply copyable,
then add a `TryFrom<u32> for ClassId` implementation (mirroring `SnapshotKind`'s
pattern) to safely decode persisted integers back into `ClassId`; update call
sites like `decide_cluster` in `cluster/mod.rs` if necessary to take `ClassId`
by value and use the new `TryFrom` for deserialization.
In `@crates/flutterdec-serwalker/src/lib.rs`:
- Around line 1-8: The crate currently exposes no public API because modules are
private; make the intended consumer-facing types and functions public by marking
modules and/or re-exporting their symbols: change module visibility for snapshot
and cluster (e.g., make mod snapshot; and mod cluster; public via pub mod or
pub(crate) as appropriate) and add pub re-exports such as pub use
snapshot::DataSnapshot; pub use cluster::Cluster; pub use raw_object::ClassId;
and pub use stream::read_and_decompress_smi; ensure the actual item declarations
(DataSnapshot, Cluster, ClassId, read_and_decompress_smi) are declared pub so
they become reachable to downstream crates.
In `@crates/flutterdec-serwalker/src/raw_object/mod.rs`:
- Around line 229-244: The placeholder structs (Array, Object, AbstractType,
FunctionType, Script, Closure, Instance, WeakArray, TypedDataBase, TypedData,
TypedDataView, GrowableObjectArray, Code, LoadingUnit and similar) need a
Default implementation so macros like DECLARE_FIXED_LENGTH_CLUSTER! that call
Box::<$name>::default() compile; add #[derive(Default)] to each of these
placeholder struct definitions (the ones using std::marker::PhantomData) so
PhantomData is initialized by Default and the upcoming cluster/mod.rs
invocations can be enabled without further errors.
In `@crates/flutterdec-serwalker/src/snapshot.rs`:
- Around line 32-53: DataSnapshot and SnapshotKind are currently module-private;
make them public so external crates can use the deserializer output by adding
the pub visibility to the struct and enum declarations (i.e., change struct
DataSnapshot and enum SnapshotKind to pub struct DataSnapshot and pub enum
SnapshotKind) and consider adding derives such as Debug (e.g., #[derive(Debug,
...)]) to both types once the API surface stabilizes; update any related items
(fields or constructors) that must be pub for external use as needed.
In `@crates/flutterdec-serwalker/src/stream.rs`:
- Around line 64-90: read_u64 and read_u32 currently slice byte_stream with
[curr_stream_offset..curr_stream_offset + N] which will panic on truncated
input; change both to return Result (e.g., Result<u64, ParseError> / Result<u32,
ParseError>) and perform a safe bounds check before slicing: check
self.remaining() >= u64_size/u32_size (or use
byte_stream.get(curr_stream_offset..curr_stream_offset + size) and return a
ParseError like UnexpectedEof/InsufficientData if None), only then convert with
try_into, call self.advance_pos(size) on success, and return Ok(value);
reference the functions read_u64, read_u32, self.byte_stream,
self.curr_stream_offset, self.advance_pos, and add/extend ParseError to
represent truncated input.
In `@crates/flutterdec-serwalker/src/utils.rs`:
- Around line 6-7: Rename the macro DECLARE_FIXED_LENGTH_CLUSTER to snake_case
(e.g., declare_fixed_length_cluster) and update all references accordingly:
change the macro definition identifier in utils.rs, update any import usages
like use crate::DECLARE_FIXED_LENGTH_CLUSTER to use
crate::declare_fixed_length_cluster, and update call sites (for example in
crates/flutterdec-serwalker/src/cluster/mod.rs) to invoke
declare_fixed_length_cluster! instead of DECLARE_FIXED_LENGTH_CLUSTER! so the
naming follows Rust/Clippy conventions.
🪄 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: CHILL
Plan: Pro
Run ID: 81abc6b3-480b-43c8-b7b9-e44ff7b00fda
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/flutterdec-serwalker/Cargo.tomlcrates/flutterdec-serwalker/src/cluster/mod.rscrates/flutterdec-serwalker/src/constants.rscrates/flutterdec-serwalker/src/lib.rscrates/flutterdec-serwalker/src/raw_object/mod.rscrates/flutterdec-serwalker/src/snapshot.rscrates/flutterdec-serwalker/src/stream.rscrates/flutterdec-serwalker/src/utils.rsrust-toolchain.toml
caverav
force-pushed
the
flutterdec-dart-serwalker
branch
from
ac2a614 to
b36733a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/flutterdec-serwalker/src/raw_object/mod.rs`:
- Around line 16-21: The struct field types like TypeArguments.instantiations
(and similar fields in the other structs referenced) must not use Option<&'a mut
X> because Rust borrow exclusivity prevents shared/cyclic Dart graphs; replace
those Option<&'a mut ...> references with a shared handle type — e.g.
Option<std::rc::Rc<std::cell::RefCell<X>>> for single-threaded shared mutable
references or Option<RefId> if you maintain a flat arena of RawObject entries —
and update the affected structs (TypeArguments and the other structs at the
listed ranges) to use the chosen handle type so multiple objects can point to
the same target without violating Rust's mutability rules.
In `@crates/flutterdec-serwalker/src/stream.rs`:
- Around line 1-4: The import list includes SIGNED_END_OF_DATA_BYTE which is
unused and will cause a warning-as-error; edit the use crate::constants { ... }
in stream.rs to remove SIGNED_END_OF_DATA_BYTE (leaving
UNSIGNED_END_OF_DATA_BYTE, UNSIGNED_MAX_DATA_PER_BYTE, and DATA_BITS_PER_BYTE)
and re-add SIGNED_END_OF_DATA_BYTE later when the signed-LEB128 reader is
implemented.
🪄 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: CHILL
Plan: Pro
Run ID: 70b7509e-0cff-4504-92a0-b2fc135bb312
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/flutterdec-serwalker/Cargo.tomlcrates/flutterdec-serwalker/src/cluster/mod.rscrates/flutterdec-serwalker/src/constants.rscrates/flutterdec-serwalker/src/lib.rscrates/flutterdec-serwalker/src/raw_object/mod.rscrates/flutterdec-serwalker/src/snapshot.rscrates/flutterdec-serwalker/src/stream.rscrates/flutterdec-serwalker/src/utils.rsrust-toolchain.toml
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- crates/flutterdec-serwalker/Cargo.toml
- crates/flutterdec-serwalker/src/lib.rs
- rust-toolchain.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/flutterdec-serwalker/src/utils.rs
- crates/flutterdec-serwalker/src/constants.rs
added 4 commits
April 27, 2026 15:40
… strings and ref ids, fixed minor bugs
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/flutterdec-serwalker/src/raw_object/mod.rs (1)
16-21:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Option<&'a mut T>cannot model Dart snapshot sharing/cycles; switch to handle/shared ownership.These reference fields enforce exclusive borrows, so multiple objects cannot point to the same target (or cyclicly point back), which is required for snapshot graphs. This will break population/fill design as clusters grow.
Use a shared representation (
Option<RefId>into an arena/object table, orRc<RefCell<T>>for single-threaded mutable sharing). Given your cluster flow already trackslast_ref_id,RefIdis the cleaner fit.Suggested direction (illustrative)
+pub type RefId = u64; pub struct Type<'a> { - pub arguments: Option<&'a mut TypeArguments<'a>>, // TypeArgumentsPtr + pub arguments: Option<RefId>, // TypeArgumentsPtr } pub struct Class<'a> { - pub super_type: Option<&'a mut Type<'a>>, // TypePtr - pub declaration_type: Option<&'a mut Type<'a>>, // TypePtr + pub super_type: Option<RefId>, // TypePtr + pub declaration_type: Option<RefId>, // TypePtr }#!/bin/bash set -euo pipefail # 1) Enumerate all mutable reference fields in raw objects. rg -nP "pub\\s+\\w+:\\s+Option<&'a mut [A-Za-z_]\\w*(?:<'a>)?>" crates/flutterdec-serwalker/src/raw_object/mod.rs # 2) Show ref-id based allocation plumbing in serwalker. rg -n --type=rust -C2 "\\blast_ref_id\\b|objs:\\s*Vec<\\(\\s*u64,\\s*Box<" crates/flutterdec-serwalker/srcAlso applies to: 24-33, 36-41, 44-48, 51-67, 69-83, 86-92, 94-106, 109-118, 133-164, 166-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/raw_object/mod.rs` around lines 16 - 21, The TypeArguments struct (and other raw object structs with fields like instantiations: Option<&'a mut Array<'a>>) currently uses exclusive borrows which prevents shared/cyclic references; change these mutable reference fields to a reference-id based representation (e.g., Option<RefId> or Option<usize>) that indexes the serwalker arena/object table instead of Option<&'a mut T>, and update the population/fill logic that uses last_ref_id/objs to allocate and resolve those RefId entries when building/patching objects; alternatively for single-threaded mutability use Rc<RefCell<T>> consistently, but prefer RefId to match existing last_ref_id allocation plumbing and to enable sharing/cycles across TypeArguments and other raw object types referenced in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/flutterdec-serwalker/src/stream.rs`:
- Around line 68-89: read_u64 and read_u32 currently panic because they take a
slice using curr_stream_offset..curr_stream_offset+size before verifying enough
bytes exist; change both functions to first check that
self.byte_stream.len().saturating_sub(self.curr_stream_offset) >= size and
return a parse error (Result::Err) if not, only then safely slice (or use
get(..) to obtain the subslice), call self.advance_pos(size) on success and
decode with from_le_bytes; update the functions' signatures to return
Result<u64, ParseError>/Result<u32, ParseError> (or the crate's error type) and
propagate that error where these readers are used so truncated input no longer
panics.
- Around line 113-121: Change read_string to be fallible by returning a
Result<String, Error> (or a crate-specific ParseError) instead of panicking:
first validate that curr_stream_offset + len <= byte_stream.len() and return an
error if the buffer is too short; then take the slice
&byte_stream[self.curr_stream_offset..final_pos], call advance_pos(len) only
after validation, and attempt UTF-8 conversion using std::str::from_utf8 or
String::from_utf8, propagating the UTF-8 error into your Result rather than
calling expect; update any call sites of read_string accordingly to handle the
Result.
- Around line 130-160: read_ref_id can read past the end of byte_stream and
overflow the 32-bit accumulator; modify read_ref_id to validate input length and
prevent overflow by (1) checking before each byte access that
self.curr_stream_offset + idx < self.byte_stream.len() and returning an error
(or Option) on truncation, (2) imposing a sane max byte count for ref-ids (e.g.,
4 or 5 iterations) to avoid unbounded loops, and (3) using a larger accumulator
or checked arithmetic (checked_shl/checked_add or i64 accumulator with explicit
limit checks) instead of unbounded left shifts on ref_id; update the signature
of read_ref_id to return Result<u32, Error> (or Option<u32>) and only call
self.advance_pos(...) on the successfully consumed byte count.
---
Duplicate comments:
In `@crates/flutterdec-serwalker/src/raw_object/mod.rs`:
- Around line 16-21: The TypeArguments struct (and other raw object structs with
fields like instantiations: Option<&'a mut Array<'a>>) currently uses exclusive
borrows which prevents shared/cyclic references; change these mutable reference
fields to a reference-id based representation (e.g., Option<RefId> or
Option<usize>) that indexes the serwalker arena/object table instead of
Option<&'a mut T>, and update the population/fill logic that uses
last_ref_id/objs to allocate and resolve those RefId entries when
building/patching objects; alternatively for single-threaded mutability use
Rc<RefCell<T>> consistently, but prefer RefId to match existing last_ref_id
allocation plumbing and to enable sharing/cycles across TypeArguments and other
raw object types referenced in this module.
🪄 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: CHILL
Plan: Pro
Run ID: 96824466-5739-4ec9-9ab2-eb3c8721eb50
📒 Files selected for processing (4)
crates/flutterdec-serwalker/src/cluster/mod.rscrates/flutterdec-serwalker/src/raw_object/mod.rscrates/flutterdec-serwalker/src/snapshot.rscrates/flutterdec-serwalker/src/stream.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/flutterdec-serwalker/src/cluster/mod.rs
Comment on lines
+113
to
+121
| // read a non null-terminated string given a length | ||
| pub fn read_string(&mut self, len: usize) -> String | ||
| { | ||
| let final_pos = self.curr_stream_offset + len; | ||
| let raw_str = &self.byte_stream[self.curr_stream_offset..final_pos]; | ||
|
|
||
| self.advance_pos(len); | ||
|
|
||
| String::from_utf8(raw_str.to_vec()).expect("Couldn't turn UTF-8 bytes into a String.") |
There was a problem hiding this comment.
Make read_string fallible.
This still slices blindly and unwraps UTF-8 conversion, so a short buffer or invalid encoding will panic the parser. Validate the length first and return an error instead of unwrapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/flutterdec-serwalker/src/stream.rs` around lines 113 - 121, Change
read_string to be fallible by returning a Result<String, Error> (or a
crate-specific ParseError) instead of panicking: first validate that
curr_stream_offset + len <= byte_stream.len() and return an error if the buffer
is too short; then take the slice
&byte_stream[self.curr_stream_offset..final_pos], call advance_pos(len) only
after validation, and attempt UTF-8 conversion using std::str::from_utf8 or
String::from_utf8, propagating the UTF-8 error into your Result rather than
calling expect; update any call sites of read_string accordingly to handle the
Result.
added 2 commits
April 29, 2026 23:38
…size alloc cluster types, implemented parse_clusters, implemented a macro to declare the ClassId enum and TryFrom for u32 for said enum
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/flutterdec-serwalker/src/cluster/mod.rs (1)
15-21: 💤 Low valueEmpty placeholder functions.
These appear to be unused stubs. Consider removing them if not needed, or mark with
todo!()if they're intended for future implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/flutterdec-serwalker/src/cluster/mod.rs` around lines 15 - 21, The two functions read_cluster_alloc and read_cluster_fill are empty stubs; either remove these unused functions or replace their bodies with a clear placeholder to indicate future work (e.g., use todo!() or unimplemented!()) so the intent is explicit; update the definitions of read_cluster_alloc and read_cluster_fill accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/flutterdec-serwalker/src/snapshot.rs`:
- Around line 101-102: The expect call on decide_cluster(cid) uses a literal
"{cid}" so the class id won't be shown; replace the panic with proper error
propagation (return Result) or at minimum format the message using format! to
include cid (e.g. format!("Couldn't find cluster implementation for class {}",
cid)); update the surrounding function signature to return Result and propagate
the error from decide_cluster (or use ?) instead of panicking so behavior
matches the parse_header change; look for decide_cluster and the cluster binding
to implement this change.
In `@crates/flutterdec-serwalker/src/utils.rs`:
- Around line 100-106: The function decode_tags currently unwraps the class id
via DECODE_IS_CID! which calls ClassId::try_from(...).unwrap(), causing a panic
on malformed/unknown class IDs; change decode_tags to return Result<DecodedTags,
ErrorType> (propagate a suitable error from ClassId::try_from) instead of
unwrapping, compute is_deeply_immutable and is_canonical as before and return
Ok(DecodedTags::new(...)), and update the caller in snapshot.rs (the call at
snapshot.rs:98) to handle the Result (propagate or map the error) so parsing no
longer panics on invalid class IDs.
---
Nitpick comments:
In `@crates/flutterdec-serwalker/src/cluster/mod.rs`:
- Around line 15-21: The two functions read_cluster_alloc and read_cluster_fill
are empty stubs; either remove these unused functions or replace their bodies
with a clear placeholder to indicate future work (e.g., use todo!() or
unimplemented!()) so the intent is explicit; update the definitions of
read_cluster_alloc and read_cluster_fill accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: 05e1b493-3408-41e1-84ef-9a16b7857c2b
📒 Files selected for processing (6)
crates/flutterdec-serwalker/src/cluster/mod.rscrates/flutterdec-serwalker/src/constants.rscrates/flutterdec-serwalker/src/raw_object/mod.rscrates/flutterdec-serwalker/src/snapshot.rscrates/flutterdec-serwalker/src/stream.rscrates/flutterdec-serwalker/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/flutterdec-serwalker/src/constants.rs
- crates/flutterdec-serwalker/src/stream.rs
- crates/flutterdec-serwalker/src/raw_object/mod.rs
…s and serializable types
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/flutterdec-serwalker/src/utils.rs`:
- Line 7: The macro currently captures a `$last_ref_id` identifier but never
binds it in the generated `read_fill` body, causing references to that
identifier inside `$fill_impl` to fail; fix by either removing `$last_ref_id`
from the macro pattern and all macro invocations (e.g., in `cluster/mod.rs`) so
the macro signature matches the `read_fill` trait method, or if `last_ref_id` is
required, add it to the trait signature for `read_fill` and update the generated
code to bind `let $last_ref_id = <appropriate_source>;` before invoking
`$fill_impl`; locate the macro definition (the arm with ($name:ident
$(<$lt:lifetime>)?, |$_self:ident, $last_ref_id:ident, $stream:ident|
$fill_impl:block) => { ...) and the generated `read_fill` binding lines (`let
$_self = self; let $stream = stream;`) to apply the change.
🪄 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: CHILL
Plan: Pro
Run ID: ba0570a5-c4e4-4c90-a890-35b13b1022d2
📒 Files selected for processing (4)
crates/flutterdec-serwalker/src/cluster/mod.rscrates/flutterdec-serwalker/src/raw_object/mod.rscrates/flutterdec-serwalker/src/snapshot.rscrates/flutterdec-serwalker/src/utils.rs
✅ Files skipped from review due to trivial changes (1)
- crates/flutterdec-serwalker/src/raw_object/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/flutterdec-serwalker/src/snapshot.rs
added 6 commits
May 14, 2026 16:46
…Rust's complains about cyclic references and other headaches, implemented read_fill for fixed-size objects
…mplemented little read_byte (it was convenient) and modified _String so it suits our purposes
…D_LENGTH_CLUSTER as it was unnecessary
…cid wouldnt interpolate in the printed error string
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
1 participant
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.
Summary
Describe what changed and why.
Validation
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceScope
type(scope): description)README.md,docs/*,context.md) when behavior changed