Merged
Conversation
📝 WalkthroughWalkthroughRefactors NBT stringify/parse to a stack-driven model, adds TagArray/ByteArray and Component, changes packet macro patterns and ID generation, adds several packet types, alters integer parsing to return remaining slices, replaces JsonStr with a char-width API, swaps a dependency, and changes data::block_state::static_bounds non‑debug to return u64. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as rgba(46,136,87,0.5) Caller
participant Stringify as rgba(52,152,219,0.5) StringifyCompound
participant NBT as rgba(155,89,182,0.5) Tag/TagArray
participant Buffer as rgba(241,196,15,0.5) Buffer
Caller->>Stringify: encode(&Compound)
Stringify->>Stringify: push frame (compound/list/array)
Stringify->>NBT: resolve TagType & value
NBT->>Buffer: write bytes (primitives/arrays)
Stringify->>Stringify: pop frame
Stringify->>Caller: return encoded bytes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protocol/src/nbt/stringify.rs (1)
67-137:⚠️ Potential issue | 🔴 CriticalFix long-array prefix consumption in
darr.
The[L;branch advances only 2 bytes, leaving;in the buffer and breaking parsing (or looping). It should skip 3 bytes like the other array branches.🐛 Proposed fix
- *n = n.get_unchecked(2..); + *n = n.get_unchecked(3..);
🤖 Fix all issues with AI agents
In `@protocol/src/clientbound.rs`:
- Around line 61-73: The commented-out packet stubs (registry_data,
resource_pack_pop, resource_pack_push, store_cookie, transfer,
update_enabled_features, update_tags, select_known_packs, custom_report_details,
server_links, clear_dialog, show_dialog, code_of_conduct) should not remain as
drifting comments; either remove these commented lines or replace each with a
single-line TODO referencing an issue or tracker ID so intent is preserved
(e.g., add TODO with issue number and short description next to the packet
name). Update the file's comment block around clientbound.rs to remove the
unused commented stubs or convert them into explicit TODO entries that reference
your issue tracker and owner to keep the codebase clean and actionable.
In `@protocol/src/nbt/stringify.rs`:
- Around line 1155-1174: The Bl::LongArray branch in stringify.rs is emitting
the byte-array prefix b"[B;" causing long arrays to be serialized as byte
arrays; update that branch so it writes the long-array prefix b"[L;" instead
(inside the Bl::LongArray match arm where buf.extend(b"[B;") is used), keeping
the rest of the loop behavior (itoa_buf formatting, DELIMITER usage, appending
'l' suffix, and closing bracket) unchanged.
In `@protocol/src/serverbound/common.rs`:
- Line 1: The file serverbound::common (protocol/src/serverbound/common.rs) is
empty; either remove the empty module file or add a brief module-level doc
comment explaining its intended purpose (e.g., "//! Placeholder for serverbound
common types and utilities — add content when needed") so it isn't a stray empty
file; update the module declaration that references it if you remove it.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protocol/src/nbt/stringify.rs (1)
86-96:⚠️ Potential issue | 🟡 MinorIncomplete boolean validation accepts malformed input.
The boolean parsing only checks the first character and input length, but doesn't verify the remaining characters. For example,
tXXXwould be accepted astrueandfXXXXasfalse.Consider validating the full boolean keyword:
🛡️ Proposed fix
- (_, 0) => match peek(n)? { - b't' | b'T' if n.len() >= 4 => (1, 4), - b'f' | b'F' if n.len() >= 5 => (0, 5), - _ => return Err(Error), - }, + (_, 0) => match n.get(..4) { + Some(b"true" | b"True" | b"TRUE") => (1, 4), + _ => match n.get(..5) { + Some(b"false" | b"False" | b"FALSE") => (0, 5), + _ => return Err(Error), + }, + },
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Around line 976-980: The Tag::String arm currently writes raw bytes and
doesn't escape quotes/backslashes, breaking roundtrip with the dqstr2 decoder;
add an encode_string(buf: &mut Vec<u8>, s: &str) helper that wraps the string in
double quotes and escapes `"` as `\"` and `\` as `\\`, then replace the
Tag::String handling to call encode_string(buf, x.as_str()) so SNBT output is
valid and decodable by dqstr2.
- Around line 1106-1117: Replace the manual byte pushes in the Bl::String(x)
match arm with the shared encode_string helper to properly escape string
contents: in the Bl::String(x) branch (inside stringify.rs) iterate over x and
for each element call encode_string(&mut buf, y.as_str()) and insert DELIMITER
between items instead of directly pushing b'"' and raw bytes; ensure you
preserve the existing delimiter logic (flag/first-item handling) and use the
same encode_string signature used elsewhere to maintain consistent escaping.
- Around line 469-481: The boolean parsing in stringify.rs uses the match arms
b't'|b'T' and b'f'|b'F' and only advances the slice pointer n without validating
the full keywords; update these branches to verify the following bytes match
"rue" (or case-insensitive) for true and "alse" for false before assigning *n =
n.get(4..) / n.get(5..) and pushing the value (list.push(1) / list.push(0)),
returning Err(Error) if the remainder doesn't match; follow the same
complete-keyword validation approach used in darr to ensure correct parsing and
error handling.
- Around line 253-267: The truncation calculation uses rest.len() which already
excludes the 4 length bytes and causes underflow; change the new_len computation
to derive from names.len() instead (e.g. let new_len = names.len() - (len + 4))
when handling the match arm for [ref rest @ .., l1, l2, l3, l4], and add a
safety check (or use checked/saturating arithmetic) to ensure len + 4 <=
names.len() before calling names.truncate(new_len); update the block around
BoxStr::new_unchecked / Tag construction accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Line 685: Replace the unsafe unreachable_unchecked() call in the match default
arm (the branch currently written as `_ => unsafe { unreachable_unchecked() }`)
with the safe macro unreachable!() so the code panics with diagnostics instead
of invoking undefined behavior; remove the surrounding unsafe block and use
unreachable!() as the default arm to preserve intent but ensure safety and
better debug information.
- Around line 815-825: The code path in stringify.rs that matches escape bytes
(the matches!(y, ESCAPE | b'\'' | ... | b'N') branch) currently panics for `\x`,
`\u`, `\U`, and `\N` because of todo!() — replace those todo!() calls so the
function returns an Err(Error) for unsupported escape sequences (or implement
full parsing) instead of panicking; specifically update the handlers for y ==
b'x', b'u', b'U', b'N' (in the block that also references buf,
n.get_unchecked(last..cur), y, last..cur) to propagate a proper parsing error
(e.g. Error::UnsupportedEscape or Error::InvalidEscapeSequence) via the
function's Result return path rather than calling todo!().
- Around line 1245-1261: The Bl::Compound branch has mismatched indentation
compared to Bl::C: change the inclusive ranges `0..=bls.len()` used with
buf.extend(SPACE) to exclusive `0..bls.len()` and ensure the indentation that
prints the separator is computed with the same bls length as in Bl::C (i.e.,
compute/emit indentation before you push the two new frames with
`bls.push((Bl::Compound(...), ...))` and `bls.push((Bl::C(...), 0))`) so the
separator and the compound content use the same indentation logic as the `Bl::C`
handling.
- Around line 863-873: The double-quoted string decoder currently calls todo!()
for hex/unicode escapes (y == b'x' | b'u' | b'U'), which can panic; mirror the
dec_sq_str fix by treating unsupported escapes safely instead of panicking: when
encountering b'x', b'u', or b'U' in the match inside the decoder (the block
using variables buf, n, last, cur and matching y against ESCAPE, b'\'', b'"',
b'n', b'x', b'u', b'U', b'N'), copy the raw escape sequence into buf or copy
just the escape introducer plus the available following bytes using checked
slicing (not get_unchecked) so you never panic on short input; replace the
todo!() branches with non-panicking logic consistent with dec_sq_str and keep
the existing handling for ESCAPE, quotes, b'n', and b'N'.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Around line 1000-1001: The loop uses unsafe bls.pop().unwrap_unchecked(),
which assumes the invariant that bls is non-empty; replace that with a safe
unwrap or expect to provide a diagnostic if the invariant is broken (e.g.,
change bls.pop().unwrap_unchecked() to bls.pop().expect("bls unexpectedly empty
in stringify loop") or bls.pop().unwrap()) so a clear panic message appears if
future changes violate the invariant; update the occurrence of
unwrap_unchecked() in the loop in stringify.rs accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Around line 309-314: The code advances past one byte after skip_ws without
checking it is a ':' which accepts malformed SNBT; change the logic around
skip_ws and the subsequent *n = match n.get(1..) to first peek the next
non-whitespace byte and verify it equals b':' (return Err(Error) if not), then
consume that byte and continue (i.e., only advance the slice after confirming
the ':'), referencing the same variables and helpers used here (skip_ws, n,
Error, and the existing get(1..) consumption).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@protocol/Cargo.toml`:
- Line 20: Update the dependency version string for unicode_names2 in Cargo.toml
to match the project's major-only style: replace unicode_names2 = "2.0.0" with
unicode_names2 = "2". Ensure no other changes are made and that the dependency
name unicode_names2 remains unchanged.
In `@protocol/src/nbt/stringify.rs`:
- Around line 839-870: The ESCAPE handling in dec_quoted_str incorrectly
advances cur early and mis-sets last, causing skipped bytes and unsafe slices;
fix it by removing the pre-increment (cur += 2 / last = cur) and instead: call
peek on n.get(cur+1..), extend buf with n.get_unchecked(last..cur) (so the
backslash isn't included), compute the total advance (adv_total) from
quoted_elsape(peek, y) — where adv_total includes the escape byte plus any
payload bytes — append the decoded char (if Some) to buf, then set last = cur +
adv_total and do cur += adv_total once; update uses of adv to refer to this
computed total so unchecked slicing uses correct bounds (symbols:
dec_quoted_str, ESCAPE, cur, last, peek, quoted_elsape, buf, n).
In `@ser/src/json.rs`:
- Around line 8-9: Document that U and E intentionally share the same sentinel
value 0xff so callers cannot distinguish a non-printable control that requires
\uXXXX escaping from an invalid UTF-8 byte; update the definitions of the
constants U and E and/or add a comment near them and the json_char_width_escaped
function to state this design choice and its implication (i.e., both are treated
as special/error and must be handled uniformly by the caller).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Around line 932-942: The b'N' match arm currently uses
core::str::from_utf8_unchecked on bytes between the \N{ and } which can cause UB
for invalid UTF-8; change the logic in that arm (the branch that calls
unicode_names2::character inside the unsafe from_utf8_unchecked) to first call
core::str::from_utf8(rest.get_unchecked(0..index)) and handle the Result—on Err
return None (or otherwise propagate/handle the parse error) and on Ok pass the
safe &str into unicode_names2::character so the unsafe conversion is removed;
keep the existing find_ascii and unicode_names2::character usage and only alter
the conversion/error handling around the byte slice.
- Around line 839-866: The branch handling ESCAPE in stringify.rs leaves `last`
pointing at the backslash so later slicing copies the raw escape bytes; update
`last` to skip the consumed escape sequence bytes after you compute `adv` (i.e.
set last = cur + adv or move the `last` assignment to after computing `adv`) in
the ESCAPE branch inside the loop that uses `cur`, `last`, and `adv` (the block
that calls `peek(...)` and `quoted_elsape(...)`) so subsequent `n[last..cur]`
slices don't include the raw escape characters.
- Around line 79-85: The parse_int_s function currently returns (0,
original_slice) on invalid input which hides parse failures; change parse_int_s
to signal failure (e.g., return Result<(i8, &[u8]), ParseError> or
Option<(i8,&[u8])>) when no digits were consumed, and update the caller in
stringify.rs (the parse_int_s::<i8>(n) call and surrounding logic that peeks for
'B') to propagate/handle that error instead of silently treating 0 as
valid—specifically ensure the caller checks that the returned slice advanced (or
matches Ok/Some) and returns an error if not, and adjust signatures and error
propagation in the surrounding parse functions accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Around line 839-852: In the ESCAPE-handling branch inside stringify (where you
call peek(..) and then match quoted_elsape(peek, y)), don't silently drop
invalid escapes: change the None arm of the quoted_elsape(peek, y) match to
return Err(Error) instead of advancing by 2; i.e., when quoted_elsape returns
None, propagate a parse error (return Err(Error)) so malformed escapes fail
rather than being skipped (update the match that currently has Some((ch, adv))
=> ... and None => 2 to use None => return Err(Error)).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ser/src/float.rs (1)
1249-1266:⚠️ Potential issue | 🔴 CriticalPropagate
is_positiveflag toparse_inf_nanto ensure correct sign handling for special values.The sign handling for numeric values is correct, but there is a bug in special value handling. When
is_positive = Some(false)(indicating a negative number) and the input is"inf"or"nan", the following occurs:
parse_numberreturnsNone(no numeric digits found)parse_inf_nan(s)is called at line 1102 without theis_positiveparameterparse_inf_nanreturns positive infinity/NaN instead of negativeFor example, with
is_positive = Some(false)and input"inf", the function returnsF::INFINITYinstead ofF::NEG_INFINITY. Theis_positiveflag information is lost because it is not propagated toparse_inf_nan.The fix requires passing
is_positivetoparse_inf_nanand using it to determine the correct sign for the returned special value.
🤖 Fix all issues with AI agents
In `@protocol/src/nbt/stringify.rs`:
- Around line 791-802: The radix-detection branch incorrectly treats any digit
followed by 'x'/'b' as a prefix (e.g., "1xFF"), so update the match in the
function that calls peek (referencing peek, variable n and enum Radix) to only
consider hexadecimal/binary prefixes when the leading digit is '0' — i.e.,
change the outer match arm from (b'0'..=b'9', rest) to distinguish b'0' and
other digits, handling [b'x'|b'X', rest @ ..] and [b'b'|b'B', rest @ ..] only
when the first byte is b'0', otherwise default to Radix::Decimal and do not
consume the leading digit.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ser/src/float.rs (2)
1255-1266:⚠️ Potential issue | 🟠 MajorSign hint leaves explicit
+/-in the buffer, causing parse failures.
Whenis_positiveisSome, a leading+/-is not consumed, so inputs likeb"+12"/b"-12"with a hint will returnNone. Consider consuming and validating the explicit sign (or explicitly asserting that the sign is already stripped).Proposed fix (consume/validate explicit sign even with a hint)
- let mut negative = false; - if let Some(is_positive) = is_positive { - negative = !is_positive; - } else if s.first() == b'-' { + let mut negative = false; + if let Some(is_positive) = is_positive { + negative = !is_positive; + if s.first_either(b'-', b'+') { + let explicit_negative = s.first_is(b'-'); + if explicit_negative != negative { + return None; + } + if s.step().is_empty() { + return None; + } + } + } else if s.first() == b'-' { negative = true; if s.step().is_empty() { return None; }
1364-1390:⚠️ Potential issue | 🟠 MajorSigned NaN/Inf path fails when a hint is provided but input includes a sign.
Withis_positive = Some,+inf/-nanwon’t match because the sign remains ins. Align this withparse_numberby consuming/validating the explicit sign and adjusting the consumed length.Proposed fix (consume/validate sign and track length)
- let (s, is_positive_) = match is_positive { - Some(x) => (s, x), - None => { - let first = s.get_first(); - if first == b'+' { - let s = s.advance(1); - (s, true) - } else if first == b'-' { - let s = s.advance(1); - (s, false) - } else { - (s, true) - } - } - }; - if is_positive_ { - if s.eq_ignore_case(b"nan") { - return (F::NAN, 4); - } else if s.eq_ignore_case(b"inf") { - return (F::INFINITY, 1 + parse_inf_rest(s)); - } - } else { - if s.eq_ignore_case(b"nan") { - return (F::NEG_NAN, 4); - } else if s.eq_ignore_case(b"inf") { - return (F::NEG_INFINITY, 1 + parse_inf_rest(s)); - } - } + let (s, is_positive_, sign_len) = match is_positive { + Some(x) => { + if s.check_first2(b'+', b'-') { + let explicit_pos = s.get_first() == b'+'; + if explicit_pos != x { + return (F::default(), 0); + } + (s.advance(1), x, 1) + } else { + (s, x, 0) + } + } + None => { + let first = s.get_first(); + if first == b'+' { + let s = s.advance(1); + (s, true, 1) + } else if first == b'-' { + let s = s.advance(1); + (s, false, 1) + } else { + (s, true, 0) + } + } + }; + if is_positive_ { + if s.eq_ignore_case(b"nan") { + return (F::NAN, sign_len + 3); + } else if s.eq_ignore_case(b"inf") { + return (F::INFINITY, sign_len + parse_inf_rest(s)); + } + } else { + if s.eq_ignore_case(b"nan") { + return (F::NEG_NAN, sign_len + 3); + } else if s.eq_ignore_case(b"inf") { + return (F::NEG_INFINITY, sign_len + parse_inf_rest(s)); + } + }
🤖 Fix all issues with AI agents
In `@ser/src/float.rs`:
- Around line 1095-1103: Add unit tests for the public function parse_float to
cover the new is_positive parameter and edge cases: write tests in the ser crate
that call parse_float with Some(true), Some(false), and None where appropriate,
asserting correct parsed values and returned byte counts for normal numbers and
for special values by also exercising parse_inf_nan and parse_number paths;
ensure tests mirror the real call patterns used by the protocol::nbt::stringify
callers so signed/unsigned inputs are covered and exported API behavior (as
exposed via ser/src/lib.rs) is validated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ser/src/float.rs`:
- Around line 1095-1103: Add a short doc comment above the public function
parse_float that clearly documents the is_positive parameter contract: explain
that is_positive is an Option<bool> where Some(true) means an explicit '+' sign
was parsed, Some(false) means an explicit '-' sign was parsed, and None means no
sign information was provided (caller is agnostic); describe how parse_float
(and its internal calls parse_number and parse_inf_nan) will interpret each case
and what return behavior callers should expect. Ensure the comment is concise
and placed immediately above pub fn parse_float<F: Float>(s: &[u8], is_positive:
Option<bool>) -> (F, usize).
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 by CodeRabbit
New Features
Refactor
Chores
Breaking Changes