Skip to content

Build/Test Tools: Apply the PHPStan node visitors to every file parsed - #13396

Closed
westonruter wants to merge 1 commit into
WordPress:trunkfrom
westonruter:phpstan-hash-notation-all-parses
Closed

Build/Test Tools: Apply the PHPStan node visitors to every file parsed#13396
westonruter wants to merge 1 commit into
WordPress:trunkfrom
westonruter:phpstan-hash-notation-all-parses

Conversation

@westonruter

@westonruter westonruter commented Sep 4, 2026

Copy link
Copy Markdown
Member

✅ Committed in r63462 (e0be8ea).


Follow-up to #13233 (r63420, f3cc113), which taught PHPStan to read hash notation.

HashNotationVisitor is tagged phpstan.parser.richParserNodeVisitor, and a visitor tagged that way runs in PHPStan's RichParser. PathRoutingParser sends a file to that parser only when it is one of the files being analyzed. A file merely read so that something it declares can be reflected goes to the simple parser instead, where the visitors never run.

PHPStan can route that way because its own two parsers agree about everything reflection exposes. The simple one is wrapped in a CleaningParser, whose CleaningVisitor empties function, method and closure bodies and touches nothing else, so docblocks and signatures come out of either parser identically. What is reflected out of a file does not depend on whether that file was analyzed — which is what makes it correct for PHPStan to key its per-file reflection cache on the file's contents alone.

HashNotationVisitor breaks that agreement. It rewrites @param, @return and @var docblocks, which are exactly what reflection exposes.

What that costs today

Analyzing a subset of the tree reports against types the visitor would have replaced, wherever one crosses a file boundary. WP_Widget_Media::get_l10n_defaults() assigns the return of _n_noop(), whose @return is hash notation, so it is such a boundary:

# phpstan analyse src/wp-includes/widgets/class-wp-widget-media.php
Static property WP_Widget_Media::$l10n_defaults (array<string>) does not accept
array<string, array|string>.

# phpstan analyse   (whole configured tree)
Static property WP_Widget_Media::$l10n_defaults (array<string>) does not accept
array<string, array<int|string, string|null>|string>.

The narrowed run is the wrong one. It is also the shape most tooling runs in: an editor analyzing the file being typed in, or a run scoped to a diff. Both under-report, against a type wider than the documentation describes, and can equally report errors a full run does not.

The reading is then stored in the shared cache, so the next full run restores it — which is how this surfaced. Core-65817 has a companion PR, #13395, isolating baseline generation from that. This addresses the cause rather than that one consequence, and the two are independent.

The change

Have the simple parser wrap the rich one. Every parse then rewrites the same docblocks, the cleaning still happens on top of it, and the reflection cache is keyed on contents correctly again.

This redefines currentPhpVersionSimpleParser in terms of currentPhpVersionRichParser, both PHPStan's own service names from its conf/parsers.neon, and neither a documented extension point. Should a release rename either, the definition stops being wired into anything and today's behavior returns silently. That tradeoff is recorded beside the definition together with what to check, and it is the reason the comment there is as long as it is. Worth a reviewer's opinion on whether it is acceptable — the alternative is leaving subset analysis wrong.

Results are unchanged

  • Analyzing the whole configured tree before and after reports the same 1544 errors, with no textual difference between the two outputs.
  • Every baseline still matches, with reportUnmatchedIgnoredErrors: true.
  • Regenerating every baseline leaves tests/phpstan/baselines byte-identical.
  • Builds and analyzes at level 10 with bleedingEdge, and completes at the 2G limit CI uses.

What it costs

Parsing that a subset run used to avoid is now work it does, so a cold cache pays for it. Three runs per cell, spread within each under 0.1s:

Target Cache Before After Delta
One file cold 3.39 / 3.42 / 3.50 3.90 / 3.93 / 3.96 +0.5s (+14%)
One file warm 2.23 / 2.23 / 2.25 2.11 / 2.12 / 2.24 none
src/wp-includes/widgets (14 files) cold 5.19 / 5.21 / 5.34 6.53 / 6.53 / 6.54 +1.3s (+24%)
src/wp-includes/widgets warm 1.50 / 1.51 / 1.60 1.53 / 1.53 / 1.53 none
Whole tree cold 29.0s 29.6s +0.6s (+2%)

A warm cache reads the same stored result either way, so it is unchanged — and that is the case an editor and a diff-scoped run are in after their first analysis. src/wp-includes/widgets is the worst ratio because it analyzes fourteen small files while reflecting most of wp-includes: the denominator is fixed and the numerator barely grows.

Memory moves from 174.5 MB to 176.5 MB. The cache on disk is the same size, 8.5 MB either way, because CleaningParser still wraps the result and the stored trees are stripped exactly as before.

CI analyzes the whole tree, so it pays the 2%, and only on a cold cache — which this change causes once anyway, its cache being keyed on the files in tests/phpstan.

Trac ticket: Core-65817

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Tracing the behavior through PHPStan's parser routing, and drafting the change, its comment and the benchmarks. Every claim above was measured rather than reasoned: the differing messages were reproduced from the two run shapes, the full-run outputs diffed, and each timing repeated three times. Reviewed by me.


🤖 Generated with Claude Code

https://claude.ai/code/session_01EdFn9fuiUhJMXzacuhmdGJ

A visitor tagged `phpstan.parser.richParserNodeVisitor` runs in PHPStan's
RichParser, and PathRoutingParser sends a file there only when it is one of the
files being analyzed. A file merely read so that something it declares can be
reflected goes to the simple parser instead, where the visitors never run.

PHPStan can route that way because its own two parsers agree about everything
reflection exposes. The simple one is wrapped in a CleaningParser, whose
CleaningVisitor empties function, method and closure bodies and touches nothing
else, so docblocks and signatures come out of either parser identically. What is
reflected out of a file does not depend on whether that file was analyzed, which
is what makes it correct to key the per-file reflection cache on the file's
contents alone.

HashNotationVisitor breaks that agreement. It rewrites `@param`, `@return` and
`@var` docblocks, which are exactly what reflection exposes. A file read through
the simple parser therefore reflects the documented `array` where the same file
analyzed reflects the shape its hash notation describes.

So analyzing a subset of the tree — an editor analyzing the file being typed in,
or a run scoped to a diff — reports against types the visitor would have replaced
wherever one crosses a file boundary, and writes that reading into the cache for
the next full run to restore. `WP_Widget_Media::$l10n_defaults`, assigned the
return of `_n_noop()`, is one such boundary: analyzed alone, its message named a
bare `array` rather than the shape.

Have the simple parser wrap the rich one. Every parse then rewrites the same
docblocks, the cleaning still happens on top of it, and the reflection cache is
keyed on contents correctly again.

Full runs report exactly what they did before: the same 1544 errors with no
textual difference between them, every baseline still matching with
`reportUnmatchedIgnoredErrors` on, and regenerating all of them leaves the files
byte-identical.

The parsing a subset avoided is now work it does, so a run over a cold cache pays
for it: 3.4s to 3.9s for a single file, 5.2s to 6.5s for a directory of fourteen,
29.0s to 29.6s for the whole tree. A warm cache reads the same stored result
either way and is unchanged, which is the case an editor and a diff-scoped run are
in after their first analysis. Memory moves 174.5 MB to 176.5 MB, and the cache on
disk is the same size, the cleaning being untouched.

Follow-up to r63420, which introduced the visitor.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@swissspidy swissspidy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds plausible 👍

wporg-sync pushed a commit that referenced this pull request Sep 4, 2026
A visitor tagged `phpstan.parser.richParserNodeVisitor` runs in PHPStan's `RichParser`, and `PathRoutingParser` sends a file there only when it is one of the files being analyzed. A file merely read so that something it declares can be reflected goes to the simple parser instead, where the visitors never run. That routing is safe for PHPStan's own parsers, which agree about everything reflection exposes, but the `HashNotationVisitor` added in r63420 rewrites `@param`, `@return` and `@var` docblocks, which are exactly what reflection exposes. Analyzing a subset of the tree therefore reports against types the visitor would have replaced wherever one crosses a file boundary, and writes that reading into the shared cache for the next full run to restore.

Have the simple parser wrap the rich one, so every parse rewrites the same docblocks, the cleaning still happens on top of it, and the reflection cache is keyed on contents correctly again. This redefines PHPStan's own `currentPhpVersionSimpleParser` service, which is not a documented extension point; the definition records what to check should a future release rename it. Full runs report exactly what they did before, and every baseline still matches. A run over a subset now does the parsing it previously skipped, so a cold cache pays a few percent for it, while a warm one is unchanged.

Developed in #13396.
Follow-up to r63420, r63460.

Props westonruter, swissspidy.
See #65817.


git-svn-id: https://develop.svn.wordpress.org/trunk@63462 602fd350-edb4-49c9-b593-d223f7449a82
@westonruter westonruter closed this Sep 4, 2026
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 4, 2026
A visitor tagged `phpstan.parser.richParserNodeVisitor` runs in PHPStan's `RichParser`, and `PathRoutingParser` sends a file there only when it is one of the files being analyzed. A file merely read so that something it declares can be reflected goes to the simple parser instead, where the visitors never run. That routing is safe for PHPStan's own parsers, which agree about everything reflection exposes, but the `HashNotationVisitor` added in r63420 rewrites `@param`, `@return` and `@var` docblocks, which are exactly what reflection exposes. Analyzing a subset of the tree therefore reports against types the visitor would have replaced wherever one crosses a file boundary, and writes that reading into the shared cache for the next full run to restore.

Have the simple parser wrap the rich one, so every parse rewrites the same docblocks, the cleaning still happens on top of it, and the reflection cache is keyed on contents correctly again. This redefines PHPStan's own `currentPhpVersionSimpleParser` service, which is not a documented extension point; the definition records what to check should a future release rename it. Full runs report exactly what they did before, and every baseline still matches. A run over a subset now does the parsing it previously skipped, so a cold cache pays a few percent for it, while a warm one is unchanged.

Developed in WordPress/wordpress-develop#13396.
Follow-up to r63420, r63460.

Props westonruter, swissspidy.
See #65817.

Built from https://develop.svn.wordpress.org/trunk@63462


git-svn-id: http://core.svn.wordpress.org/trunk@62642 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants