Release 0.5.1#33
Merged
Merged
Conversation
…dent attribution investigation)
…ction rework viable)
…eading (Step 4 commit 2)
… flag (Step 4 commit 5)
…d (bug found in post-pass on Java 25 corpus)
…tching engine Bug C' (Step 4 commit 7)
… fix + 6 retry shipped)
…ingSpan, .text() returns String via lazy materialization (Cleanup F.1)
…erialization (Cleanup F.3)
… real workload benefits)
📝 WalkthroughWalkthroughThis release (0.5.1) implements context-independent trivia reattribution via a new ChangesTrivia Post-Pass Attribution Rework
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 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)
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: 5
🧹 Nitpick comments (5)
docs/HANDOVER.md (1)
690-690: ⚡ Quick winFinal summary line is extremely dense.
Line 690 is a single paragraph containing:
- Branch and commit count
- Test counts (805 + 1 skipped for peglib-core)
- Trivia rework state
- Cumulative selfhost results (-5%)
- Reference fixture results (+30%)
- StringSpan addition
- Move B post-mortem reference
- Three next-session options
Consider splitting this into a bulleted list or multiple paragraphs for readability. The information is valuable but difficult to parse in a single 250+ word block.
♻️ Suggested format improvement
- Branch `release-0.5.1` at `0b29c78` — 20 commits past 0.5.1 base. peglib-core 805 tests + 1 pre-existing skipped, all green at the new default (post-pass + StringSpan). **Trivia rework + cleanup state: SHIPPED + bench-validated.** Cumulative selfhost wallclock now **-5% under legacy** (was at parity); reference fixture +30% over legacy (per-NonTerminal scan dominates; future work). 30 new StringSpanTest cases + StringSpan added as `org.pragmatica.peg.tree.StringSpan` (CharSequence view with lazy String materialization). Move B post-mortem: [`docs/incremental/THROUGHPUT-ENGINE-MOVE-B.md`](incremental/THROUGHPUT-ENGINE-MOVE-B.md) §11. Next session: Lever B retry (fallback-rule-bypass blocker remains; trivia-context-loss blocker is RESOLVED) OR Lever C IR unification OR address reference-fixture per-NonTerminal scan cost (cache scanWhitespace, skip NonTerminals with full-coverage children, batch gap-scans). +**Branch state:** `release-0.5.1` at `0b29c78` — 20 commits past 0.5.1 base. + +**Test status:** peglib-core 805 tests + 1 pre-existing skipped, all green at the new default (post-pass + StringSpan). + +**Trivia rework + cleanup state: SHIPPED + bench-validated.** +- Cumulative selfhost wallclock: **-5% under legacy** (was at parity) +- Reference fixture: +30% over legacy (per-NonTerminal scan dominates; future work) +- 30 new StringSpanTest cases +- StringSpan added as `org.pragmatica.peg.tree.StringSpan` (CharSequence view with lazy String materialization) + +**Move B post-mortem:** [`docs/incremental/THROUGHPUT-ENGINE-MOVE-B.md`](incremental/THROUGHPUT-ENGINE-MOVE-B.md) §11 + +**Next session options:** +- Lever B retry (fallback-rule-bypass blocker remains; trivia-context-loss blocker is RESOLVED) +- Lever C IR unification +- Address reference-fixture per-NonTerminal scan cost (cache scanWhitespace, skip NonTerminals with full-coverage children, batch gap-scans)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/HANDOVER.md` at line 690, The final summary paragraph (the dense block mentioning Branch `release-0.5.1` at `0b29c78`, peglib-core test counts, Trivia rework, cumulative selfhost -5%, reference fixture +30%, addition of StringSpan / org.pragmatica.peg.tree.StringSpan, and the Move B post-mortem reference THROUGHPUT-ENGINE-MOVE-B.md plus three next-session options) is too dense; split it into multiple short paragraphs or a bulleted list so each item is clear: e.g., one line for branch/commit and test counts, one for Trivia/StringSpan changes (mention StringSpan class), one for performance results (selfhost and reference fixture), one linking the Move B post-mortem (THROUGHPUT-ENGINE-MOVE-B.md), and one listing the three next-session options as separate bullets.peglib-core/src/test/java/org/pragmatica/peg/perf/TriviaAdversarialTest.java (2)
656-661: 💤 Low valueUnused helper method.
The
firstChildhelper is defined but never called within this file. Consider removing it to avoid dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@peglib-core/src/test/java/org/pragmatica/peg/perf/TriviaAdversarialTest.java` around lines 656 - 661, The helper method firstChild(CstNode node) is dead code and should be removed to avoid unused methods; delete the private static method firstChild that references CstNode and CstNode.NonTerminal (and any imports only used by it) from TriviaAdversarialTest.java, or if it was intended to be used, replace its removal by adding calls to firstChild where appropriate—prefer removing the method if there are no call sites.
690-708: 💤 Low valuePotential resource leak: URLClassLoader not closed.
The
URLClassLoadercreated at line 706 is never closed. While acceptable in test code since the JVM will reclaim resources, consider using try-with-resources for cleanliness, especially if tests are run repeatedly.♻️ Optional: Use try-with-resources pattern
- private static Class<?> compileAndLoad(String source, String className) throws Exception { + private static Class<?> compileAndLoad(String source, String className) throws Exception { Path tempDir = Files.createTempDirectory("peglib-adv-test"); var packagePath = className.substring(0, className.lastIndexOf('.')).replace('.', '/'); var simple = className.substring(className.lastIndexOf('.') + 1); var pkgDir = tempDir.resolve(packagePath); Files.createDirectories(pkgDir); var srcFile = pkgDir.resolve(simple + ".java"); Files.writeString(srcFile, source); var compiler = ToolProvider.getSystemJavaCompiler(); var rc = compiler.run(null, null, null, "-d", tempDir.toString(), "-cp", System.getProperty("java.class.path"), srcFile.toString()); if (rc != 0) { throw new RuntimeException("compile failed: " + className); } - var cl = new URLClassLoader(new URL[]{tempDir.toUri().toURL()}); - return cl.loadClass(className); + // Note: Closing the loader would unload the class; keep open intentionally + `@SuppressWarnings`("resource") + var cl = new URLClassLoader(new URL[]{tempDir.toUri().toURL()}); + return cl.loadClass(className); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@peglib-core/src/test/java/org/pragmatica/peg/perf/TriviaAdversarialTest.java` around lines 690 - 708, The URLClassLoader instance created in compileAndLoad is never closed; update compileAndLoad to ensure the URLClassLoader (variable cl) is closed after loading the compiled class—e.g., use a try-with-resources around the URLClassLoader or call cl.close() after obtaining the Class (via cl.loadClass or Class.forName with the loader) so the class is loaded and the loader is properly closed before returning.peglib-core/src/test/java/org/pragmatica/peg/tree/TriviaPostPassSpliceOffsetTest.java (1)
270-305: 💤 Low valueDuplicated helper methods across test files.
The
reconstruct,emitBody,emitLeading,emitTrailinghelpers are duplicated acrossTriviaPostPassTest,TriviaPostPassSpliceOffsetTest, andTriviaPostPassFlagTest. Consider extracting these to a shared test utility class if the pattern persists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@peglib-core/src/test/java/org/pragmatica/peg/tree/TriviaPostPassSpliceOffsetTest.java` around lines 270 - 305, Extract the duplicated helpers reconstruct, emitBody, emitLeading, and emitTrailing into a shared test utility class (e.g., TriviaTestUtils) as static methods, move the existing implementations from TriviaPostPassTest, TriviaPostPassSpliceOffsetTest, and TriviaPostPassFlagTest into that single class, then update those test classes to call TriviaTestUtils.reconstruct(...) and the other static helpers instead of defining their own copies; ensure method signatures remain the same (accepting CstNode and StringBuilder where used) and update imports accordingly.peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.java (1)
325-329: 💤 Low valueMinor inconsistency: use cached
triviaPostPassfield instead ofconfig.triviaPostPass().The comment at lines 41-45 explicitly states the intent to cache the flag to avoid re-reading config, but this method reads from config directly. While not a bug (config is immutable), using the cached field maintains consistency.
♻️ Suggested fix
private CstNode applyTriviaPostPass(String input, CstNode root, int leadingScanFrom) { - if (!config.triviaPostPass()) { + if (!triviaPostPass) { return root; } return TriviaPostPass.assignTrivia(input, root, grammar, leadingScanFrom); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.java` around lines 325 - 329, The method applyTriviaPostPass reads the flag from config directly; replace that call with the cached boolean field triviaPostPass to follow the class intent—change the if-check to use triviaPostPass and keep the rest of the method (calling TriviaPostPass.assignTrivia(input, root, grammar, leadingScanFrom)) unchanged so the cached value is used consistently instead of config.triviaPostPass().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 8-10: The changelog entry for the 0.5.1 release shows a dated
header "## [0.5.1] - 2026-05-09" but still contains the line "_Unreleased —
patch cycle following 0.5.0._"; remove or replace that line so the dated section
no longer includes the "Unreleased" label (edit the paragraph under the header
"## [0.5.1] - 2026-05-09" to reflect a released state or remove the sentence
entirely).
In `@docs/HANDOVER.md`:
- Around line 600-603: Update the user-facing documentation to call out the
35.8% regression on small fixtures and give explicit guidance for users: add an
entry to CHANGELOG.md and migration notes summarizing the regression and that
the new behavior is the default but can be disabled via the ParserConfig
constructor (reference ParserConfig and the legacy attribution option),
recommend opt-out guidance such as "use legacy attribution for one-shot parses
under ~2,000 LOC" (or a clarified threshold), and add a short user-facing
paragraph explaining when to prefer the legacy mode vs new default so users
parsing small inputs won't be surprised.
In `@docs/incremental/TRIVIA-ADVERSARIAL-FINDINGS.md`:
- Around line 72-77: Add a language tag to the fenced grammar block so Markdown
lint rule MD040 is satisfied and syntax highlighting improves: update the
untyped fence around the grammar containing Start, Pair, Triple and %whitespace
to include the language identifier (e.g., "peg") immediately after the opening
triple backticks.
In `@peglib-core/src/main/java/org/pragmatica/peg/PegParser.java`:
- Around line 246-247: The builder's default for the instance field
triviaPostPass in PegParser currently initializes it to false causing
PegParser.builder(...).build() to diverge from the historical default (post-pass
enabled); update the builder's initialization so triviaPostPass defaults to true
(or otherwise match the release default) and ensure any related builder
docs/comments (the "Default: off" text near the builder) are updated to reflect
the new default; check and adjust usages/fields referenced in PegParser (e.g.,
the triviaPostPass field and the builder methods that set it) so the built
parser behavior matches the 0.5.1 default.
In `@peglib-core/src/main/java/org/pragmatica/peg/tree/TriviaPostPass.java`:
- Around line 671-688: The parser silently fails when a Repetition has invalid
bounds (min > max); add a validation step in Grammar.validate() that iterates
Repetition expressions and checks rep.min() <= rep.max(). If the check fails,
raise/return a clear semantic error referencing the offending Repetition
(include min and max in the message) so invalid bounds like {5,1} are rejected
at grammar validation rather than causing silent match failures in
matchRepetition.
---
Nitpick comments:
In `@docs/HANDOVER.md`:
- Line 690: The final summary paragraph (the dense block mentioning Branch
`release-0.5.1` at `0b29c78`, peglib-core test counts, Trivia rework, cumulative
selfhost -5%, reference fixture +30%, addition of StringSpan /
org.pragmatica.peg.tree.StringSpan, and the Move B post-mortem reference
THROUGHPUT-ENGINE-MOVE-B.md plus three next-session options) is too dense; split
it into multiple short paragraphs or a bulleted list so each item is clear:
e.g., one line for branch/commit and test counts, one for Trivia/StringSpan
changes (mention StringSpan class), one for performance results (selfhost and
reference fixture), one linking the Move B post-mortem
(THROUGHPUT-ENGINE-MOVE-B.md), and one listing the three next-session options as
separate bullets.
In `@peglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.java`:
- Around line 325-329: The method applyTriviaPostPass reads the flag from config
directly; replace that call with the cached boolean field triviaPostPass to
follow the class intent—change the if-check to use triviaPostPass and keep the
rest of the method (calling TriviaPostPass.assignTrivia(input, root, grammar,
leadingScanFrom)) unchanged so the cached value is used consistently instead of
config.triviaPostPass().
In
`@peglib-core/src/test/java/org/pragmatica/peg/perf/TriviaAdversarialTest.java`:
- Around line 656-661: The helper method firstChild(CstNode node) is dead code
and should be removed to avoid unused methods; delete the private static method
firstChild that references CstNode and CstNode.NonTerminal (and any imports only
used by it) from TriviaAdversarialTest.java, or if it was intended to be used,
replace its removal by adding calls to firstChild where appropriate—prefer
removing the method if there are no call sites.
- Around line 690-708: The URLClassLoader instance created in compileAndLoad is
never closed; update compileAndLoad to ensure the URLClassLoader (variable cl)
is closed after loading the compiled class—e.g., use a try-with-resources around
the URLClassLoader or call cl.close() after obtaining the Class (via
cl.loadClass or Class.forName with the loader) so the class is loaded and the
loader is properly closed before returning.
In
`@peglib-core/src/test/java/org/pragmatica/peg/tree/TriviaPostPassSpliceOffsetTest.java`:
- Around line 270-305: Extract the duplicated helpers reconstruct, emitBody,
emitLeading, and emitTrailing into a shared test utility class (e.g.,
TriviaTestUtils) as static methods, move the existing implementations from
TriviaPostPassTest, TriviaPostPassSpliceOffsetTest, and TriviaPostPassFlagTest
into that single class, then update those test classes to call
TriviaTestUtils.reconstruct(...) and the other static helpers instead of
defining their own copies; ensure method signatures remain the same (accepting
CstNode and StringBuilder where used) and update imports 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3cbcc5e-3729-4287-9675-f562bf7087c7
📒 Files selected for processing (36)
CHANGELOG.mddocs/HANDOVER.mddocs/incremental/TRIVIA-ADVERSARIAL-FINDINGS.mdpeglib-core/pom.xmlpeglib-core/src/jmh/java/org/pragmatica/peg/bench/Java25ParseBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/bench/PackratStatsProbe.javapeglib-core/src/main/java/org/pragmatica/peg/PegParser.javapeglib-core/src/main/java/org/pragmatica/peg/generator/ParserGenerator.javapeglib-core/src/main/java/org/pragmatica/peg/parser/ParserConfig.javapeglib-core/src/main/java/org/pragmatica/peg/parser/ParsingContext.javapeglib-core/src/main/java/org/pragmatica/peg/parser/PegEngine.javapeglib-core/src/main/java/org/pragmatica/peg/tree/CstNode.javapeglib-core/src/main/java/org/pragmatica/peg/tree/StringSpan.javapeglib-core/src/main/java/org/pragmatica/peg/tree/TriviaPostPass.javapeglib-core/src/test/java/org/pragmatica/peg/generator/GeneratedParserTriviaPostPassTest.javapeglib-core/src/test/java/org/pragmatica/peg/grammar/LeftRecursionTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase1ParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2AllStructuralParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2ChoiceDispatchAndMarkResetParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2ChoiceDispatchParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2InlineLocationsParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2MarkResetChildrenParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2SelectivePackratEmptySetParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/Phase2SelectivePackratParityTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/TriviaAdversarialTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/TriviaPostPassFlagTest.javapeglib-core/src/test/java/org/pragmatica/peg/tree/StringSpanTest.javapeglib-core/src/test/java/org/pragmatica/peg/tree/TriviaPostPassSpliceOffsetTest.javapeglib-core/src/test/java/org/pragmatica/peg/tree/TriviaPostPassTest.javapeglib-formatter/pom.xmlpeglib-incremental/pom.xmlpeglib-incremental/src/main/java/org/pragmatica/peg/incremental/internal/TreeSplicer.javapeglib-incremental/src/main/java/org/pragmatica/peg/incremental/internal/TriviaRedistribution.javapeglib-maven-plugin/pom.xmlpeglib-playground/pom.xmlpom.xml
Comment on lines
+8
to
+10
| ## [0.5.1] - 2026-05-09 | ||
|
|
||
| _Unreleased — patch cycle following 0.5.0._ |
There was a problem hiding this comment.
Remove the “Unreleased” label from the dated 0.5.1 release section.
Line 10 contradicts the dated heading at Line 8 (2026-05-09) and may confuse consumers parsing release state from the changelog.
- _Unreleased — patch cycle following 0.5.0._
+ _Patch cycle following 0.5.0._📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## [0.5.1] - 2026-05-09 | |
| _Unreleased — patch cycle following 0.5.0._ | |
| ## [0.5.1] - 2026-05-09 | |
| _Patch cycle following 0.5.0._ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 8 - 10, The changelog entry for the 0.5.1 release
shows a dated header "## [0.5.1] - 2026-05-09" but still contains the line
"_Unreleased — patch cycle following 0.5.0._"; remove or replace that line so
the dated section no longer includes the "Unreleased" label (edit the paragraph
under the header "## [0.5.1] - 2026-05-09" to reflect a released state or remove
the sentence entirely).
Comment on lines
+600
to
+603
| | Reference (1900 LOC) | 19.78 ms | 26.87 ms | +35.8% | | ||
| | Selfhost (37k LOC) | 934.7 ms | 943.6 ms | +0.95% (within noise) | | ||
|
|
||
| Reference is 36% slower than legacy; selfhost is at parity. The post-pass overhead is real on small inputs (one tree-walk) but amortizes for large inputs where buffer save/restore dominated. **Acceptable as default**; opt-out remains available via explicit `ParserConfig` constructor. |
There was a problem hiding this comment.
Performance regression on small fixtures: verify user communication.
The documented +35.8% slowdown on the 1900-LOC reference fixture (line 600) is a significant user-facing change, even though selfhost is at parity. The doc marks this "acceptable as default" with opt-out available via explicit ParserConfig constructor.
Consider:
- Is this performance tradeoff prominently documented in user-facing CHANGELOG.md and migration notes?
- Should the default flip include guidance on when to opt-out (e.g., "use legacy attribution for sub-2000 LOC one-shot parses")?
- The structural explanation (lines 679-682) is clear for engineers but may not help users decide whether to opt-out.
The transparency here is excellent, but ensure users parsing small inputs aren't surprised by the regression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/HANDOVER.md` around lines 600 - 603, Update the user-facing
documentation to call out the 35.8% regression on small fixtures and give
explicit guidance for users: add an entry to CHANGELOG.md and migration notes
summarizing the regression and that the new behavior is the default but can be
disabled via the ParserConfig constructor (reference ParserConfig and the legacy
attribution option), recommend opt-out guidance such as "use legacy attribution
for one-shot parses under ~2,000 LOC" (or a clarified threshold), and add a
short user-facing paragraph explaining when to prefer the legacy mode vs new
default so users parsing small inputs won't be surprised.
Comment on lines
+72
to
+77
| ``` | ||
| Start <- Triple / Pair | ||
| Pair <- 'x' 'y' | ||
| Triple <- 'x' 'y' 'z' | ||
| %whitespace <- [ ]+ | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced grammar block.
The code fence is untyped, which triggers markdown lint rule MD040 and reduces syntax highlighting quality.
Proposed doc fix
-```
+```peg
Start <- Triple / Pair
Pair <- 'x' 'y'
Triple <- 'x' 'y' 'z'
%whitespace <- [ ]+</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/incremental/TRIVIA-ADVERSARIAL-FINDINGS.md around lines 72 - 77, Add a
language tag to the fenced grammar block so Markdown lint rule MD040 is
satisfied and syntax highlighting improves: update the untyped fence around the
grammar containing Start, Pair, Triple and %whitespace to include the language
identifier (e.g., "peg") immediately after the opening triple backticks.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+246
to
247
| private boolean triviaPostPass = false; | ||
|
|
There was a problem hiding this comment.
Builder default for triviaPostPass is inconsistent with the release default.
Line 246 sets triviaPostPass to false, and Line 271 documents “Default: off”, but 0.5.1’s default behavior is post-pass enabled. This makes PegParser.builder(...).build() diverge from default parser creation behavior unless callers set it manually.
Suggested fix
- private boolean triviaPostPass = false;
+ private boolean triviaPostPass = ParserConfig.DEFAULT.triviaPostPass();
...
- * returns its CST. Default: off.
+ * returns its CST. Default: follows {`@link` ParserConfig#DEFAULT}.Also applies to: 267-272, 279-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@peglib-core/src/main/java/org/pragmatica/peg/PegParser.java` around lines 246
- 247, The builder's default for the instance field triviaPostPass in PegParser
currently initializes it to false causing PegParser.builder(...).build() to
diverge from the historical default (post-pass enabled); update the builder's
initialization so triviaPostPass defaults to true (or otherwise match the
release default) and ensure any related builder docs/comments (the "Default:
off" text near the builder) are updated to reflect the new default; check and
adjust usages/fields referenced in PegParser (e.g., the triviaPostPass field and
the builder methods that set it) so the built parser behavior matches the 0.5.1
default.
Comment on lines
+671
to
+688
| private static int matchRepetition(String input, int pos, int limit, Expression.Repetition rep, Grammar grammar) { | ||
| int cursor = pos; | ||
| int count = 0; | ||
| int max = rep.max() | ||
| .isPresent() | ||
| ? rep.max() | ||
| .unwrap() | ||
| : Integer.MAX_VALUE; | ||
| while (count < max) { | ||
| int next = matchExpression(input, cursor, limit, rep.expression(), grammar); | ||
| if (next < 0 || next == cursor) break; | ||
| cursor = next; | ||
| count++ ; | ||
| } | ||
| return count >= rep.min() | ||
| ? cursor | ||
| : - 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Repetition expression validation exists elsewhere in the codebase
rg -n "Repetition" --type=java -A 5 -B 2 | head -100Repository: siy/java-peglib
Length of output: 11111
🏁 Script executed:
# Search for Repetition class definition
rg -n "class Repetition" --type=java -A 10Repository: siy/java-peglib
Length of output: 41
🏁 Script executed:
# Check EdgeCaseTest.java for repetition edge cases
fd -t f "EdgeCaseTest.java" | xargs cat | head -200Repository: siy/java-peglib
Length of output: 6802
🏁 Script executed:
# Search for any validation of min/max in grammar parsing
rg -n "min.*max|max.*min" --type=java peglib-core/src/main/java | head -30Repository: siy/java-peglib
Length of output: 839
🏁 Script executed:
# Look at the full GrammarParser repetition parsing logic
rg -n "bounded.*[Rr]epetition|parseRepetition|Repetition.*min.*max" --type=java -B 5 -A 15 peglib-core/src/main/java/org/pragmatica/peg/grammar/GrammarParser.java | head -100Repository: siy/java-peglib
Length of output: 2244
🏁 Script executed:
# Check GrammarParserTest for the bounded repetition test
rg -n "parse_boundedRepetition" --type=java -A 30 peglib-core/src/test/java/org/pragmatica/peg/grammar/GrammarParserTest.javaRepository: siy/java-peglib
Length of output: 1458
🏁 Script executed:
# Search for any validation of Repetition constraints
rg -n "min.*>|max.*<" --type=java peglib-core/src/main/java/org/pragmatica/peg/grammar/ | head -20Repository: siy/java-peglib
Length of output: 567
🏁 Script executed:
# Check if there's grammar validation logic that validates repetition constraints
rg -n "validate|Validation|check.*[Rr]epetition" --type=java peglib-core/src/main/java/org/pragmatica/peg/grammar/ | head -20Repository: siy/java-peglib
Length of output: 2015
🏁 Script executed:
# Look at GrammarResolver which might do semantic validation
rg -n "class GrammarResolver" --type=java -A 20 peglib-core/src/main/java/org/pragmatica/peg/grammar/GrammarResolver.java | head -50Repository: siy/java-peglib
Length of output: 781
🏁 Script executed:
# Search for any tests that try to create invalid repetitions like {5,1}
rg -n "{[0-9]+,[0-9]+}" --type=java peglib-core/src/test/java/org/pragmatica/peg/ | head -20Repository: siy/java-peglib
Length of output: 150
🏁 Script executed:
# Look at Grammar.validate() method to see what it validates
rg -n "private static Result<Grammar> validate" --type=java -A 50 peglib-core/src/main/java/org/pragmatica/peg/grammar/Grammar.javaRepository: siy/java-peglib
Length of output: 3390
🏁 Script executed:
# Search for actual test cases with repetition bounds in different formats
rg -n "\\{[0-9]+\\}" --type=java peglib-core/src/test/java/org/pragmatica/peg/ | head -30Repository: siy/java-peglib
Length of output: 1786
🏁 Script executed:
# Check if there's any mention of invalid repetition bounds in comments or error messages
rg -n "min.*max|bound|invalid.*repetition" --type=java peglib-core/src/main/java/org/pragmatica/peg/ -i | head -20Repository: siy/java-peglib
Length of output: 2809
🏁 Script executed:
# Check if there's any test specifically for invalid repetition bounds like {5,1}
rg -n "{[0-9]+," --type=java peglib-core/src/test/java/ -A 2 -B 2 | grep -E "(min|max|test|Test)" | head -30Repository: siy/java-peglib
Length of output: 143
🏁 Script executed:
# Look at the full matchRepetition context to understand the edge case handling better
cat -n peglib-core/src/main/java/org/pragmatica/peg/tree/TriviaPostPass.java | sed -n '671,690p'Repository: siy/java-peglib
Length of output: 841
🏁 Script executed:
# Check if there are any other tests mentioning edge cases for repetition
rg -n "repetition.*edge|edge.*repetition|min.*max|max.*min" --type=java -i peglib-core/src/test/java/org/pragmatica/peg/ | head -20Repository: siy/java-peglib
Length of output: 163
Add validation for invalid repetition bounds or document the behavior.
The matchRepetition method implementation is correct, but there's a gap: Grammar.validate() does not check that min ≤ max for Repetition expressions. When min > max occurs (e.g., {5,1}), the parser silently fails to match instead of reporting an error. The min == 0 edge case (ZeroOrMore bounded by max) is already tested in EdgeCaseTest.zeroOrMore_withEmpty_succeeds(). Consider either:
- Adding validation in
Grammar.validate()to rejectmin > maxwith a semantic error, or - Adding a test case demonstrating the expected behavior for invalid bounds
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@peglib-core/src/main/java/org/pragmatica/peg/tree/TriviaPostPass.java` around
lines 671 - 688, The parser silently fails when a Repetition has invalid bounds
(min > max); add a validation step in Grammar.validate() that iterates
Repetition expressions and checks rep.min() <= rep.max(). If the check fails,
raise/return a clear semantic error referencing the offending Repetition
(include min and max in the message) so invalid bounds like {5,1} are rejected
at grammar validation rather than causing silent match failures in
matchRepetition.
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
Release 0.5.1 of peglib — context-independent trivia attribution + StringSpan + perf cleanup arc. 26 commits past main.
Headline outcomes
Trivia rework — long-standing issue closed:
triviaPostPass=trueis the new default. Context-independent attribution by re-derivation from(input, span)coordinates.parseRuleAtnow produces structurally identical body subtree to full reparse (Lever B trivia-context-loss blocker RESOLVED).Perf wins on the GC-bound workload:
StringSpan added (
org.pragmatica.peg.tree.StringSpan) — CharSequence view with lazy String materialization. Eliminates substring/byte[] allocation on token capture hot path. New public API; CstNode.Terminal/Token internals migrated.Engineering arc
8 ships, 4 reverts, 1 deferral across the cleanup arc. All bench-gated; no silent regressions:
Test counts
Breaking changes
CstNode.TerminalandCstNode.Tokenrecord components changed:text: String→textSpan: StringSpan. Public.text(): Stringaccessor preserved (lazy materialization). Pattern-matchers usingcase Terminal t -> t.text()continue to work; component-name deconstruction does not (none found in audit).triviaPostPassdefault flipped totrue. Legacy buffer-driven attribution available via explicitnew ParserConfig(..., triviaPostPass=false, ...).CstParseResult.text(generator-emitted) widened fromStringtoCharSequence. Internal type; no public API impact.Test plan
mvn installgreen — local m2 freshDocs
docs/incremental/THROUGHPUT-ENGINE-MOVE-B.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation