refactor(opcache): replace path-scoped PHPStan ignores with struct shapes - #271
Merged
Conversation
…apes
The opcache pointer-surgery files were held at level max only through
path-scoped ignore blocks in phpstan.dist.neon (property.nonObject,
binaryOp.invalid, cast.int, argument.type, assignOp.invalid) because their
multi-hop FFI\CData reads resolve to mixed after the first hop. Narrow those
reads to the generated ZEngine\Generated\* engine-struct stubs the rest of the
codebase already uses, surfaced through the docblock boundary-narrowing
convention (native `object`, `@param`/`@var`/`@return` stub type), so the
walkers type-check without any behaviour change.
Removed the path-scoped ignore blocks for all four files:
- src/OpCache/PayloadRelocator.php (property.nonObject, binaryOp.invalid,
cast.int, argument.type)
- src/OpCache/ScriptSerializer.php (property.nonObject, binaryOp.invalid,
cast.int, argument.type)
- src/OpCache/ReflectionOpcacheFile.php (property.nonObject, argument.type,
binaryOp.invalid, cast.int, assignOp.invalid)
- tests/OpCache/BoundsValidationTest.php (argument.type)
How the errors were retired:
- pointerAtAddress('T *', ...) string casts -> the stub class-string form
(pointerAtAddress(T::class, ...)), which the TypedEntryPointReturnExtension
types as the stub while the runtime value stays FFI\CData;
- generic walker params typed to the owning stub via @param/@var docblocks
(native type stays `object` - the stubs are analysis-only and would raise a
runtime TypeError if used as native types);
- IS_PTR bucket reads go through the typed zend_value->lval accessor;
- raw uintptr_t slot dereferences go through a single readSlot() primitive
guarded by assert(is_int(...)), matching Core::threadLocalStorageBase();
- FFI::string/FFI::memcpy sizes stated non-negative with max(...,0), matching
HashTable::count()'s analyser clamp;
- the trivial unserializeType/serializeType field wrappers inlined to
unserializeTypeStruct($x->type) on the now-typed owners.
Justified surviving inline @PHPStan-Ignore argument.type (8 total), each an
FFI::addr() on a pointer field that must stay inline to yield the field SLOT
address (a by-value hop through Core::addr() addresses a pointer copy - proven
by BoundsValidationTest) and cannot be CData-typed because the field name is
dynamic: PayloadRelocator ptrValue/writePtrField, ScriptSerializer
ptrValue/put/defer, ReflectionOpcacheFile addMethodFrom (scope re-point), and
BoundsValidationTest's two filename-slot corruptions. This mirrors the existing
Compiler.php precedent for inline FFI::addr ignores.
phpstan-baseline.neon is untouched. Full suite is byte-for-byte identical:
536 tests / 5408 assertions / 5 skipped / 5 incomplete before and after, in
default, opcache-runner and release --group opcache modes, and in the
debug84 container.
Fixes #126
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDcCQiYqbMkjRPyhWgLL6M
The rebase of #126 onto #122 kept both #126's standalone `private readonly object $script` declaration and #122's promoted constructor property of the same name, a fatal redeclaration. Keep #122's promoted property (per the merge resolution) and move #126's `@var zend_persistent_script` narrowing onto the promoted param so PHPStan types $this->script as the stub rather than the FFI\CData|zend_persistent_script @PARAM union, restoring #126's zero-path-scoped-ignore struct-shape typing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BDcCQiYqbMkjRPyhWgLL6M
lisachenko
force-pushed
the
claude/opcache-123-bounds-validation
branch
from
August 20, 2026 00:30
d49bf0f to
7a1f13f
Compare
6 tasks
lisachenko
force-pushed
the
claude/opcache-126-phpstan-shapes
branch
from
August 20, 2026 00:49
8696e6e to
225bbca
Compare
lisachenko
marked this pull request as ready for review
August 20, 2026 00:49
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
2 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.
What this changes
Fixes #126 — tech-debt cleanup on top of the now-merged relocator/serializer chain (#267–#270), rebased onto current
8.4and covering every path-scoped ignore that chain accumulated. No behavior change — a typing-only refactor.Replaces the path-scoped PHPStan ignore blocks on all four opcache files with narrowing to the generated
ZEngine\Generated\*engine-struct stubs (the convention the rest of the codebase already uses), surfaced through docblock boundary-narrowing:phpstan.dist.neon: theproperty.nonObject/binaryOp.invalid/cast.int/argument.type(+assignOp.invalid) blocks forPayloadRelocator,ScriptSerializer,ReflectionOpcacheFile, and theargument.typeblock forBoundsValidationTest. Global CData rules and unrelated ignores untouched;phpstan-baseline.neonneither grew nor shrank.pointerAtAddress('T *', …)→ the class-string form typed by the existingTypedEntryPointReturnExtension; generic walker params narrowed to the owning stub via docblocks (native type staysobject); a newreadSlot()primitive for raw slot reads guarded byassert(is_int(...)).Correctness gotchas caught and respected (documented inline): the generated stubs are analysis-only, so they must stay docblock types, not native param types (native
objectavoids a runtimeTypeError); andFFI::addr()on a pointer field must stay inline (routing it through a by-value call addresses a copy, not the field slot — caught byBoundsValidationTest). That leaves 8 justified inline@phpstan-ignore argument.type, allFFI::addr()-of-pointer-field, each mirroring the existingCompiler.phpprecedent.The second commit reconciles this refactor with #122's
ReflectionOpcacheFileconstructor (the$imageOwnerbuffer-lifetime pin, merged while this PR was in flight): the promoted$script/$imageOwnerform is kept, with the stub narrowing carried in docblocks.Environment it was verified on
php -v): PHP 8.4.19 (cli) (built: Mar 30 2026 19:28:35) (NTS)--group opcache-relocator --fail-on-skippedOK, 48 tests, zero skips)--enable-debug)? yes — debug84 container--group opcachegreenOn the rebased tree: full suite 542 tests / 5469 assertions green, opcache group 70/70 with
--fail-on-skipped, PHPStan level max clean, cs-fixer clean.Checklist
8.4)composer testpasses on the matching PHP minorcomposer phpstan(level max) andcomposer cs:checkare greentools/generator/symbols.phpunchanged — nothing underinclude/,stubs/or.phpstorm.meta.phptouched🤖 Generated with Claude Code
https://claude.ai/code/session_01BDcCQiYqbMkjRPyhWgLL6M
Generated by Claude Code