Skip to content

Fix CR line tracking and block string normalization - #4518

Open
andimarek wants to merge 2 commits into
masterfrom
codex/fix-parser-line-terminators
Open

andimarek wants to merge 2 commits into
masterfrom
codex/fix-parser-line-terminators

Conversation

@andimarek

@andimarek andimarek commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Standalone carriage returns did not advance the lexer's line counter, so AST nodes and errors after a CR were reported on the previous line. For example, parsing { f(arg: "😀")\r ) } reported line 1, column 16; it now reports line 2, column 2.

This change applies the GraphQL Line Terminators rules throughout parsing:

  • Count standalone CR and LF as line breaks, and count CRLF once, including after comments and within block strings. The lexer tracks positions directly, preserving the original source characters and ignored-character captures.
  • Split block-string values on CR, LF, or CRLF before removing indentation and joining lines with LF. Previously, CR could remain in the resulting value and interfere with indentation and blank-line processing.
  • Reject U+2028 and U+2029 between tokens. They remain valid content inside strings and comments.

Added 32 Spock regression cases covering AST and error locations, code-point columns after emoji, comments, EOF, validation errors, multiple sources, mixed line endings, reader-buffer boundaries, block-string values, and captured whitespace. Thirteen cases failed before the fix. Updated the existing ignored-character test to expect the corrected positions after CR, and restored the JVM-wide parser options after the existing options test to prevent it from affecting later tests.

Added explicit ignored-character capture tests for 43 AST node types, checking exact surrounding spaces, tabs, commas, CR, LF, and CRLF when capture is enabled and empty captures when disabled. These tests use per-parse options. This preserves coverage after fixing the global-options leak: the master baseline had exercised 33 builders only because an earlier test left capture enabled for later tests. The coverage baseline is unchanged.

Validation:

  • Full Java 25 test suites and coverage: ./gradlew test testng jacocoTestReport (excluding two uncommitted local Unicode investigation tests).
  • All 32 newline regression cases and 43 ignored-character capture cases pass.
  • Java 25: 6,071 passed, 57 skipped, zero failures across Spock and TestNG.
  • The unchanged per-class coverage gate reports zero regressions across 992 classes. Covered lines: 30,226 versus 30,210 on master; covered methods: 8,080 versus 8,074.
  • git diff --check and the repository's pre-commit file checks pass.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 6128 (+75 🟢) 6072 (+75 🟢) 0 (±0) 0 (±0) 56 (±0)
Java 17 6128 (+75 🟢) 6071 (+75 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 21 6128 (+75 🟢) 6071 (+75 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 25 6128 (+75 🟢) 6071 (+75 🟢) 0 (±0) 0 (±0) 57 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 24544 (+300 🟢) 24317 (+300 🟢) 0 (±0) 0 (±0) 227 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 30226 3080 90.8% ±0.0%
Branches 8868 1508 85.5% ±0.0%
Methods 8080 1185 87.2% ±0.0%

Changed Class Coverage (7 classes)

Class Line Branch Method
g.e.ExecutionStrategy ±0.0% +0.8% 🟢 ±0.0%
g.l.DirectiveExtensionDefinition
$Builder
+6.5% 🟢 ±0.0% +9.1% 🟢
g.l.IgnoredChar +11.1% 🟢 ±0.0% +20.0% 🟢
g.l.SchemaExtensionDefinition
$Builder
+5.9% 🟢 ±0.0% +8.3% 🟢
g.p.GraphqlLexerATNSimulator +100.0% 🟢 +100.0% 🟢 +100.0% 🟢
g.p.MultiSourceReader
$SourcePart
±0.0% +12.5% 🟢 ±0.0%
g.p.StringValueParsing ±0.0% ±0.0% +2.4% 🟢

Full HTML report: build artifact jacoco-html-report

Updated: 2026-10-30 06:15:28 UTC

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant