Skip to content

Use ICollection<T> check instead of IList<T> in TryCopyTo #125304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jkotas merged 15 commits into from
Oct 19, 2026
Merged

Use ICollection<T> check instead of IList<T> in TryCopyTo #125304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
dcdc850
Use ICollection<T> check instead of IList<T> in TryCopyTo
prozolic Mar 8, 2026
4754015
Add test data in ImmutableArrayTest.InsertRangeData
prozolic Mar 8, 2026
bc35c73
Update src/libraries/System.Collections.Immutable/src/System/Collecti…
prozolic Mar 8, 2026
4dff47d
Fix comment
prozolic Mar 8, 2026
a088eaa
Fix comment
prozolic Apr 10, 2026
4eb1db5
Store the result of ICollection<T> cast to avoid redoing
prozolic Apr 11, 2026
99cdff3
Merge branch 'main' into TryCopyTo
prozolic Jun 6, 2026
d11bef9
Merge branch 'main' into TryCopyTo
prozolic Jun 19, 2026
70fdd50
Merge branch 'main' into TryCopyTo
prozolic Jul 19, 2026
31068fd
Merge branch 'main' into TryCopyTo
prozolic Aug 13, 2026
7150038
Merge branch 'main' into TryCopyTo
prozolic Sep 14, 2026
0995f47
Merge branch 'main' into TryCopyTo
prozolic Oct 17, 2026
a3532ce
Simplify TryCopyTo to use ICollection<T>.CopyTo
prozolic Oct 18, 2026
b8c3309
Remove List<T> fast path, fix TryCopyTo comment
prozolic Oct 18, 2026
1540cd2
Check is Array before GetType in TryCopyTo on .NET Framework
prozolic Oct 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 10 additions & 28 deletions ...tem.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,52 +80,34 @@ internal static int GetCount<T>(ref IEnumerable<T> sequence)

/// <summary>
/// Tries to copy the elements in the sequence to the specified array,
/// if the sequence is a well-known collection type. Otherwise, does
/// if the sequence implements <see cref="ICollection{T}"/>. Otherwise, does
/// nothing and returns <c>false</c>.
/// </summary>
/// <typeparam name="T">The type of element in the sequence.</typeparam>
/// <param name="sequence">The sequence to copy.</param>
/// <param name="array">The array to copy the elements to.</param>
/// <param name="arrayIndex">The index in the array to start copying.</param>
/// <returns><c>true</c> if the elements were successfully copied; <c>false</c> otherwise.</returns>
/// <remarks>
/// <para>
/// The reason we don't copy anything other than for well-known types is that a malicious interface
/// implementation of <see cref="ICollection{T}"/> could hold on to the array when its <see cref="ICollection{T}.CopyTo"/>
/// method is called. If the array it holds onto underlies an <see cref="ImmutableArray{T}"/>, it could violate
/// immutability by modifying the array.
/// </para>
/// </remarks>
internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int arrayIndex)
{
Debug.Assert(sequence != null);
Debug.Assert(array != null);
Debug.Assert(arrayIndex >= 0 && arrayIndex <= array.Length);

// IList is the GCD of what the following types implement.
if (sequence is IList<T>)
if (sequence is ICollection<T> collection)
{
if (sequence is List<T> list)
{
list.CopyTo(array, arrayIndex);
return true;
}

// Array.Copy can throw an ArrayTypeMismatchException if the underlying type of
// the destination array is not typeof(T[]), but is assignment-compatible with T[].
#if !NET
// On .NET Framework, an array's ICollection<T>.CopyTo calls Array.Copy, which throws ArrayTypeMismatchException for some arrays whose element type is not T
// (for example, a uint[] passed as IEnumerable<int>), so conservatively fall back to enumeration for any array that is not exactly a T[].
// See https://github.com/dotnet/runtime/issues/14794 for more info.
if (sequence.GetType() == typeof(T[]))
if (collection is Array && collection.GetType() != typeof(T[]))
{
var sourceArray = (T[])sequence;
Array.Copy(sourceArray, 0, array, arrayIndex, sourceArray.Length);
return true;
return false;
}
#endif

if (sequence is ImmutableArray<T> immutable)
{
Array.Copy(immutable.array!, 0, array, arrayIndex, immutable.Length);
return true;
}
collection.CopyTo(array, arrayIndex);
Comment thread
prozolic marked this conversation as resolved.
Resolved
return true;
}

return false;
Expand Down
41 changes: 41 additions & 0 deletions src/libraries/System.Collections.Immutable/tests/CopyToTrackingCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;

namespace System.Collections.Immutable.Tests
{
internal sealed class CopyToTrackingCollection<T> : ICollection<T>
{
private readonly T[] _items;

public CopyToTrackingCollection(params T[] items)
{
_items = items;
}

public int CopyToCallCount { get; private set; }

public int Count => _items.Length;

public bool IsReadOnly => true;

public void CopyTo(T[] array, int arrayIndex)
{
CopyToCallCount++;
Array.Copy(_items, 0, array, arrayIndex, _items.Length);
}

public IEnumerator<T> GetEnumerator() => throw new ShouldNotBeInvokedException();

IEnumerator IEnumerable.GetEnumerator() => throw new ShouldNotBeInvokedException();

public bool Contains(T item) => throw new ShouldNotBeInvokedException();

public void Add(T item) => throw new NotSupportedException();

public void Clear() => throw new NotSupportedException();

public bool Remove(T item) => throw new NotSupportedException();
}
}
29 changes: 29 additions & 0 deletions src/libraries/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,19 @@ public void AddRangeIEnumerable()
Assert.Equal(Enumerable.Range(1, 4), builder);
}

[Fact]
public void AddRangeICollectionUsesCopyTo()
{
var builder = ImmutableArray.CreateBuilder<int>(1);
builder.Add(1);

var items = new CopyToTrackingCollection<int>(2, 3);
builder.AddRange(items); // exceeds capacity

Assert.Equal(1, items.CopyToCallCount);
Assert.Equal(new[] { 1, 2, 3 }, builder);
}

[Fact]
public void Add()
{
Expand Down Expand Up @@ -332,6 +345,22 @@ public void InsertRange()
AssertExtensions.Throws<ArgumentOutOfRangeException>("index", () => builder.InsertRange(100, new ImmutableArray<int>()));
}

[Theory]
[InlineData(0)]
[InlineData(1)]
[InlineData(2)]
public void InsertRangeICollectionUsesCopyTo(int index)
{
var builder = ImmutableArray.CreateBuilder<int>();
builder.AddRange(1, 2);

var items = new CopyToTrackingCollection<int>(5, 6);
builder.InsertRange(index, items);

Assert.Equal(1, items.CopyToCallCount);
Assert.Equal(new[] { 1, 2 }.Take(index).Concat(new[] { 5, 6 }).Concat(new[] { 1, 2 }.Skip(index)), builder);
}

[Fact]
public void Remove()
{
Expand Down
27 changes: 27 additions & 0 deletions src/libraries/System.Collections.Immutable/tests/ImmutableArrayTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,18 @@ public void CreateRange(IEnumerable<int> source)
Assert.Equal(source, ImmutableArray.CreateRange(source));
}

[Fact]
public void CreateRangeICollectionUsesCopyTo()
{
var items = new CopyToTrackingCollection<int>(1, 2, 3);

Assert.Equal(new[] { 1, 2, 3 }, ImmutableArray.CreateRange(items));
Assert.Equal(1, items.CopyToCallCount);

Assert.Equal(new[] { 1, 2, 3 }, items.ToImmutableArray());
Assert.Equal(2, items.CopyToCallCount);
}

[Fact]
public void CreateRangeInvalid()
{
Expand Down Expand Up @@ -1458,6 +1470,21 @@ public static IEnumerable<object[]> InsertRangeData()
yield return new object[] { s_manyElements, 3, new uint[] { 4, 5, 6 } };
}

[Theory]
[InlineData(0)]
[InlineData(1)]
[InlineData(3)]
public void InsertRangeICollectionUsesCopyTo(int index)
{
ImmutableArray<int> immutableArray = s_manyElements;
var items = new CopyToTrackingCollection<int>(7, 8);

ImmutableArray<int> result = immutableArray.InsertRange(index, items);

Assert.Equal(1, items.CopyToCallCount);
Assert.Equal(s_manyElements.Take(index).Concat(new[] { 7, 8 }).Concat(s_manyElements.Skip(index)), result);
}

[Theory]
[MemberData(nameof(RemoveAtData))]
public void RemoveAt(IEnumerable<int> source, int index)
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Collections.Immutable/tests/System.Collections.Immutable.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<Compile Include="..\src\Validation\Requires.cs" Link="Validation\Requires.cs" />
<Compile Include="BadHasher.cs" />
<Compile Include="BinaryTreeProxy.cs" />
<Compile Include="CopyToTrackingCollection.cs" />
<Compile Include="EverythingEqual.cs" />
<Compile Include="Frozen\FrozenFromKnownValuesTests.cs" />
<Compile Include="Frozen\KeyAnalyzerTests.cs" />
Expand Down
Loading