release: peglib 0.6.0 — tokens-first redesign#34
Merged
Merged
Conversation
…ap, banked lessons
… annotated var declarations
… alias refinements
…round, V51-generated bench
…nce, 11.83x selfhost
…t of parity sweep
…n pragmatica-lite:core
…diagnostic + synthetic token
…vation; 20/20 corpus round-trip
…ed inline literals
…sGenerator parses cleanly (13,529 -> 0 diag)
📝 WalkthroughWalkthroughIntroduces PEG v6: new lexer DFA/engine, parser generator/compiler, flat CST runtime with diagnostics, incremental editing, visitor/formatter, Maven Mojo, playground, benchmarks, grammar/resource updates, extensive tests, and documentation; modules/poms bump to 0.6.0 and add peglib-runtime. ChangesPEG v6 End-To-End
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
peglib-core/src/main/java/org/pragmatica/peg/grammar/GrammarResolver.java (1)
169-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImported
%recoversets are still dropped during grammar compositionAt Line 176, only
root.recoverSets()is forwarded. Imported rules are renamed/inlined, but their recovery sets are never remapped into the composed grammar, so recovery behavior for imported rules is silently lost.💡 Proposed fix
@@ - // Produce composed Grammar. imports are cleared (surface-level composition is complete). + // Compose recover sets: root + imported rules (with rename mapping). + var composedRecoverSets = new LinkedHashMap<String, Set<Character>>(); + root.recoverSets().forEach((name, chars) -> composedRecoverSets.put(name, Set.copyOf(chars))); + + // Produce composed Grammar. imports are cleared (surface-level composition is complete). @@ for (var originalName : closure) { var rule = ruleMap.get(originalName); String renamedName; @@ var rewritten = rewriteReferences(rule, renamedName, ref -> { @@ }); composedRules.put(renamedName, rewritten); + + var importedRecover = g.recoverSets().get(originalName); + if (importedRecover != null) { + composedRecoverSets.putIfAbsent(renamedName, Set.copyOf(importedRecover)); + } } } @@ return Grammar.grammar( new ArrayList<>(composedRules.values()), root.startRule(), root.whitespace(), root.word(), root.suggestRules(), List.of(), - root.recoverSets()); + Map.copyOf(composedRecoverSets));🤖 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/grammar/GrammarResolver.java` around lines 169 - 176, The composed Grammar currently only forwards root.recoverSets(), dropping any recovery sets from imported/inlined grammars; update GrammarResolver to collect and remap recovery sets from imported grammars (using the same renaming/inline mapping applied to rules) and merge them with root.recoverSets() before calling Grammar.grammar. Locate the composition logic around composedRules.values() and the Grammar.grammar(...) call and produce a combined List of remapped recovery sets (preserving rule name remapping used when inlining imports) to replace the empty List.of() argument so imported %recover sets are preserved in the final grammar.README.md (2)
410-410:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate version number in Maven plugin example.
The Maven plugin configuration example shows version
0.5.0, inconsistent with the 0.6.0 release being documented.📝 Suggested fix
<plugin> <groupId>org.pragmatica-lite</groupId> <artifactId>peglib-maven-plugin</artifactId> - <version>0.5.0</version> + <version>0.6.0</version> <executions>🤖 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 `@README.md` at line 410, Update the Maven plugin example version tag shown as <version>0.5.0</version> to match the documented release (0.6.0); locate the example in README.md where the <version>0.5.0</version> snippet appears and change the value to 0.6.0 so the sample configuration is consistent with the 0.6.0 release.
62-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate version number in dependency example.
The Quick Start section shows version
0.5.0in the Maven dependency example, but this README is documenting the0.6.0release. Update to reflect the current version being released.📝 Suggested fix
<dependency> <groupId>org.pragmatica-lite</groupId> <artifactId>peglib</artifactId> - <version>0.5.0</version> + <version>0.6.0</version> </dependency>🤖 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 `@README.md` at line 62, Update the Maven dependency example's version tag from 0.5.0 to the release version 0.6.0 by replacing the <version>0.5.0</version> element in the Quick Start dependency snippet; search for the <version>...</version> line in the Maven example and change its value to 0.6.0 so the README matches the documented release.
🟡 Minor comments (11)
peglib-runtime/src/main/java/org/pragmatica/peg/v6/diagnostic/Diagnostic.java-69-70 (1)
69-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize offsets and CRLF handling in diagnostic rendering.
Line 70 should clamp negative offsets, and Lines 82-87 should trim
\rbefore rendering the line text. Otherwise Rust-style output can show invalid columns or malformed line display on CRLF inputs.🩹 Proposed fix
- int clamped = Math.min(offset, input.length()); + int clamped = Math.max(0, Math.min(offset, input.length())); @@ - int end = input.indexOf('\n', lineStart); - return end < 0 - ? input.length() - : end; + int end = input.indexOf('\n', lineStart); + int lineEnd = end < 0 ? input.length() : end; + if (lineEnd > lineStart && input.charAt(lineEnd - 1) == '\r') { + lineEnd--; + } + return lineEnd;Also applies to: 82-87
🤖 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-runtime/src/main/java/org/pragmatica/peg/v6/diagnostic/Diagnostic.java` around lines 69 - 70, The locate method in Diagnostic currently clamps only the upper bound; update Diagnostic.locate to also clamp negative offsets (e.g., use Math.max(0, Math.min(offset, input.length()))) so offsets below zero are normalized, and when rendering the extracted line text (the variables used in the rendering block around lines 82-87), strip any trailing '\r' before computing column positions and before printing the line so CRLF inputs don't produce invalid columns or malformed line display.peglib-core/src/test/java/org/pragmatica/peg/v6/analyzer/LeftRecursionDetectorTest.java-183-186 (1)
183-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse classpath resource loading for
java25.pegto avoid cwd-dependent test failuresLine 184 hardcodes a relative filesystem path. This is brittle outside module-root execution contexts.
💡 Proposed fix
- var grammarPath = Paths.get("src/test/resources/java25.peg"); - var grammarText = Files.readString(grammarPath, StandardCharsets.UTF_8); + var resource = LeftRecursionDetectorTest.class.getResource("/java25.peg"); + assertTrue(resource != null, "Missing test resource: /java25.peg"); + var grammarText = Files.readString(Path.of(resource.toURI()), StandardCharsets.UTF_8);🤖 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/v6/analyzer/LeftRecursionDetectorTest.java` around lines 183 - 186, Replace filesystem-based loading of the test grammar in java25Grammar_isFreeOfLeftRecursion by reading the resource from the classpath; locate the test method java25Grammar_isFreeOfLeftRecursion and change the Files.readString(Paths.get("src/test/resources/java25.peg"), ...) usage to use getResourceAsStream or ClassLoader.getResource to load "java25.peg" from the test resources, then convert the stream to a String (e.g., using new String(stream.readAllBytes(), StandardCharsets.UTF_8)) before passing it to org.pragmatica.peg.grammar.GrammarParser.parse to avoid cwd-dependent failures.peglib-playground/src/test/java/org/pragmatica/peg/playground/v6/PlaygroundReplV6Test.java-35-36 (1)
35-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify non-quit commands do not signal exit.
These tests currently ignore
handleCommand(...)’s boolean for normal commands, so an accidentaltrue(exit) would not be caught.Proposed patch
- repl.handleCommand("12 + 34"); + boolean exit = repl.handleCommand("12 + 34"); + assertThat(exit).isFalse(); @@ - repl.handleCommand("a#x"); + boolean exit = repl.handleCommand("a#x"); + assertThat(exit).isFalse(); @@ - repl.handleCommand(":status"); + boolean exit = repl.handleCommand(":status"); + assertThat(exit).isFalse();Also applies to: 73-74, 90-90
🤖 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-playground/src/test/java/org/pragmatica/peg/playground/v6/PlaygroundReplV6Test.java` around lines 35 - 36, The tests call PlaygroundReplV6.handleCommand("...") for non-quit inputs but ignore its boolean return (which signals exit), so update each invocation (e.g., in PlaygroundReplV6Test where repl.handleCommand("12 + 34"), other occurrences) to capture and assert the result is false (use the test framework's assertFalse/assertEquals(false, ...) on the returned boolean) to ensure non-quit commands do not signal exit; apply this change to all similar calls in the test class (including the other places noted).peglib-core/src/test/java/org/pragmatica/peg/v6/generator/RecoverDirectiveGeneratorTest.java-172-183 (1)
172-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
doesNotContain("{}")is a no-op for this extractedarrayBody.
arrayBodyexcludes the closing}, so this assertion does not actually prove the sync set is populated with|. Please assert presence of the|token kind directly.Suggested fix
var idx = generated.source() .indexOf("DEFAULT_SYNC = new int[] {"); + assertThat(idx).isPositive(); var end = generated.source() .indexOf('}', idx); var arrayBody = generated.source() .substring(idx, end); - // '|' must be present (proves override populated it); no commas in - // the kind-list interior means the array is non-empty (a single int - // followed by '}' with no leading comma). - assertThat(arrayBody) - .doesNotContain("{}"); + var pipeKind = built.kinds().inlineLiteralToKind().get("|/cs"); + assertThat(pipeKind).isNotNull(); + assertThat(arrayBody).contains(String.valueOf(pipeKind));🤖 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/v6/generator/RecoverDirectiveGeneratorTest.java` around lines 172 - 183, The current test extracts arrayBody from generated.source() between "DEFAULT_SYNC = new int[] {" and '}' but then checks doesNotContain("{}") which is a no-op because arrayBody excludes the closing brace; replace that assertion with a direct presence check for the '|' token kind (e.g. assertThat(arrayBody).contains("|")) so the test actually verifies the sync set was populated; update the assertion near variables generated, idx, end, arrayBody and the DEFAULT_SYNC extraction accordingly.peglib-formatter/src/test/java/org/pragmatica/peg/formatter/v6/V6FormatterCorpusGateTest.java-48-58 (1)
48-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix double negation in assertion.
Line 50 uses
assertFalse(!Files.isReadable(...)), which is a double negation and reduces readability. UseassertTruedirectly.🐛 Proposed fix
- assertFalse(!Files.isReadable(GRAMMAR_PATH), + assertTrue(Files.isReadable(GRAMMAR_PATH), "java25 grammar not readable at " + GRAMMAR_PATH.toAbsolutePath());🤖 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-formatter/src/test/java/org/pragmatica/peg/formatter/v6/V6FormatterCorpusGateTest.java` around lines 48 - 58, Replace the double-negated assertion in the setUp method: change the assertFalse(!Files.isReadable(GRAMMAR_PATH), ...) to use assertTrue with Files.isReadable(GRAMMAR_PATH) so the check is clearer; update the message string as-is (it can remain "java25 grammar not readable at " + GRAMMAR_PATH.toAbsolutePath()) and ensure this change is applied in the setUp() method where GRAMMAR_PATH, PegParser.clearCache(), parser = PegParser.fromGrammar(grammar).unwrap(), and fixtures = listFixtures() are used.peglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6Formatter.java-89-96 (1)
89-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't attribute every formatter exception to the root rule.
Any exception below the root — a child formatter rule, trivia policy, or even renderer bug — is currently wrapped as
RuleFailed(cst.kindNameAt(root), e). That makes failures misleading. Capture the failing node/rule whereapplyUserRule(...)runs and propagate that metadata upward instead.🤖 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-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6Formatter.java` around lines 89 - 96, The catch block is attributing every exception to the root rule; change the error propagation so the actual failing CST node/rule is reported: when invoking user rules (applyUserRule(...)) and other per-node logic (e.g., inside walk, wrapRootWithFileTrivia or Renderer.render call sites), wrap runtime exceptions in a small carrier (or throw a V6FormatterError.RuleFailed with cst.kindNameAt(failingNode)) that carries the specific node/kind where the failure occurred, then let that propagate to the outer try/catch so the final catch constructs V6FormatterError.RuleFailed using the failing node/kind from that carrier rather than always using root; update walk and applyUserRule call sites to annotate exceptions with the node identity.peglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6FormatterConfig.java-55-60 (1)
55-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate all
Builderconstructor inputs, not justrules.
Builderis a public type, so callers can bypassV6FormatterConfig.builder()and construct it directly withdefaultIndent < 0,maxLineWidth <= 0, ortriviaPolicy == null. Mirror the outer record's validation here so the builder can't exist in an invalid state.Suggested fix
public Builder { + if (defaultIndent < 0) { + throw new IllegalArgumentException("defaultIndent must be >= 0"); + } + if (maxLineWidth <= 0) { + throw new IllegalArgumentException("maxLineWidth must be > 0"); + } + if (triviaPolicy == null) { + throw new IllegalArgumentException("triviaPolicy must not be null"); + } if (rules == null) { throw new IllegalArgumentException("rules must not be null"); } rules = Map.copyOf(rules); }🤖 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-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6FormatterConfig.java` around lines 55 - 60, The Builder constructor currently only validates rules; add the same checks as the outer V6FormatterConfig record so the Builder cannot be instantiated with invalid values: verify defaultIndent is not negative, maxLineWidth is greater than 0, and triviaPolicy is not null (in addition to the existing rules null check), and call Map.copyOf(rules) as before; update the Builder constructor (class Builder) to throw IllegalArgumentException with clear messages for each failing parameter (defaultIndent, maxLineWidth, triviaPolicy, rules) so direct construction mirrors V6FormatterConfig validation and prevents invalid Builder state.peglib-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstArrayBuilder.java-169-177 (1)
169-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the advertised single-shot contract with an explicit state check.
build()nulls the backing arrays, so a secondbuild()call — or any later mutation — currently fails with an incidentalNullPointerExceptioninstead of the documented fail-fast behavior. Guardbuild()and the mutators withif (built) throw new IllegalStateException(...).Suggested fix
public CstArray build(int rootIndex) { + if (built) { + throw new IllegalStateException("CstArrayBuilder is single-use"); + } var trimmed = Arrays.copyOf(nodes, nodeCount * CstArray.NODE_STRIDE); var ruleTableCopy = ruleTable.clone(); built = true; nodes = null; lastChild = null;You'd want the same guard on
beginNode(...),endNode(...),setFlag(...), andtruncate(...)as well.🤖 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-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstArrayBuilder.java` around lines 169 - 177, The builder currently nulls its backing arrays and sets built=true in CstArrayBuilder.build(), but does not enforce the single-shot contract; add explicit state checks that throw IllegalStateException when reused: at the start of CstArrayBuilder.build() guard with "if (built) throw new IllegalStateException(...)" and do the same guard in the mutator methods beginNode(...), endNode(...), setFlag(...), and truncate(...); ensure the message clearly indicates the builder has already been built so callers fail-fast instead of getting a NullPointerException.peglib-core/src/test/java/org/pragmatica/peg/v6/generator/LexerGeneratorTest.java-257-278 (1)
257-278:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail if the DFA probe never finds an accepting byte.
If no start-state transition reaches an accepting state, this test falls through after checking only empty-input parity and still passes. Add an explicit assertion after the loop so the probe actually guards what the comment describes.
Suggested fix
- for (int b = 0; b < 256; b++ ) { + boolean foundAcceptedByte = false; + for (int b = 0; b < 256; b++ ) { int next = built.dfa() .dfa() .transition(0, b); @@ var engineTokens = built.engine() .lex(probe); var compiledTokens = compiled.lex(probe); assertTokenArraysEqual(engineTokens, compiledTokens); - return; + foundAcceptedByte = true; + break; } + assertThat(foundAcceptedByte) + .as("java25 DFA should accept at least one start-state byte") + .isTrue();🤖 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/v6/generator/LexerGeneratorTest.java` around lines 257 - 278, The loop in LexerGeneratorTest that probes bytes using built.dfa().dfa().transition(...) and built.dfa().dfa().acceptKind(...) can fall through without finding an accepting byte and the test will still pass; after the for-loop add an explicit assertion (e.g., fail or assertTrue) that ensures at least one accepting byte was found so the probe actually validates the DFA parity check, referencing the same built and compiled lex(...) comparisons already used in the loop to determine success.peglib-core/src/main/java/org/pragmatica/peg/v6/generator/LexerGenerator.java-72-73 (1)
72-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe empty-match warning text is stale.
The generated
lex()no longer throws here; it forces progress by emitting a synthetic one-character whitespace token on no-match. Since this warning is surfaced to users via the mojo, it currently describes the wrong runtime behavior.🤖 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/v6/generator/LexerGenerator.java` around lines 72 - 73, The warning message added in LexerGenerator when dfa.acceptKind(Dfa.START_STATE) != Dfa.NO_ACCEPT is stale; update the string passed to warnings.add to describe the new behavior: instead of generated lex() throwing on any input, the lexer now forces progress by emitting a synthetic one-character whitespace token when no rule matches. Locate the warning construction in LexerGenerator (the warnings.add(...) call that mentions "DFA start state is accepting") and replace the message text to explain the synthetic whitespace-token fallback and advise tightening the offending rule (e.g., use '+' instead of '*').docs/MIGRATION-0.5-TO-0.6.md-22-22 (1)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the temporary nature of the v6 package prefix.
The table indicates the entry point is
org.pragmatica.peg.v6.PegParserwith a note "(during 0.6.0 development; the.v6suffix will collapse toorg.pragmatica.pegat GA)". Since this is documentation for the 0.6.0 release (not a pre-release), clarify whether users should expect the.v6prefix in the released version or if it will be removed before the release is finalized.If
.v6is present in 0.6.0 release, consider whether this might cause confusion given it's called "0.6.0" (suggesting stability) rather than "0.6.0-rc".🤖 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/MIGRATION-0.5-TO-0.6.md` at line 22, The table row referencing the entry point currently shows org.pragmatica.peg.v6.PegParser with an ambiguous parenthetical; clarify whether the .v6 package prefix will actually appear in the 0.6.0 release or will be removed before release. Update the text for the PegParser entry to explicitly state the release behavior (e.g., "In 0.6.0 the entry point is org.pragmatica.peg.v6.PegParser — the .v6 package prefix is temporary and will be collapsed to org.pragmatica.peg at GA" if .v6 ships, or change to "org.pragmatica.peg.PegParser" if it will be removed), so readers know exactly which class name to use and why the versioned package exists.
🧹 Nitpick comments (10)
peglib-core/src/test/java/org/pragmatica/peg/v6/lexer/DfaBuilderTest.java (1)
26-26: ⚡ Quick winFix spacing around comparison operator.
Missing space before the
<operator:Dfa.START_STATE< dfa.stateCount()should beDfa.START_STATE < dfa.stateCount().🎨 Proposed fix
- if (Dfa.START_STATE< dfa.stateCount()) { + if (Dfa.START_STATE < dfa.stateCount()) {🤖 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/v6/lexer/DfaBuilderTest.java` at line 26, Fix the spacing around the comparison operator in the test: change the expression using Dfa.START_STATE and dfa.stateCount() to use a space around the '<' operator so it reads "Dfa.START_STATE < dfa.stateCount()"; update the occurrence in the method that contains the conditional referencing Dfa.START_STATE and dfa.stateCount() to match this spacing style.peglib-core/src/test/java/org/pragmatica/peg/v6/lexer/RuleClassifierTest.java (1)
247-254: 💤 Low valueConsider using a test logger instead of System.out.
The diagnostic output at lines 247-253 uses
System.out.printlnfor reporting classifier results. While acceptable in tests, using a proper logging framework (or JUnit's standard output capture) would be more maintainable.🤖 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/v6/lexer/RuleClassifierTest.java` around lines 247 - 254, Replace the ad-hoc System.out.println diagnostic prints in RuleClassifierTest (the block that prints counts and classification.warnings()) with a test logger or JUnit/Slf4j logging: locate the prints in RuleClassifierTest around the variable classification and counts, create or reuse a logger (e.g., a private static final Logger in the test or use JUnit's TestInfo/TestReporter) and replace the System.out.println calls with logger.info/debug or TestReporter.publishEntry calls so test output is captured and configurable instead of printing directly to stdout.peglib-core/src/main/java/org/pragmatica/peg/v6/lexer/DfaBuilder.java (1)
956-998: ⚡ Quick winConsider reformatting the large switch expression for readability.
The
compileExpressionswitch expression spans 43 lines with dense, single-line case arms. While functionally correct, the formatting makes it difficult to scan and review. Consider breaking each case onto multiple lines or extracting complex case logic into helper methods.Example reformatting:
-return switch (expr) {case Expression.Literal lit -> Result.success(compileLiteral(nfa, lit));case Expression.CharClass cc -> Result.success(compileCharClass(nfa, - cc)); +return switch (expr) { + case Expression.Literal lit -> Result.success(compileLiteral(nfa, lit)); + case Expression.CharClass cc -> Result.success(compileCharClass(nfa, cc));🤖 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/v6/lexer/DfaBuilder.java` around lines 956 - 998, The large switch expression in compileExpression is hard to read; split each case into its own multi-line arm and move complex logic into small helpers: extract the Sequence case body (including the delimited-block check) into a helper that calls compileDelimitedBlock and compileSequence, break chained map calls into separate statements or helpers for ZeroOrMore/OneOrMore/Optional (wrapZeroOrMore, wrapOneOrMore, wrapOptional), and put compileChoice/compileRepetition/unsupported(...) cases on separate lines with clear formatting; ensure each case uses a clear yield/return path so the switch is easy to scan.peglib-playground/src/test/java/org/pragmatica/peg/playground/v6/PlaygroundEngineV6Test.java (1)
55-57: ⚡ Quick winAssert success before calling
unwrap()in the trivia test.If this case regresses to failure, the test currently fails via
unwrap()instead of a clear assertion on engine status.Proposed patch
`@Test` void run_triviaCount_reflectsLexedWhitespace() { var request = new ParseRequest(GRAMMAR, "1 + 2"); - var outcome = PlaygroundEngineV6.run(request).unwrap(); + var result = PlaygroundEngineV6.run(request); + assertThat(result.isSuccess()).isTrue(); + var outcome = result.unwrap(); // Two whitespace runs lexed as trivia tokens. assertThat(outcome.stats().triviaCount()).isGreaterThan(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 `@peglib-playground/src/test/java/org/pragmatica/peg/playground/v6/PlaygroundEngineV6Test.java` around lines 55 - 57, The test should assert the engine returned a successful result before calling unwrap(); locate the ParseRequest creation and the call to PlaygroundEngineV6.run (the variables request and outcome) and add an assertion that the run result indicates success (e.g. check outcome.isSuccess() or the equivalent status accessor on the run result) immediately after obtaining outcome, then call unwrap() only after that assertion to avoid panics when the engine fails.peglib-core/src/test/java/org/pragmatica/peg/v6/generator/ParserGeneratorTest.java (1)
277-286: 💤 Low valueConsider using streams for countParserRules.
The manual counter loop can be expressed more idiomatically using Java streams, especially since this targets Java 25.
♻️ Proposed refactor using streams
private static int countParserRules(RuleClassifier.Classification c) { - int n = 0; - for (var k : c.kinds() - .values()) { - if (k == org.pragmatica.peg.v6.lexer.RuleKind.PARSER || k == org.pragmatica.peg.v6.lexer.RuleKind.MIXED) { - n++ ; - } - } - return n; + return (int) c.kinds() + .values() + .stream() + .filter(k -> k == org.pragmatica.peg.v6.lexer.RuleKind.PARSER + || k == org.pragmatica.peg.v6.lexer.RuleKind.MIXED) + .count(); }🤖 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/v6/generator/ParserGeneratorTest.java` around lines 277 - 286, Refactor the countParserRules method to replace the manual loop with a Java Stream: use c.kinds().values().stream().filter(k -> k == org.pragmatica.peg.v6.lexer.RuleKind.PARSER || k == org.pragmatica.peg.v6.lexer.RuleKind.MIXED).count() and convert the long result to int (or use Math.toIntExact) so the method returns the correct int; update the implementation in countParserRules to use this stream-based count instead of the explicit counter and loop.peglib-core/src/test/java/org/pragmatica/peg/v6/PegParserGateTest.java (1)
179-195: 💤 Low valueConsider optimizing StringBuilder capacity in escape(String).
The
escape(String)method allocates a StringBuilder withs.length() + 4, but escape sequences can expand significantly (e.g.,\u00XXis 6 chars). For strings with many control characters, this may trigger multiple reallocations.♻️ Proposed optimization
private static String escape(String s) { - var sb = new StringBuilder(s.length() + 4); + var sb = new StringBuilder((int) (s.length() * 1.2) + 4); for (int i = 0; i < s.length(); i++) { sb.append(escape(s.charAt(i))); } return sb.toString(); }🤖 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/v6/PegParserGateTest.java` around lines 179 - 195, The StringBuilder in escape(String) is under-provisioned causing reallocations when many chars expand (escape(String) and escape(char)); fix by initializing the StringBuilder with a safe worst-case capacity (e.g., s.length() * 6 to accommodate sequences like "\u00xx") or compute needed capacity by scanning and summing expansion lengths using escape(char) logic before appending, then use that capacity when constructing the StringBuilder.peglib-core/src/test/java/org/pragmatica/peg/v6/generator/Java25ParserGateTest.java (1)
344-355: 💤 Low valueConsider simplifying switch expression syntax.
The switch expression uses traditional case arrow syntax with braces. For simple single-statement cases, Java 14+ allows omitting braces for more concise code.
♻️ Proposed simplification
private static String escape(char c) { return switch (c) { - case'\n' -> "\\n"; - case'\r' -> "\\r"; - case'\t' -> "\\t"; - case'"' -> "\\\""; - case'\\' -> "\\\\"; - default -> c < 32 || c == 127 - ? String.format("\\u%04x", (int) c) - : String.valueOf(c); + case '\n' -> "\\n"; + case '\r' -> "\\r"; + case '\t' -> "\\t"; + case '"' -> "\\\""; + case '\\' -> "\\\\"; + default -> c < 32 || c == 127 ? String.format("\\u%04x", (int) c) : String.valueOf(c); }; }Note: Also added spacing after
casefor consistency (line 346-350).🤖 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/v6/generator/Java25ParserGateTest.java` around lines 344 - 355, The switch inside the escape(char c) method should use the concise, consistent switch-expression style: add a space after each case token (e.g., change case'\n' to case '\n') and normalize all case labels to the compact form already used for single-statement arms; keep the default arm as an expression but format it consistently (e.g., case '\n' -> "\\n"; ... default -> (c < 32 || c == 127) ? String.format("\\u%04x", (int) c) : String.valueOf(c);) so the switch arms are uniform and concise.peglib-core/src/main/java/org/pragmatica/peg/v6/generator/VisitorGenerator.java (1)
64-78: ⚡ Quick winGenerated visitor source order is nondeterministic due raw
MapiterationIterating unsorted
ruleKindscan produce unstable source ordering across runs/JDKs, which hurts reproducible builds and golden-file tests.Emit in deterministic key order
- for ( var e : ruleKinds.entrySet()) { + for ( var e : ruleKinds.entrySet().stream().sorted(Map.Entry.comparingByKey()).toList()) { @@ - for ( var e : ruleKinds.entrySet()) { + for ( var e : ruleKinds.entrySet().stream().sorted(Map.Entry.comparingByKey()).toList()) { @@ - for ( var name : ruleKinds.keySet()) { + for ( var name : ruleKinds.keySet().stream().sorted().toList()) {Also applies to: 96-101
🤖 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/v6/generator/VisitorGenerator.java` around lines 64 - 78, The visitor generator iterates ruleKinds in insertion/HashMap order causing nondeterministic output; modify VisitorGenerator to iterate entries in a deterministic key order (e.g., sort ruleKinds.keySet() or use a TreeMap view) when emitting the RULE_... constants and the switch/case visit methods (both the block that builds the protected static final int RULE_..._KIND lines and the switch case emission around visit... calls, also the similar emission at the other location referenced around lines 96-101). Ensure you consistently use the same sorted iteration for all places where ruleKinds is traversed so generated source order is stable across runs.peglib-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstArray.java (1)
83-85: 💤 Low valueConsider returning a defensive copy of ruleTable.
The
ruleTable()method returns the internalString[]directly, which could allow callers to mutate it. If the array is intended to be immutable, consider returningruleTable.clone()or wrapping in an unmodifiable view.Proposed fix
public String[] ruleTable() { - return ruleTable; + return ruleTable.clone(); }Alternatively, if performance is critical and callers are trusted, document the immutability contract.
🤖 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-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstArray.java` around lines 83 - 85, The ruleTable() method in class CstArray exposes the internal String[] ruleTable directly allowing external mutation; change ruleTable() to return a defensive copy (e.g., clone or Arrays.copyOf(ruleTable, ruleTable.length)) so callers cannot mutate the internal array, and update any relevant Javadoc to note the returned array is a copy; if you deliberately want to avoid the copy for performance, instead document the immutability/trust contract on CstArray.ruleTable and ruleTable().CHANGELOG.md (1)
62-62: 💤 Low valueConsider breaking up the extremely long JBCT conformance paragraph.
Line 62 contains a single sentence of approximately 1,900 characters describing the JBCT 0.25.0 conformance changes. While technically correct, this creates readability challenges. Consider breaking this into multiple sentences or a bulleted list to improve scanability for users reviewing the changelog.
♻️ Suggested restructuring
- **JBCT 0.25.0 conformance pass on v6** (2026-05-10): - All 123 strict-mode lint errors eliminated in both framework code and generated parser/lexer source - Defensive null/bounds checks removed from internal helpers (callers within v6 are trusted; JVM catches genuine OOB) - `Result<Void>` converted to `Result<Unit>` on `ParserGenerator` emit visitors - Null returns converted to `Option<T>` in `DfaBuilder`, `IncrementalParser`, `RuleClassifier` methods - Reflection wrappers rewritten as `Result.lift(...).unwrap()` for typed failure causes - Generated parser/lexer no longer throw exceptions for stall/null/unknown-rule conditions - Hot-path void mutators carry `@SuppressWarnings("JBCT-RET-01"/"JBCT-EX-01")` - Lint passes with 0 errors, 287 style warnings - Bench: reference `7.06 ± 0.19 ms` (vs `~7.4ms` baseline), selfhost `97.4 ± 27.9 ms` (vs `~97ms` baseline) — within noise, no 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 `@CHANGELOG.md` at line 62, The JBCT conformance paragraph on "JBCT 0.25.0 conformance pass on v6" is extremely long and hurts readability; split it into multiple sentences or, preferably, a short bulleted list (as suggested) that separates the major changes (lint error removal, removed defensive guards in CstArray/CstArrayBuilder/TokenArray/TokenArrayBuilder/Diagnostic, Result<Void>→Result<Unit> in ParserGenerator, null→Option<T> conversions in DfaBuilder/IncrementalParser.tryPartialReparse/RuleClassifier, reflection unwrap changes in LexerCompiler.CompiledLexer.lex and ParserCompiler.CompiledParser.{parse, parseRuleFrom, ruleKinds}, generated parser/lexer behavior changes, `@SuppressWarnings` on hot-path mutators, and lint/bench results) so each key point (use the symbols above to locate text) becomes its own list item or short sentence for better scanability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ba6c3cb-5e0d-4248-b340-6f0ec62598e8
📒 Files selected for processing (117)
.gitignoreCHANGELOG.mdREADME.mddocs/HANDOVER.mddocs/MIGRATION-0.5-TO-0.6.mdpeglib-core/pom.xmlpeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/IncrementalEditBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/Java25LargeFixturesBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/Java25LargeFixturesV51Benchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/Java25V51GeneratedParseBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/Java25V51ParseBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/Java25V6ColdCompileBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/Java25V6ParseBenchmark.javapeglib-core/src/jmh/java/org/pragmatica/peg/v6/perf/JavacParseOnlyBenchmark.javapeglib-core/src/main/java/org/pragmatica/peg/grammar/Grammar.javapeglib-core/src/main/java/org/pragmatica/peg/grammar/GrammarParser.javapeglib-core/src/main/java/org/pragmatica/peg/grammar/GrammarResolver.javapeglib-core/src/main/java/org/pragmatica/peg/v6/Parser.javapeglib-core/src/main/java/org/pragmatica/peg/v6/PegParser.javapeglib-core/src/main/java/org/pragmatica/peg/v6/analyzer/LeftRecursionCause.javapeglib-core/src/main/java/org/pragmatica/peg/v6/analyzer/LeftRecursionDetector.javapeglib-core/src/main/java/org/pragmatica/peg/v6/analyzer/NamedCaptureCause.javapeglib-core/src/main/java/org/pragmatica/peg/v6/analyzer/NamedCaptureDetector.javapeglib-core/src/main/java/org/pragmatica/peg/v6/generator/LexerCompiler.javapeglib-core/src/main/java/org/pragmatica/peg/v6/generator/LexerGenerator.javapeglib-core/src/main/java/org/pragmatica/peg/v6/generator/ParserCompiler.javapeglib-core/src/main/java/org/pragmatica/peg/v6/generator/ParserGenerator.javapeglib-core/src/main/java/org/pragmatica/peg/v6/generator/VisitorGenerator.javapeglib-core/src/main/java/org/pragmatica/peg/v6/incremental/IncrementalParser.javapeglib-core/src/main/java/org/pragmatica/peg/v6/lexer/Dfa.javapeglib-core/src/main/java/org/pragmatica/peg/v6/lexer/DfaBuilder.javapeglib-core/src/main/java/org/pragmatica/peg/v6/lexer/LexerEngine.javapeglib-core/src/main/java/org/pragmatica/peg/v6/lexer/RuleClassifier.javapeglib-core/src/main/java/org/pragmatica/peg/v6/lexer/RuleKind.javapeglib-core/src/test/java/org/pragmatica/peg/grammar/RecoverDirectiveTest.javapeglib-core/src/test/java/org/pragmatica/peg/perf/SelfhostFixtureGenerator.javapeglib-core/src/test/java/org/pragmatica/peg/v6/PegParserGateTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/PegParserTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/analyzer/LeftRecursionDetectorTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/diagnostic/FactoryClassGeneratorDiagTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/CutOperatorTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/IdentifierFallbackTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/Java25BisectTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/Java25ParserGateTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/LexerGeneratorTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/NamedCaptureTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/ParserGeneratorTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/RecoverDirectiveGeneratorTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/RecoveryTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/generator/VisitorGeneratorTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/incremental/IncrementalParserTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/AliasDetectionTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/DfaBuilderTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/Java25CorpusGateTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/KeywordResolutionTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/LexerEngineTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/RuleClassifierTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/TriviaClassificationTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/lexer/UnicodeAndCommentsTest.javapeglib-core/src/test/java/org/pragmatica/peg/v6/token/TokenArraySpliceTest.javapeglib-core/src/test/resources/bench-fixtures/Java25SelfHost-v51.java.txtpeglib-core/src/test/resources/java25.pegpeglib-core/src/test/resources/perf-corpus-baseline/format-examples/Annotations.java.hashpeglib-core/src/test/resources/perf-corpus-baseline/format-examples/Annotations.java.ruleHits.txtpeglib-core/src/test/resources/perf-corpus-baseline/format-examples/CompoundAssignments.java.hashpeglib-core/src/test/resources/perf-corpus-baseline/format-examples/CompoundAssignments.java.ruleHits.txtpeglib-core/src/test/resources/perf-corpus-baseline/ruleCoverage.txtpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/Annotations.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/BlankLines.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/Comments.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/CompoundAssignments.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/ExhaustiveSwitchPatterns.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/Imports.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/KeywordPrefixedIdentifiers.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/Lambdas.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/LineWrapping.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/MultilineArguments.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/TernaryOperators.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/format-examples/TextBlocks.java.hashpeglib-core/src/test/resources/perf-corpus-interpreter-baseline/large/FactoryClassGenerator.java.txt.hashpeglib-formatter/pom.xmlpeglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6FormatContext.javapeglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6Formatter.javapeglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6FormatterConfig.javapeglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6FormatterRule.javapeglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6TriviaPolicy.javapeglib-formatter/src/main/java/org/pragmatica/peg/formatter/v6/package-info.javapeglib-formatter/src/test/java/org/pragmatica/peg/formatter/v6/V6FormatterCorpusGateTest.javapeglib-formatter/src/test/java/org/pragmatica/peg/formatter/v6/V6FormatterTest.javapeglib-incremental/pom.xmlpeglib-maven-plugin/pom.xmlpeglib-maven-plugin/src/main/java/org/pragmatica/peg/maven/GenerateV6Mojo.javapeglib-maven-plugin/src/test/java/org/pragmatica/peg/maven/MojoIntegrationTest.javapeglib-playground/pom.xmlpeglib-playground/src/main/java/org/pragmatica/peg/playground/v6/PlaygroundEngineV6.javapeglib-playground/src/main/java/org/pragmatica/peg/playground/v6/PlaygroundReplV6.javapeglib-playground/src/test/java/org/pragmatica/peg/playground/v6/PlaygroundEngineV6Test.javapeglib-playground/src/test/java/org/pragmatica/peg/playground/v6/PlaygroundReplV6Test.javapeglib-runtime/pom.xmlpeglib-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstArray.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstArrayBuilder.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/cst/CstNode.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/cst/ParseResult.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/diagnostic/Diagnostic.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/diagnostic/Severity.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/token/LexFn.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/token/TokenArray.javapeglib-runtime/src/main/java/org/pragmatica/peg/v6/token/TokenArrayBuilder.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/cst/CstArrayBuilderTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/cst/CstArrayTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/cst/CstNodeViewTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/cst/FindCheckpointAncestorTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/cst/ParseResultTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/cst/SpliceSubtreeTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/diagnostic/DiagnosticTest.javapeglib-runtime/src/test/java/org/pragmatica/peg/v6/token/TokenArrayTest.javapom.xml
| mvn install | ||
| | | | | ||
| |---|---| | ||
| | **Active branch** | `release-0.6.0` at `4a2799d`, tagged `v0.6.0-candidate` | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the commit and tag referenced in handover
echo "Checking commit 4a2799d:"
git log --oneline -1 4a2799d 2>/dev/null || echo "✗ Commit 4a2799d not found"
echo -e "\nChecking tag v0.6.0-candidate:"
git tag -l "v0.6.0-candidate" | grep -q "v0.6.0-candidate" && \
echo "✓ Tag v0.6.0-candidate exists" || \
echo "✗ Tag v0.6.0-candidate not found"
echo -e "\nChecking branch release-0.6.0:"
git branch -r | grep -q "origin/release-0.6.0" && \
echo "✓ Branch release-0.6.0 exists" || \
echo "✗ Branch release-0.6.0 not found"Repository: siy/java-peglib
Length of output: 336
Update handover references: tag v0.6.0-candidate and branch release-0.6.0 do not exist.
The handover document at line 13 references commit 4a2799d on branch release-0.6.0 with tag v0.6.0-candidate. While the commit exists, neither the tag nor the branch can be found in the repository. Update the document with the correct branch/tag information or remove these references if they are no longer applicable.
🤖 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 13, The HANDOVER.md references a non-existent
branch `release-0.6.0` and tag `v0.6.0-candidate` for commit `4a2799d`; edit the
line that currently reads "| **Active branch** | `release-0.6.0` at `4a2799d`,
tagged `v0.6.0-candidate` |" to either replace `release-0.6.0` and
`v0.6.0-candidate` with the actual existing branch/tag names that point to
`4a2799d` or remove the branch/tag portion entirely and keep only the commit SHA
`4a2799d` (or update to the correct commit if needed) so the document no longer
references missing refs.
Comment on lines
+111
to
+138
| private static Class<?> compileAndLoad(String source, String fqcn) throws Exception { | ||
| Path tempDir = Files.createTempDirectory("peglib-v51gen-bench"); | ||
| var packagePath = fqcn.substring(0, fqcn.lastIndexOf('.')).replace('.', '/'); | ||
| var simpleClassName = fqcn.substring(fqcn.lastIndexOf('.') + 1); | ||
|
|
||
| var packageDir = tempDir.resolve(packagePath); | ||
| Files.createDirectories(packageDir); | ||
|
|
||
| var sourceFile = packageDir.resolve(simpleClassName + ".java"); | ||
| Files.writeString(sourceFile, source); | ||
|
|
||
| var compiler = ToolProvider.getSystemJavaCompiler(); | ||
| if (compiler == null) { | ||
| throw new IllegalStateException("JDK compiler not available (not running on a JDK?)"); | ||
| } | ||
| int rc = compiler.run(null, null, null, | ||
| "-d", tempDir.toString(), | ||
| "-cp", System.getProperty("java.class.path"), | ||
| sourceFile.toString()); | ||
| if (rc != 0) { | ||
| throw new RuntimeException("Compilation failed for " + fqcn + " (rc=" + rc + ")"); | ||
| } | ||
|
|
||
| var classLoader = new URLClassLoader( | ||
| new URL[]{tempDir.toUri().toURL()}, | ||
| Java25V51GeneratedParseBenchmark.class.getClassLoader()); | ||
| return classLoader.loadClass(fqcn); | ||
| } |
There was a problem hiding this comment.
Resource leak: temp directory and URLClassLoader not cleaned up.
The compileAndLoad method creates a temporary directory (line 112) and a URLClassLoader (line 134-136) but neither is cleaned up. Over multiple benchmark trials/forks, this accumulates temp directories in the system temp space.
Considerations:
- JMH benchmarks typically run for extended periods, and temp directory accumulation can exhaust disk space.
- The URLClassLoader holds references to the temp directory, preventing OS cleanup even after JVM exit on some platforms.
- Since this is a benchmark measuring parse performance (not classloading), cleanup overhead is outside the measurement and acceptable.
♻️ Proposed fix: Add cleanup and use try-with-resources
private static Class<?> compileAndLoad(String source, String fqcn) throws Exception {
Path tempDir = Files.createTempDirectory("peglib-v51gen-bench");
+ try {
var packagePath = fqcn.substring(0, fqcn.lastIndexOf('.')).replace('.', '/');
var simpleClassName = fqcn.substring(fqcn.lastIndexOf('.') + 1);
var packageDir = tempDir.resolve(packagePath);
Files.createDirectories(packageDir);
var sourceFile = packageDir.resolve(simpleClassName + ".java");
Files.writeString(sourceFile, source);
var compiler = ToolProvider.getSystemJavaCompiler();
if (compiler == null) {
throw new IllegalStateException("JDK compiler not available (not running on a JDK?)");
}
int rc = compiler.run(null, null, null,
"-d", tempDir.toString(),
"-cp", System.getProperty("java.class.path"),
sourceFile.toString());
if (rc != 0) {
throw new RuntimeException("Compilation failed for " + fqcn + " (rc=" + rc + ")");
}
- var classLoader = new URLClassLoader(
+ try (var classLoader = new URLClassLoader(
new URL[]{tempDir.toUri().toURL()},
- Java25V51GeneratedParseBenchmark.class.getClassLoader());
- return classLoader.loadClass(fqcn);
+ Java25V51GeneratedParseBenchmark.class.getClassLoader())) {
+ return classLoader.loadClass(fqcn);
+ }
+ } finally {
+ // Clean up temp directory after class is loaded
+ if (Files.exists(tempDir)) {
+ Files.walk(tempDir)
+ .sorted(java.util.Comparator.reverseOrder())
+ .forEach(p -> {
+ try { Files.deleteIfExists(p); } catch (Exception ignored) {}
+ });
+ }
+ }
}Note: Closing the URLClassLoader immediately after loadClass may cause issues if the JVM needs to access class resources later. An alternative is to store the classLoader as a field and clean it up in a @TearDown(Level.Trial) method.
📝 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
| private static Class<?> compileAndLoad(String source, String fqcn) throws Exception { | |
| Path tempDir = Files.createTempDirectory("peglib-v51gen-bench"); | |
| var packagePath = fqcn.substring(0, fqcn.lastIndexOf('.')).replace('.', '/'); | |
| var simpleClassName = fqcn.substring(fqcn.lastIndexOf('.') + 1); | |
| var packageDir = tempDir.resolve(packagePath); | |
| Files.createDirectories(packageDir); | |
| var sourceFile = packageDir.resolve(simpleClassName + ".java"); | |
| Files.writeString(sourceFile, source); | |
| var compiler = ToolProvider.getSystemJavaCompiler(); | |
| if (compiler == null) { | |
| throw new IllegalStateException("JDK compiler not available (not running on a JDK?)"); | |
| } | |
| int rc = compiler.run(null, null, null, | |
| "-d", tempDir.toString(), | |
| "-cp", System.getProperty("java.class.path"), | |
| sourceFile.toString()); | |
| if (rc != 0) { | |
| throw new RuntimeException("Compilation failed for " + fqcn + " (rc=" + rc + ")"); | |
| } | |
| var classLoader = new URLClassLoader( | |
| new URL[]{tempDir.toUri().toURL()}, | |
| Java25V51GeneratedParseBenchmark.class.getClassLoader()); | |
| return classLoader.loadClass(fqcn); | |
| } | |
| private static Class<?> compileAndLoad(String source, String fqcn) throws Exception { | |
| Path tempDir = Files.createTempDirectory("peglib-v51gen-bench"); | |
| try { | |
| var packagePath = fqcn.substring(0, fqcn.lastIndexOf('.')).replace('.', '/'); | |
| var simpleClassName = fqcn.substring(fqcn.lastIndexOf('.') + 1); | |
| var packageDir = tempDir.resolve(packagePath); | |
| Files.createDirectories(packageDir); | |
| var sourceFile = packageDir.resolve(simpleClassName + ".java"); | |
| Files.writeString(sourceFile, source); | |
| var compiler = ToolProvider.getSystemJavaCompiler(); | |
| if (compiler == null) { | |
| throw new IllegalStateException("JDK compiler not available (not running on a JDK?)"); | |
| } | |
| int rc = compiler.run(null, null, null, | |
| "-d", tempDir.toString(), | |
| "-cp", System.getProperty("java.class.path"), | |
| sourceFile.toString()); | |
| if (rc != 0) { | |
| throw new RuntimeException("Compilation failed for " + fqcn + " (rc=" + rc + ")"); | |
| } | |
| try (var classLoader = new URLClassLoader( | |
| new URL[]{tempDir.toUri().toURL()}, | |
| Java25V51GeneratedParseBenchmark.class.getClassLoader())) { | |
| return classLoader.loadClass(fqcn); | |
| } | |
| } finally { | |
| // Clean up temp directory after class is loaded | |
| if (Files.exists(tempDir)) { | |
| Files.walk(tempDir) | |
| .sorted(java.util.Comparator.reverseOrder()) | |
| .forEach(p -> { | |
| try { Files.deleteIfExists(p); } catch (Exception ignored) {} | |
| }); | |
| } | |
| } | |
| } |
🤖 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/jmh/java/org/pragmatica/peg/v6/perf/Java25V51GeneratedParseBenchmark.java`
around lines 111 - 138, The compileAndLoad method creates a tempDir and a
URLClassLoader that are never cleaned up; modify
compileAndLoad/Java25V51GeneratedParseBenchmark to ensure the temp directory is
deleted and the URLClassLoader is closed when no longer needed: either (a)
return both the loaded Class and its classloader and close the URLClassLoader
and delete tempDir in a new `@TearDown`(Level.Trial) method, or (b) store the
URLClassLoader and tempDir as fields on Java25V51GeneratedParseBenchmark and
close the classLoader and recursively delete tempDir in `@TearDown`, ensuring you
reference the tempDir and URLClassLoader created in compileAndLoad and avoid
closing the loader while the benchmark still needs the loaded classes.
Comment on lines
+41
to
+47
| private static final Path GRAMMAR_PATH = Path.of("src/test/resources/java25.peg"); | ||
|
|
||
| private String grammarText; | ||
|
|
||
| @Setup(Level.Iteration) | ||
| public void setup() throws Exception { | ||
| grammarText = Files.readString(GRAMMAR_PATH, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Use classpath resource loading instead of a working-directory-relative path.
Path.of("src/test/resources/java25.peg") makes this benchmark fragile outside module-root execution. It can fail in CI/JAR/plugin-driven runs.
Proposed patch
- private static final Path GRAMMAR_PATH = Path.of("src/test/resources/java25.peg");
+ private static final String GRAMMAR_RESOURCE = "/java25.peg";
@@
`@Setup`(Level.Iteration)
public void setup() throws Exception {
- grammarText = Files.readString(GRAMMAR_PATH, StandardCharsets.UTF_8);
+ try (var in = Java25V6ColdCompileBenchmark.class.getResourceAsStream(GRAMMAR_RESOURCE)) {
+ if (in == null) {
+ throw new IllegalStateException("Missing benchmark resource: " + GRAMMAR_RESOURCE);
+ }
+ grammarText = new String(in.readAllBytes(), StandardCharsets.UTF_8);
+ }
PegParser.clearCache();
}🤖 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/jmh/java/org/pragmatica/peg/v6/perf/Java25V6ColdCompileBenchmark.java`
around lines 41 - 47, Replace the working-directory-dependent Path usage with
classpath resource loading: remove the GRAMMAR_PATH constant and, in the setup()
method, load "java25.peg" via getClass().getResourceAsStream("/java25.peg") or
Thread.currentThread().getContextClassLoader().getResourceAsStream("java25.peg")
and read the stream into grammarText (e.g., new
String(inputStream.readAllBytes(), StandardCharsets.UTF_8)), failing fast with a
clear error if the resource is null; update references to
GRAMMAR_PATH/grammarText accordingly.
Comment on lines
+47
to
+49
| private static final Path GRAMMAR_PATH = Path.of("src/test/resources/java25.peg"); | ||
| private static final Path FIXTURE_DIR = Path.of("src/test/resources/perf-corpus/format-examples"); | ||
|
|
There was a problem hiding this comment.
Resource paths are brittle for JMH runs; load grammar/fixtures from classpath.
Using src/test/resources/... ties execution to a specific working directory layout and can break in benchmark tooling environments.
Proposed patch
- private static final Path GRAMMAR_PATH = Path.of("src/test/resources/java25.peg");
- private static final Path FIXTURE_DIR = Path.of("src/test/resources/perf-corpus/format-examples");
+ private static final String GRAMMAR_RESOURCE = "/java25.peg";
+ private static final String FIXTURE_PREFIX = "/perf-corpus/format-examples/";
@@
`@Setup`(Level.Trial)
public void setup() throws Exception {
- String grammarText = Files.readString(GRAMMAR_PATH, StandardCharsets.UTF_8);
+ String grammarText = readResource(GRAMMAR_RESOURCE);
PegParser.clearCache();
parser = PegParser.fromGrammar(grammarText).unwrap();
- input = Files.readString(FIXTURE_DIR.resolve(fixture), StandardCharsets.UTF_8);
+ input = readResource(FIXTURE_PREFIX + fixture);
}
+
+ private static String readResource(String path) throws Exception {
+ try (var in = Java25V6ParseBenchmark.class.getResourceAsStream(path)) {
+ if (in == null) {
+ throw new IllegalStateException("Missing benchmark resource: " + path);
+ }
+ return new String(in.readAllBytes(), StandardCharsets.UTF_8);
+ }
+ }Also applies to: 71-75
🤖 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/jmh/java/org/pragmatica/peg/v6/perf/Java25V6ParseBenchmark.java`
around lines 47 - 49, Replace hard-coded filesystem paths GRAMMAR_PATH and
FIXTURE_DIR with classpath resource lookups: use the class loader to locate
"java25.peg" and the "perf-corpus/format-examples" resource directory (e.g.,
ClassLoader.getResource / getResourceAsStream or getResources and then
Paths.get(resource.toURI()) when available) inside the Java25V6ParseBenchmark
class so benchmarks work regardless of working directory; update any code using
GRAMMAR_PATH and FIXTURE_DIR to accept an InputStream/URL/Path obtained from the
classpath lookup and handle the case where the resource is missing by throwing a
clear exception.
Comment on lines
+99
to
+104
| // Fresh task per invocation: parsing state is per-task. null DiagnosticListener | ||
| // and null Writer route diagnostics to System.err; we don't expect any on the | ||
| // reference fixture (hand-written Java 25). For selfhost (synthesized parser | ||
| // source), syntactic diagnostics may appear but parse() still returns a tree. | ||
| JavacTask task = (JavacTask) compiler.getTask(null, null, null, null, null, sources); | ||
| Iterable<? extends CompilationUnitTree> trees = task.parse(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify benchmark call sites still using null output/listener for JavaCompiler#getTask.
rg -nP --type=java 'getTask\(\s*null\s*,\s*null\s*,\s*null'Repository: siy/java-peglib
Length of output: 238
Remove stderr I/O from benchmark timing path by suppressing compiler diagnostics.
Line 103 uses null writer and listener, which route javac diagnostics to System.err. Even diagnostic-free fixtures risk unexpected output from synthesized sources, introducing unmeasured I/O overhead that skews parse timing. Suppress diagnostics explicitly using a null writer and no-op listener.
🔧 Proposed fix
- JavacTask task = (JavacTask) compiler.getTask(null, null, null, null, null, sources);
+ JavacTask task = (JavacTask) compiler.getTask(
+ java.io.Writer.nullWriter(),
+ null,
+ diagnostic -> {},
+ null,
+ null,
+ sources
+ );📝 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
| // Fresh task per invocation: parsing state is per-task. null DiagnosticListener | |
| // and null Writer route diagnostics to System.err; we don't expect any on the | |
| // reference fixture (hand-written Java 25). For selfhost (synthesized parser | |
| // source), syntactic diagnostics may appear but parse() still returns a tree. | |
| JavacTask task = (JavacTask) compiler.getTask(null, null, null, null, null, sources); | |
| Iterable<? extends CompilationUnitTree> trees = task.parse(); | |
| // Fresh task per invocation: parsing state is per-task. null DiagnosticListener | |
| // and null Writer route diagnostics to System.err; we don't expect any on the | |
| // reference fixture (hand-written Java 25). For selfhost (synthesized parser | |
| // source), syntactic diagnostics may appear but parse() still returns a tree. | |
| JavacTask task = (JavacTask) compiler.getTask( | |
| java.io.Writer.nullWriter(), | |
| null, | |
| diagnostic -> {}, | |
| null, | |
| null, | |
| sources | |
| ); | |
| Iterable<? extends CompilationUnitTree> trees = task.parse(); |
🤖 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/jmh/java/org/pragmatica/peg/v6/perf/JavacParseOnlyBenchmark.java`
around lines 99 - 104, The benchmark currently passes null DiagnosticListener
and Writer to compiler.getTask, which routes diagnostics to System.err and can
add I/O overhead; instead supply an explicit no-op DiagnosticListener and a
null-printing Writer to suppress stderr: replace the null listener and writer
arguments in the call to compiler.getTask(...) (the place where JavacTask task =
(JavacTask) compiler.getTask(...) is created) with a DiagnosticListener
implementation that does nothing and a PrintWriter/Writer that discards output
(e.g., wraps OutputStream.nullOutputStream or similar), so task.parse() cannot
write to System.err during benchmarking.
Comment on lines
+145
to
+150
| // Generous bounds — JIT warmup variance allowed. Spec target is 600ms warm; we | ||
| // budget 1500ms cold for safety. Cache hit should be sub-millisecond. | ||
| long coldMs = coldNanos / 1_000_000L; | ||
| long warmMicros = warmNanos / 1_000L; | ||
| assertTrue(coldMs < 5_000L, () -> "cold latency " + coldMs + "ms exceeds 5000ms ceiling"); | ||
| assertTrue(warmMicros < 5_000L, () -> "cache hit took " + warmMicros + "µs (>5000µs)"); |
There was a problem hiding this comment.
Drop the hard wall-clock ceilings from this test.
These assertions depend on runner load, JIT warmup, and the CI machine, so they can fail unrelated changes even when caching is correct. Keep the cache-hit assertion here and move latency thresholds to a benchmark or an opt-in perf gate.
🤖 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/v6/PegParserTest.java` around
lines 145 - 150, The test currently enforces hard wall-clock ceilings via the
coldMs and warmMicros assertions; drop those absolute time ceilings. Remove the
coldMs assertTrue entirely and change the warmMicros assertion in PegParserTest
to a relative cache validation (e.g., assertTrue(warmMicros < coldMs, () ->
"cache hit not faster: warm " + warmMicros + "µs vs cold " + coldMs + "ms")),
and move any fixed latency thresholds into a separate benchmark or opt-in perf
gate instead.
Comment on lines
+81
to
+121
| private static void appendWhitespace(List<Doc> parts, String ws) { | ||
| if (ws.isEmpty()) { | ||
| return; | ||
| } | ||
| var run = new StringBuilder(); | ||
| for (var i = 0; i < ws.length(); i++) { | ||
| var c = ws.charAt(i); | ||
| if (c == '\n') { | ||
| if (!run.isEmpty()) { | ||
| parts.add(text(run.toString())); | ||
| run.setLength(0); | ||
| } | ||
| parts.add(new Doc.HardLine()); | ||
| } else { | ||
| run.append(c); | ||
| } | ||
| } | ||
| if (!run.isEmpty()) { | ||
| parts.add(text(run.toString())); | ||
| } | ||
| } | ||
|
|
||
| private static void appendLineComment(List<Doc> parts, String raw) { | ||
| var stripped = raw.endsWith("\n") ? raw.substring(0, raw.length() - 1) : raw; | ||
| parts.add(text(stripped)); | ||
| parts.add(new Doc.HardLine()); | ||
| } | ||
|
|
||
| private static void appendBlockComment(List<Doc> parts, String raw) { | ||
| if (raw.indexOf('\n') < 0) { | ||
| parts.add(text(raw)); | ||
| return; | ||
| } | ||
| var lines = raw.split("\n", -1); | ||
| for (var i = 0; i < lines.length; i++) { | ||
| if (i > 0) { | ||
| parts.add(new Doc.HardLine()); | ||
| } | ||
| parts.add(text(lines[i])); | ||
| } | ||
| } |
There was a problem hiding this comment.
CRLF handling leaks raw \r into formatted output
Whitespace/comment rendering only treats \n as a line break. For CRLF/CR inputs, carriage returns can be emitted as literal text and skew output.
Normalize line endings before token-to-Doc conversion
private static void appendWhitespace(List<Doc> parts, String ws) {
+ ws = ws.replace("\r\n", "\n").replace('\r', '\n');
if (ws.isEmpty()) {
return;
}
@@
private static void appendLineComment(List<Doc> parts, String raw) {
+ raw = raw.replace("\r\n", "\n").replace('\r', '\n');
var stripped = raw.endsWith("\n") ? raw.substring(0, raw.length() - 1) : raw;
parts.add(text(stripped));
parts.add(new Doc.HardLine());
}
@@
private static void appendBlockComment(List<Doc> parts, String raw) {
+ raw = raw.replace("\r\n", "\n").replace('\r', '\n');
if (raw.indexOf('\n') < 0) {
parts.add(text(raw));
return;
}Also applies to: 123-134
🤖 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-formatter/src/main/java/org/pragmatica/peg/formatter/v6/V6TriviaPolicy.java`
around lines 81 - 121, The code emits literal '\r' characters because
appendWhitespace, appendLineComment, and appendBlockComment only treat '\n' as
line breaks; normalize line endings at the start of each of these methods by
removing or converting CR characters (e.g., replace "\r\n" with "\n" and then
replace remaining '\r' or just strip '\r') so subsequent logic sees only '\n'
line breaks; update appendWhitespace to operate on the normalized ws string and
appendLineComment/appendBlockComment to use a normalized raw before trimming,
splitting, or checking for '\n'.
Comment on lines
+78
to
+100
| if (allUpToDate(lexerTarget, parserTarget, visitorTarget)) { | ||
| getLog().info("peglib:generate-v6 skipped (up-to-date): " | ||
| + lexerTarget.getFileName() + ", " | ||
| + parserTarget.getFileName() + ", " | ||
| + visitorTarget.getFileName()); | ||
| return; | ||
| } | ||
| var generated = readGrammar(grammarFile.toPath()).flatMap(this::buildAll); | ||
| if (generated instanceof Result.Failure<?> failure) { | ||
| throw new MojoFailureException(failure.cause().message()); | ||
| } | ||
| var bundle = generated.unwrap(); | ||
| var write = writeAll(bundle, lexerTarget, parserTarget, visitorTarget); | ||
| if (write instanceof Result.Failure<?> failure) { | ||
| throw new MojoExecutionException(failure.cause().message()); | ||
| } | ||
| for (var w : bundle.lexer().warnings()) { | ||
| getLog().warn("peglib:generate-v6 lexer warning: " + w); | ||
| } | ||
| getLog().info("peglib:generate-v6 wrote " | ||
| + lexerTarget.getFileName() + ", " | ||
| + parserTarget.getFileName() + ", " | ||
| + visitorTarget.getFileName()); |
There was a problem hiding this comment.
Register outputDirectory as a source root on every run.
This mojo writes files during generate-sources, but it never calls MavenProject.addCompileSourceRoot(...). Maven will not compile an arbitrary outputDirectory just because generation ran, and the up-to-date early return currently skips any chance to register it. Add the source root before the early return.
🤖 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-maven-plugin/src/main/java/org/pragmatica/peg/maven/GenerateV6Mojo.java`
around lines 78 - 100, The mojo must always register the generated source
directory as a compile source root even when up-to-date: call
MavenProject.addCompileSourceRoot(outputDirectory.getPath()) (or
project.addCompileSourceRoot(...)) before the early return that follows the
allUpToDate(lexerTarget, parserTarget, visitorTarget) check in GenerateV6Mojo so
the directory is registered on every run; keep the existing early-return
behavior but ensure outputDirectory is added first.
Comment on lines
+88
to
+106
| if (handleCommand(line.trim())) { | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void banner() { | ||
| out.println("peglib v6 playground REPL (grammar: " + grammarPath.toAbsolutePath() + ")"); | ||
| out.println("type ':help' for commands, ':quit' to exit."); | ||
| } | ||
|
|
||
| /** Handle a single input line. Returns {@code true} iff the REPL should exit. */ | ||
| boolean handleCommand(String line) throws IOException { | ||
| if (line.startsWith(":")) { | ||
| return handleMetaCommand(line); | ||
| } | ||
| loadGrammarIfChanged(); | ||
| runParse(line); | ||
| return false; |
There was a problem hiding this comment.
REPL currently trims parse input, changing grammar-visible text
Line 88 trims every line before parsing, so non-command input loses leading/trailing whitespace. That can change parse outcomes and diagnostics.
Preserve raw input for parsing; trim only for command detection
- if (handleCommand(line.trim())) {
+ if (handleCommand(line)) {
return;
}
@@
boolean handleCommand(String line) throws IOException {
- if (line.startsWith(":")) {
- return handleMetaCommand(line);
+ String cmdLine = line.stripLeading();
+ if (cmdLine.startsWith(":")) {
+ return handleMetaCommand(cmdLine.trim());
}
loadGrammarIfChanged();
runParse(line);
return false;
}🤖 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-playground/src/main/java/org/pragmatica/peg/playground/v6/PlaygroundReplV6.java`
around lines 88 - 106, The REPL currently trims the input before handling which
strips significant whitespace for parsing; change the flow so the raw line is
preserved for parsing and only use a trimmed version to detect commands: stop
trimming at the call site and update handleCommand(String line) to compute a
local trimmed variable (e.g., trimmed = line.trim()), use
trimmed.startsWith(":") to dispatch to handleMetaCommand(trimmed) but pass the
original line into loadGrammarIfChanged() / runParse(line) so parsing and
diagnostics see the raw input; keep method names handleCommand,
handleMetaCommand, loadGrammarIfChanged, and runParse to locate where to change.
Comment on lines
+35
to
+38
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where assertj version is managed/pinned in the repo.
fd -i pom.xml | xargs -I{} sh -c '
echo "== {} ==";
rg -n -C2 "assertj-core|assertj.version|dependencyManagement" "{}"
'Repository: siy/java-peglib
Length of output: 2137
Upgrade assertj-core to version 3.27.7 or later to patch GHSA-rqfh-9r24-8c9r (XXE vulnerability in XML assertions).
The repo uses AssertJ 3.26.0, which is vulnerable. Update the assertj.version property in the root pom.xml (line 52) from 3.26.0 to 3.27.7+. This will fix the dependency across all modules that inherit from the parent dependencyManagement.
🤖 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-runtime/pom.xml` around lines 35 - 38, Update the AssertJ dependency
version used by the project by changing the assertj.version property in the root
POM from 3.26.0 to 3.27.7 (or later) so the dependencyManagement and all modules
pick up the patched release; ensure the assertj-core artifact
(org.assertj:assertj-core) in the pom inherits this updated property and run a
build to verify no version conflicts.
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
Clean-slate redesign delivering parity with javac on Java parsing.
Performance (real-world Java25)
Highlights
peglib-runtime(25KB) — standalone-parser invariantTests
1440 passing across 7 modules. Java25 corpus 20/20 clean. FactoryClassGenerator (1900 LOC real-world JBCT generator): 0 diagnostics.
Migration
See
docs/MIGRATION-0.5-TO-0.6.md.Known limitations
Documented in
docs/HANDOVER.mdunder "Known limitations". Bounded; non-blocking for ship.Test plan
mvn installcleanSummary by CodeRabbit
New Features
Documentation
Chores