Skip to content

Optimize Lexer::readString #1948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OpaqueRock wants to merge 7 commits into
base: master
Choose a base branch
from
+126 −71
Open

Optimize Lexer::readString #1948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b3eab7
Optimize Lexer::readString and Visitor::visitInParallel
OpaqueRock Jul 16, 2026
ef0969c
Merge remote-tracking branch 'origin/master' into pr-1948-OpaqueRock
spawnia Jul 20, 2026
5605b5b
Fix visitInParallel() dropping visitors for custom Node kinds
OpaqueRock Jul 22, 2026
44f76e8
Address review feedback on Lexer/Visitor optimization
OpaqueRock Jul 22, 2026
cf27f7e
Remove visit() custom-kind bugfix, split into #1952
OpaqueRock Jul 22, 2026
65089c9
Remove Visitor optimization, split into #1953
OpaqueRock Jul 22, 2026
031cb52
Fix loose switch comparison mistaking NUL byte for EOF in string escapes
OpaqueRock Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 108 additions & 71 deletions src/Language/Lexer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,15 @@ class Lexer
private const TOKEN_PIPE = 124;
private const TOKEN_BRACE_R = 125;

/**
* Bytes that must be individually inspected while scanning a string body:
* the closing quote ("), the escape character (\), and all C0 control
* characters (0x00-0x1F) except TAB (0x09), which is a legal, unescaped
* SourceCharacter per the GraphQL spec and is scanned through like any
* other ordinary character.
*/
private const STRING_STOP_BYTES = "\"\\\x00\x01\x02\x03\x04\x05\x06\x07\x08\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f";

public Source $source;

/** @phpstan-var ParserOptions */
Expand Down Expand Up @@ -391,18 +400,46 @@ private function readString(int $line, int $col, Token $prev): Token
{
$start = $this->position;

// Skip leading quote and read first string char:
[$char, $code, $bytes] = $this->moveStringCursor(1, 1)
->readChar();
// Skip leading quote
$this->moveStringCursor(1, 1);

$chunk = '';
// Bulk-scan runs of bytes that need no special handling via strcspn()
// (a single native call) instead of decoding/validating one UTF-8
// character at a time.
//
// The stop-byte set includes ASCII control characters (0x00-0x1F),
// not just backslash/quote/line-terminators, because
// assertValidStringCharacterCode() must still reject most of them.
// See self::STRING_STOP_BYTES for details on the excluded TAB (0x09).

$body = $this->source->body;
$bodyLength = strlen($body);
$value = '';

while (! in_array($code, [null, 10, 13], true)) { // not LineTerminator
if ($code === 34) { // Closing Quote (")
while (true) {
$byteStart = $this->byteStreamPosition;
$runLength = strcspn($body, self::STRING_STOP_BYTES, $byteStart);

if ($runLength > 0) {
$chunk = substr($body, $byteStart, $runLength);
$value .= $chunk;
// $this->position counts decoded characters, not bytes: count the
// non-continuation bytes (i.e. NOT matching 10xxxxxx) in the chunk to
// keep it accurate for multi-byte UTF-8 content.
$continuationBytes = preg_match_all('/[\x80-\xBF]/', $chunk);
assert(is_int($continuationBytes), 'Pattern is valid, so preg_match_all() cannot fail');
$this->position += strlen($chunk) - $continuationBytes;
Comment on lines +429 to +431

@spawnia spawnia Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three-line preg_match_all + assert + arithmetic block can be replaced with a single mb_strlen call that returns the codepoint count directly. For valid UTF-8, mb_strlen($chunk, 'UTF-8') is semantically equivalent to strlen($chunk) - count(continuation bytes), avoids PCRE overhead, and makes the intent ("count codepoints") explicit.

Suggested change
$continuationBytes = preg_match_all('/[\x80-\xBF]/', $chunk);
assert(is_int($continuationBytes), 'Pattern is valid, so preg_match_all() cannot fail');
$this->position += strlen($chunk) - $continuationBytes;
$this->position += mb_strlen($chunk, 'UTF-8');

$this->byteStreamPosition = $byteStart + $runLength;
}

$bytePos = $this->byteStreamPosition;
if ($bytePos >= $bodyLength) {
break; // EOF mid-string
}

$code = ord($body[$bytePos]);

// Skip quote
if ($code === 34) { // Closing Quote (")
$this->moveStringCursor(1, 1);

return new Token(
Expand All @@ -416,79 +453,79 @@ private function readString(int $line, int $col, Token $prev): Token
);
}

$this->assertValidStringCharacterCode($code, $this->position);
$this->moveStringCursor(1, $bytes);

if ($code === 92) { // \
$value .= $chunk;
[, $code] = $this->readChar(true);
if ($code === 10 || $code === 13) { // LineTerminator

@spawnia spawnia Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: could extracting 10 (LF) and 13 (CR) into named constants have a performance cost in such a hot path? Either way, whichever approach we go with should be consistent across the file — right now it's magic numbers everywhere.

break;
}

switch ($code) {
case 34:
$value .= '"';
break;
case 47:
$value .= '/';
break;
case 92:
$value .= '\\';
break;
case 98:
$value .= chr(8); // \b (backspace)
break;
case 102:
$value .= "\f";
break;
case 110:
$value .= "\n";
break;
case 114:
$value .= "\r";
break;
case 116:
$value .= "\t";
break;
case 117:
$position = $this->position;
[$hex] = $this->readChars(4);
if (preg_match('/[0-9a-fA-F]{4}/', $hex) !== 1) {
throw new SyntaxError($this->source, $position - 1, "Invalid character escape sequence: \\u{$hex}");
}
// Any other control character reaching here must be invalid
// (TAB, the only legal one, is excluded from the stop-byte set).
$this->assertValidStringCharacterCode($code, $this->position);

$code = hexdec($hex);
assert(is_int($code), 'Since only a single char is read');
// $code === 92 (\), i.e. an escape sequence.
$this->moveStringCursor(1, 1);
[, $code] = $this->readChar(true);

// UTF-16 surrogate pair detection and handling.
$highOrderByte = $code >> 8;
if ($highOrderByte >= 0xD8 && $highOrderByte <= 0xDF) {
[$utf16Continuation] = $this->readChars(6);
if (preg_match('/^\\\u[0-9a-fA-F]{4}$/', $utf16Continuation) !== 1) {
throw new SyntaxError($this->source, $this->position - 5, 'Invalid UTF-16 trailing surrogate: ' . $utf16Continuation);
}
// null means EOF; checked separately since switch is not a strict comparison.
if ($code === null) {
break;
}

$surrogatePairHex = $hex . substr($utf16Continuation, 2, 4);
$value .= mb_convert_encoding(pack('H*', $surrogatePairHex), 'UTF-8', 'UTF-16');
break;
switch ($code) {
case 34:
$value .= '"';
break;
case 47:
$value .= '/';
break;
case 92:
$value .= '\\';
break;
case 98:
$value .= chr(8); // \b (backspace)
break;
case 102:
$value .= "\f";
break;
case 110:
$value .= "\n";
break;
case 114:
$value .= "\r";
break;
case 116:
$value .= "\t";
break;
case 117:
$position = $this->position;
[$hex] = $this->readChars(4);
if (preg_match('/[0-9a-fA-F]{4}/', $hex) !== 1) {
throw new SyntaxError($this->source, $position - 1, "Invalid character escape sequence: \\u{$hex}");
}

$code = hexdec($hex);
assert(is_int($code), 'Since only a single char is read');

// UTF-16 surrogate pair detection and handling.
$highOrderByte = $code >> 8;
if ($highOrderByte >= 0xD8 && $highOrderByte <= 0xDF) {
[$utf16Continuation] = $this->readChars(6);
if (preg_match('/^\\\u[0-9a-fA-F]{4}$/', $utf16Continuation) !== 1) {
throw new SyntaxError($this->source, $this->position - 5, 'Invalid UTF-16 trailing surrogate: ' . $utf16Continuation);
}

$this->assertValidStringCharacterCode($code, $position - 2);

$value .= Utils::chr($code);
$surrogatePairHex = $hex . substr($utf16Continuation, 2, 4);
$value .= mb_convert_encoding(pack('H*', $surrogatePairHex), 'UTF-8', 'UTF-16');
break;
// null means EOF, will delegate to general handling of unterminated strings
case null:
continue 2;
default:
$chr = Utils::chr($code);
throw new SyntaxError($this->source, $this->position - 1, "Invalid character escape sequence: \\{$chr}");
}
}

$chunk = '';
} else {
$chunk .= $char;
}
$this->assertValidStringCharacterCode($code, $position - 2);

[$char, $code, $bytes] = $this->readChar();
$value .= Utils::chr($code);
break;
default:
$chr = Utils::chr($code);
throw new SyntaxError($this->source, $this->position - 1, "Invalid character escape sequence: \\{$chr}");
}
}

throw new SyntaxError($this->source, $this->position, 'Unterminated string.');
Expand Down
18 changes: 18 additions & 0 deletions tests/Language/LexerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,23 @@ public function testLexesStrings(): void
);
}

public function testLexesStringsWithUnescapedTabCharacter(): void
{
$source = <<<'GRAPHQL'
"before after"
GRAPHQL;

self::assertArraySubset(
[
'kind' => Token::STRING,
'start' => 0,
'end' => strlen($source),
'value' => substr($source, 1, -1), // strip the surrounding quotes
Comment on lines +309 to +310

@spawnia spawnia Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected end and value are derived from $source at assertion time (strlen, substr). If the test input were wrong, the assertion would be wrong in the same direction and the test would still pass. Since the source is a fixed 14-byte literal ("before<TAB>after"), both values can be hardcoded.

Suggested change
'end' => strlen($source),
'value' => substr($source, 1, -1), // strip the surrounding quotes
'end' => 14,
'value' => "before\tafter",

],
(array) $this->lexOne($source)
);
}
Comment thread
spawnia marked this conversation as resolved.

/** @see it('lexes block strings') */
public function testLexesBlockString(): void
{
Expand Down Expand Up @@ -423,6 +440,7 @@ public static function reportsUsefulStringErrors(): iterable
yield ['"bad \\uD835\\uXXXX esc"', 'Invalid UTF-16 trailing surrogate: \\uXXXX', self::loc(1, 13)];
yield ['"bad \\uD835\\uFXXX esc"', 'Invalid UTF-16 trailing surrogate: \\uFXXX', self::loc(1, 13)];
yield ['"bad \\uD835\\uXXXF esc"', 'Invalid UTF-16 trailing surrogate: \\uXXXF', self::loc(1, 13)];
yield ['"bad \\' . "\0" . ' esc"', 'Invalid character escape sequence: \\' . "\0", self::loc(1, 7)];
}

/**
Expand Down
Loading