fix: throw when an array-backed container is modified during enumeration [patch] - #84
Merged
Merged
Conversation
…ion [patch] ContiguousCollection, ContiguousSet, ContiguousMap, OrderedMap and InsertionOrderMap enumerated with a bare index loop and no version counter. Removing or adding during a foreach silently skipped or repeated elements instead of throwing InvalidOperationException, unlike their List-backed siblings. Each now keeps a version, bumped by every add, insert, remove, clear and indexer set. Their enumerators, including the maps' Keys and Values, capture it when GetEnumerator() is called and throw on the next MoveNext once it has changed. A set Add of a duplicate, or a Remove of a missing item, changes nothing and does not invalidate enumeration. RingBuffer is left as it is: it is written to in real time, and the issue marks it optional. Fixes #70 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PtWVWaDh3nqJL1eKD7vFxU
Each enumerator is now a one-line GetEnumerator that passes the version to a private iterator, rather than a wrapper around a local function. That removes the repeated block Sonar counted as duplicated new code. The map enumerators check the version before reading the entry, and the three identical map test sets now call one shared assertion helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PtWVWaDh3nqJL1eKD7vFxU
The three maps each carried their own versioned pair, key and value iterators. The code was identical and sat inside runs the maps already shared, so Sonar counted it as duplicated new code. Each map now exposes Version, KeyAt and ValueAt through an internal IVersionedEntries interface, and Enumeration.Pairs/Keys/Values do the iterating once. The version is still captured when GetEnumerator() is called. ContiguousMap.Clear bumps the version first rather than mid-method, so the bump does not extend the run it shares with InsertionOrderMap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PtWVWaDh3nqJL1eKD7vFxU
|
Contributor
Author
|
The SonarCloud quality gate passes now. Duplication on new code dropped from 10.1% to 0.4% after e2a87d9, which moves the versioned map enumeration into Sonar still lists 2 new issues that don't block the gate. The Sonar project is private, so I can't see which rules they are. If someone with access pastes the rule IDs and lines here, I'll fix them on this branch. Generated by Claude Code |
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
2 participants
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 #70
What was wrong
ContiguousCollection,ContiguousSet,ContiguousMap,OrderedMapandInsertionOrderMapenumerated with a barefor (i < Count) yield return items[i]and kept no version counter. Changing the collection inside aforeachnever threw. Elements were silently skipped or visited twice. TheList<T>-backed siblings do throw, so switching to the faster contiguous type changed a loud failure into wrong data.Change
int version. It is bumped by every structural or value change:RemoveAtversionwhenGetEnumerator()is called, asList<T>does, and checks it on eachMoveNext, including the one after the last element. This covers the main enumerator and the maps'Keys/Valuesviews. A change throwsInvalidOperationExceptionwith the BCL message.Enumeration.ThrowIfModified.ContiguousSet.Addof a duplicate, or aRemoveof a missing item or key. SoUnionWith(this)still works.Clone()andGetRange()don't bump, since they build a fresh instance.RingBufferis unchanged. It is written to in real time, and the issue marks it optional.Tests
New tests for each affected type:
foreachthrowsKeysandValuesenumerators are all checkedMoveNextor after the last element throws (ContiguousCollection)Results:
dotnet test).dotnet build Containerssucceeds for net10.0, net9.0, net8.0, netstandard2.1 and netstandard2.0.🤖 Generated with Claude Code
https://claude.ai/code/session_01PtWVWaDh3nqJL1eKD7vFxU
Generated by Claude Code