Repository navigation
Keep generic types on concrete collection classes - #56
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
delacry
force-pushed
the
fix/concrete-collection-types
branch
from
October 13, 2026 10:58
e391bd1 to
1c1d888
Compare
Member
Author
|
I realized that my previous fix went too far, as I narrowed few return types to So I force-pushed smaller fix, @nikophil if you already tried the branch - chunks on concrete classes are now typed #57 is rebased on top, with no changes |
A value typed as a concrete class (ImmutableHashSet, ImmutableArrayList, or a class built on the logic traits) resolves its methods from the traits, where a docblock that diverges from the interface went unnoticed: every type test reached the library through the interfaces. chunked(), windowed(), zip() and zipWithNext() returned a bare ImmutableList. Collection declared ListInterface<...>, which does not fit the traits' native ImmutableList, so PHPStan dropped it with its generics. CollectionLogic now declares them itself. chunked() and windowed() narrow only the outer list, to ImmutableList<ListInterface<E>>: the element type is invariant, so ImmutableList<ImmutableList<E>> would not fit the interface's ListInterface<ListInterface<E>>. Collection stays unchanged, and a PHPDoc that spells out the result keeps working. groupBy() in ListLogic, SetLogic and CollectionLogic dropped the interfaces' conditional return type, so with a value transform the groups were typed as the original elements while they hold the transformed values in an ImmutableList. add(), addFirst() and addAll() on immutable lists and sets overrode the interface's @PARAM NE with E, which left NE in the inherited return type unresolved and rejected the documented widening. They now widen as documented. A class that rebuilds itself through newCollectionOf() and validates its elements keeps them strict with @method; the extending and upgrade guides show how, and the extending fixtures do it. Fixes #49
delacry
force-pushed
the
fix/concrete-collection-types
branch
from
October 26, 2026 23:47
1c1d888 to
2131a40
Compare
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.
Fixes #49.
A value typed as a concrete class (
ImmutableHashSet,ImmutableArrayList, or your own class using the logic traits) got worse PHPStan types than the same value typed as its interface. Some trait docblocks didn't match the interfaces, and our type tests only went through the interfaces, so nobody noticed.What changed:
chunked(),windowed(),zip()andzipWithNext()inCollectionLogicdeclare their generic return types. TheCollectioninterface stays as it is:ImmutableList<ListInterface<E>>fits itsListInterface<ListInterface<E>>, so PHPDocs that spell out the result keep working.groupBy()in the traits uses the same conditional return type as the interfaces.add(),addFirst()andaddAll()on immutable lists and sets widen the type as documented, instead of leakingNE.One thing can break, described in
UPGRADE-0.2.md: a class built on the base traits that rebuilds itself and validates its elements can now receive a foreign element fromadd(). Declare@method self add(YourType $element)to keep it strict. The extending guide and test fixtures show this.The new
tests/Type/Collection/ConcreteClassTypeTest.phpfails on0.2.xand passes here.