[Sequence] Add "terminal" methods: element access - #29
Conversation
First of four PRs bringing the terminal vocabulary of decision 5 to
Sequence: first/firstOrNull, last/lastOrNull, single/singleOrNull,
elementAt/elementAtOrNull, find and expect. Until now a sequence could
only be ended by toList/toSet/toArray, so sequenceOf($rows)->filter(...)
->first() did not exist.
Supersedes the "terminals duplicated, not refactored" decision: bodies
identical on both sides now live in a shared IterableTerminalsLogic trait
used by CollectionLogic and SequenceLogic alike. A generic trait whose
PHPDoc is {@inheritdoc}, resolving against whichever interface the
consumer implements, is clean at PHPStan level 9 - that was the open risk
of sharing rather than duplicating.
A message naming its own subject used to be a second reason to duplicate,
since a sequence must not be told a "Collection" is empty. It no longer
is: NoSuchElementException derives that noun from the subject it is
handed, so single() is shared too. That unblocks min/max/minOf/maxOf/avg
for the aggregation PR, which are duplicated for the same reason alone.
What still cannot be shared is structural: the eager side answers
first/last from its store in O(1) while a sequence has to pull, so
first() returns inside the foreach to keep it to one element and last()
drains, the last element being knowable only at the end.
elementAt/elementAtOrNull are new vocabulary, on Sequence only. The
library's index accessor is ListInterface::get(), O(1) random access a
sequence cannot offer; Kotlin draws the same line between List.get() and
Sequence.elementAt(), and the separate name is what carries the O(n) cost
to the reader. They are deliberately not in the shared trait either:
Collection does not declare them, so putting them there would add an
undeclared public method to every set and map view.
Terminals carry #[NoDiscard] unlike their Collection counterparts:
discarding one silently burns the only pass a sequence may have.
The eager messages had no test coverage, so nothing would have caught the
shared factory changing them; CollectionSingle and CollectionAggregate now
assert all seven.
Codecov Reportβ All modified and coverable lines are covered by tests. π’ Thoughts on this report? Let us know! |
delacry
left a comment
There was a problem hiding this comment.
btw, one small unrelated thing, few commits have Co-Authored-By: Claude trailer.. how you write the code is yours to decide, but the way I see it a tool isn't an author - it's a bit like crediting the keyboard or the IDE π so I'd rather not carry those in the history here
I'll probably just write it down in a CONTRIBUTING.md so it's a repo rule.. but not sure if you have to rebase it and force push because of that.. maybe it will get lost when I squash-merge but I'm not sure about that
A negative index used to drain the whole sequence before failing; both accessors now throw IndexOutOfBoundsException up front instead.
Iterating $this runs user closures on the lazy side, so the try-catch swallowed real NoSuchElementExceptions raised inside a pipeline stage.
27c7752 to
390efe1
Compare
yeah sorry for this, I usually configure Claude not to do this, but I'm juggling between several computers, and the one I created the PR with has not the same harness than my main computer.
yeah I actually feel the same. BTW I'm definitely not a "vibe coding" person, although I tend to use Claude a lot these days, nothing leaves the computer without a full review/rewrite π
I'm not sure, but I guess GitHub will keep the (co)autohors after squash merge, I've just rebased and removed it from the authors |
that's what we tell ourselves π I also don't consider myself a vibe-coder, but most of the code I've "written" this year was AI-generated, until we can understand the code it's fine, it is much worse for new programmers - there's no real need to understand the code if it works for them and they can ask ai what it does thanks for removing the attribution |
|
Lol yeah I also got a new best friend by the end of 2025 π |
Removing @phpstan-require-implements took the last reference to Sequence in the file with it, and phpcs fails on the import that stayed behind.
Supersedes the "#[NoDiscard] on every terminal" decision. The element access terminals lose the attribute: first/firstOrNull, last/lastOrNull, single/singleOrNull, elementAt/elementAtOrNull, find and expect now carry none, like their Collection counterparts. The rule the eager side keeps everywhere is that the attribute marks what hands back a new container while leaving the subject untouched - transformation, ordering, conversion, plus countBy. That is what makes $collection->filter(...) on a MutableCollection worth warning about. An element access returns an element, not a container, so it falls outside the rule whichever side it is on. Sequence was stricter on the argument that a discarded terminal burns the only pass a sequence may have. The argument stands, but widening the rule changes what the attribute means across the library, and materializing for a side effect may well be a legitimate call; that question is tracked in noctud#30 and belongs to a PR covering both types, not to this one. Until it is settled, Sequence follows Collection rather than diverging. This also removes an incoherence the attribute already had here: Sequence declared #[NoDiscard] on single() while the body it inherits from the shared IterableTerminalsLogic carries none, so GeneratorSequence::single() was attributed on the interface and bare on the implementation.
|
thanks! |
Second of four PRs bringing the terminal vocabulary to `Sequence` β element access landed in #29, querying here, then aggregation and `forEach`/`toMap`. This adds `isEmpty`/`isNotEmpty`, `contains`/`containsAll`, `all`/`any`/`none` and `count()`/`countWhere`. ## Five bodies move into the shared trait #29 settled that a body identical on both sides lives in `IterableTerminalsLogic` rather than being copied. `all`, `any`, `none`, `countWhere` and `isNotEmpty` qualify, so they leave `CollectionLogic` for the trait β hence 50 lines deleted there against 61 added in the trait. `Collection` behaviour is unchanged, and the existing suite is what proves it. ## What stays per side, and why `isEmpty`, `contains` and `count` are the same structural split as `first`/`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 shared The eager body is *already* generic: ```php foreach ($elements as $x) { if (!$this->contains($x)) { return false; } } ``` It calls `$this->contains()` once per value, and every store specialises that call β which is exactly where a hash set gets its O(m): | store | `contains()` | β `containsAll()` | |---|---|---| | `HashElementStore` (Set) | `array_key_exists(hash)` β O(1) | **O(m)** | | `ArrayIndexStore` (List) | `in_array(β¦, true)` β O(n) | O(nΒ·m) | | `HashKeyValueStore` (Map values) | `array_any` β O(n) | O(nΒ·m) | 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 throws `NonReplayableSourceException`. 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 whether `1` is there rather than whether it is there twice β the answer the eager side gives, pinned by a parity assertion. The comparison stays `===`, consistent with `Sequence::contains()`. Hashing the wanted values through `KeyHasher` would bring it to O(n + m), but `hashSetKey()` is deliberately not `===` β a `Hashable` collides two distinct instances by design, floats round at `%.12F` β so `contains()` and `containsAll()` would disagree with each other on the same sequence. Every `Collection` type is self-consistent on that point and `Sequence` should be too. ## Three methods the plan had excluded from v1 `isEmpty`/`isNotEmpty`/`containsAll` sat on the v1 exclusion list 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` β there is a test for that. ## `count()` does not make `Sequence` countable Native `count($sequence)` stays a `TypeError` by design, and `SequenceFactoryTest` already asserts the type is not `Countable`. 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 `bool` or an `int` rather 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. The `Sequence` type tests were the only ones left flat at the root, so they move under `tests/Type/Sequence/`. `SequenceTerminalTypeTest` becomes `AccessTypeTest`: 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. Related to #23, follows #29. --------- Co-authored-by: delacry <45132928+delacry@users.noreply.github.com>
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
Add elementAt() and elementAtOrNull() to Collection, returning the element at a position in iteration order. A Set or a map view no longer needs toList() or find() to reach its nth element. CollectionLogic checks the index against count() and then walks to the position. ListLogic overrides both with random access through its store. List, Set, MapKeySet, MapEntrySet and MapValueCollection all gain the methods. elementAt() throws IndexOutOfBoundsException for a negative index or one past the end. elementAtOrNull() returns null for both. Sequence::elementAtOrNull() now also returns null for a negative index instead of throwing, matching List::getOrNull() and Kotlin. It returns before walking, so it never drains the source or runs forever on an infinite one. elementAt() still rejects a negative index. Add runtime tests covering every collection type, Sequence tests for a negative index on finite and infinite sources, and type assertions. Refs #29
Related to #23.
First of four PRs bringing the terminal vocabulary to
Sequenceβ element access here, then querying, aggregation, andforEach/toMap.Until now a sequence could only be ended by
toList()/toSet()/toArray(), sosequenceOf($rows)->filter(β¦)->first()did not exist: you had to materialize a list first, which defeats the point of the type. This addsfirst/firstOrNull,last/lastOrNull,single/singleOrNull,elementAt/elementAtOrNull,findandexpect.Terminals are shared, not duplicated
The original plan was to copy the
foreach ($this β¦)bodies fromCollectionLogicintoSequenceLogicand keep the footprint on existing files near zero. With ~25 shareable bodies in total across the four PRs, that would have meant two copies drifting apart, so this goes the other way: bodies identical on both sides live in a newIterableTerminalsLogic, used byCollectionLogicandSequenceLogicalike.The open risk was PHPStan. The trait carries
@template E, is consumed through/** @use IterableTerminalsLogic<E> */, and its PHPDoc is reduced to{@inheritDoc}so that each method resolves against whichever interface the consumer implements βCollection<E>orSequence<E>. That turns out to be clean at level 9 with no new ignores, which is what made sharing viable.The exception derives the subject's name
A message naming its own subject used to be a second reason to duplicate: a sequence must not be told a "Collection" is empty. Rather than keep two copies of
single()for two nouns,NoSuchElementExceptionnow derives the noun from the subject it is handed:One body, the right noun. The
matchlives in an@internal NamesItsSubjecttrait shared by the exception classes; its parameter typeCollection|Sequencemakes it exhaustive, so thedefaultarm is the Collection case rather than a catch-all.This also unblocks the aggregation PR:
min,max,minOf,maxOfandavgare duplicated today for that reason and that reason only.CollectionSingleandCollectionAggregatenow assert the eager messages. They asserted only the exception class before, so nothing would have caught the shared factory silently changing'Collection is empty'β without those seven assertions, a green suite would not have proven the refactor preserved the eager side.What deliberately stays per side
first/lastare not shared, and that is structural rather than incidental: the eager side answers them from its store in O(1) (array_key_first/array_key_last), while a sequence has to pull. Sofirst()returns inside theforeachto keep it to exactly one element, andlast()drains β the last element is only knowable once the source runs out. Same reasoning will apply tocontainsandcountin the querying PR.elementAt/elementAtOrNullThese are new vocabulary, and on
Sequenceonly. The library's index accessor isListInterface::get(), which is O(1) random access a sequence cannot offer. Kotlin draws the same line betweenList.get()andSequence.elementAt(), and the distinct name is what carries the O(n) cost to the reader βget(int)on a sequence would suggest a direct access it cannot do.They are also deliberately not in the shared trait:
Collectiondoes not declare them, so putting them there would add an undeclared public method to every set and map view. Happy to expose them onCollectiontoo if that is preferred, but that is an API addition rather than sharing.Note this makes three places where
Sequencediverges fromCollection:onEach(already merged, itsCollectioncounterpart being a separate follow-up), plus these two.#[NoDiscard]follows theCollectionruleTerminals carried the attribute everywhere in the first version, on the argument that discarding one burns the only pass a sequence may have. Dropped after review: the rule kept everywhere on the eager side is that
#[NoDiscard]marks what hands back a new container while leaving the subject untouched β transformation, ordering, conversion β and an element access returns an element, not a container. Sofirst(),single(),elementAt(),find()and friends carry no attribute here either, whilefilter()/map()/toList()keep theirs.This also removes an incoherence the attribute already had:
Sequencedeclared it onsingle()while the body inherited from the sharedIterableTerminalsLogiccarries none. Whether to widen the rule forCollectionandSequencetogether is tracked in #30.Other details worth flagging
nullis indistinguishable from an empty one throughlastOrNull()β the same ambiguityCollection::lastOrNull()has, kept on purpose.first()pulls exactly one element,single()at most two,find()stops at the match,elementAt()stops at the position,last()drains.find/elementAttests pin after afilterreindexes.Verification
composer qagreen β PHPStan level 9 over 246 files with no new ignores, phpcs clean, 6338 tests.php bin/generate-all.phpleaves the generated narrowing files untouched.