GeezerVison is showing github.com — Fix line counting in get_location and print_block_string by arxhr007 · Pull Request #295 · graphql-python/graphql-core
Conversation
Fixes #294
Summary
Source.get_location() and print_block_string() used str.splitlines(), which recognizes more line boundaries than the GraphQL spec / graphql-js (\r\n, \n, \r only). This produced three issues, all fixed by splitting on \r\n|[\n\r] — the same regex print_source_location() already uses:
IndexError crash: an error location computed via get_location() could reference a line that doesn't exist in print_source_location()'s line array (e.g. source containing a vertical tab), making str(GraphQLSyntaxError) itself raise.- Wrong line numbers at line starts:
splitlines() drops the trailing segment after a final newline, so every character immediately following a line terminator was reported on the previous line (Source("a\nb").get_location(2) → (1,2) instead of (2,1)).
- Location/serialization drift from graphql-js:
\v, \f, \x1c–\x1e, \x85,
,
were counted as line breaks in get_location() (graphql-js counts only \r\n|[\n\r]), and print_block_string() split values on the same too-broad set.
Found by differential-fuzzing parse/print output against graphql-js on mutated documents. After the fix, error locations and printed block strings match graphql-js byte-for-byte on the cases that previously diverged (e.g. "Q\x0b… → both report Unterminated string at 1:30).
Test plan
-
pytest tests/ — 3,369 passed, 17 skipped
- New regression tests: line-start + non-GraphQL-terminator locations (
test_source.py), get_location-derived location rendering (test_print_location.py), non-GraphQL line breaks in print_block_string (test_block_string.py)
-
ruff check + ruff format --check clean on all touched files
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Use the same GraphQL line terminator set (LF, CR, CRLF) that graphql-js
and print_source_location() use, instead of str.splitlines() which also
splits on VT, FF, FS/GS/RS, NEL and U+2028/U+2029 and drops a trailing
empty line segment.
This fixes three issues:
- characters right after a line terminator were reported on the
previous line
- characters after non-GraphQL line breaks got wrong line numbers
- print_source_location() could crash with IndexError when an error
location pointed past the last printed line (issue #294)
print_block_string() also splits raw values into lines and had the
same drift: values containing e.g. VT printed as multi-line in Python
but single-line in graphql-js.
Merging this PR will not alter performance
✅ 20 untouched benchmarks
Comparing arxhr007:fix/splitlines-location-parity (7524332) with main (f9bdf7c)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks for reporting and fixing this! It is a clear improvement, so I merge it as it is. I still plan some follow-up changes to bring this code fully in line with GraphQL.js.
Fixed by this PR on main:
str(error) no longer raises IndexError when the source contains characters like \v, \f, \x85 or \u2028.get_location now reports the correct line for positions at the start of a line, and only counts \r\n, \n and \r as line terminators.print_block_string now splits lines the same way as GraphQL.js.
Planned as part of the regular porting from GraphQL.js:
Also planned:
- Backporting the fix to the 3.2.x line, because this PR only targets
main.
- Removing the leftover
or [""] in print_block_string, which can no longer take effect.
I'll take care of that, you don't need to send a separate PR.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
The follow-up is done, on main and on js-16.x.x, and will ship in 3.3.2 and 3.2.14:
get_location now uses the same scanning algorithm as GraphQL.js (60bcd297). Positions inside a \r\n report column 0 and positions outside the source body also match now. I compared it with GraphQL.js v17.1.0 and v16.15.0 on about 100,000 positions and found no differences.- The location tests from test: cover source location lookups graphql/graphql-js#4935 are ported.
- The leftover
or [""] in print_block_string is gone (0aaea717).
- This PR and the changes above are backported to the 3.2.x line (
001ca2c2, which keeps your authorship, and 2e681e64).
The upstream performance changes that share location lookups (graphql/graphql-js#4936 and graphql/graphql-js#4958) follow with the regular porting.
Thanks again for reporting and fixing this!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Cito
commented
Nov 6, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
The follow-up is done, on main and on js-16.x.x, and shipped in 3.3.2 and 3.2.14:
get_location now uses the same scanning algorithm as GraphQL.js (60bcd297). Positions inside a \r\n report column 0 and positions outside the source body also match now. I compared it with GraphQL.js v17.1.0 and v16.15.0 on about 100,000 positions and found no differences.- The location tests from test: cover source location lookups graphql/graphql-js#4935 are ported.
- The leftover
or [""] in print_block_string is gone (0aaea717).
- This PR and the changes above are backported to the 3.2.x line (
001ca2c2, which keeps your authorship, and 2e681e64).
The upstream performance changes that share location lookups (graphql/graphql-js#4936 and graphql/graphql-js#4958) are ported as well (0a57746d, 1c7791b8, and on 3.2.x 3489973c, a0e66c11).
Thanks again for reporting and fixing this!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Cito
Cito approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
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.
|