Release 0.3.5 — trivia round-trip + %recover wiring#26
Merged
Conversation
…ding duplication (Bug C)
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughParser correctness fixes for trivia attribution across backtracking, packrat cache-hits, and rule-exit handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Parser Caller
participant Peg as PegEngine
participant Context as ParsingContext
participant Cache as PackratCache
participant Gen as GeneratedRule (ParserGenerator)
Caller->>Peg: parseRule(rule)
Peg->>Context: savePendingLeadingTrivia()
Peg->>Cache: lookup(rule,pos)
alt cache hit (success)
Cache-->>Peg: cachedNode + endPos
Peg->>Context: drain pendingLeadingTrivia
Peg->>Context: skipWhitespace(if tokenBoundaryDepth==0)
Peg->>Gen: attach reconstructed leading trivia to cachedNode
Peg-->>Caller: return success with cachedNode at endPos
else cache miss / parse body
Peg->>Gen: execute generated rule method
Gen->>Context: may recordFailureRecoveryOverride(term) on failure
Gen-->>Peg: node or null
alt success
Peg->>Context: clearPendingRecoveryOverride()
Peg->>Peg: attachTrailingToTail(unclaimed pendingLeadingTrivia)
Peg->>Cache: store adjusted cacheable node
Peg-->>Caller: return success
else failure
Peg->>Context: recordFailureRecoveryOverride(term)
Peg-->>Caller: return failure (recovery selection consults pending override)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 16 minutes and 20 seconds. 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: 2
🧹 Nitpick comments (1)
peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.java (1)
1929-1934: 💤 Low valueConsider consolidating duplicate trivia concatenation methods.
concatTrivia(line 1929) andcombineTrivia(line 2025) perform nearly identical operations. The difference is thatcombineTriviahas early-return optimizations for empty lists. Consider consolidating into a single method with the optimizations.♻️ Proposed consolidation
- private static List<Trivia> concatTrivia(List<Trivia> first, List<Trivia> second) { - var combined = new ArrayList<Trivia>(first.size() + second.size()); - combined.addAll(first); - combined.addAll(second); - return List.copyOf(combined); - } - // ... later in the file ... private static List<Trivia> combineTrivia(List<Trivia> first, List<Trivia> second) { if (first.isEmpty()) return second; if (second.isEmpty()) return first; var combined = new ArrayList<Trivia>(first.size() + second.size()); combined.addAll(first); combined.addAll(second); return List.copyOf(combined); }Then replace all
concatTriviacalls withcombineTrivia(which has the early-return optimizations).Also applies to: 2025-2032
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.java` around lines 1929 - 1934, Replace the two near-duplicate helpers by keeping a single optimized method (preferably combineTrivia) that implements the early-return checks for empty lists and concatenation logic, then remove concatTrivia and update all callers to call combineTrivia instead; locate the methods named concatTrivia and combineTrivia in PegEngine, copy combineTrivia's empty-check/early-return behavior into the retained method, ensure it returns an immutable List (List.copyOf) and update any references to concatTrivia to use combineTrivia.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@peglib-core/src/main/java/org/pragmatica/peg/generator/ParserGenerator.java`:
- Around line 2694-2704: The left-recursive wrapper path sets finalResult and
cacheableResult from lastSeed without draining pendingLeadingTrivia, so trailing
trivia consumed by the LR body remains unclaimed; modify the LR wrapper exit
(the branch that uses lastSeed, attachLeadingTrivia, restoreLocation and
finalResult = CstParseResult.success(...)) to drain pendingLeadingTrivia and
attach it as trailing trivia to the returned node (similar to the non-LR
parseRule fix), ensure the node passed to CstParseResult.success includes those
trailing Trivia nodes, and update cacheableResult to reflect the
drained/modified lastSeed so the cached seed contains the trivia changes.
In `@peglib-core/src/main/java/org/pragmatica/peg/parser/ParsingContext.java`:
- Around line 393-406: Backtracking combinators currently restore location and
pending-leading-trivia but do not restore or clear
pendingFailureRecoveryOverride, letting a failed alternative's override leak
into subsequent alternatives; update each backtracking path in
parseChoiceWithMode, parseOptionalWithMode, parseZeroOrMoreWithMode,
parseOneOrMoreWithMode, and parseRepetitionWithMode to save the current
pendingFailureRecoveryOverride before trying an alternative and restore it on
backtrack (or call clearPendingRecoveryOverride() on entry/after restoring
state) so that recordFailureRecoveryOverride() is isolated to the attempt
context and first-write-wins only applies within the same attempt.
---
Nitpick comments:
In `@peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.java`:
- Around line 1929-1934: Replace the two near-duplicate helpers by keeping a
single optimized method (preferably combineTrivia) that implements the
early-return checks for empty lists and concatenation logic, then remove
concatTrivia and update all callers to call combineTrivia instead; locate the
methods named concatTrivia and combineTrivia in PegEngine, copy combineTrivia's
empty-check/early-return behavior into the retained method, ensure it returns an
immutable List (List.copyOf) and update any references to concatTrivia to use
combineTrivia.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f9fe090-6c58-46a5-b2ee-7b2865aec541
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mddocs/RELEASE-PLAN-0.3.5-0.4.0.mddocs/TRIVIA-ATTRIBUTION.mdpeglib-core/pom.xmlpeglib-core/src/main/java/org/pragmatica/peg/generator/ParserGenerator.javapeglib-core/src/main/java/org/pragmatica/peg/parser/ParsingContext.javapeglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.javapeglib-core/src/test/java/org/pragmatica/peg/error/RecoverDirectiveProofTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/RoundTripTest.javapeglib-core/src/test/resources/perf-corpus-baseline/large/FactoryClassGenerator.java.txt.hashpeglib-core/src/test/resources/perf-corpus-baseline/large/FactoryClassGenerator.java.txt.ruleHits.txtpeglib-core/src/test/resources/perf-corpus-baseline/ruleCoverage.txtpeglib-formatter/pom.xmlpeglib-incremental/pom.xmlpeglib-maven-plugin/pom.xmlpeglib-playground/pom.xmlpom.xml
Comment on lines
2694
to
+2704
| sb.append(" if (lastSeed.isSuccess()) {\n"); | ||
| sb.append(" restoreLocation(lastSeed.endLocation.unwrap());\n"); | ||
| sb.append(" var node = attachLeadingTrivia(lastSeed.node.unwrap(), leadingTrivia);\n"); | ||
| sb.append(" finalResult = CstParseResult.success(node, lastSeed.text.or(\"\"), lastSeed.endLocation.unwrap());\n"); | ||
| // Bug C fix: cache the empty-leading lastSeed (body emission attaches no | ||
| // leading trivia). The wrapped node above is for return only. Bug C' is | ||
| // intentionally NOT applied at the LR settle path: PegEngine's LR path | ||
| // does not apply rule-exit trailing-trivia attribution either, and the | ||
| // interpreter's parseRule (non-LR) fix alone is sufficient for full | ||
| // round-trip equivalence on the corpus. | ||
| sb.append(" cacheableResult = lastSeed;\n"); |
There was a problem hiding this comment.
Apply the rule-exit trailing-trivia fix to left-recursive wrappers too.
This branch still returns/caches lastSeed without draining pendingLeadingTrivia, so any trivia consumed by the LR body’s final inter-element skipWhitespace() is left to be claimed by the next sibling instead of this rule’s tail. That means Bug C' is still present for left-recursive rules even though the non-LR path was fixed. Based on learnings: Group whitespace and comments as Trivia nodes in the CST for lossless tree representation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@peglib-core/src/main/java/org/pragmatica/peg/generator/ParserGenerator.java`
around lines 2694 - 2704, The left-recursive wrapper path sets finalResult and
cacheableResult from lastSeed without draining pendingLeadingTrivia, so trailing
trivia consumed by the LR body remains unclaimed; modify the LR wrapper exit
(the branch that uses lastSeed, attachLeadingTrivia, restoreLocation and
finalResult = CstParseResult.success(...)) to drain pendingLeadingTrivia and
attach it as trailing trivia to the returned node (similar to the non-LR
parseRule fix), ensure the node passed to CstParseResult.success includes those
trailing Trivia nodes, and update cacheableResult to reflect the
drained/modified lastSeed so the cached seed contains the trivia changes.
Comment on lines
+393
to
+406
| public void recordFailureRecoveryOverride(String terminator) { | ||
| if (terminator != null && !terminator.isEmpty() && pendingFailureRecoveryOverride.isEmpty()) { | ||
| pendingFailureRecoveryOverride = Option.some(terminator); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 0.3.5 (Phase 5) — clear any pending failure override. Called on rule | ||
| * SUCCESS so that an override recorded by a failed inner alternative of | ||
| * a backtracking combinator (Choice/Optional/etc.) does not leak forward | ||
| * into a later, unrelated recovery cycle. | ||
| */ | ||
| public void clearPendingRecoveryOverride() { | ||
| pendingFailureRecoveryOverride = Option.none(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites for pending recovery override APIs =="
rg -n -C4 '\brecordFailureRecoveryOverride\s*\(|\bclearPendingRecoveryOverride\s*\(' \
peglib-core/src/main/java/org/pragmatica/peg/parser
echo
echo "== Nearby backtracking / recovery logic in PegEngine =="
rg -n -C6 'clearPendingRecoveryOverride|recordFailureRecoveryOverride|restoreLocation|Choice|Optional|And|Not' \
peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.javaRepository: siy/java-peglib
Length of output: 31059
Confirm: pending recovery override leaks between alternatives in backtracking combinators.
When a Choice/Optional/ZeroOrMore alternative fails and backtracks, it restores the parser location and pending-leading-trivia, but NOT the pending-failure-recovery-override. This allows a failed alternative's override to pollute subsequent alternatives:
- Alt1 fails, records override via its rule's
%recover - Choice restores location + pending-leading-trivia only
- Alt2 fails, attempts
recordFailureRecoveryOverride(), but first-write-wins prevents it from overwriting Alt1's stale override - Alt2 is forced to use Alt1's terminator when recovery is needed
This violates the isolation expectation for distinct attempt contexts. The fix is to save/restore or explicitly clear pendingFailureRecoveryOverride when backtracking in parseChoiceWithMode, parseOptionalWithMode, parseZeroOrMoreWithMode, parseOneOrMoreWithMode, and parseRepetitionWithMode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@peglib-core/src/main/java/org/pragmatica/peg/parser/ParsingContext.java`
around lines 393 - 406, Backtracking combinators currently restore location and
pending-leading-trivia but do not restore or clear
pendingFailureRecoveryOverride, letting a failed alternative's override leak
into subsequent alternatives; update each backtracking path in
parseChoiceWithMode, parseOptionalWithMode, parseZeroOrMoreWithMode,
parseOneOrMoreWithMode, and parseRepetitionWithMode to save the current
pendingFailureRecoveryOverride before trying an alternative and restore it on
backtrack (or call clearPendingRecoveryOverride() on entry/after restoring
state) so that recordFailureRecoveryOverride() is isolated to the attempt
context and first-write-wins only applies within the same attempt.
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
Trivia attribution correctness — full byte-equal round-trip on all 22 perf-corpus fixtures — plus
%recoverdirective wiring on the interpreter side.RoundTripTestre-enabled.Five distinct trivia-attribution bugs (Bug A through Bug C'') diagnosed and fixed in this release, plus the
%recoverend-to-end wiring fix:ParsingContext.savePending/restorePendingchanged from size-only to fullList<Trivia>so backtracked branches don't permanently lose drained pending trivia.attachLeadingTriviashort-circuit then preserved stale leading on empty-pending hits, duplicating trivia. Fix: cache an empty-leading version, return the leading-applied version.skipWhitespace(e.g. before an empty ZoM/Optional) ends up in pending with no claimant. At rule exit, attach to the last child'strailingTrivia. Pos is not rewound — predicate combinators rely on it.childrenon element failure. Symptom: a successful trailing comma appeared as a child of both the inner ZoM-NT and the outer Sequence. Fix: snapshot children at Sequence start, restore on element failure.%recover— Override popped infinallybeforeparseWithRecoveryconsulted it. Fix: capture into per-context field with deepest-wins semantics; new regression test proves override and default produce distinct recovery landing points.Generator-side
%recoverper-rule overrides remain deferred to 0.3.6.large/FactoryClassGenerator.java.txtCST hash baseline regenerated (the Bug C'' fix removes a duplicate trailing comma in enum-constant lists). Other 21 fixtures unchanged.Test plan
Summary by CodeRabbit
New Features
%recoverwiring completed; recovery overrides are honored.Bug Fixes
Tests
%recoverchanges landing points.Documentation