Conversation
📝 WalkthroughWalkthroughBumps several crate versions to 3.0, adds #[mser(varint)] to protocol enums, moves CustomClickAction from configuration to common, refactors varint encode/decode logic in haya_ser, adjusts haya_ser_macro attribute handling, and enhances CI with Clippy, Miri, and rustfmt checks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/rust.yml (1)
23-24: 🧹 Nitpick | 🔵 TrivialPin
actions/checkoutto a specific version or SHA for reproducibility and security.Using
@mastermeans the workflow could break or be compromised if the action's master branch changes unexpectedly. Consider pinning to a tagged version likev4or a specific commit SHA.♻️ Suggested fix
- name: Checkout - uses: actions/checkout@master + uses: actions/checkout@v4 with: submodules: recursive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 23 - 24, The workflow Checkout step currently uses actions/checkout@master which is not pinned; update the uses field in the "Checkout" step to reference a fixed tagged version or commit SHA (for example actions/checkout@v4 or a specific commit SHA) to ensure reproducible and secure CI runs—locate the "Checkout" step and replace actions/checkout@master with the chosen tag or SHA.haya_ser_macro/src/lib.rs (1)
17-32: 🧹 Nitpick | 🔵 TrivialThe returned attribute is redundantly parsed and unused.
The
crate_namefunction returns anOption<&syn::Attribute>that is captured as_attrbut never used in theserializeordeserializefunctions. Meanwhile, bothserialize.rs(lines 67–69) anddeserialize.rs(lines 68–71) independently iterate through and parse themserattributes frominput.attrsto check for thevarintflag. This means the attribute found and returned bycrate_nameis discarded, and the modules re-parse the attributes from scratch. Either use the returned attribute to avoid redundant parsing, or remove the return value if it's not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@haya_ser_macro/src/lib.rs` around lines 17 - 32, crate_name currently returns an Option<&syn::Attribute> that is found but never used downstream (captured as _attr), causing serialize.rs and deserialize.rs to re-scan input.attrs for the same `mser` attribute; fix by either (A) using the returned Option<&syn::Attribute> from crate_name in serialize and deserialize so they parse the found attribute once (pass the attribute through the calls to the functions that check for the `varint` flag), or (B) simplify crate_name to only return the syn::Path (remove the Option<&syn::Attribute> from its signature and callers) and let serialize.rs/deserialize.rs own attribute parsing—update function signatures and all call sites accordingly (look for crate_name, serialize.rs, deserialize.rs, and the _attr variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@haya_ser/src/varint.rs`:
- Around line 225-262: The empty if branch around the 4-byte case makes the
logic hard to follow; refactor the block that writes continuation bytes in the
function handling varint (the section using n, w.write and w.write_byte) so the
4-byte case is explicit instead of an empty branch: detect the 4-byte range (n &
0xFFFFFFFFF0000000 == 0) and handle it in its own branch that writes the
four-byte sequence, otherwise emit the initial three bytes with 0x80 and then
fall through to the existing branches that append bytes using (n >> 28), (n >>
35), etc.; keep references to w.write and w.write_byte and preserve all existing
bit checks but remove the empty if to improve readability.
- Around line 309-311: The unsafe cast-and-dereference of
buf.as_ptr().cast::<[u8; 8]>() before calling u64::from_le_bytes assumes the
8-byte alignment of buf and may UB on strict-alignment platforms; instead copy
the eight bytes into a local [u8; 8] (or use ptr::read_unaligned) and then call
u64::from_le_bytes on that array, and replace buf = unsafe {
buf.get_unchecked(8..) } with a safe slice advance (e.g., buf = &buf[8..]) or
otherwise documented safe pointer advance; update the code paths involving
u64::from_le_bytes, the cast buf.as_ptr().cast::<[u8; 8]>() and
buf.get_unchecked(8..) accordingly.
- Around line 21-51: The V21::read implementation fails to mask the third byte
before shifting, allowing the high bit of the third byte to overflow the 21-bit
range; in function V21::read (variables a and p) change the third-byte
accumulation from p |= (a as u32) << 14 to p |= ((a & 0x7F) as u32) << 14 so the
third byte is masked like the first two, leaving the rest of the control flow
(cold_path(), Error handling) untouched.
---
Outside diff comments:
In @.github/workflows/rust.yml:
- Around line 23-24: The workflow Checkout step currently uses
actions/checkout@master which is not pinned; update the uses field in the
"Checkout" step to reference a fixed tagged version or commit SHA (for example
actions/checkout@v4 or a specific commit SHA) to ensure reproducible and secure
CI runs—locate the "Checkout" step and replace actions/checkout@master with the
chosen tag or SHA.
In `@haya_ser_macro/src/lib.rs`:
- Around line 17-32: crate_name currently returns an Option<&syn::Attribute>
that is found but never used downstream (captured as _attr), causing
serialize.rs and deserialize.rs to re-scan input.attrs for the same `mser`
attribute; fix by either (A) using the returned Option<&syn::Attribute> from
crate_name in serialize and deserialize so they parse the found attribute once
(pass the attribute through the calls to the functions that check for the
`varint` flag), or (B) simplify crate_name to only return the syn::Path (remove
the Option<&syn::Attribute> from its signature and callers) and let
serialize.rs/deserialize.rs own attribute parsing—update function signatures and
all call sites accordingly (look for crate_name, serialize.rs, deserialize.rs,
and the _attr variable).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/rust.yml (1)
23-24:⚠️ Potential issue | 🟠 MajorPin GitHub Actions to immutable refs to reduce supply chain risk.
Using
@mastercan introduce unreviewed changes into CI. Pin bothactions/checkout(line 24) andcrate-ci/typos(line 41) to a specific commit SHA (preferred) or a stable version tag, not to branch heads or@latest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 23 - 24, Replace branch/head refs for the GitHub Actions usages with immutable refs: change uses: actions/checkout@master to a specific tag or commit SHA (e.g., actions/checkout@v3 or @<commit-sha>) and likewise pin crate-ci/typos (uses: crate-ci/typos@...) to a stable tag or commit SHA; update both the actions/checkout and crate-ci/typos entries in the workflow so the workflow references explicit immutable versions rather than branch names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/rust.yml:
- Around line 23-24: Replace branch/head refs for the GitHub Actions usages with
immutable refs: change uses: actions/checkout@master to a specific tag or commit
SHA (e.g., actions/checkout@v3 or @<commit-sha>) and likewise pin crate-ci/typos
(uses: crate-ci/typos@...) to a stable tag or commit SHA; update both the
actions/checkout and crate-ci/typos entries in the workflow so the workflow
references explicit immutable versions rather than branch names.
Summary by CodeRabbit
Chores
Refactor