Consolidation pass: parser internals, LSP, CI, tests - #153
Merged
Merged
Conversation
corpus/corpus.json records the URL and branch of every corpus repository so a fresh clone can restore them. corpus/update now clones the missing entries before pulling, and gains --add, --clone-only, --no-clone and --write-manifest.
Line and pattern churn from upstream projects only; no lint rule output changed for patterns present in both runs.
addcslashes escaped every byte above 0x7E, so a pattern such as /《붉은별》/iu was reported as /\343\200\212…/iu. Under /u that is a different pattern — \343 is ã, which has a case — and it could not be pasted back into PHP. DisplayEscaper now escapes control bytes only, falling back to full byte escaping when the text is not valid UTF-8. Inline flag diagnostics also claimed a flag was "already set globally" when an earlier inline flag group had set it.
Adds TextFormatter, smarty, WhichBrowser/Parser, minify, highlight.php, browscap-php, php-markdown, html5-php, html-sanitizer and mail-mime-parser: 67 new findings, no diagnostic change on the patterns already covered. Non-ASCII patterns are now logged verbatim.
json_encode() rejects invalid UTF-8, so linting a codebase holding a byte-mode pattern aborted the whole JSON report with "Failed to encode JSON". The checkstyle and JUnit formatters were quieter but no better: htmlspecialchars() returns an empty string for such values, so the pattern simply vanished from the report. Both now go through DisplayEscaper, like the console output already did.
Well-known vendors and regex-heavy code: Google (api-php-client, gax, site-kit), Automattic Jetpack, Yoast SEO, Azure, Shopify, SendGrid, Mailgun, Algolia, PayPal, GitLab, Slack, MediaWiki, moodle, Mautic, roundcube, phpMyAdmin, silverstripe, craftcms, statamic, grav, concrete, pimcore, ProcessWire, sabre dav/vobject, TCPDF, html2pdf, pdfparser, laminas filter/validator, simplepie, spyc, jBBCode, zxcvbn. 530 repositories, 726 new findings, no diagnostic change on the 1296 patterns already covered.
Two patterns PCRE compiles were rejected, both found by linting the corpus: - the validator refused the negation of the POSIX class "word" only, while PCRE negates every class it supports; - the lexer is not aware of /x, so the '[' of a comment such as "# match $type not containing whitespace : [ or ]" opened a character class that never closed. Comments are now consumed as literals up to the end of the line, the way PCRE reads them.
Only the pattern-level "x" modifier turned on extended mode, so "(?x)" and "(?x:...)" left whitespace significant and "#" literal — and a "[" in what should have been a comment opened a character class. The lexer now follows the modifier through the group structure, the parser skips ignorable whitespace and builds comment nodes accordingly, and the compiler renders a comment as "# ..." and keeps "\ " escaped whenever extended mode is in force at that point rather than only when the whole pattern carries the flag. Scoping matches PCRE: a bare "(?x)" holds until the end of the enclosing group and crosses "|", while "(?x:...)" ends with its group.
The AST holds no node for the whitespace that /x ignores, so compiling a parsed pattern back collapsed "/ a b /x" to "/ab/x" and flattened documented multi-line patterns onto a single line. RegexNode now carries the body it was parsed from, and the compiler reads the gaps between nodes back from it — only when they hold nothing but whitespace, so the nodes stay the single source of truth for what a pattern matches. An AST built by hand has no source and compiles as before, and pretty-printing still reflows on purpose.
Escaping punctuation is optional in most places, a backreference has several syntaxes and a code point can be written as an escape or as itself. The compiler picked one form for each, so recompiling a parsed pattern rewrote code nobody asked to have rewritten — "(?P=name)" came back as "\k<name>", "[@\[\]]" as "[@[\]]", "«" as "\xC2\xAB". Literals, character literals, backreferences and the opening bracket of a character class now keep the source text when it is available, guarded by a check that both spellings mean the same thing, so a stale offset can never change what a pattern matches. Text inside \Q...\E is left alone: the quoting is dropped on the way out, so those literals still need escaping. Conditionals no longer add parentheses around an assertion condition. Pattern comparison keeps using the normalized form through the new preserveSpelling flag, so the optimizer still reports the same suggestions.
The docblocks of the promoted constructor parameters kept the indentation they had before the constructor was expanded over several lines.
The cached payload embedded the cache version unquoted, so every file it wrote held "if (Regex::CACHE_VERSION !== 1.3.0)" — a syntax error. Loading one raised a ParseError that the filesystem cache swallowed, so the cache never gave anything back while still counting the read as a hit. The version is now written as a literal, a load that comes back empty counts as a miss, and the cache version is bumped: the AST nodes gained properties since 1.3.0, and entries written by that release would be restored with those properties uninitialized.
CI ran tests/Unit and tests/Integration only, so the functional, regression and documentation tests were never executed by anything but a developer running the suite by hand. The tests job now runs the phpunit configuration as it is, which covers the whole tests/ tree. Mutation testing gets a job of its own. Running it over the whole source tree would take hours, so it mutates the lines a pull request touches and holds them to the thresholds that infection.json.dist already declared.
Columns were counted in bytes while the protocol counts UTF-16 code units, so every diagnostic on a line holding an accent or an emoji landed a few characters off. They are now converted both ways, with a byte fallback for a line that is not valid UTF-8, and the line lookup bisects instead of walking the file for each position. Reading the body of a message only gave up when fread() returned false. A closed connection returns an empty string instead, so a disconnected editor left the server spinning at full speed. Nothing caught what a handler threw, so a single pattern hitting a recursion or resource limit took the whole session down. A failed request is now answered with an internal error and the loop carries on. The messages are read from an injected stream and the responses are written to one, which is what lets the tests drive a session and read its answers back. The handshake also stops advertising a version of its own.
Anchors compiled to an epsilon transition whatever their place in the pattern. At the edges of an alternative that is right — a whole-string match starts at the start and ends at the end — but in the middle it is not: "/a^b/" matches nothing, and the solver answered that it was equivalent to "/ab/". The placement check that partial match mode already ran now also guards full match mode, so a pattern whose anchors carry meaning is refused instead of being answered wrongly.
Two paths outside the state budgets went through all 1 114 112 code points.
Reading "\w", "\s" or "\d" under /u ran a preg_match per code point, and
folding the case of a class ran two mb_* calls and a union per code point it
covered, so a wide class cost seconds and no budget ever noticed.
The classes are now matched a block at a time, with the block encoded in one
conversion instead of code point by code point. Case folding walks the few
thousand code points that have a mapping at all, so folding "[\x{0}-\x{10FFFF}]"
costs what folding "[a]" costs.
Reading "\w" under /u drops from 1.7s to 0.8s, and a wide case-insensitive
class from seconds to milliseconds.
Python has no regex literal, so formatLiteral() returned the pattern as a raw string — and dropped every flag on the way: "/foo/i" came back as r'foo', matching case-sensitively. The flags are now spelled inline, at the start of the pattern, which is where Python accepts them. A quote inside the pattern was escaped with a backslash, but a raw string keeps that backslash, so the pattern changed. The delimiter is chosen to suit the pattern instead, and one holding both quotes falls back to an ordinary string with its backslashes doubled.
The curl and wget fallbacks ran without any timeout flag, so an unreachable mirror left the updater hanging until someone killed it. Both now give up on connecting after ten seconds and on the transfer after thirty, which is what the stream context already did.
The Laravel and Symfony console formatters were the same 347 lines twice over: after renaming, the two files differed by their namespace, their class name and their docblock. Laravel builds its console on Symfony's, so they render a report identically and always will. The rendering moves to a shared abstract formatter and each bridge keeps the name it exposes.
Five commands and formatters carried the same byte-escaping loop to show a counterexample: quote it, write the control bytes as escapes, say so in words when it is empty. It now lives next to the other display escaping, with the console markup passed in by the caller.
The two compiler visitors write very different dialects, but they refuse an unsupported construct the same way and strip the whitespace /x allows inside a quantifier the same way, each with its own copy. Both now extend a base that holds the context they compile against and those two behaviours, which is also where a third target would start.
No path ever produced a UnicodeNode: a "\x{...}" or "\u{...}" escape becomes
a CharLiteralNode, and has for as long as the parser has existed. The node
still sat in NodeVisitorInterface, so twenty-five visitors carried a
visitUnicode() method that could not be called, several lint rules branched
on a type they could not see, and two test classes existed only to reach
those branches.
ReDoSAnalyzerInterface goes with it: nothing implemented it, not even
ReDoSAnalyzer, which has the same signature.
The removals are listed in UPGRADING.md. The cache version stays at 1.4.0,
which is not released yet and already covers this shape change.
The lexer cached its two compiled token regexes under a key mixing the byte mode with the PHP version, but the patterns it compiles are constants and only the byte mode changes what comes out. A process reading patterns for several PHP versions compiled the same two regexes again for each of them. Nothing else in the lexer read the version, so the constructor argument goes with the key. Choosing a PHP version explicitly still reaches the parser, which is what decides the modifiers a pattern may carry.
Sixteen places read a key their own array shape declares as always present and fell back to null "just in case", which static analysis has been reporting as dead for a while. Two of them were not dead: a test built a lint result without its pattern key, and another passed one array level too many to a private method through reflection. The fallbacks were hiding malformed test data rather than guarding real input, so the fixtures are fixed and the fallbacks are gone.
analyze() ran four subsystems and caught \Throwable around each of them, writing whatever came out into the report as an error on the pattern. A TypeError or a broken collaborator therefore came back as "your regex is invalid", and no report could ever point at a bug in the library. Each block now catches the exceptions RegexParser raises for a pattern it cannot handle, and nothing else. That leaves a cache that throws while being read. Writing to a cache was already allowed to fail silently; reading now is too, since parsing the pattern again is always an option and a broken cache is never the pattern's fault.
The help command carried its own copy of the command table, and it had already drifted: three commands were missing from it altogether, and four descriptions no longer matched what the command itself says. "regex help graph" answered "Unknown command". The application now hands the help what it has registered, so the summary, the list of available commands and each page take their name and their description from the command. The pages for graph, clear-cache and version are written, and a test walks every registered command to keep the two in step. The wiring the binary carried moves to a factory, which is what lets that test build the real application.
The compatibility file ran class_alias() over twenty-five classes as soon as the package was autoloaded, and class_alias() loads its target: every process using the library paid for the whole automata layer, whether it compared two languages or only parsed a pattern. Twenty-seven RegexParser classes were loaded before a line of user code ran; now none are. The old names are aliased by an autoloader that answers for them, so they resolve to the same class as before, on the first mention rather than up front.
Fifty-five tests parsed a pattern and asserted nothing: they caught a crash
and nothing else, so a parser that quietly dropped a construct passed them
all. They are now one case per construct, each asserting what the parser
read by writing the pattern back out, and — for the patterns PCRE accepts —
that what comes out still compiles.
Nine of them do not come back exactly as they went in: "(?P'name'...)",
"\g{name}" and the conditionals around an assertion are written back in
another spelling. The expected output records that rather than hiding it.
What is left of the old file is the half that always asserted: the syntax
the parser must refuse, and the places where it reads ahead and rewinds. It
is named for that now.
"(?(R)yes|no)" asks whether the pattern is currently recursing. The parser reads the condition as a subroutine reference, and the compiler wrote it back as a subroutine call — "(?((?R))yes|no)" — which PCRE refuses. Any round trip through the compiler therefore broke such a pattern. Found while giving the assertion-free tests something to assert.
Two hundred and twenty-eight tests ran a pattern through the parser, the
validator or the sample generator and asserted nothing at all. They were
green whatever the library returned, as long as it did not throw.
The patterns they covered are kept, with an assertion each:
- every construct is parsed and written back out, and what comes back is
compared to the pattern that went in — three hundred and fifteen cases,
each recording the spelling the compiler produces;
- the patterns the validator must accept say so, instead of running the
validator and ignoring it;
- a generated sample is now checked against the pattern it came from,
eight times over, since the generator picks its characters at random.
The files whose only content was assertion-free are gone; the error cases
that lived among them move to the parser's rejection tests.
Twenty-three lexer tests asserted that tokenizing produced at least one
token, which is true of every input, empty ones included. They are now one
case per input, each spelling out the tokens that come out.
Writing them down records something worth knowing: the lexer folds the
negation of a unicode property into the token value, so "\P{L}" and "\p{^L}"
are the same token by the time the parser sees them.
The three remaining "at least one of something" assertions elsewhere are
replaced by what the test was actually after, or dropped where the line
above already compared the whole result.
The twenty-eight rules were only exercised through the linter as a whole and through the corpus, which counts issues per project rather than naming them. A rule that stopped firing, or started firing on sound patterns, would have gone unnoticed. Each of the thirty-two rule ids now has a pattern it must report and a close one it must leave alone — "[a-a]" against "[a-f]", "(a+)+" against "(?>a+)+". A last test compares the table with the registry, so a new rule cannot be registered without an example.
The corpus test read its expectations out of the rendered report, then ran two hundred lines of heuristics to decide which of the rendered warnings a bare pattern still deserved — re-implementing the rules it was testing. A rule could not disagree with itself, and a pattern the reconstruction had mangled was skipped in silence. The reconstruction now happens once, in a generator, and what it produces is committed: a fixture saying which rules each of the 1493 patterns raises. The test compares the whole list rather than checking that a few expected ids are among the reported ones, so a rule that starts firing where it did not is caught too. A pattern PCRE accepts but the parser rejects stops the generator instead of being skipped by the test.
rector.php asked for PHPUnitSetList::PHPUNIT_110, which the installed rector-phpunit no longer defines: the tool stopped at configuration load, so every "task lint" run and every CI job failed before analysing a line — and had been failing since the package dropped its per-version sets. COMPOSER_BASED replaces it: it reads the installed PHPUnit and applies what suits it. The changes Rector had not been able to make in the meantime come with this — mostly assignments rewritten as "??=".
The projects moved: two patterns are gone, five are new, and a good number of the others changed line. Nothing the linter says about a pattern present in both runs changed — same warnings, same optimization suggestions — so this is upstream churn only. The lint expectations are regenerated with it, since they are read from the log.
The lexer rewrites what it reads: it strips the backslash off "\d", reads
"\P{Greek}" back as "{^Greek}", keeps only the name inside "\N{...}". The
value of a token is therefore not the text it was cut from, and everyone
who needed that text — the parser, for its error positions and for the
comments it rebuilds — was left to guess it from the value.
A token now carries the length of its span, and end() gives the offset just
past it. Nothing reads it yet; the next commits replace the guessing with it.
The invariant that makes it worth trusting is asserted: the tokens of a
pattern tile it exactly, checked over the 1496 patterns of the corpus.
Rebuilding the text of a comment from the tokens inside it meant a table of twenty-eight cases guessing how each token had been spelled: put the backslash back on this one, wrap that one in braces, leave this other alone. It could only ever approximate — a "\d" inside a comment is a backslash and a letter, not a character type, and the table had no way to tell. Tokens carry their span now, so the text is read from the pattern. The table goes, and with it the two tests that existed to walk every one of its arms; what they were standing in for — a comment keeping the text it was written with — is asserted directly.
Every atom computed where it ended by adding lengths together: the value, plus one for a backslash the lexer had eaten, plus one more for the letter after it. Unicode properties needed a correction on top of that — minus one here, plus one there, depending on whether the pattern had spelled the negation as "\P" or as "^" — written twice, once for the pattern and once for the inside of a character class. The token knows where it ends, so it is asked. Two state flags went with it. They existed to shift the position of a duplicate group name backwards by the length of whatever came before, and they were shifting it onto the wrong character: "/(?<name>a)|(?<name>b)/" blamed the "|", and "/(?J)(?<name>a)(?-J)(?<name>b)/" blamed a byte in the middle of "(?-J)". The caret now points at the group that repeats the name.
Six methods of the parser did nothing with the pattern around them: given
"\x41", "\u{1F600}", "\o{101}", "\101", "\N{LATIN SMALL LETTER A}" or the
letter after a "\c", they answered which character it names. One of them
reaches for intl, which is a surprising thing to find in a recursive-descent
parser.
They move to a reader of their own, with a name for the -1 they return when
a spelling cannot be read — the escape keeps its text in the tree and the
validator decides whether that is an error.
The three tests that reached the private methods through reflection are
replaced by tests of the reader itself, covering the forms they never did.
Two chains of a dozen "if the next token is this type, build that node" sat
side by side, one for the pattern and one for the inside of a character
class, agreeing on nine of the token types and building the same nodes from
them twice.
They share a table now, and each context adds what only it accepts: a dot, a
subroutine call or a "\K" outside, a nested class, a stray "-" or a POSIX
class inside. An anchor, an assertion and a backreference stay outside-only,
since a class turns those into something else and a class holding one has
been built by hand.
Sharing the table settles a disagreement the two chains had: "\N{U+0041}"
was read outside a class and refused inside one, though PCRE takes it in
both places.
The parser carried ten methods for moving through the tokens — check, match, advance, previous, consume and their literal-matching cousins — plus a three-field cache of the current token, guarding an array index behind a position comparison and an invalidation flag that four other places had to remember to clear. They belong to the stream. TokenStream is a cursor now, the cache is gone — the array index it was standing in for is the cache — and the parser reads the tokens through it. The reflection tests that reached those private methods become tests of the stream, which needs no reflection to be asked what it does at the end of the input.
PCRE spells the name of a group four ways — "(?<name>", "(?'name'", "(?P<name>", "(?P=name" — and Python patterns bring double quotes along. One method read all of them, another kept the names already used, and a flag on the parser said whether "J" currently allowed two groups to share one. The three move together: a reader holds the names it has seen and answers whether duplicates are allowed, which is the only reason it needed to know about "J" at all. The two tests that reached the private method through reflection become tests of the reader, and cover what they did not: a quote left open, a name starting with a digit, a reference that must not claim the name it reads.
Fifty-three test files were named for the metric they were written to move: CoverageTest, CompleteCoverageTest, Complete100PercentCoverageTest, FinalCoverageBoostTest. Nothing in a name said what the file held, and eight of them held a little of everything — two hundred and thirty-seven cases spread across the lexer, the parser and seven visitors, grouped by nothing. The cases are kept and regrouped by what they exercise: a sweep per subject under tests/Integration/Sweep. The rest are renamed for their subject, and the five small lexer files become one. One case is dropped rather than moved: it ran eighteen patterns through the validator inside a try/catch that ignored the result.
The Symfony bridge had a Severity that meant pass, warn, fail or critical — the outcome of a check — sitting beside the library's Severity, which grades a lint issue. And an AnalysisReport of sections and issues, beside the AnalysisReport that Regex::analyze() returns, with nothing in common. Reading either file meant checking which import was in scope. They are CheckOutcome and SecurityReport now, which is what they hold.
Two classes find regex patterns in PHP source — one reads the tokens, the other builds a syntax tree — and they sat apart in the lint namespace with the interface they share. They are together now, under Lint\Extraction. The second was called PhpStanExtractionStrategy though it has nothing to do with PHPStan: it uses nikic/php-parser, which PHPStan happens to bring along. It is PhpParserExtractionStrategy. The old names still resolve. The test overrides for the functions these classes call are mirrored into the new namespace, since a function override only reaches the namespace it is declared in.
Rector cannot resolve a PHPUnit assertion called statically through self:: and stops on the file. The three tests written that way call them on $this like every other test in the suite.
"(?im-sx)" says what it turns on and what it turns off, and "(?^im)" turns off everything it does not list. Three files knew that separately: the lexer, to decide where /x starts; the parser, to build the group and to refuse a modifier set and unset at once; and the compiler, to write the group back out. They share a small value object now. It answers the three questions each of them asked in its own way — what does this turn on, is this modifier in force inside the group, and what do the modifiers look like once applied.
"(?(VERSION>=10.4)yes|no)" branches on the version of PCRE reading the pattern. Working out what it asks is string work — the word, the comparison, the digits between the dots — and it was spread through fifty lines of stream walking, rewinding three separate times when the text turned out to say something else. The reading moves to a value object that takes the collected word. The parser keeps the walking and the one rewind it needs.
Most of these are backtracking verbs, but PCRE spells three other things the same way: the alphabetic form of a lookaround, a script run, and the match limit. Deciding which is which is string work on the text between the parentheses — a table of nine names, two prefixes, one number to read, and the shorthand that turns "(*:name)" into a mark. It moves to a reader. The parser keeps what needs it: parsing the payload of an assertion or a script run, which only it can do.
"(?P'name'...)" had its own reader: its own loop over the tokens, its own three error messages, and no check that the name reads like a name or that it is not already taken. "(?'name'...)", which PCRE treats identically, went through the shared reader all along. Both go through it now. A repeated name and a name starting with a digit are refused where they were accepted, which is what PCRE does with them.
Nine blocks read the body of a group, expected the closing parenthesis and built a node from it — the same three lines each time, differing only in which kind of group they named. A method for exactly that already existed and five of the nine used it. They all do now, and the two tables that say which character introduces which lookaround replace four more copies. Naming a group goes the same way: the four spellings PCRE accepts share one reader instead of three.
Six readers answered "not this kind of condition" with false, so the caller tested each of them with its own if and its own variable — thirty lines of scaffolding for a chain of six attempts. They answer with null, and the chain is a chain. The two guards that checked for a "D" or a "V" before calling the reader are gone with it: each reader already collects its word and rewinds when the word is not the one it wanted.
Reading a "(?...)" modifier group did three things at once: collect the letters, work out what they mean, and build the group. The collecting is its own step now — it has to start by picking up a "^" that reached the parser as an anchor token, which is worth a name and a sentence. The same for the quantifier that may follow a quoted run: it repeats the last quoted character rather than the run, which nine lines in the middle of the sequence loop expressed without ever saying so. And the range endpoints of a character class are checked by one guard instead of two copies of the same message.
The three tests written for the new readers used self:: for their assertions, which Rector cannot resolve; it stops on the file rather than analysing it.
Infection belongs on a developer's machine, where it can be pointed at the code being worked on. Running it on the lines a pull request touches makes every review wait on it, and a wide pull request gives it far too much to mutate. "composer infection" and "task infection" still run it locally, with the thresholds infection.json.dist declares.
yoeunes
force-pushed
the
consolidation
branch
from
September 2, 2026 18:05
0020e80 to
26f5452
Compare
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.
A consolidation pass over the whole library: correctness fixes first, then
the structural work the fixes made possible. 58 commits, each one green on
its own.
Fixed
so every file it wrote was a syntax error, swallowed on load and counted as
a hit.
(?(R)yes|no)recompiled to(?((?R))yes|no), which PCRE refuses.or an inline flag group came before it.
UTF-16 units, spun at full speed once the editor disconnected, and died on
anything a handler threw.
/a^b/matches the same strings as/ab/.quote.
\N{U+0041}was refused inside a character class, which PCRE accepts.was dropped upstream.
Performance
\w,\sor\dunder/uno longer walks 1.1M code points oneat a time (1.7s to 0.8s); case folding a wide class went from seconds to
milliseconds.
into every process that autoloaded the package, now none are.
Structure
Parser.phpfrom 2520 to 1883 lines. Position arithmetic replaced by thespan each token now carries; code point reading, group names, inline flags,
PCRE verbs and version conditions moved to readers of their own; the token
cursor moved onto
TokenStream.the transpiler targets share their common parts.
UnicodeNodeand the 25visitUnicodemethods itrequired,
ReDoSAnalyzerInterface.Tests and CI
never run there. Mutation testing runs on the lines a pull request touches.
parsed and written back out, generated samples are checked against their
pattern, token streams are spelled out.
refuses a new rule without one.
the linter's own output.
task lintgreen.Breaking changes are listed in UPGRADING.md. Linting the 12 929 corpus patterns
produces a byte-identical report.