Repository navigation
Implement CopyTo on synchronized view lists - #127
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes CopyTo behavior for synchronized view lists so they can participate in the ICollection<T>.CopyTo fast path used by Enumerable.ToList(), addressing the ToViewList().ToList() exception reported in #116.
Changes:
- Introduces
CopyTo(Span<TView>)onNotifyCollectionChangedSynchronizedViewList<TView>and wires bothICollection<T>.CopyToandICollection.CopyToto it with argument validation. - Implements/overrides
CopyTo(Span<...>)in synchronized view list implementations. - Adds
ObservableList<T>.CopyTo(Span<T>)and comprehensive unit tests forCopyToonObservableList<T>and synchronized view lists.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ObservableCollections.Tests/CopyToTest.cs | Adds coverage validating CopyTo semantics across ObservableList and synchronized view lists. |
| src/ObservableCollections/SynchronizedViewList.cs | Implements CopyTo(Span<TView>) overrides on synchronized view lists. |
| src/ObservableCollections/ObservableList.OptimizeView.cs | Delegates optimized view CopyTo(Span<T>) to the parent list. |
| src/ObservableCollections/ObservableList.cs | Adds CopyTo(Span<T>) and refactors array CopyTo to use spans. |
| src/ObservableCollections/ObservableDictionary.Views.cs | Cleans up usings/formatting. |
| src/ObservableCollections/IObservableCollection.cs | Adds CopyTo(Span<TView>) and implements generic/non-generic CopyTo for view lists. |
Suppressed comments (1)
src/ObservableCollections/IObservableCollection.cs:223
ICollection.CopyTo(Array, int)doesn't validate thatarrayIndexis within the array's upper bound / that there is sufficient capacity before copying. In particular, if the collection is empty and the target is notTView[](e.g.,object[]as used by WPF),arrayIndex > array.Lengthwill currently succeed silently because theforeachnever executes.
if (array is TView[] typedArray)
{
CopyTo(typedArray.AsSpan(arrayIndex));
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public void CopyTo(T[] array, int arrayIndex) | ||
| { | ||
| if (array is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(array)); | ||
| } | ||
|
|
||
| CopyTo(array.AsSpan(arrayIndex)); | ||
| } |
| void ICollection<TView>.CopyTo(TView[] array, int arrayIndex) | ||
| { | ||
| if (array is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(array)); | ||
| } | ||
|
|
||
| CopyTo(array.AsSpan(arrayIndex)); | ||
| } |
| /// <summary> | ||
| /// Ensures ArgumentOutOfRangeException is thrown when arrayIndex is negative. | ||
| /// </summary> | ||
| [Fact] | ||
| public void NegativeArrayIndexThrows() | ||
| { | ||
| Action act = () => CreateSource().CopyTo(new int[3], -1); | ||
|
|
||
| act.Should().ThrowExactly<ArgumentOutOfRangeException>(); | ||
| } | ||
| } |
|
I'm going to close and immediately reopen this PR to trigger PR Harness again. The goal is to verify the fix for Cysharp/Actions#173 (Cysharp/Actions#175) in a real run. |
|
The re-triggered run passed |
…idate arrayIndex Cysharp#139 removed the Unsafe.As-based CollectionsMarshal shim, so CopyTo(Span) of ObservableList<T> and NonFilteredSynchronizedViewList now uses the real CollectionsMarshal.AsSpan only on NET8_0_OR_GREATER and copies element by element on netstandard. CopyTo(T[], int) of ObservableList<T> and both ICollection.CopyTo of the synchronized view lists now reject an arrayIndex beyond the end of the array with ArgumentException, as List<T>.CopyTo does, instead of an ArgumentOutOfRangeException from AsSpan or, for an empty collection copied into object[], no exception at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fix ICollection.CopyTo on synchronized view lists (fixes #116)
NotifyCollectionChangedSynchronizedViewList<TView>threwNotSupportedExceptionfrom bothCopyToimplementations.Enumerable.ToList()takes theICollection<T>fast path, soToViewList().ToList()always threw on non-empty lists.Adds
public virtual void CopyTo(Span<TView> span), overridden by the three derived types, and implements both interface methods on top of it with the documented argument validation.ObservableList<T>gainsCopyTo(Span<T>).