Fix: unused import, duplicate issue shortcode, Windows CI opcache/bypass-finals conflict#11901
Closed
alies-dev wants to merge 5 commits into
Closed
Fix: unused import, duplicate issue shortcode, Windows CI opcache/bypass-finals conflict#11901alies-dev wants to merge 5 commits into
alies-dev wants to merge 5 commits into
Conversation
Merge commit 290eaec (6.x into master) resolved a conflict in StatementsAnalyzer's property/constructor block by taking 6.x's version wholesale, while the separate analyze() method kept master's version wholesale. The two halves no longer matched: - private int $depth = 0 was dropped, but $this->depth++/-- in analyze() remained, causing an uncaught RuntimeException on PHP 8.2+ (Psalm's ErrorHandler converts the dynamic-property deprecation into a crash). - The $root_scope constructor-promoted property was dropped too, so $this->root_scope always read as an undefined property. - The constructor logic that populates $this->taint_flow_graph and $this->variable_use_graph from the shared codebase graph was replaced by 6.x's simpler version, which never assigns either property. This silently disabled unused-variable detection and taint tracking wherever the crash above didn't already stop execution (e.g. under PHPUnit, which doesn't install that ErrorHandler). Restores all three to match the pre-merge master constructor (commit 5c1c31c) exactly, alongside the type_variable_tracker logic 6.x legitimately added. Full test suite goes from 374 failing/erroring tests to 72 (all pre-existing/unrelated), and self-analysis from 6139 issues to 12 (all pre-existing/unrelated). Fixes vimeo#11899
Not referenced anywhere in the file; caught by phpcs.
IncompatibleTypeParameters (added in vimeo#11875) reused shortcode 362, already owned by ImpureGlobalVariable. DocumentationTest:: testShortcodesAreUnique caught the collision; reassigns the newer issue to 368, the next free shortcode.
dg/bypass-finals strips `final` from source via a require-time stream wrapper, which is well known to be incompatible with OPcache: cached bytecode bypasses the wrapper, so classes already in the opcache serve their un-patched (still-final) source. This uniquely broke Windows CI (only workflow enabling opcache.enable_cli; Linux CI leaves it off) with intermittent "TestConfig cannot extend final class Config" fatals in 2 of 8 chunks, since which classes hit a cache populated before BypassFinals wraps them is a race. Linux CI already runs without opcache.enable_cli; this aligns Windows with it.
The type-variable-support feature (vimeo#11875) landed without @psalm-mutation-free/@psalm-pure/@psalm-external-mutation-free annotations on several pure methods, and psalm's own self-analysis (Shepherd CI) has been failing on these 12 findings since. Added via `psalm --alter --issues=MissingPureAnnotation,MissingImmutableAnnotation`, reviewed individually. TypeVariableBounds is deliberately mutable (TypeVariableTracker appends to its bounds arrays in place so that TTypeVariable instances already holding a reference see later constraints), so it can't actually be marked @psalm-immutable as psalm suggested; suppressed that one specific finding with a reason instead.
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.
Context
Stacked on #11900 (branched from
fix/11899-statementsanalyzer-depth; this diff will shrink to just this branch's own commits once #11900 merges). While investigating why CI was still red on #11900 after fixing theStatementsAnalyzerregression, I found several unrelated pre-existing issues that #11900's fix let the test suite and self-analysis reach for the first time (previously everything crashed before getting this far):Code Style Analysis: an unusedTClosureimport inFilterUtils.php.DocumentationTest::testShortcodesAreUnique:IncompatibleTypeParameters(added in Support better template inference with type variables #11875) reused shortcode 362, already owned byImpureGlobalVariable.Class Psalm\Tests\TestConfig cannot extend final class Psalm\Config.dg/bypass-finalsstripsfinalfrom source at require-time via a stream wrapper, a known incompatibility with OPcache (cached bytecode bypasses the wrapper).windows-ci.ymlis the only workflow that setsopcache.enable_cli=1; Linux'sci.ymldoesn't. Confirmed via the Windows job log:Zend OPcache v8.5.7is active there. Could not reproduce locally (different OS), so this fix is based on the config asymmetry and the well-documented bypass-finals/OPcache incompatibility rather than a local repro.Run Shepherd(self-analysis): 12 pre-existingMissingPureAnnotation/MissingImmutableAnnotationfindings, all in the type-variable-support feature (Support better template inference with type variables #11875), which never got these annotations added.Solution
Independent, minimal fixes, one per commit:
IncompatibleTypeParametersto shortcode 368 (next free one).opcache.enable_cli=1from the Windows CI ini-values, matching Linux.psalm --alter, reviewed individually. One of the twelve (TypeVariableBounds) is genuinely mutable by design (TypeVariableTrackerappends to its bounds arrays in place soTTypeVariableinstances already holding a reference see later constraints), so that one is@psalm-suppressed with a reason rather than incorrectly marked@psalm-immutable.Full suite locally: 72 failing tests → 71 (the shortcode test now passes; the remaining 71 are the pre-existing PHP-8.5-host
InternalCallMapHandlerTestbaseline (70) plus one unrelatedReportOutputTestSARIF-metadata drift, both out of scope here). Self-analysis: 12 issues → 0.Known remaining CI failure on this PR:
Check backward compatibilitywill still flag the shortcode reassignment as a breaking constant-value change.roave-backward-compatibility-checkcompares straight againstorigin/masterand has no baseline/ignore mechanism, so it can't distinguish "this class was merged three commits ago and never released" from "long-standing public API" — renumbering either colliding class trips it the same way. SinceIncompatibleTypeParametershas never shipped in a tagged release, I don't think this is a real-world break, but flagging it explicitly rather than silently reverting the correct fix (which would just re-breaktestShortcodesAreUniqueinstead). Happy to revert the shortcode commit if maintainers would rather see BCC green here.Run Psalm (mac OS)is unrelated to any of this:setup-phpfails to install PHP 8.5 on the macOS runner itself (php: command not found), reproducing identically onmasterHEAD regardless of PR content.