[Sequence] Add "terminal" methods: querying - #31
Merged
Merged
Conversation
tests/Type/ holds one directory per family - Collection/, List/, Map/, Set/ - each split by aspect: AccessTypeTest, QueryTypeTest, TransformTypeTest. The Sequence ones were the only files left flat at the root, carrying the family in their name instead of in their path. They move under tests/Type/Sequence/, and SequenceTerminalTypeTest becomes AccessTypeTest: it only ever covered element access, and "terminal" stops naming anything specific once the querying family - just as terminal - lands beside it. The behaviour test takes the same correction, SequenceTerminalTest becoming SequenceAccessTest.
Second of four PRs bringing the terminal vocabulary of decision 5 to Sequence: isEmpty/isNotEmpty, contains/containsAll, all/any/none and count/countWhere. The split follows the line the element access batch drew. all, any, none, countWhere and isNotEmpty have bodies identical on both sides, so they move out of CollectionLogic into the shared IterableTerminalsLogic rather than being copied into SequenceLogic. isEmpty, contains and count stay per side, and structurally so: the eager side answers them from its store in O(1) while a sequence has to pull - isEmpty stops at the first element, contains at the first match, count drains. containsAll is the one worth reading. Its eager body is already generic - it calls $this->contains() once per value, and every store specialises that call, which is where a hash set gets its O(m). A sequence cannot pay that: one contains() costs a whole pass, and a second one on a single-pass source does not cost anything, it throws. So the sequence walks once instead, carrying the values it still looks for and dropping them as it meets them, stopping as soon as none is left. Every equal entry drops at once, so containsAll([1, 1]) asks whether 1 is there rather than whether it is there twice - the answer the eager side gives, pinned by a parity assertion. The plan had isEmpty/isNotEmpty/containsAll excluded from v1 among the buffering ops. They do not belong there: all three are single-pass and short-circuiting. And without isEmpty, asking a sequence whether it holds anything comes down to firstOrNull() !== null, which is wrong the moment it can hold null. count() does not make Sequence Countable. Native count($sequence) stays a TypeError by design; the interface docblock now says that next to $sequence ->count() being the explicit O(n) terminal that drains a pass. None of these carry #[NoDiscard]: they hand back a bool or an int rather than a new container, which is where the rule settled in noctud#29 puts the attribute.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nikophil
commented
Aug 23, 2026
nikophil
commented
Aug 23, 2026
delacry
requested changes
Aug 23, 2026
delacry
left a comment
Member
There was a problem hiding this comment.
btw contains() is fine as is, only Set uses hashing, other collections like List or Map->values do === comparison
Collection gets that bound for free: it extends Countable, and PHPStan knows Countable::count() returns int<0, max>. Sequence does not extend it - count() is a plain terminal there - so the bound has to be written down. countWhere() and SequenceLogic::count() already carried it; the interface method was the one place left where the count widened back to a bare int. The array_values() wrapping containsAll's array_filter served nothing. What is left of $missing is only ever compared against [] and filtered again, never indexed, so reindexing it once per element was work with no reader. Claude-Session: https://claude.ai/code/session_019QgiDpSLAXmLLWrrUrso3x
delacry
requested changes
Aug 23, 2026
Member
|
lgtm, and good call pulling |
nikophil
added a commit
to nikophil/collection
that referenced
this pull request
Aug 24, 2026
Third of four PRs bringing the terminal vocabulary of decision 5 to Sequence: fold, reduce/reduceOrNull, sum, avg/avgOrNull, min/max (+ OrNull), minOf/maxOf (+ OrNull) and joinToString. Every body in this batch walks $this once and reads nothing else, so all of them move out of CollectionLogic into the shared IterableTerminalsLogic rather than being copied into SequenceLogic - the line noctud#29 drew and noctud#31 followed. SequenceLogic gains nothing at all: both sides already use the trait, so the sequence picks these up by declaring them on the interface. Collection behaviour is unchanged, and its existing suite is what proves it. The OrNull variants stop being try-catch wrappers, which is the one behavioural change here. reduceOrNull() around reduce() swallowed an UnsupportedOperationException raised by the caller's own operation, and minOrNull()/maxOrNull()/minOfOrNull()/maxOfOrNull() swallowed a NoSuchElementException raised by the caller's selector - answering "empty" about a subject that was not empty. Each now walks on its own and returns null when nothing was found; two parity tests on the eager side pin the propagation. The empty-subject messages move to named constructors on UnsupportedOperationException (cannotReduceEmptySubject, cannotAverageEmptySubject) next to NoSuchElementException::emptySubject, so a sequence is told a "sequence" is empty. lcfirst leaves avg()'s eager wording byte-identical; reduce() gains the message it never carried. None of these take #[NoDiscard]: they hand back a scalar or an element rather than a new container, which is where noctud#29 put the attribute. Claude-Session: https://claude.ai/code/session_01JCXbhGog8jh6JAeYSCjy9y
nikophil
added a commit
to nikophil/collection
that referenced
this pull request
Sep 18, 2026
Fourth and last of the terminal batches: forEach, findLast/expectLast and toMap. forEach is the terminal counterpart of onEach, and returns void. Collection::forEach() still hands the collection back for chaining; aligning the two - Collection::onEach() plus deprecating that return - is decision 11 of the plan, a separate sub-feature, so Collection's API is untouched here. The divergence is deliberate and says out loud in the docblock which side it stands on. findLast/expectLast were sitting on the plan's v1 exclusion list among the buffering ops, and they do not belong there any more than isEmpty/containsAll did in noctud#31: both walk once and hold nothing but the current best match. Their eager bodies were already plain foreach ($this ...) walks with no store access, so this is the same move as the aggregation batch - CollectionLogic to IterableTerminalsLogic, no new algorithm, and Collection's existing suite proves the behaviour did not move with them. Kotlin ships findLast and lastOrNull(predicate) on Sequence for the same reason. toMap closes the materialization family. The workaround was toList()->toMap(), which builds a whole ImmutableList only to throw it away - exactly the double materialization a sequence exists to avoid. It cannot be shared: the eager body reads $this->store and calls newMapOf(), so the sequence gets its own, iterating $this and delegating to mapOf(). tests/Type/Sequence/ConversionTypeTest.php is new. It pins toMap's K/V templates, and picks up toList/toSet/toArray on the way - they had no type test at all, and adding toMap's alone would have read as an omission. Still out, on purpose: takeLast*/dropLast* (buffering intermediates, not terminals - and Kotlin ships neither on Sequence, while it does ship sorted and shuffled), and random/randomOrNull, which the eager side answers from its store and Kotlin does not offer on a Sequence at all. Claude-Session: https://claude.ai/code/session_01JCXbhGog8jh6JAeYSCjy9y
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.
Related to #23, follows #29.
Second of four PRs bringing the terminal vocabulary to
Sequence— element access landed in #29, querying here, then aggregation andforEach/toMap. This addsisEmpty/isNotEmpty,contains/containsAll,all/any/noneandcount()/countWhere.Five bodies move into the shared trait
#29 settled that a body identical on both sides lives in
IterableTerminalsLogicrather than being copied.all,any,none,countWhereandisNotEmptyqualify, so they leaveCollectionLogicfor the trait — hence 50 lines deleted there against 61 added in the trait.Collectionbehaviour is unchanged, and the existing suite is what proves it.What stays per side, and why
isEmpty,containsandcountare the same structural split asfirst/last: the eager side answers them from its store in O(1), a sequence has to pull.isEmpty()stops at the first element,contains()at the first match,count()drains — each pinned by a test that counts exactly what the source handed out.containsAll— the one that cannot be sharedThe eager body is already generic:
It calls
$this->contains()once per value, and every store specialises that call — which is exactly where a hash set gets its O(m):contains()containsAll()HashElementStore(Set)array_key_exists(hash)— O(1)ArrayIndexStore(List)in_array(…, true)— O(n)HashKeyValueStore(Map values)array_any— O(n)A sequence has no store to specialise. One
contains()costs a whole pass, and a second one on a single-pass source does not cost anything — it throwsNonReplayableSourceException. Sharing that body would not merely be slow on the lazy side, it would be wrong.So the sequence walks once instead, carrying the values it still looks for and dropping them as it meets them, stopping as soon as none is left. Every equal entry drops at once, so
containsAll([1, 1])asks whether1is there rather than whether it is there twice — the answer the eager side gives, pinned by a parity assertion.The comparison stays
===, consistent withSequence::contains(). Hashing the wanted values throughKeyHasherwould bring it to O(n + m), buthashSetKey()is deliberately not===— aHashablecollides two distinct instances by design, floats round at%.12F— socontains()andcontainsAll()would disagree with each other on the same sequence. EveryCollectiontype is self-consistent on that point andSequenceshould be too.Three methods the plan had excluded from v1
isEmpty/isNotEmpty/containsAllsat on the v1 exclusion list among the buffering ops. They do not belong there: all three are single-pass and short-circuiting. And withoutisEmpty(), asking a sequence whether it holds anything comes down tofirstOrNull() !== null, which is wrong the moment it can holdnull— there is a test for that.count()does not makeSequencecountableNative
count($sequence)stays aTypeErrorby design, andSequenceFactoryTestalready asserts the type is notCountable. The interface docblock used to say counting would silently consume a pass; it now says$sequence->count()is the explicit O(n) terminal that drains one.#[NoDiscard]None of these carry it — they hand back a
boolor anintrather than a new container, which is where #29 settled the rule.Test file moves (separate commit)
tests/Type/holds one directory per family —Collection/,List/,Map/,Set/— each split by aspect. TheSequencetype tests were the only ones left flat at the root, so they move undertests/Type/Sequence/.SequenceTerminalTypeTestbecomesAccessTypeTest: it only ever covered element access, and "terminal" stops naming anything specific once querying — just as terminal — lands beside it. The behaviour test takes the same correction.Verification
composer qagreen — PHPStan level 9 with no new ignores, phpcs clean, 6357 tests (+19).