Resolve comment token ids at runtime in the native parser - #6180
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ondrejmirtes
changed the title
| # the mounted workspace): the docker-library images carry neither a | ||
| # composer nor an unzip. | ||
| - name: "Install PHP" | ||
| uses: "shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240" # v2.37.2 |
| coverage: "none" | ||
| php-version: "8.3" | ||
|
|
||
| - uses: "ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda" # v4.0.0 |
| run: echo "base_sha=${{ github.event.pull_request.base.sha }}" >> "$GITHUB_OUTPUT" | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| run: echo "base_sha=$BASE_SHA" >> "$GITHUB_OUTPUT" |
| run: | | ||
| echo "log<<MESSAGE" >> "$GITHUB_OUTPUT" | ||
| git log ${{ steps.previous-commit.outputs.sha }}..${{ github.event.after }} --reverse --pretty='https://github.com/phpstan/phpstan-src/commit/%H %s' >> "$GITHUB_OUTPUT" | ||
| git log "$PREVIOUS_SHA".."$AFTER_SHA" --reverse --pretty='https://github.com/phpstan/phpstan-src/commit/%H %s' >> "$GITHUB_OUTPUT" |
The userland T_* token ids are assigned by the bison that generated the interpreter's zend_language_parser.c, and the official php.net tarballs ship the numbering of whichever bison the release manager ran: the 8.3.21 tarball carries the Bison 3.8.2 numbering (T_COMMENT=387), the 8.3.22 one the Bison 3.0.4 numbering (T_COMMENT=392). The docker-library php images build those tarballs without regenerating the parser and inherit the split, while the prebuilt .so artifacts are compiled against distro or setup-php builds that regenerate with a modern bison. A T_* id baked into the extension at compile time is therefore silently wrong on the other numbering's builds: the parse stays valid, but the comment annotation pass in ParserRunner.cpp matches nothing, so every comment — and with it every PHPDoc — vanishes. The new turbo-token-numbering job loads each freshly built Linux binary into docker-library images of both numberings and requires byte-identical native parses on each, via the new token-id-probe.php differential and the existing parser corpus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW4JjcWAHx3KHf8hhXwfFB
The userland T_* ids are assigned by the bison that generated the interpreter's zend_language_parser.c, and the official php.net tarballs ship the numbering of whichever bison the release manager ran — the 8.3.21 tarball carries the Bison 3.8.2 numbering (T_COMMENT=387), the 8.3.22 one the Bison 3.0.4 numbering (T_COMMENT=392). The docker-library php images build those tarballs without regenerating the parser, so the T_COMMENT/T_DOC_COMMENT/T_WHITESPACE macros baked into the prebuilt binary matched nothing on such builds and the comment-annotation pass silently no-oped: the parse stayed valid, but every comment — and with it every PHPDoc — vanished. Route the five compile-time uses in ParserRunner.cpp through runtime resolution instead, shared with the sites that already did it right (getCommentBeforeToken, handleHaltCompiler): tokenId*() accessors declared in ParserEngine.h over the cached tokenConstant() resolver in ParserRunnerHelpers.cpp. zend_language_parser.h is no longer included anywhere, so any future compile-time T_* reference fails to compile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW4JjcWAHx3KHf8hhXwfFB
ignore-cache on composer-install (no cache restore in a workflow that publishes runtime artifacts), explicit read-only permissions, and env indirection for the matrix image names in run scripts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW4JjcWAHx3KHf8hhXwfFB
Composer installs and setup-node no longer use caches anywhere in the workflow — it publishes runtime artifacts (the phar and the turbo binaries), so a cache poisoned from an unprivileged context must not be able to reach them. Read-only workflow-level GITHUB_TOKEN with per-job elevation where needed (paths-filter, cross-run artifact download), persist-credentials: false on all checkouts except the phpstan-dist one whose persisted bot token the publish push deliberately uses, and env indirection for every template expansion inside run scripts. Remaining accepted findings, both documented inline: the deliberate credential persistence above, and the Windows build's cmd shell that php-sdk's tooling requires. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW4JjcWAHx3KHf8hhXwfFB
Contributor
|
Should/Can we have a test which starts failling as soon php-src changes token ids again? |
ondrejmirtes
force-pushed
the
turbo-token-numbering-ci
branch
from
August 4, 2026 21:38
b901b3b to
d6ba8dc
Compare
Member
Author
|
@staabm I understand that |
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
3 participants
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.
Fixes the turbo comment-loss bug and adds the CI reproduction that caught it.
Closes phpstan/phpstan#15037
Closes phpstan/phpstan#15043
Root cause
The userland
T_*token ids are assigned by the bison that generated the interpreter'szend_language_parser.c, and the official php.net tarballs ship the numbering of whichever bison the release manager ran:It is not a uniform shift — the two bisons order the whole token enum differently (all 146
T_*constants differ). Regenerating the 8.3.22 grammar with Bison 3.8.2 yields exactly the 387 scheme, so the numbering is purely a function of the bison version. That explains the non-monotonic affected-version pattern in the issue (8.3.4/7/9/15/17/20/22 are the Bison-3.0.4-generated releases) and rules out libc/arch.The docker-library
php:images build the tarballs without regenerating the parser and inherit the split. The prebuilt.soartifacts are compiled against distro/setup-php builds that do regenerate with a modern bison. On a 392-numbered build the compile-timeT_COMMENT/T_DOC_COMMENT/T_WHITESPACEcomparisons inParserRunner.cppmatched nothing, soannotateComments()no-oped: the parse stayed valid, but every comment — and with it every PHPDoc — vanished. (The baked 387/388 alias toT_ATTRIBUTE/T_INCthere, but mis-attachment is unreachable: the walk-back scan breaks on real whitespace, whose id is also shifted — comments are only ever lost, matching all field reports.)Fix
Route the five compile-time uses in
ParserRunner.cpp(makeComment,commentEnterNode,annotateComments) through runtime resolution, shared with the sites that already did it right (getCommentBeforeToken,handleHaltCompiler):tokenId*()accessors declared inParserEngine.hover the cachedtokenConstant()resolver.zend_language_parser.his no longer included anywhere in the extension, so any future compile-timeT_*reference fails to compile — it was the only generated-per-build header turbo included.A sweep for the same class of bug found nothing else: the remaining
PHP_VERSION_IDgates are minor-boundary API selection (binaries are built per minor), the drop-token/symbol tables were already built from runtime data (which is why the AST itself was always correct), and there are no other baked ids.CI reproduction
The first commit adds
turbo-token-numbering: six legs (gnu/musl × x86_64/arm64 + the gnu ZTS pair) load each freshly built binary into docker-library images of both numberings (php:8.3.21-*andphp:8.3.22-*) and require byte-identical native parses via the newtoken-id-probe.phpand the existing parser corpus. Before the fix commit, all six legs failed on the Bison 3.0.4 probe (run); with the fix they pass.Verification
php:8.3.22-cli-bookworm(the previously broken environment).sobuilt against 387-numbered PHPs, run on both numberings, native + Docker gnu/musl): 6/6 byte-identicalPHPSTAN_TURBO=0: byte-identical🤖 Generated with Claude Code
https://claude.ai/code/session_01SW4JjcWAHx3KHf8hhXwfFB