Detect analyser fixtures missing a sufficient // lint >= comment for the version-specific PHP syntax they use

[phpstan-bot]

Summary

New test fixtures regularly slip into the repository using PHP syntax that is newer than the minimum PHP version they declare (or without declaring one at all). Because local development happens on modern PHP, these only blow up later in CI on PHP 7.4/8.0 as hard parse errors (see e.g. #5850).

tests/PHPStan/Analyser/nsrt fixtures are excluded from parallel-lint (Makefile) and instead rely on a first-line <?php // lint >= X.Y comment to be skipped on older PHP versions. Nothing verified that this comment was actually present and high enough for the syntax used. This change adds that verification and brings the existing fixtures into compliance.

As suggested in the issue, the detection runs as a TypeInferenceTestCase-style test rather than a PHPStan rule, since fixture files are excluded from PHPStan's own analysis.

Changes

• Add build/PHPStan/Build/RequiredPhpVersionVisitor.php — a php-parser visitor that walks a fixture's AST and reports the minimum PHP version required to parse it, together with a human-readable reason. Detected features:
  • enums, readonly properties, readonly promoted properties, pure intersection types, first-class callable syntax → 8.1
  • readonly classes, standalone null/false/true types, disjunctive normal form types → 8.2
  • typed class constants, dynamic class constant fetch → 8.3
  • property hooks, asymmetric visibility → 8.4
  • the pipe operator → 8.5
• Add tests/PHPStan/Build/RequiredPhpVersionCommentTest.php:
  • testFixtureHasRequiredLintComment parses every nsrt fixture and asserts the version guaranteed by its // lint comment (only lower-bound >=/>/== constraints guarantee newer syntax) covers the version the syntax requires, with an actionable failure message.
  • testDetectedVersion unit-tests the visitor against a snippet per feature.
• Bring 20 existing nsrt fixtures into compliance by adding/bumping their // lint >= comment: array-key-exists-on-subtracted, array-map, array_values, bug-10037, bug-12691, bug-13061, bug-13828, bug-13872, bug-14772, bug-14774, bug-4890-php8, bug-6904, bug-7698, bug-7944, bug-nullsafe-prop-static-access, class-constant-narrowing, discussion-13395, ds-copy, override-property-union-type-with-tag-subtype, static-late-binding.

Root cause

The nsrt fixtures are excluded from parallel-lint and depend solely on the // lint comment to be skipped on PHP versions that cannot parse them. The convention was applied by hand and unenforced, so a fixture could use e.g. an enum (PHP 8.1) while declaring // lint >= 8.0 or no comment at all. On a modern host that parses and analyses fine, hiding the latent CI parse error. The new test enforces the convention mechanically across the whole directory.

Test

• RequiredPhpVersionCommentTest::testFixtureHasRequiredLintComment reproduced the bug: before the fixture fixes it reported 20 failures (e.g. "bug-14774.php uses enums which requires PHP 8.1. Add a <?php // lint >= 8.1 comment …"). After bumping the comments it is green.
• RequiredPhpVersionCommentTest::testDetectedVersion covers each detected feature (enum, readonly property/promoted/class, intersection/DNF types, standalone null/false/true types, typed class constant, dynamic class constant fetch, first-class callable, property hook, asymmetric visibility, pipe operator) plus a plain-code negative case.
• make phpstan passes for the new sources; the affected NodeScopeResolverTest and rule tests (CallMethodsRuleTest, MatchExpressionRuleTest) referencing the touched fixtures stay green.

Fixes phpstan/phpstan#14816

[ondrejmirtes]

Oh, this is so nice.

[staabm]

I think we can extend this directory list and add all data named folders below tests/PHPStan (no matter in which level of nesting)

[staabm]

I stand corrected. only below tests/PHPStan/Rules

[phpstan-bot]

Done. Here's a summary of how I addressed staabm's review.

What the review asked

extend this directory list to add all data named folders below tests/PHPStan (no matter in which level of nesting)

What I did

1. Extended the fixture coverage (RequiredPhpVersionCommentTest.php)

dataFixtures() now scans the nsrt directory plus every data directory anywhere below tests/PHPStan (54 data dirs, ~4900 fixtures total). While wiring this up I had to account for how data fixtures differ from nsrt:

• Comment placement — nsrt fixtures are runtime-skipped by TypeInferenceTestCase::isFileLintSkipped, which requires the // lint comment immediately after <?php. data fixtures aren't skipped that way (they're excluded from parallel-lint as whole dirs/files and gated by markTestSkipped in the rule tests), and many keep their comment after declare(...) on line 1. The detection now matches the comment anywhere on the first line, so I could bring fixtures into compliance by appending to line 1 — no line numbers shift, keeping the rule tests' line-based expectations valid.
• Parse-error fixtures — some data fixtures intentionally contain syntax errors (e.g. parse-error.php, abstract-final-property-hook-parse-error.php); these are now skipped instead of failing.
• Printer round-trip snapshots — cleaning-property-hooks-before/after.php are asserted verbatim as CleaningParserTest's pretty-printer output, so a first-line comment can't be expressed in them. They're parsed by nikic/php-parser regardless of host PHP and excluded from parallel-lint, so they're explicitly excluded via a documented constant.

2. Brought 57 data fixtures into compliance — added/bumped the // lint >= X.Y comment (appended to line 1) on every data fixture that used version-specific syntax without a sufficient declared minimum (enums/readonly/first-class-callable → 8.1, readonly classes/standalone types → 8.2, typed/dynamic class constants → 8.3, property hooks/asymmetric visibility → 8.4, #[NoDiscard]/pipe → 8.5).

Verification

• The extended test reported 59 failures before the fixture fixes (reproducing the gap the reviewer pointed at), green after.
• make phpstan — No errors
• make tests — 17561 tests OK (96 standard version-gated skips)
• make cs — clean

Two commits pushed (test change, then fixture compliance).

[phpstan-bot]

Both green. Here's a summary of what I did.

Summary

The review asked to scope the fixture scan in RequiredPhpVersionCommentTest to only data directories below tests/PHPStan/Rules.

Context I uncovered

When I started, the remote branch had already moved past the original PR diff: two commits (214a6eedf, 443257a18) had implemented the reviewer's first suggestion — scanning every data directory below tests/PHPStan. The current review ("I stand corrected. only below tests/PHPStan/Rules") is the correction to that, so the task was to narrow the scan.

My local exploratory commit conflicted with that newer remote work, so I reset to the remote HEAD and applied the narrowing on top of the existing infrastructure.

Change made (commit 095a8811d)

In tests/PHPStan/Build/RequiredPhpVersionCommentTest.php:

• Changed the data-directory finder from ->in(__DIR__ . '/..') (all of tests/PHPStan) to ->in(__DIR__ . '/../Rules').
• Removed the now-unneeded EXCLUDED_FIXTURES allowlist and its in_array filter — the only entries (cleaning-property-hooks-*) live under Parser/data, which is no longer scanned.
• Updated the docblock to say "every data directory below tests/PHPStan/Rules".

The lenient first-line // lint matching the remote introduced stays — I confirmed why it's correct: SkipLintProcess runs the skip script via php -r after str_replace('<?php', '', $script), which strips <?php from inside the matching regex too, so parallel-lint honours a // lint comment anywhere on the first line (e.g. after declare(...)), while TypeInferenceTestCase keeps the strict <?php-anchored placement for nsrt.

Verification

• RequiredPhpVersionCommentTest green (4376 assertions).
• Confirmed it still catches a real gap by temporarily dropping an un-commented enum fixture into Rules/Comparison/data (failed with the actionable message), then removed it.
• make tests green (17045 tests, 96 pre-existing skips).
• make phpstan green.

Pushed to create-pull-request/patch-ml2b3e8.

One note for the reviewer: the earlier remote commit 443257a18 added // lint comments to some non-Rules data fixtures (e.g. under Parser/data) that are no longer scanned after this narrowing. I left them in place since they're harmless and valid — let me know if you'd prefer those reverted.

[VincentLanglet]

It's unclear to me why // lint >= was added on so much file given the fact test were already working (?)

[staabm]

most files changed (or whole folders) were on the --exclude list in the Makefile.

consider the changed files a "baseline cleanup", so for the future the newly added test only needs to deal with newly added files (and we don't need to list all the excludes additionally somewhere else).

the PR is more about what this enables in the future. see motivation described in phpstan/phpstan#14816

[VincentLanglet]

most files changed (or whole folders) were on the --exclude list in the Makefile.

Should we remove some ?

[staabm]

Should we remove some ?

we can't remove them because the files contain e.g. syntax/linting errors, see e.g. #5856

[VincentLanglet]

Should we also check for mixed param in this PR ?
See #5854 (comment) for example

[staabm]

add support for a mixed native type which requires PHP 8.0+

[phpstan-bot]

All done. Here's a summary of how I addressed staabm's review.

Review

add support for a mixed native type which requires PHP 8.0+

What I implemented

1. Detection in the visitor (RequiredPhpVersionVisitor.php)
Added a checkMixedType() check that flags a native mixed type used as a parameter, return, or property type, requiring PHP 8.0. While wiring it in I extracted the shared type-position logic (param/property/const type, function/method/closure/arrow return type) into a getDeclaredType() helper now reused by both the standalone-type and mixed-type checks.

2. Unit tests (RequiredPhpVersionCommentTest::dataDetectedVersion)
Added cases for mixed as a param, return, and property type (each → 80000).

Key finding that justifies the check

mixed is not a hard parse error on PHP < 8.0 — there php-parser yields a Node\Name (class type), only becoming the builtin Node\Identifier mixed type on 8.0+. So PHPStan configured to 7.4 analyses mixed as a class, making assertType expectations diverge. Gating these fixtures to >= 8.0 is exactly what's needed.

Handling the upper-bound edge case

Two fixtures (mb-convert-encoding-php7.php → <= 8.0, array-column.php → < 8.2) deliberately target old PHP to assert version-specific function signatures and would break their assertions if moved to >= 8.0. Because mixed parses fine on old PHP (unlike enums/hooks, which are real parse errors), I exempted files carrying an upper-bound // lint constraint from the mixed requirement only — parse-breaking features are still rejected.

Fixture compliance

Brought 95 fixtures into compliance by adding // lint >= 8.0. For the 6 nsrt fixtures with declare() on line 1, I moved the comment to immediately follow <?php (required by TypeInferenceTestCase::isFileLintSkipped) with declare() on its own line, matching the PR's established pattern; data fixtures keep the comment appended to line 1.

Verification

• RequiredPhpVersionCommentTest — green (4700 tests). It reported 97 failures before the fixture fixes, green after.
• make phpstan — No errors
• make tests — 17369 tests OK (96 standard version-gated skips)
• make cs — clean

Two commits pushed to create-pull-request/patch-ml2b3e8 (detection + tests, then fixture compliance).

[staabm]

track and report the line of the node which is the reason for the minimum required php version

[phpstan-bot]

All green (17370 tests, 96 standard version-gated skips).

Summary

I addressed staabm's review: "track and report the line of the node which is the reason for the minimum required php version."

Changes (commit 00faf28e9)

build/PHPStan/Build/RequiredPhpVersionVisitor.php

• Added a ?int $reasonLine field and a getReasonLine() getter alongside the existing getRequiredVersionId() / getReason().
• Changed the private require() method to take the triggering Node and record $node->getStartLine() whenever it adopts a higher requirement — so the reported line always matches the version/reason that won.
• Threaded the most specific node into each require() call: the inner UnionType member for DNF / standalone-true, the VariadicPlaceholder for first-class callables, the type node for standalone/mixed types, and the node itself everywhere else.

tests/PHPStan/Build/RequiredPhpVersionCommentTest.php

• The failure message now reads "Fixture uses <reason> on line <N> which requires PHP X.Y…", pulling the line from getReasonLine().
• Retrieved $reasonLine just before the final assertion (after the expectNotToPerformAssertions() exemption branches) to avoid a risky-test conflict, with assertNotNull to satisfy phpstan's nullable return.
• Added testReasonLine() unit test asserting an enum on line 4 of a multi-line snippet reports version 80100, reason enums, line 4.

Verification

• I confirmed the message renders correctly by dropping a temporary un-commented enum fixture into Rules/Comparison/data — it failed with "Fixture uses enums on line 5 which requires PHP 8.1…" — then removed it.
• RequiredPhpVersionCommentTest — green, no risky tests.
• make phpstan — No errors.
• make cs — clean.
• make tests — 17370 tests OK (96 standard skips).

Pushed to create-pull-request/patch-ml2b3e8.