fix(Deque): a Clear() that removes nothing no longer invalidates enumerators - #334
Merged
Merged
Conversation
…erators Deque<T>.Clear() bumped _version outside the guard that skips the array clearing, so clearing an already-empty deque tore down every live enumerator. It was the only count-based collection in the library that did this: the other 28 return early on an empty collection and never touch _version. It also contradicted Deque's own documented contract, which calls the enumerator guard a check for "structural modification" and separately notes that an indexer set does not invalidate. Option A of the issue: match Celerity's own family rather than the BCL, which points both ways (Dictionary<K,V>.Clear() bumps only when non-empty while Queue<T> / Stack<T> bump unconditionally). Resetting _head on an empty deque was pure normalization and is not observable through the public surface, so skipping it changes nothing else. The rule was previously pinned only per-collection, for a handful of types, which is how the outlier shipped. ClearNoOpVersionTests now asserts it once per count-based collection (29 of them), each in all three states that matter: never populated, populated, and emptied by a preceding Clear(). It also pins the two deliberate exceptions - BitSet and FenwickTree are fixed-length, so establishing "already empty" costs the same scan as the unconditional clear it would skip - so they read as a decision rather than as the same oversight repeated. The probabilistic sketches track no version and expose no enumerator, so they are out of scope. Closes #333 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion suites The parity expectation listed the per-layer coverage (behavioural, CsCheck, fuzz) but not the family-wide invariant suites, which is the gap that let Deque ship without the no-op-Clear() row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an enumerator-invalidation edge case in Deque<T>.Clear() by ensuring that a no-op Clear() on an already-empty deque does not bump the internal version, aligning Deque with the established behavior across the rest of Celerity’s count-based collections. It also adds both dedicated regression/behavior tests and a new cross-collection invariant suite to prevent this contract from drifting again, plus accompanying documentation updates.
Changes:
- Move
Deque<T>.Clear()’s version bump behind an early-return for the empty case, so an empty no-op clear no longer invalidates enumerators. - Add focused
Dequeenumeration/clear tests and a new family-wideClearNoOpVersionTestssuite covering all count-based, version-tracked collections (with documented exceptions). - Update docs/roadmap/changelog to document the contract and the new invariant suite coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Celerity/Collections/Deque.cs | Fix Clear() so empty clears are true no-ops (no version bump). |
| src/Celerity.Tests/Collections/DequeEnumerationTests.cs | Add regression + behavioral tests for enumerator validity and slot clearing. |
| src/Celerity.Tests/Collections/ClearNoOpVersionTests.cs | Add cross-collection invariant suite for no-op Clear() version behavior. |
| ROADMAP.md | Record the parity/correctness item as done with rationale and exceptions. |
| docs/testing.md | Document the “family-wide invariant suites” testing category. |
| docs/api/collections.md | Document Deque<T>.Clear() no-op enumerator contract. |
| CONTRIBUTING.md | Add contributor guidance about adding new collections to cross-collection suites. |
| CHANGELOG.md | Add release notes for the fix and the new invariant suite. |
Coverage
|
- Deque.Clear()'s early-return comment now says "no observable state to reset": _head can be non-zero while _count is 0 (after a drain by popping), and the old wording read as if it could not. - The _version field comment no longer implies clear/trim always bump; both return early when there is nothing to do. - Tightened the two CHANGELOG bullets to observable behaviour, dropping the test-suite and guard-placement internals that belong in the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Celerity.Tests/Collections/DequeEnumerationTests.cs:175
- This test claims to verify that
Clear()releases references by reaching bothArray.Clearruns in a wrapped layout, but the assertions only check logical behavior (Count == 0,Contains/enumeration). Those would still pass even ifClear()forgot to clear the underlying slots, leaving stale references pinned for GC. Consider asserting the backing array contents are reset todefaultfor the occupied physical indices (e.g., via reflection on the private_items/_headfields) so the test actually pins the reference-release behavior it describes.
// Guards the early-out against being placed above the reference-releasing Array.Clear calls: a wrapped
// layout clears two runs, and both must still be reached for a non-empty deque.
var deque = new Deque<string>(4);
deque.PushBack("b");
deque.PushBack("c");
CHANGELOG.md:30
- These new CHANGELOG bullets read more like implementation notes than the brief, user-facing entries required by the CONTRIBUTING “Changelog entries” guidance (“a few sentences at most”, avoid private-field/implementation detail). Can these be condensed to the observable behavior change + the existence of the new family-wide test suite (keeping the rationale/details in the PR body)?
- `Deque<T>.Clear()` no longer invalidates active enumerators when the deque is already empty. It was the only count-based collection where a `Clear()` that removed nothing tore down live enumerators, so a defensive clear mid-enumeration threw. Clearing a populated deque still invalidates them, as before. Closes [#333](https://github.com/marius-bughiu/Celerity/issues/333).
- The no-op-`Clear()` contract is now pinned across the whole collection family by a new cross-collection test suite, so it cannot drift again. Closes [#333](https://github.com/marius-bughiu/Celerity/issues/333).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Celerity.Tests/Collections/DequeEnumerationTests.cs:170
Clear_ShouldReleaseEveryOccupiedSlot_WhenTheLayoutIsWrappedaims to guard thatClear()actually releases references viaArray.Clear, but the current assertions (Contains("a")/Assert.Empty(deque)) don't verify that: onceCountis 0,Containsand enumeration never inspect the underlying slots, so the test would still pass even if the backing array retained stale references. Consider asserting the backing array slots are cleared (for this setup, all slots should be null afterClear()).
Assert.Equal(0, deque.Count);
Assert.Equal(capacityBefore, deque.Capacity);
Assert.False(deque.Contains("a"));
Assert.Empty(deque);
The wrapped-layout test asserted only logical state (Count, Contains, enumeration), all of which still hold if Clear() skips the Array.Clear runs entirely - with _count at 0 no reader consults the slots, so a leaked reference is invisible through the public surface. The test's name promised more than it checked. It now reads the private _items array and asserts every physical slot is null. Verified to have teeth: commenting out the Array.Clear calls fails it with 3 of 4 slots non-null, where the logical assertions all still passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Round-2 review came back with no new inline comments but two findings suppressed as low confidence. Both read, both answered — noting them here since a suppressed finding has no thread to reply on. 1. This one was right and worth acting on. The test asserted It now reads the private Reflecting on a private field is a new precedent for this suite (the existing reflection in 2. The quoted snippet is the tightened version. As it now stands the first bullet is three sentences of observable behaviour and the second is one line noting the contract is pinned by a cross-collection suite — no private fields, no guard placement, no test-suite internals; those all live in this PR body. I read it as satisfying both the "few sentences at most" bar and the reason that rule exists ( |
Benchmarks8 regressions Highlights
Collections (508)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
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.
Closes #333.
The bug
Deque<T>.Clear()bumped_versionoutside the guard that skips the array clearing, so clearing an already-empty deque invalidated every live enumerator. It was the only count-based collection in the library that did this — the other 28 return early on an empty collection and never touch_version. It also contradictedDeque's own documented contract, which describes the enumerator guard as a check for a structural modification and separately notes that an indexer set does not invalidate.Narrow in practice (you have to
Clear()an already-empty deque mid-enumeration to see it), but it is exactly the kind of family-wide invariant that drifts silently when nothing pins it.The decision
Option A of the issue: align
Dequewith the rest of the family. The BCL is not a tiebreaker —Dictionary<K,V>.Clear()bumps only when non-empty, whileQueue<T>andStack<T>bump unconditionally, andDequeis the queue-shaped one. Celerity's own family points only one way, and the library is otherwise strict about it (FenwickTreedocuments a zero delta as a no-op,BTreeDictionarya rejected duplicateTryAdd,LruCachea hit on the already-MRU entry).The old code also reset
_head = 0unconditionally. That was pure normalization — with_count == 0no reader consults_head, and the next push writes relative to it either way — so skipping it changes nothing observable. A test covers the drained-by-popping shape where the head is parked mid-buffer to prove that.Clearing a populated deque still invalidates enumerators, exactly as before.
Parity rollout
Fix —
src/Celerity/Collections/Deque.cs: the bump moves inside the guard, which becomes an earlyreturn; the XML doc states the no-op contract.Dedicated tests —
DequeEnumerationTests.csgains three:Clear_ShouldNotInvalidateEnumerator_WhenTheDequeIsAlreadyEmpty— the regression, across all three ways a deque reaches empty (never populated, emptied by a priorClear(), drained by popping).Clear_ShouldInvalidateEnumerator_WhenTheDequeHeldElements— the positive control, so the fix cannot degenerate into "never bump".Clear_ShouldReleaseEveryOccupiedSlot_WhenTheLayoutIsWrapped— guards the early-out against being hoisted above the two reference-releasingArray.Clearruns.Cross-collection shared suite — new
ClearNoOpVersionTests.cs. The rule was previously pinned per-collection for a handful of types (EnumeratorInvalidationAndClearCoverageTests), which is how the outlier shipped. This asserts it once per collection for all 29 count-based version-tracking types — every hashed dictionary and set,BTreeDictionary/BTreeSet,EnumMap/EnumSet,Trie,SparseSet,DisjointSet,StringInternTable,CelerityMultiMap/CelerityMultiSet,LruCache,IndexedPriorityQueue,Deque— each driven through the three states that matter (never populated → populated → emptied by a precedingClear()). Enumerators are taken through the non-genericIEnumeratorso one helper covers every element shape in the family.It also pins the two deliberate exceptions:
BitSetandFenwickTreeare fixed-length, so establishing "already empty" means scanning every word — the same work as the unconditional clear it would be trying to skip. Both bump every time, they agree with each other, and the tests say so explicitly, so the behaviour reads as a decision rather than as the same oversight repeated. The five probabilistic sketches are out of scope: they track no version and expose no enumerator, so a redundantClear()has nothing to invalidate.Docs —
docs/api/collections.mdDeque<T>Clear()entry states the no-op contract (the surrounding sections already state the analogous rule forFenwickTreeandBTreeDictionary).docs/testing.mdgains a "family-wide invariant suites" bullet describing the new category and why it exists.CHANGELOG — two bullets under
[Unreleased]→Fixed, one for the behaviour fix and one for the family-wide suite.ROADMAP — rostered under milestone 2.4.0, "drop-in parity and correctness in the shipped surface", status
done, recording the Option A rationale and the two exceptions.Parity items that do not apply
Program.csregistration — no new type and no new public API; the change is one branch on an empty-Clear()path, which no benchmark measures and which cannot move a number.web/index.html,web/dev/bench/{index,detail}.html) — theCOLLECTIONSarrays and ship cards key off collections, andDequeis already listed in all three.README.md— itsDeque<T>entries (collections list, "Sequences" section, decision table) describe shape and performance, not per-method enumerator semantics; nothing there was made stale. Enumerator-invalidation detail belongs in the API reference, where it now lives.Test plan
dotnet build— 0 errors; the new files add no warnings.dotnet test— 5088 passed, 0 failed on each of net8.0 / net9.0 / net10.0, plus the three showcase projects (46 + 37 + 30), all green.Deque.cschange makesDequeClear_ShouldNotBumpTheVersion_WhenAlreadyEmptyandClear_ShouldNotInvalidateEnumerator_WhenTheDequeIsAlreadyEmptyfail while the two positive controls still pass.returnbranch is covered from both sides, so the branch gate should hold.mainneeds no new card, sinceDequeis already charted.🤖 Generated with Claude Code