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

[prozolic]

#88181

This PR change the type check from IList to ICollection. With this change, collections that implement ICollection will have ICollection.CopyTo invoked.

• Change the type check from IList<T> to ICollection<T>
• If the type does not match List<T>, T[], or ImmutableArray<T>, fall back to ICollection<T>.CopyTo
• Add conditional compilation( #if NET) to ensure the sequence is not an array before falling back to ICollection<T>.CopyTo to avoid issues with array covariance in .NET Framework

Benchmark

#88181 (comment)

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR changes ImmutableExtensions.TryCopyTo to use ICollection<T> as the type gate instead of IList<T>. This allows collections that implement ICollection<T> but not IList<T> (such as Dictionary<K,V>.Keys and Dictionary<K,V>.Values) to benefit from the fast CopyTo path rather than falling back to per-element enumeration. The performance motivation is that LINQ's own ToArray already takes this ICollection<T> path, and the previous restriction was making immutable collection operations unnecessarily slower.

Changes:

• Changes the outer type check from IList<T> to ICollection<T>, broadening the fast-copy path to any ICollection<T> implementation.
• After the well-known-type fast paths (List<T>, exact T[], ImmutableArray<T>), falls through to a generic ICollection<T>.CopyTo call for all other implementors.
• Adds a #if !NET guard to skip arrays that are not exactly T[] (covariant arrays) when running on .NET Framework, where ICollection<T>.CopyTo on such arrays can throw ArrayTypeMismatchException.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

---

You can also share your feedback on Copilot code review. Take the survey.

[stephentoub]

https://github.com/dotnet/runtime/pull/125304/changes#diff-79ebb1b84d82a6117697d669c596bdcb999803d9ecf5a5339d6431d1ac0c5f7eR93-R96 suggests the lack of this is on purpose

[prozolic]

Thanks for the pointer. You're right that the original IList restriction was intentional.
The implementation approach follows what was discussed and considered in #88181 (#88093 (comment)).
@adamsitnik also confirmed in #88181 that the benchmark results look good.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[tannergooding]

Are all these secondary type checks (where it has to enumerate through the types/interfaces to see if it is that type) better than just doing collection.CopyTo(...)?

I'd expect the virtual dispatch overhead, especially with DPGO/guarded-devirtualization to be cheaper and that we could get this down to basically just the type check for ICollection and one more for the array special handling for the common path. Rather than needing up to 4-5.

[prozolic]

Thanks for the suggestion. I think that simplifying to the array special handling and ICollection<T>.CopyTo is a reasonable approach for .NET, where DPGO and guarded devirtualization can optimize the virtual dispatch.
How about something like this?

        internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int arrayIndex)
        {
            if (sequence is ICollection<T> collection)
            {
#if !NET
                if (collection.GetType() != typeof(T[]) && collection is Array)
                {
                    return false;
                }
#endif
                collection.CopyTo(array, arrayIndex);
                return true;
            }

            return false;
        }

My concern is whether this could regress performance on .NET Framework, where those optimizations are not available. If that's an issue, a #if NET split to preserve the explicit type checks on .NET Framework might be worth considering.
I'd like to hear others' thoughts on both the simplified approach itself and whether a #if NET split would be needed.

[jkotas]

Microbenchmark numbers are the best way to decide questions like this.

[prozolic]

The benchmark results are as follows. On net472, the simplified version regresses only for List<T>.
On net11, I think that there is no regression for any collection type.

• net472

Method	input	Mean	Error	StdDev	Median	Min	Max	Allocated
PR	Array	48.788 ns	0.7753 ns	0.7252 ns	48.442 ns	48.154 ns	50.912 ns	-
PR_Simplified	Array	49.233 ns	1.9642 ns	2.2620 ns	48.398 ns	48.169 ns	57.831 ns	-
PR	DictValues	91.992 ns	3.7286 ns	4.2939 ns	90.405 ns	89.813 ns	106.749 ns	-
PR_Simplified	DictValues	91.521 ns	2.7121 ns	3.1232 ns	90.104 ns	89.858 ns	102.573 ns	-
PR	HashSet	106.052 ns	0.6292 ns	0.5886 ns	105.884 ns	105.413 ns	107.558 ns	-
PR_Simplified	HashSet	105.879 ns	1.5209 ns	1.4227 ns	105.522 ns	105.163 ns	110.951 ns	-
PR	ICollection	24.860 ns	1.1025 ns	1.2696 ns	24.386 ns	24.270 ns	29.677 ns	-
PR_Simplified	ICollection	24.277 ns	0.1839 ns	0.1720 ns	24.216 ns	23.970 ns	24.605 ns	-
PR	IEnumerable	5.787 ns	0.6049 ns	0.6966 ns	5.521 ns	5.476 ns	8.122 ns	-
PR_Simplified	IEnumerable	4.865 ns	0.0238 ns	0.0223 ns	4.858 ns	4.830 ns	4.912 ns	-
PR	IList	25.111 ns	0.6291 ns	0.7245 ns	24.879 ns	24.662 ns	27.545 ns	-
PR_Simplified	IList	24.636 ns	0.4881 ns	0.4566 ns	24.504 ns	24.384 ns	26.241 ns	-
PR	ImmutableArray	20.858 ns	2.1911 ns	2.5233 ns	19.807 ns	19.734 ns	29.216 ns	-
PR_Simplified	ImmutableArray	22.977 ns	0.1145 ns	0.1071 ns	22.964 ns	22.781 ns	23.206 ns	-
PR	List	13.092 ns	1.1985 ns	1.3803 ns	12.505 ns	11.902 ns	16.128 ns	-
PR_Simplified	List	19.011 ns	0.0879 ns	0.0823 ns	18.986 ns	18.901 ns	19.233 ns	-

• net11

Method	input	Mean	Error	StdDev	Median	Min	Max	Allocated
PR	Array	6.1776 ns	0.1712 ns	0.1971 ns	6.1610 ns	5.8437 ns	6.4629 ns	-
PR_Simplified	Array	6.6889 ns	0.6831 ns	0.7867 ns	6.3118 ns	6.2194 ns	9.2689 ns	-
PR	DictValues	56.8282 ns	6.2581 ns	7.2069 ns	53.3827 ns	51.5016 ns	74.8816 ns	-
PR_Simplified	DictValues	51.9682 ns	0.1728 ns	0.1616 ns	51.9340 ns	51.7708 ns	52.4342 ns	-
PR	HashSet	86.3227 ns	0.8258 ns	0.7725 ns	86.0793 ns	85.6178 ns	88.7023 ns	-
PR_Simplified	HashSet	79.2293 ns	0.8654 ns	0.8095 ns	79.0607 ns	78.5970 ns	81.9988 ns	-
PR	ICollection	8.1243 ns	0.7820 ns	0.9005 ns	7.8809 ns	7.5083 ns	11.5654 ns	-
PR_Simplified	ICollection	6.2598 ns	0.1683 ns	0.1938 ns	6.2204 ns	5.9885 ns	6.6185 ns	-
PR	IEnumerable	1.0592 ns	0.0048 ns	0.0045 ns	1.0592 ns	1.0519 ns	1.0665 ns	-
PR_Simplified	IEnumerable	0.8723 ns	0.0056 ns	0.0052 ns	0.8708 ns	0.8667 ns	0.8830 ns	-
PR	IList	8.6967 ns	0.9371 ns	1.0792 ns	8.2324 ns	7.9107 ns	12.1146 ns	-
PR_Simplified	IList	6.3230 ns	0.1689 ns	0.1946 ns	6.3095 ns	5.9923 ns	6.7738 ns	-
PR	ImmutableArray	6.8573 ns	0.7276 ns	0.8379 ns	6.5145 ns	6.3095 ns	9.6131 ns	-
PR_Simplified	ImmutableArray	6.2418 ns	0.6434 ns	0.7410 ns	6.0347 ns	5.6379 ns	8.4593 ns	-
PR	List	6.9636 ns	1.0816 ns	1.2456 ns	6.2800 ns	5.7477 ns	9.2334 ns	-
PR_Simplified	List	6.7189 ns	1.2294 ns	1.4158 ns	5.9877 ns	5.3658 ns	9.1378 ns	-

Benchmark source

    [MemoryDiagnoser]
    [BenchmarkCategory(Categories.Libraries, Categories.Collections)]
    public class Perf_TryCopyTo
    {
        private int[] _dest;

        public IEnumerable<object> Arguments()
        {
            yield return TestData.IEnumerable;
            yield return TestData.ICollection;
            yield return TestData.IList;
            yield return TestData.Array;
            yield return TestData.List;
            yield return TestData.ImmutableArray;
            yield return TestData.DictValues;
            yield return TestData.HashSet;
        }

        [GlobalSetup]
        public void Setup()
        {
            _dest = new int[100];
        }

        [Benchmark] 
        [ArgumentsSource(nameof(Arguments))]
        [MemoryRandomization]
        public bool PR(TestData input) => TryCopyTo(input.Collection, _dest, 0);

        [Benchmark]
        [ArgumentsSource(nameof(Arguments))]
        [MemoryRandomization]
        public bool PR_Simplified(TestData input) => TryCopyTo_Simplified(input.Collection, _dest, 0);

        static bool TryCopyTo<T>(IEnumerable<T> sequence, T[] array, int arrayIndex)
        {
            if (sequence is ICollection<T> collection)
            {
                if (collection is List<T> list)
                {
                    list.CopyTo(array, arrayIndex);
                    return true;
                }

                if (collection.GetType() == typeof(T[]))
                {
                    var sourceArray = (T[])collection;
                    Array.Copy(sourceArray, 0, array, arrayIndex, sourceArray.Length);
                    return true;
                }

                if (collection is ImmutableArray<T> immutable)
                {
                    immutable.CopyTo(array, arrayIndex);
                    return true;
                }

                if (collection is Array)
                {
                    return false;
                }

                collection.CopyTo(array, arrayIndex);
                return true;
            }
            return false;
        }

        static bool TryCopyTo_Simplified<T>(IEnumerable<T> sequence, T[] array, int arrayIndex)
        {
            if (sequence is ICollection<T> collection)
            {
#if !NET
                if (collection.GetType() != typeof(T[]) && collection is Array)
                {
                    return false;
                }
#endif
                collection.CopyTo(array, arrayIndex);
                return true;
            }
            return false;
        }
    }

    public class TestData
    {
        internal const int Size = 100;

        private static readonly int[] array = Enumerable.Range(0, Size).ToArray();
        internal static readonly TestData Array = new(array);
        internal static readonly TestData List = new(new List<int>(array));
        internal static readonly TestData ImmutableArray = new(Collections.Immutable.ImmutableArray.Create(array));
        internal static readonly TestData DictValues = new(array.ToDictionary(x => x, x => x).Values);
        internal static readonly TestData HashSet = new(new HashSet<int>(array));
        internal static readonly TestData IEnumerable = new(ToEnumerable(Size));
        internal static readonly TestData IList = new(new IListWrapper<int>(array));
        internal static readonly TestData ICollection = new(new ICollectionWrapper<int>(array));

        private TestData(IEnumerable<int> collection) => Collection = collection;

        internal IEnumerable<int> Collection { get; }

        private static IEnumerable<int> ToEnumerable(int size)
        {
            for (int i = 0; i < size; i++)
            {
                yield return i;
            }
        }

        public override string ToString()
        {
            switch (Collection)
            {
                case int[] _:
                    return "Array";
                case List<int> _:
                    return "List";
                case ImmutableArray<int> _:
                    return "ImmutableArray";
                case Dictionary<int, int>.ValueCollection _:
                    return "DictValues";
                case HashSet<int> _:
                    return "HashSet";
                case IList<int> _:
                    return "IList";
                case ICollection<int> _:
                    return "ICollection";
                default:
                    return "IEnumerable";
            }
        }

        private class ICollectionWrapper<T> : ICollection<T>
        {
            private readonly T[] _array;
            public ICollectionWrapper(T[] array) => _array = array;

            public IEnumerator<T> GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();
            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();

            public int Count => _array.Length;
            public bool IsReadOnly => true;
            public bool Contains(T item) => System.Array.IndexOf(_array, item) >= 0;
            public void CopyTo(T[] array, int arrayIndex) => _array.CopyTo(array, arrayIndex);

            public void Add(T item) => throw new NotImplementedException();
            public void Clear() => throw new NotImplementedException();
            public bool Remove(T item) => throw new NotImplementedException();
        }

        private class IListWrapper<T> : IList<T>
        {
            private readonly T[] _array;
            public IListWrapper(T[] array) => _array = array;

            public IEnumerator<T> GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();
            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => ((IEnumerable<T>)_array).GetEnumerator();

            public int Count => _array.Length;
            public bool IsReadOnly => true;
            public T this[int index]
            {
                get { return _array[index]; }
                set { throw new NotImplementedException(); }
            }
            public bool Contains(T item) => System.Array.IndexOf(_array, item) >= 0;
            public void CopyTo(T[] array, int arrayIndex) => _array.CopyTo(array, arrayIndex);
            public int IndexOf(T item) => System.Array.IndexOf(_array, item);

            public void Add(T item) => throw new NotImplementedException();
            public void Clear() => throw new NotImplementedException();
            public bool Remove(T item) => throw new NotImplementedException();
            public void Insert(int index, T item) => throw new NotImplementedException();
            public void RemoveAt(int index) => throw new NotImplementedException();
        }
    }

[prozolic]

Hi @tannergooding, it's been a while since I posted the benchmark results.

As shown above, the simplified approach only regresses for List<T> on net472 in the benchmark. So, how about an implementation that performs T[]/List<T> special processing only under #if !NET?

        internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int arrayIndex)
        {

            // ICollection<T> implementations use the generic ICollection<T>.CopyTo.
            if (sequence is ICollection<T> collection)
            {
#if !NET
                if (collection 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[].
                // See https://github.com/dotnet/runtime/issues/14794 for more info.
                if (collection.GetType() != typeof(T[]) && collection is Array)
                {
                    return false;
                }
#endif

                collection.CopyTo(array, arrayIndex);
                return true;
            }

            return false;
        }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "70fdd50c94647a893048b4a782c1a540b5bb071f",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "f43516f45fff5920eabd11753bd5f1803cfacd0c",
  "last_reviewed_commit": "70fdd50c94647a893048b4a782c1a540b5bb071f",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "f43516f45fff5920eabd11753bd5f1803cfacd0c",
  "last_recorded_worker_run_id": "29707107730",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "d11bef921e399b9483e96f6772893d3d15936990",
      "review_id": 4730524832
    },
    {
      "commit": "70fdd50c94647a893048b4a782c1a540b5bb071f",
      "review_id": 4731654663
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The PR addresses a real gap (issue #88181): TryCopyTo only took the fast CopyTo path for IList<T>, so ICollection<T>-only sequences like Dictionary<K,V>.Keys/Values fell back to per-element enumeration even though LINQ's own ToArray already uses the ICollection<T> path. The performance motivation is legitimate.

Approach: Broadening the gate to ICollection<T> and caching the cast is reasonable, and the #if !NET covariant-array guard correctly avoids ArrayTypeMismatchException on .NET Framework. However, the generic collection.CopyTo(array, arrayIndex) fallback for arbitrary implementers directly contradicts the security rationale documented in the method's own <remarks>.

Summary: ⚠️ Needs Human Review. The change is functionally correct for benign collections and well tested for Dictionary.Keys, but it reintroduces the untrusted-CopyTo hazard the method was explicitly written to avoid, and the code now conflicts with its own documentation. A maintainer should decide whether that threat model still applies before merging. See the inline comment for the security concern.

---

Detailed Findings

⚠️ Security / Documentation contradiction — untrusted ICollection<T>.CopyTo fallback

See the inline comment on ImmutableExtensions.Minimal.cs line 140. The new fallback hands the destination array (which becomes the backing store of a returned ImmutableArray<T> in the ToArray and InsertRange callers) to an arbitrary user-supplied CopyTo, exactly the immutability-violation scenario the <remarks> on lines 91-98 says the well-known-type gate exists to prevent. Either the rationale should be updated with justification, or the fallback should be restricted to trusted types.

💡 Count consistency of the fallback path

The builder callers (ImmutableArray_1.Builder.cs lines 277/285 and 343/347) pre-size using GetCount/TryGetCount and then rely on collection.CopyTo copying exactly Count elements. This holds for framework collections, but a custom ICollection<T> whose Count disagrees with the number of elements its CopyTo writes could corrupt _count. This is a corollary of the trust concern above rather than a separate bug for well-behaved types.

✅ .NET Framework covariant-array guard

The #if !NET block correctly returns false for Array instances that are not exactly T[], preserving the enumeration fallback and avoiding ArrayTypeMismatchException from covariant arrays on .NET Framework. The exact-T[] fast path above still handles the common case on all targets.

✅ Test coverage

The two new InsertRangeData rows exercise Dictionary<int,int>.Keys (an ICollection<T> that is not IList<T>) at index 0 and a non-zero index, covering the newly enabled path. Reusing the existing [Theory] is consistent with repo conventions. A malicious/mismatched-Count ICollection<T> test would be worth adding if the fallback is kept.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 80.5 AIC · ⌖ 11.1 AIC · ⊞ 10K

[github-actions]

This fallback appears to reintroduce the exact hazard the method's own <remarks> (lines 91-98) documents. The remark states copying is restricted to well-known types because "a malicious interface implementation of ICollection<T> could hold on to the array when its ICollection<T>.CopyTo method is called. If the array it holds onto underlies an ImmutableArray<T>, it could violate immutability by modifying the array."

Calling collection.CopyTo(array, arrayIndex) on an arbitrary user-supplied ICollection<T> hands out a reference to array, and for the ToArray (line 169) and ImmutableArray<T>.InsertRange (ImmutableArray_1.cs line 363/380) callers that array becomes the backing store of a returned ImmutableArray<T>. A hostile CopyTo could capture that reference and mutate the immutable array afterward — the immutability-violation scenario the original well-known-type gate was designed to prevent.

Please reconcile this: either the security rationale in the <remarks> no longer applies (and should be updated/removed with justification), or the fallback should be limited to trusted collection types. As written, the code and its documented contract contradict each other.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs:141

• Calling ICollection.CopyTo for arbitrary ICollection implementations leaks the destination array to user code. This breaks the immutability guarantee for ImmutableArray created by APIs like CreateRange/InsertRange: a malicious ICollection.CopyTo can retain the backing array reference and mutate it after the ImmutableArray is returned (this is exactly the scenario described in the method remarks). Consider restricting the CopyTo fallback to trusted framework implementations (e.g., those in the same assembly as List) and otherwise returning false to use the enumeration fallback.

                collection.CopyTo(array, arrayIndex);
                return true;

[github-actions]

Holistic Review

Motivation: The PR addresses a real gap (issue #88181): TryCopyTo only took the fast CopyTo path for IList<T>, so ICollection<T>-only sequences like Dictionary<K,V>.Keys/Values fell back to per-element enumeration even though LINQ's own ToArray already uses the ICollection<T> path. The performance motivation is legitimate.

Approach: Broadening the gate to ICollection<T> and caching the cast is reasonable, and the #if !NET covariant-array guard correctly avoids ArrayTypeMismatchException on .NET Framework. The generic collection.CopyTo(array, arrayIndex) fallback for arbitrary implementers remains in tension with the security rationale documented in the method's own <remarks>.

Summary: ⚠️ Needs Human Review. The PR patch has not changed since the prior review: this re-review was triggered by a retarget/merge-base update (the previous and current cumulative patch IDs are identical), so there are no base-to-head content changes to analyze and no new actionable findings. The cumulative assessment is unchanged — the change is functionally correct and well tested for Dictionary.Keys, but the untrusted-CopyTo fallback still conflicts with the method's documented threat model, and a maintainer should decide whether that rationale still applies before merging.

---

Assessment History

• review 4730524832 reviewed commit d11bef9 with verdict ⚠️ Needs Human Review. Current verdict: ⚠️ Needs Human Review. Assessment unchanged — verdict, motivation, approach, and risk are all the same; the PR patch content is identical (patch IDs match) and only the merge base changed.

Detailed Findings

No new findings. The cumulative concerns from the prior review still stand (untrusted ICollection<T>.CopyTo fallback vs. the <remarks> security rationale, and the associated Count/CopyTo consistency assumption), but they pertain to code that is unchanged since the previous review and are therefore not re-raised here as new actionable findings.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 34.1 AIC · ⌖ 14.6 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.Minimal.cs:141

• Calling ICollection.CopyTo for arbitrary ICollection implementations can break ImmutableArray immutability: a malicious collection can retain the provided array reference and mutate it after the ImmutableArray is constructed (the remarks above explicitly call out this threat). To preserve the immutability guarantee, restrict the generic CopyTo fallback to trusted framework implementations (e.g., types from corelib or this assembly) and otherwise return false so callers fall back to per-element enumeration.

                collection.CopyTo(array, arrayIndex);
                return true;