[Copilot]

ImmutableArray<T>.Builder.InsertRange corrupts values when inserting itself at a middle index: [1, 2, 3] inserted at index 1 yields [1, 1, 1, 1, 2, 3] instead of [1, 1, 2, 3, 2, 3].

• Copying: Add the sealed builder type to TryCopyTo’s trusted paths, using its overlap-safe CopyTo without exposing backing arrays to arbitrary collection implementations.
• Regression coverage: Compare self-insertion with List<T> across empty, single-element, and three-element builders, every insertion index, and capacity growth/no growth.

• Fixes ImmutableArray<T>.Builder.InsertRange returns unexpected values when it use the same instance #136026

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[tannergooding]

Why is this the general setup instead of checking sequence is ICollection<T> collection and calling collection.CopyTo(array, arrayIndex)? Presumably most if not all of these other cases are already optimizing their CopyTo to use Array/Span.CopyTo where possible and that ensures it works with overlapping src/dst already.

-- Separately, this is notably also consistently one of those cases where we find ourselves optimizing based on the ability to get a limited scoped ROSpan<T> from an arbitrary IEnumerable<T>. There's some nuance with it, like it "detaching" from the original collection if you modify the collection (not memory unsafe, just potential for a logic bug). But it is so commonly needed in our and user code for this type of thing that it's probably worth exposing anyways

[eiriktsarpalis]

It's a good question. @copilot please investigate simplifying this to one ICollection type check.

[Copilot]

Investigated: yes, this can use a single ICollection<T> check on modern .NET. The concrete-type restriction was intentional—the remarks explain that a custom CopyTo could retain the immutable backing array—but #88181 explicitly proposes accepting that tradeoff.

There is one compatibility exception: on .NET Framework, non-exact array types need the enumeration fallback (e.g. uint[] passed as IEnumerable<int>, covered by #14794).

Open PR #125304 already implements exactly this simplification, including the #if !NET array guard, caller tests, and benchmark results. It also fixes this self-insertion issue, so I recommend landing that change there rather than duplicating it here, and retaining this PR’s self-insertion regression coverage. No files changed or tests run during this investigation.

[eiriktsarpalis]

@tannergooding what do you think? Given this change is already scoped to that other PR I would be inclined to simply fix the builder issue without refactoring the containing scaffolding.

[tannergooding]

Is it realistically a concern? Code can also do things like ImmutableCollectionsMarshal.AsArray, so is it worth pessimizing things and adding increasingly more custom checks as compared to the typical and optimal path? Do we really need to beholden ourselves to limitations of the type system as written 21 years ago and if so, should we consider some ICopyable API that supports a CopyTo(Span<T> array) so it cannot be captured?

I agree we should fix the general bug and look at discussing the other separately, but I think it's something we can/should settle on because this is a repeat problem and comes with repeated cost addition for every new if (T is ...) path we have to tack on.

[jkotas]

the remarks explain that a custom CopyTo could retain the immutable backing array

This is not a legitimate concern.

Open PR #125304 already implements exactly this simplification

Just merged