PushParser: scope the input chunk's lifetime to write() instead of the struct - #98
PushParser: scope the input chunk's lifetime to write() instead of the struct#98dgrantpete wants to merge 2 commits into
Conversation
…e struct Fixes kaidokert#97. PushContentBuilder no longer stores the chunk slice (and so loses its 'input parameter); write() pairs the persistent builder state with the chunk in a per-call PushChunkExtractor view, which implements ContentExtractor and DataSource for the duration of the call. Since any partial token is already copied into the scratch buffer before write() returns, no behavior changes — set_chunk/reset_input simply disappear along with the possibility of the chunk outliving the call. This lets callers feed chunks from a reused receive buffer (the embedded network-loop shape), which previously failed to borrow-check. New regression test covers that pattern, including tokens split across reuse boundaries (strings, \u escapes, surrogate pairs, multi-byte UTF-8, numbers). Note: breaking for code that names PushParser's lifetimes explicitly (PushParser<'input, 'scratch, H, C> -> PushParser<'scratch, H, C>); construction through new() with inference is unaffected.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
DataSourceimpl forPushChunkExtractorintroduces four lifetime parameters ('i, 's, 'a, 'chunk, 'scratch) with a singlewhere 'chunk: 'iconstraint; consider simplifying this by tying'i/'sdirectly to'chunk/'scratch(or using fewer named lifetimes) to make the lifetime relationships easier to understand and maintain. - Several
PushChunkExtractormethods (e.g.handle_byte_accumulation,apply_unescaped_reset_if_queued) are thin delegations toPushContentBuilder; you might consider whether some of these can be called directly on the builder from the call sites to keep the wrapper surface smaller and reduce indirection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DataSource` impl for `PushChunkExtractor` introduces four lifetime parameters (`'i, 's, 'a, 'chunk, 'scratch`) with a single `where 'chunk: 'i` constraint; consider simplifying this by tying `'i`/`'s` directly to `'chunk`/`'scratch` (or using fewer named lifetimes) to make the lifetime relationships easier to understand and maintain.
- Several `PushChunkExtractor` methods (e.g. `handle_byte_accumulation`, `apply_unescaped_reset_if_queued`) are thin delegations to `PushContentBuilder`; you might consider whether some of these can be called directly on the builder from the call sites to keep the wrapper surface smaller and reduce indirection.
## Individual Comments
### Comment 1
<location path="picojson/src/push_content_builder.rs" line_range="259-268" />
<code_context>
+ pub(crate) fn copy_partial_content_to_scratch(&mut self) -> Result<(), ParseError> {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The number token start-position invariant is subtle; consider making it more explicit or guarded.
The `State::Number(start_pos)` branch assumes `start_pos` always refers to the character *before* the first digit, so `content_start = start_pos + 1` is correct only under that invariant. If the parser ever changes `State::Number` to point at the first digit instead, this will silently become off‑by‑one. Adding an assertion for this invariant or a helper to derive `content_start` from `State` would make the code more robust and maintainable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub(crate) fn copy_partial_content_to_scratch(&mut self) -> Result<(), ParseError> { | ||
| // Determine the start of the current token content based on parser state | ||
| let content_start = match self.builder.parser_state { | ||
| State::String(start_pos) | State::Key(start_pos) => { | ||
| // For strings and keys, content starts after the opening quote | ||
| start_pos + 1 | ||
| } | ||
| State::Number(start_pos) => { | ||
| // For numbers, start_pos points to the character before the first digit | ||
| // so we need to add 1 to get to the actual number content |
There was a problem hiding this comment.
suggestion (bug_risk): The number token start-position invariant is subtle; consider making it more explicit or guarded.
The State::Number(start_pos) branch assumes start_pos always refers to the character before the first digit, so content_start = start_pos + 1 is correct only under that invariant. If the parser ever changes State::Number to point at the first digit instead, this will silently become off‑by‑one. Adding an assertion for this invariant or a helper to derive content_start from State would make the code more robust and maintainable.
… trait lifetimes The receiver's implied bounds already provide 'chunk: 'i / 'scratch: 's, and a comment now explains why DataSource's lifetimes are not tied to the struct's: call sites take short reborrows of the view, which lives shorter than the chunk.
|
Went through the Sourcery feedback — one item taken, two declined with reasons: Lifetime parameters on the Thin delegation methods — declined. The call sites in Number start-position invariant ( |
1 participant
Fixes #97.
What
PushParserloses its'inputstruct lifetime;write()now takes thechunk with a per-call borrow. Callers can feed chunks from a reused receive
buffer — the embedded network-loop shape — which previously failed to
borrow-check.
How
PushContentBuilderkeeps only the state that must survive acrosswrite()calls and no longer stores the chunk slice.
write()pairs it with thecurrent chunk in a new per-call
PushChunkExtractorview (crate-internal),which implements
ContentExtractorandDataSourcefor the duration of thecall. Since
write()already copied any partial token into the scratchbuffer before returning (
copy_partial_content_to_scratch) and then resetthe stored slice, no behavior changes —
set_chunk/reset_inputsimplydisappear, along with the possibility of the chunk outliving the call.
Tests
push_parser_buffer_reuse.rs: parses through a single fixed bufferthat is overwritten (and scrambled past the chunk length) between
writecalls, at chunk sizes 1–32, with tokens deliberately split across reuse
boundaries: strings,
\uescapes, a surrogate pair, multi-byte UTF-8,numbers.
tests). The only test edit is
push_parser_stress_test.rsdropping thenow-gone lifetime parameter.
through a reused 4 KB buffer at chunk sizes 1 / 7 / 1379 / 4096 and
compared event streams against whole-buffer parses — identical.
Semver note
Breaking for code that names the lifetimes explicitly
(
PushParser<'input, 'scratch, H, C>→PushParser<'scratch, H, C>);construction through
new()with inference is unaffected.🤖 Generated with Claude Code
https://claude.ai/code/session_014XVVgKWoTRowDeeVySv5Sj
Summary by Sourcery
Scope PushParser input chunks to each write call via a new per-call extractor, removing the long-lived input lifetime from the parser while preserving behavior.
Enhancements:
Tests: