Compute the AST cache version instead of remembering to raise it - #155
Merged
Merged
Conversation
The cache version guarded the shape of the nodes: bump it when one gains or loses a property. That is half the story. A cached tree is only worth restoring while the current code would build the same one, and the last two dozen commits changed what the lexer and the parser build without touching a single node property — a byte-mode quoted run, a version condition, a group spelling. Anyone following main with the default cache kept being answered by the old behaviour, and the guard that exists for exactly this never fired. The test now fingerprints the code that decides what a pattern parses into — the lexer, the parser, the nodes and the readers they use — with comments and formatting stripped, so rewording a docblock costs nobody their cache. It fails until the version is bumped and the new fingerprint recorded. The version moves to 1.5.0 for the changes already made, and CONTRIBUTING says when to move it again.
"(*:label)" is "(*MARK:label)" written short, and the tree keeps only the long form, so the short one was rewritten on the way out. The same treatment as the group names and the script runs: the source says which was written. Nothing about the tree changes, only how it is rendered, so a cached tree stays good and the cache version stays put.
The validator knew four kinds of condition and treated everything else as malformed, so "(?(VERSION>=10.4)y|n)" — a pattern PCRE compiles — was reported as an invalid conditional construct. It knows the version condition now, and asks the one question worth asking about it: PCRE compares with "=" and ">=" and nothing else, so the four other comparisons the parser reads are reported for what they are, with an error code of their own. That is the division of labour the two already had: the parser reads what it can so that a pattern still produces a tree to look at, and the validator says what will not compile.
A version somebody has to bump by hand is a version somebody forgets, and
the last two dozen commits proved it: the constant sat at 1.4.0 while the
parser changed under it, and anyone following main kept being answered from
a cache holding trees the code no longer builds.
So it is no longer a number. Regex::CACHE_VERSION is now the fingerprint of
the code that decides what a pattern parses into — the lexer, the parser, the
nodes, the readers they use — and a command writes it:
task cache-version # or composer cache-version
"task lint" runs it, so a normal round of work updates the constant and the
change shows up in git diff like any other. The test suite fails while the
constant and the code disagree, and the tool takes --check for a report
without a write.
The fingerprint covers the code alone, comments and formatting stripped: a
reworded docblock costs nobody their cache. And it stops at what builds the
tree — changing how a tree is rendered leaves every cached tree valid.
PHPStan reported "Variable $argv might not be defined" on PHP 8.5 and 8.6 and nowhere else, so the pipeline was green here and red there. $argv exists only when register_argc_argv is on, which nothing in the tool can promise; $_SERVER['argv'] with a fallback says as much. The static analysis configuration now names the range of PHP versions the package supports rather than analysing whatever version happens to be installed, so a version-specific report has a chance of appearing locally. It does not reproduce this particular one — the difference between the CI versions lies elsewhere — but analysing one version out of five was never going to be enough.
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.
The parsed AST is cached, keyed on
Regex::CACHE_VERSION. A cached tree isonly worth restoring while the current code would build the same one — and the
constant sat at
1.4.0while two dozen commits changed what the lexer and theparser build. Anyone following
mainwith the default cache kept beinganswered with trees this code no longer produces, which is exactly how a
reviewer's re-test of the previous pull request came back "still broken" when
it was not.
The version is computed now
It is no longer a number somebody remembers to raise: it is a fingerprint of
the code that decides what a pattern parses into — the lexer, the parser, the
nodes and the readers they use.
task cache-version # or: composer cache-versiontask lintruns it, so a normal round of work updates the constant and thechange shows up in
git difflike any other. The test suite fails while theconstant and the code disagree, and the tool takes
--checkfor a reportwithout a write.
Two properties worth knowing:
rewording a docblock costs nobody their cache.
commit below that keeps the spelling of a mark, for instance — leaves every
cached tree valid, and the fingerprint does not move.
Also fixed
(?(VERSION>=10.4)y|n)included,though PCRE compiles it: the validator knew four kinds of condition and read
anything else as malformed. It knows this one now, and reports the four
comparisons PCRE does not make — it compares with
=and>=— under anerror code of their own rather than as an unrecognised condition.
(*:label)came back as(*MARK:label). The short spelling of a mark iskept, as the group and script run spellings already are.
6680 tests,
task lintgreen, and the report for the 12 929 corpus patterns isbyte-identical.