Repository navigation
Conversation
The netstandard2.0 / netstandard2.1 / net6.0 builds reached the private fields of List<T> by reinterpreting the instance with Unsafe.As as view classes with an assumed field layout. The AsSpan shim read the backing array that way, and the AddRange / InsertRange(ReadOnlySpan<T>) polyfills wrote _items, _size and _version directly. If the layout ever differed, this would silently read or write the wrong memory of a live List<T>. Remove both shims and use only the public List<T> API where the real one is not available: - AlternateIndexList keeps using CollectionsMarshal.AsSpan on net6.0 and later (the API exists since .NET 5; the shim was only shadowing it). On netstandard it reads, modifies and writes back the IndexedValue struct through the indexer. - ObservableList.RemoveRange copies the removed range with List<T>.CopyTo through a new CloneCollection constructor, with the same range validation as Span<T>.Slice so the thrown exception is unchanged. - The filtered ObservableList view inserts a range through the IEnumerable<T> overload before .NET 8, as SynchronizedViewList already does. Add tests for the AlternateIndexList setters and for RemoveRange. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…with #if NETSTANDARD The fallbacks exist only because netstandard has no public API to access the backing array of List<T>. #if NETSTANDARD states that rule directly, matching the convention proposed in Cysharp#135. For the current target frameworks it selects the same code as NET6_0_OR_GREATER did. The NET8_0_OR_GREATER guard in ObservableList.Views.cs is left as is: List<T>.InsertRange(int, ReadOnlySpan<T>) is a .NET 8 API, so net6.0 still needs the fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit made the netstandard build of AlternateIndexList write modified elements back through the List<T> indexer, which increments the list version. A filtered synchronized view enumerates this list while holding a reentrant lock, so adding an element that the filter rejects, or replacing an accepted one, on the same thread during that enumeration started to throw InvalidOperationException on netstandard only. That path was also not exercised by the net6.0 test run. Replace the List<IndexedValue> with a private IndexedValueList that owns its backing array and returns elements by reference. Elements are now modified in place on every target framework, so there is a single code path and no #if. Like List<T>, its enumerator throws when the list is structurally changed or sorted, but not when an element is modified in place, which matches the behavior before this branch. Also add tests for the boundaries of RemoveRange, the storage operations of AlternateIndexList and modifications during enumeration, and clarify the comments and test summaries pointed out in review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IndexedValueList: - Enumerate with a struct enumerator that captures the version when it is created, like List<T>.Enumerator, instead of an iterator method that allocated on every enumeration and captured it on the first MoveNext. - Cap the growth at Array.MaxLength and skip clearing the array when the element type contains no references, as List<T> does. - Rename the size field so that the RemoveRange parameter no longer hides it, and make the list field readonly. - Explain why in-place modification does not invalidate enumeration, and note the lifetime of the references and spans it hands out. Also clarify why CloneCollection validates the range, why the view inserts through a copy before .NET 8, and that InsertIterator can be enumerated only once. Tests: - Make the enumeration test insert the rejected element at the front, so that the alternate indexes are actually shifted, and check them with a later replacement. - Add filtered view tests for InsertRange into the middle, RemoveRange and AddRange of the source. - Add AlternateIndexList tests for growth on a middle insertion, reuse after Clear, sparse RemoveRange, an empty list, Remove by value, RemoveRange of zero elements and a trivial sort during enumeration. - Fix test names and summaries, and assert exact exception types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Make the IndexedValueList comments accurate: a kept reference or span is stale after any insertion, removal or reordering, not only after growth; a replacement is non-structural only when the filter accepts both values; describe which operations change the version. - State the CloneCollection range contract as current behavior instead of history, and do not claim a single bulk copy for List<T>.InsertRange, which copies through a temporary array on .NET Framework and Mono. - Replace an element after each range operation in Filter_RangeOperations, so that a missing shift is detected; before, the shifts of InsertRange and RemoveRange cancelled each other out. - Compare by value in RemoveByValue with a string instance that is not the interned literal. - Align the remaining test summaries with what the tests assert. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The private IndexedValueList was a general-purpose growable list that happened to be nested in AlternateIndexList<T>, taking up about half of the file. Move it to Internal/, next to the other array-based helpers, as RefList<T>. The only IndexedValue-specific member, IndexOfValue, moves back to AlternateIndexList<T>, so IndexedValue stays private. No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
Static review found no blocking issues in the storage replacement or compatibility paths; tests were not executed.
No open issues 🎉
What changed in this PR
Removes reliance on List<T>’s private field layout, addressing #136 through owned storage and public collection APIs.
Changes:
- Introduces
RefList<T>for in-place updates inAlternateIndexList<T>. - Replaces unsafe range operations and removes obsolete shims and warning suppressions.
- Adds regression tests for range boundaries, filtered views, and enumeration behavior.
| File | Description |
|---|---|
| tests/ObservableCollections.Tests/ToNotifyCollectionChangedTest.cs | Tests filtered-view updates and range operations. |
| tests/ObservableCollections.Tests/ObservableListTest.cs | Tests removal notifications and range validation. |
| tests/ObservableCollections.Tests/AlternateIndexListTest.cs | Tests mutations, boundaries, growth, and enumeration. |
| src/ObservableCollections/SynchronizedViewList.cs | Removes obsolete warning suppressions. |
| src/ObservableCollections/Shims/CollectionsMarshalEx.cs | Deletes unsafe backing-array access shim. |
| src/ObservableCollections/Shims/Collections.cs | Removes unsafe span range extensions. |
| src/ObservableCollections/ObservableList.Views.cs | Adds public-API insertion fallback before .NET 8. |
| src/ObservableCollections/ObservableList.cs | Copies removed ranges through CloneCollection<T>. |
| src/ObservableCollections/Internal/RefList.cs | Adds owned storage with reference access and versioned enumeration. |
| src/ObservableCollections/Internal/CloneCollection.cs | Adds validated copying from a list range. |
| src/ObservableCollections/AlternateIndexList.cs | Migrates storage to RefList<T>. |
Review effort: Balanced
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 #136.
Problem
The
netstandard2.0/netstandard2.1/net6.0builds accessed the private fields ofList<T>by reinterpreting the instance withUnsafe.Asas view classes with an assumed field layout (ListView<T>/LegacyListView<T>).Shims/CollectionsMarshalEx.csread the backing array this way to emulateCollectionsMarshal.AsSpan.Shims/Collections.csemulatedAddRange/InsertRange(ReadOnlySpan<T>)by writing_items,_sizeand_versiondirectly.Unsafe.Asperforms no type check. If the layout ever differed from the assumption, the shims would read or write the wrong memory of a liveList<T>instead of failing cleanly.Approach
Both shims are removed. .NET 8 and later already used the real APIs and are not affected. The approach follows R3 (Cysharp/R3#395), ZLogger (Cysharp/ZLogger#248) and ZLinq (Cysharp/ZLinq#269): own the storage where the library needs in-place access, and use only the public
List<T>API elsewhere.AlternateIndexList<T>modifies itsIndexedValuestruct elements in place, which needs access to the backing array. It now stores them in a new internalRefList<T>(Internal/RefList.cs) that owns its array and returns elements by reference, instead of aList<IndexedValue>. This is the same code on every target framework, with no#if. It followsList<T>for the operations it implements (BinarySearchandSortuse the sameArraymethods asList<T>). Its enumerator throws when the list is structurally changed or sorted, but not when an element is modified in place. This matches the behavior before this PR.ObservableList<T>.RemoveRange: the removed range is copied into the pooled buffer withList<T>.CopyTo(int, T[], int, int)through a newCloneCollection<T>constructor. This is the same code on every target framework. The constructor validates the range the same way asSpan<T>.Slice, so an invalid range still throwsArgumentOutOfRangeException.ObservableList<T>view: before .NET 8, a range is now inserted through theIEnumerable<T>overload ofList<T>.InsertRange, viaCloneCollection<T>.SynchronizedViewListalready handles the same case this way.The
CS0436suppressions are also removed. They were needed only because the shim redeclaredSystem.Runtime.InteropServices.CollectionsMarshalonnet6.0, where the real type exists.There are no public API changes. The only observable difference is that the setter of the public
AlternateIndexList<T>indexer now throwsArgumentOutOfRangeExceptionfor a position outside the list, the same as the getter. Before, it threwIndexOutOfRangeExceptionfrom the span. The library itself never passes such a position.Note
This touches the same shim files as #135, which changes their
#ifguards to#if NETSTANDARD. Whichever PR is merged second needs a small rebase; this PR deletes the files, so the resolution is to keep them deleted.🤖 Generated with Claude Code