Let a type mapper cover several types at once - #8231
Merged
Conversation
Eight mappers were byte-identical to a sibling except for the PHPStan type they
named. They could not be merged, because getNodeClass() returned a single class
and the two groups share the exact same ancestors (AccessoryType, CompoundType,
Type) while mapping to different output - one to "string", the other to "array".
Widen the contract to getNodeClasses(): array, matching NodeTypeResolverInterface
which already uses that shape, and merge the two groups:
Accessory{LiteralString,NonEmptyString,NonFalsyString,NumericString}TypeMapper
-> AccessoryStringTypeMapper
{HasOffset,HasOffsetValue,NonEmptyArray,OversizedArray}TypeMapper
-> AccessoryArrayTypeMapper
NeverTypeMapper and ResourceTypeMapper are also identical, but "never" and
"resource" are unrelated concepts, so they stay apart on purpose.
Member
Author
|
LGTM 👍 |
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
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.
Follow-up to #8230.
Why the duplication could not be removed before
Eight mappers were byte-identical to a sibling except for the PHPStan type they named:
The obvious fix — one mapper registered against a shared parent type — does not work here. All eight share exactly the same ancestors:
AccessoryTypecovers both groups, but one group maps tostringand the other toarray, so no single class-string separates them.getNodeClass(): stringwas the blocker.The change
Widen the contract to return several classes:
Not a new shape for the codebase —
NodeTypeResolverInterfacenext door already declaresgetNodeClasses(): arrayand is dispatched the same way.The two groups then collapse to
AccessoryStringTypeMapperandAccessoryArrayTypeMapper: 8 classes to 2.NeverTypeMapperandResourceTypeMapperare identical too, butneverandresourceare unrelated concepts — merging them would shorten the file count and nothing else, so they stay apart.Cost
Every remaining mapper gains one bracket pair:
29 files, mechanical. Net
-170lines.Behavior
Checked, not assumed. I ran all eight collapsed types through the container's
PHPStanStaticTypeMapperon this branch and onmain, comparing the phpdoc node plus the PhpParser node forPARAM,RETURNandPROPERTYkinds — 32 outputs, all identical.To make sure the two runs really executed different code (a shared
vendor/autoloader would otherwise resolve both to the same sources), the baseline ran in a separate worktree with a checkout-first autoloader prepended. It demonstrably loadedmain'sAccessoryLiteralStringTypeMapper, and rejected this branch'sAccessoryStringTypeMapperagainstmain's narrower interface.TypeMapperOrderTestfrom #8230 is updated for the widened contract and still guards registration order across every declared class.