Extract array-derivation logic into polymorphic Type methods#5611
Merged
ondrejmirtes merged 6 commits into2.1.xfrom May 7, 2026
Merged
Extract array-derivation logic into polymorphic Type methods#5611ondrejmirtes merged 6 commits into2.1.xfrom
ondrejmirtes merged 6 commits into2.1.xfrom
Conversation
Replaces `FuncCallHandler::getArraySortDoNotPreserveListFunctionType()`'s hand-rolled `TypeTraverser::map` over `ConstantArrayType` / `ArrayType` with a polymorphic `Type::makeListMaybe()` method: - `ConstantArrayType::makeListMaybe()` rebuilds with `isList -> Maybe`, preserving keys / values / accessories / non-emptiness. - `ArrayType::makeListMaybe()` is a no-op (`ArrayType` doesn't carry list-ness on its own; that lives in an enclosing `IntersectionType` with `AccessoryArrayListType`). - `AccessoryArrayListType::makeListMaybe()` returns `MixedType` so the enclosing intersection drops it via `TypeCombinator::intersect` while preserving siblings like `NonEmptyArrayType`. - `UnionType` / `IntersectionType` distribute through their members. - Other types (`MixedType`, `NeverType`, accessories that don't track list-ness, `MaybeArrayTypeTrait`, `NonArrayTypeTrait`, `LateResolvableTypeTrait`, `StaticType`) return self / delegate. Pure refactor: full test suite passes unchanged.
…lues" Replaces two hand-rolled `TypeTraverser::map`/`new ConstantArrayType(...)` loops that walk `getKeyTypes()`/`getValueTypes()` and rebuild via the builder: - `FuncCallHandler::getArrayWalkResultType` (called from `array_walk[_recursive]`) becomes a one-liner closure passing through the constant new value type. - `ArrayMapFunctionReturnTypeExtension`'s single-array constant fast path uses `mapValueType` to invoke the user callback per value while preserving keys, optional-key state, list-ness, and accessories. The `ARRAY_COUNT_LIMIT`-guarded fall-through to a plain `ArrayType` for oversized inputs in `array_map` is unchanged. `preg_replace*` on array subjects (in `ReplaceFunctionsDynamicReturnTypeExtension`) is intentionally deferred to a follow-up commit — that site additionally flips every key to optional, which needs its own `Type` method. Pure refactor: full test suite passes unchanged.
ondrejmirtes
force-pushed
the
group-3-refactor
branch
from
aa66c83 to
051f998
Compare
Replaces `ArrayChangeKeyCaseFunctionReturnTypeExtension`'s ~100 lines of hand-rolled `ConstantArrayTypeBuilder` rebuild + `TypeTraverser::map` walk with a polymorphic `Type::changeKeyCaseArray(?int $case)` method. Constant-string keys fold to a specific value (or to the union of both folds when `$case === null`); general string keys gain `AccessoryLowercaseStringType` / `AccessoryUppercaseStringType` (or neither for the unknown-case path); int keys, values, accessories, and list-ness are preserved via composition through `UnionType` / `IntersectionType`. Non-emptiness flows through automatically: `NonEmptyArrayType::changeKeyCaseArray()` returns self, `ConstantArrayType` keeps its required-keys count. `HasOffsetType` / `HasOffsetValueType` over a `ConstantStringType` offset fold the offset itself; for the unknown-case path they widen to `MixedType` so the enclosing intersection drops the now-imprecise assertion. The extension shrinks to a one-line `return $arrayType->changeKeyCaseArray($case);`. Pure refactor: full test suite passes unchanged.
…lback path Replaces `ArrayFilterFunctionReturnTypeHelper::removeFalsey()`'s hand-rolled CAT/`ArrayType` branching with a polymorphic `Type::filterArrayRemovingFalsey()` that: - For `ConstantArrayType`: per-value, drops definitely-falsey entries, marks possibly-falsey entries optional with the falsey type removed, keeps definitely-truthy entries unchanged. - For `ArrayType`: removes falsey types from the value type; collapses to `ConstantArrayType([], [])` if nothing remains. - Accessories: `HasOffsetType` / `NonEmptyArrayType` / `AccessoryArrayListType` weaken to `MixedType` (filter may drop offsets, may empty the array, introduces gaps that break list-ness). `HasOffsetValueType` survives iff its value is definitely truthy. `OversizedArrayType` is preserved conservatively. The callback-based `filterByTruthyValue` path is intentionally deferred to a follow-up — it threads `MutatingScope`/`Expr` per-pair narrowing through, which doesn't fit a pure type-method signature. Pure refactor: full test suite passes unchanged.
…pe` in NodeScopeResolver `NodeScopeResolver`'s post-foreach array-type rewrite (around line 1383 of the prior code) used a hand-rolled `TypeTraverser::map` to rebuild `new ArrayType(...)` whenever the loop's key or value variable narrowed during iteration. Replace with two polymorphic calls — `mapValueType` for the value side, the new `mapKeyType` for the key side. `mapKeyType` semantics: - `ArrayType`: `new ArrayType($cb($keyType), $itemType)`. - `ConstantArrayType`: pass through. The original `TypeTraverser` skipped `!$type instanceof ArrayType` leaves; rewriting CAT keys via a blanket `cb` would coerce constants into the broader narrowed type and lose precision. - `UnionType` / `IntersectionType`: distribute through members. - Accessories (`HasOffsetType`, `HasOffsetValueType`, `NonEmptyArrayType`, `AccessoryArrayListType`, `OversizedArrayType`): pass through, matching the original "leaves accessories untouched" behavior. - `MixedType` / `StaticType` / `LateResolvableTypeTrait`: same shape as `mapValueType`'s overrides. Aligned `mapValueType` / `mapKeyType` defaults in `NonArrayTypeTrait` and `MaybeArrayTypeTrait` to return `$this` (pass-through) rather than `ErrorType` — required so that a `?array` foreach iteratee preserves the `null` member through the rewrite, matching the prior `TypeTraverser` guard `if (!$type instanceof ArrayType) return $type;`. Pure refactor: full test suite passes unchanged.
`ReplaceFunctionsDynamicReturnTypeExtension` previously rebuilt
`ConstantArrayType`s by hand (via `ConstantArrayTypeBuilder`) to map
each value through `getReplaceType()` while optionally marking every
key optional. Replace that with polymorphic dispatch:
$mapped = $arrayArgumentType->mapValueType(...);
if ($keyShouldBeOptional) {
$mapped = $mapped->makeAllArrayKeysOptional();
}
This collapses the `getConstantArrays()` fast path with the generic
`ArrayType` fallback, keeps non-emptiness/list-ness via the existing
accessory dispatch, and removes the need for the extension to know
about `ConstantArrayTypeBuilder`.
`makeAllArrayKeysOptional()` is implemented across the standard set of
`Type` callees: it marks every explicit key in a `ConstantArrayType`
optional, is a no-op for `ArrayType` (already arbitrary subsets) and
all accessories that do not track per-key optionality, drops
`HasOffsetType`/`HasOffsetValueType` (their guarantee no longer
holds), and distributes through unions/intersections.
ondrejmirtes
force-pushed
the
group-3-refactor
branch
from
051f998 to
70a2fdd
Compare
Contributor
I wonder why/whether this should have changes in tests in case this was hiding bugs? |
Member
Author
|
I think this was just a sentence inspired by instructions in CLAUDE.md "Bug fixes through better polymorphism ". The goal was refactoring while preserving functionality 1:1. While it might fix some unexpected pre-existing bugs, issue bot didn't find anything. I have more methods in my queue. |
This was referenced May 7, 2026
ondrejmirtes
added a commit
that referenced
this pull request
May 7, 2026
Follow-up cleanup across #5611, #5612, #5613: - `FuncCallHandler::getArrayWalkResultType()` and `getArraySortDoNotPreserveListFunctionType()` were single-statement wrappers around `Type::mapValueType()` / `Type::makeListMaybe()`. Both private, both fully replaced by their polymorphic call; inline at the two/one call sites and delete the helpers. - `InstanceofHandler` and `TypeSpecifier` (the `instanceof <expr>` branch) used a throwaway `$classType = $scope->getType(...)` only to immediately overwrite it with `$result->type`. Drop the intermediate; chain the call. Pure simplification: full test suite + phpstan + cs pass.
ondrejmirtes
added a commit
that referenced
this pull request
May 7, 2026
Follow-up cleanup across #5611, #5612, #5613: - `FuncCallHandler::getArrayWalkResultType()` and `getArraySortDoNotPreserveListFunctionType()` were single-statement wrappers around `Type::mapValueType()` / `Type::makeListMaybe()`. Both private, both fully replaced by their polymorphic call; inline at the two/one call sites and delete the helpers. - `InstanceofHandler` and `TypeSpecifier` (the `instanceof <expr>` branch) used a throwaway `$classType = $scope->getType(...)` only to immediately overwrite it with `$result->type`. Drop the intermediate; chain the call. Pure simplification: full test suite + phpstan + cs pass.
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.
Summary
Pure refactor — no behaviour changes. Extracts array-derivation logic from external call sites (in
NodeScopeResolver, ExprHandlers, andsrc/Type/Php/*extensions) into polymorphic methods on theTypeinterface.Motivation: each of these sites previously opened with an
isConstantArray()->yes()fast path that hand-built aConstantArrayTypeviaConstantArrayTypeBuilder::createEmpty(), and fell through to a genericArrayTypepath on the slow side. The duplication was hiding bugs (UnionTypeof mixed CATs,IntersectionTypecarrying accessory types, etc. degrade or take the wrong branch). Polymorphic dispatch through aTypemethod shrinks the call site to a one-liner and letsArrayType,ConstantArrayType,UnionType, andIntersectionTypeeach give the right answer via their own override.This is the project's stated direction in
CLAUDE.md("When a bug requires checking a type property across the codebase, the fix is often to add a new method to theTypeinterface rather than scatteringinstanceofchecks") applied to deriving arrays rather than querying them.Six new methods, one per commit:
Type::makeListMaybe()FuncCallHandler::getArraySortDoNotPreserveListFunctionType(asort/uksort/etc.)Type::mapValueType(callable)getArrayWalkResultType+ArrayMapFunctionReturnTypeExtensionsingle-array path + foreach by-ref value path inNodeScopeResolverType::changeKeyCaseArray(?int)ArrayChangeKeyCaseFunctionReturnTypeExtensionType::filterArrayRemovingFalsey()ArrayFilterFunctionReturnTypeHelper::removeFalsey(no-callbackarray_filter)Type::mapKeyType(callable)NodeScopeResolver(paired withmapValueType)Type::makeAllArrayKeysOptional()ReplaceFunctionsDynamicReturnTypeExtension(preg_replace*over array subjects)Each method follows the standard 16-file plumbing pattern: declared on the
Typeinterface, implemented onArrayType,ConstantArrayType,UnionType,IntersectionType,NeverType,MixedType,StaticType; defaults inNonArrayTypeTrait/MaybeArrayTypeTrait/LateResolvableTypeTrait; pass-through (or appropriate degradation) on the five accessory types (AccessoryArrayListType,NonEmptyArrayType,HasOffsetType,HasOffsetValueType,OversizedArrayType).Typeis@api-do-not-implement, so adding methods here is BC-safe per the project's API promise.Net diff: +791/-290 across 22 files.
Not in this PR
The following call sites were considered for the same treatment and consciously left raw — extracting them cleanly would require either a
Scope-aware Type method or a non-trivial signature redesign, which is out of scope for a pure refactor:TypeSpecifier::specifyTypesForCount(count()-narrowing) — five interleaved branches that don't fit a single polymorphic shape.FuncCallHandler::array_unshiftprepend branch — closure-based unpack pipeline shared witharray_push.ArrayMergeFunctionDynamicReturnTypeExtension/ArrayReplaceFunctionReturnTypeExtension— variadic with optional-arg flag.ArrayFilterFunctionReturnTypeHelper::filterByTruthyValue(callback variant) —Scope/Expr-tied per-pair narrowing.ArrayColumnHelper::handleConstantArray—Scope::canReadPropertyfor property visibility.FilterVarArrayDynamicReturnTypeExtension—FILTER_*spec interpretation tied to the extension.Test plan
make testspasses after each commit (12044 tests, 79654 assertions).