[Collection] Add elementAt() and elementAtOrNull() - #46
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
delacry
requested changes
Sep 26, 2026
delacry
left a comment
Member
There was a problem hiding this comment.
Thank you, just one thing to resolve at elementAtOrNull(), and feel free to add similar checks to CollectionLogic, it might improve performance in some cases.
| { | ||
| // @phpstan-ignore smaller.alwaysFalse (defensive guard: the phpdoc type does not bind untyped callers) | ||
| if ($index < 0) { | ||
| throw new IndexOutOfBoundsException('Cannot use a negative index.'); |
Member
There was a problem hiding this comment.
I would keep this guard, just return null instead of throwing, because now elementAtOrNull(-1) walks the whole source (or gets stuck if it's never-ending) before returning null
same guard would make sense maybe also in the CollectionLogic::elementAt()/elementAtOrNull(), so they don't walk all elements for a negative index
Give every collection the positional vocabulary Sequence already had. CollectionLogic walks to the position, ListLogic overrides it with the random access it already has, so List, Set, MapKeySet, MapEntrySet and MapValueCollection all gain it at once. Align Sequence::elementAtOrNull() on the same answer for a negative index: null rather than a throw. It is what ListInterface::getOrNull() has always returned and what Kotlin returns, and it makes one rule instead of two. The throwing elementAt() still rejects it.
A negative index made Sequence::elementAtOrNull() drain the source before returning null, and never return on an infinite one. Collections know their size, so they now reject any out-of-bounds index without walking.
nikophil
force-pushed
the
feature/collection-element-at
branch
from
September 28, 2026 13:48
0ccea6a to
2a1a242
Compare
The bounds check guarantees the walk finds the index, so the fallthrough now throws a LogicException and is excluded from coverage.
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.
Answers #29 (comment) — getting the 3rd
item of a
Setcurrently meanstoList()first, orfind(fn ($v, $i) => $i === 3), whichnobody guesses.
Two implementations, as suggested in that thread:
CollectionLogic::elementAt()walks the elements and counts — the only thing aSetcan do.ListLogicoverrides both with$this->store->get(), so a list keeps its O(1) access.List,Set,MapKeySet,MapEntrySetandMapValueCollectionall mountCollectionLogic,so one trait of tests (
CollectionFirstLast) covers the five of them, both code paths included.The generic walk has no negative-index guard, unlike the
Sequenceone: positions start at0, so a negative index never matches and falls through to the same out-of-bounds error as an
index past the end. Less code, same behaviour.
One behaviour change:
elementAtOrNull(-1)Sequence::elementAtOrNull(-1)currently throws. That makes it the odd one out:List::getOrNull(-1)nullnull(untouched)Sequence::elementAtOrNull(-1)nullCollection::elementAtOrNull(-1)nullelementAt(-1), every typeKotlin's
elementAtOrNullopens withif (index < 0) return null, andgetOrNull()hasreturned null for a negative index since 0.1.x. The rejection made sense on
elementAt()— #29documents it deliberately — but it looks like it was carried over to the
OrNullvariantwithout that being the intent, and
asSequence()makes the divergence easy to walk into. Sothe rule is now one line: the throwing variant rejects a negative index, the
OrNullvariantanswers "nothing there". 0.2.x is unreleased, so this costs nothing.
The
@param non-negative-intonSequence::elementAtOrNull()goes with it — the runtime nowaccepts what the signature was forbidding.
Codegen is neutral:
elementAt()returnsE, so there is nothing for the narrowing passes torewrite.