Use CollectionsMarshal to get span from list in collection expression by huoyaoyuan · Pull Request #85007 · dotnet/roslyn

huoyaoyuan · 2026-08-24T07:40:18Z

Microsoft Reviewers: Open in CodeFlow

azure-pipelines · 2026-08-24T07:40:38Z

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

Copilot

Pull request overview

Updates C# collection-expression lowering when the target is Span<T>/ReadOnlySpan<T> and the elements’ length can’t be determined up front, aiming to avoid the extra List<T>.ToArray() allocation/copy by using CollectionsMarshal.AsSpan.

Changes:

Adds a new lowering path for unknown-length span targets that uses CollectionsMarshal.AsSpan(list) (when available) instead of list.ToArray() + span ctor.
Factors out shared “try optimized array” logic for array/span backing storage decisions.
Extends well-known member support to include the Span<T> -> ReadOnlySpan<T> implicit conversion operator and updates IL baselines accordingly.

Reviewed changes

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

File	Description
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs	Uses `CollectionsMarshal.AsSpan` for unknown-length span targets (with fallback to `ToArray`) and shares optimized-array selection logic.
src/Compilers/Core/Portable/WellKnownMembers.cs	Adds signature/name metadata for `System_Span_T__op_Implicit_ReadOnlySpan_T`.
src/Compilers/Core/Portable/WellKnownMember.cs	Adds the corresponding `WellKnownMember` enum entry.
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs	Updates expected IL to reflect the new span-lowering shape that uses `CollectionsMarshal.AsSpan`.

CyrusNajmabadi · 2026-08-24T09:06:59Z

+                {
+                    array = CreateAndPopulateArray(node, arrayType);
+                }
+                // https://github.com/dotnet/roslyn/issues/68785: Emit Enumerable.TryGetNonEnumeratedCount() and avoid intermediate List<T> at runtime.


not sure if this comment is needed.

ah, you were trying to preserve this comment as a move. tha't fine then.

CyrusNajmabadi · 2026-08-24T09:07:28Z


-                var wellKnownMember = isReadOnlySpan ? WellKnownMember.System_ReadOnlySpan_T__ctor_Array : WellKnownMember.System_Span_T__ctor_Array;
-                var spanConstructor = _factory.WellKnownMethod(wellKnownMember).AsMember(spanType);
+                if (tryCreateOptimizedArray(node, arrayType, isReadOnlySpan) is { } optimizedArrayValue)


i'd prefer the standard bool+out-var approach.

The code patterns (tryXXX returning nullable, is { }) were replicating existing patterns in the enclosing method. Should they be updated individually?

In that case, best to keep consistent with the surrounding code :-). Thanks!

CyrusNajmabadi · 2026-08-24T09:08:37Z

+                    return wrapArrayInSpan(array, spanType, isReadOnlySpan);
+                }
+
+                BoundExpression wrapArrayInSpan(BoundExpression arrayValue, NamedTypeSymbol spanType, bool isReadOnlySpan)


can you make this static? it's unclear to me what this is capturing.

It's capturing the instance _factory from this.

Those can be pass in (including passing in 'this'). For my, that's preferable as it makes it clear what it is not capturing

There's also _compilation used for assertion. I haven't found existing practice for explicitly passing _factory.

You can pass in 'this'

I'm still unsure about this. All of the local functions in this method were capturing this only.

I think if we wrote it fresh today, we would probably use static and explicit @this parameter. But aligning with the pattern in this area is fine.

CyrusNajmabadi · 2026-08-24T09:09:53Z

+                    // Array/Span collections cannot have with-elements.  So we can create a List<T> in an optimal
+                    // fashion depending on our analysis of the elements.
+                    rewrittenReceiver: null);
+                Debug.Assert(list.Type is { });


can you just say is not null?

CyrusNajmabadi · 2026-08-24T09:10:44Z

+                Debug.Assert(list.Type is { });
+                Debug.Assert(list.Type.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T), TypeCompareKind.AllIgnoreOptions));
+
+                if (_factory.WellKnownMethod(WellKnownMember.System_Runtime_InteropServices_CollectionsMarshal__AsSpan_T, isOptional: true) is { } asSpanMethod)


can you invert this, so the simple case is easy to see right away, then the more compelx case.

consider a small amount of docs here indicating what actual code pattern this is generating.

Currently the code is ordered by priority. If BCL introduced more optimized helper in the future, where should it be placed then? if (helper1) {} else if (helper2) {} else { fallback } looks natural to me.

That works for me. Consider doc'ing that. Also consider breaking this into an easy set of calls for each case to a local function for each case. Will make the ordering clear.

CyrusNajmabadi · 2026-08-24T09:10:53Z

-                // of the base type, while usually such conversion requires stloc+ldloc with the local of the base type
-                assertTypesAreCompatible(_compilation, arrayType, spanConstructor.Parameters[0].Type, isReadOnlySpan);
-                return _factory.New(spanConstructor, arrayValue);
+                    Debug.Assert(list.Type is { });


same is not null.

CyrusNajmabadi · 2026-08-24T09:11:17Z

+                    {
+                        var implicitOperator = _factory.WellKnownMethod(WellKnownMember.System_Span_T__op_Implicit_ReadOnlySpan_T).AsMember((NamedTypeSymbol)listSpanValue.Type);
+                        listSpanValue = _factory.Call(null, implicitOperator, listSpanValue);
+                    }


makes sense, but docs ould be good.

CyrusNajmabadi · 2026-08-24T09:12:10Z

@@ -17138,35 +17138,37 @@ static MyCollection<object> F2(MyCollection<object> c)
            verifier.VerifyIL("Program.F2",


surprised there are so few test changes.

note: we should have tests that hit all teh interesting code paths here. for example, assigning to span vs ros.

I didn't find any dedicated test for this path, only indirect usage via collection builder. I can add more dedicated tests.

Copilot

Pull request overview

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

Copilot

Pull request overview

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

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs:405

The List type assertions here are redundant with the immediately preceding asserts (lines 395-397). Keeping only one set reduces noise without changing debug coverage.

                    Debug.Assert(list.Type is { });
                    Debug.Assert(list.Type.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T), TypeCompareKind.AllIgnoreOptions));

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs:37369

Same as above: add a WorkItem link for traceability back to the tracked issue.

                """);
        }

+        [Fact]


Copilot

🔵 Needs a closer look

It changes compiler lowering and the well-known member tables (high regression impact) and should get final confirmation from a human maintainer despite the added coverage.

Review details

Files reviewed: 6/6 changed files
Comments generated: 0 new
Review effort level: Lite

333fred

@dotnet/roslyn-compiler for a second review

Copilot

🟡 Changes recommended

AsSpan may expose an array previously passed to user-controlled ICollection<T>.CopyTo, enabling later mutation of the resulting span.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs:37369

This issue-linked regression test should carry the originating WorkItem URL, as required by .github/memory/TESTING_STRATEGY.md:29-31.
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs:37420
This issue-linked regression test should carry the originating WorkItem URL, as required by .github/memory/TESTING_STRATEGY.md:29-31.
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs:37501
This issue-linked regression test should carry the originating WorkItem URL, as required by .github/memory/TESTING_STRATEGY.md:29-31.
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs:37468
This issue-linked regression test should carry the originating WorkItem URL, as required by .github/memory/TESTING_STRATEGY.md:29-31.

Files reviewed: 6/6 changed files
Comments generated: 2
Review effort level: Balanced

333fred · 2026-09-02T18:17:02Z

+                    // Use CollectionsMarshal.AsSpan(list) to avoid copying the underlying array.
+                    // The array is exclusively used by the list and can be safely acquired.


Ok, this is a complicated scenario, but it does seem like it's accurate that this is a concern. What's a bit unclear to me is whether an ICollection<T>.CopyTo behaving this way would be considered "invalid" behavior; after all, List<T> effectively already has this dependency that ICollection<T>.CopyTo doesn't actually reference the array, or the version invariants of List<T> wouldn't hold.

a custom spread can retain this array and mutate the returned Span/ReadOnlySpan after the collection expression completes

I think there's no need to defend against this. A reasonable CopyTo impl would not do this. If needed we can adjust spec to indicate that such CopyTo is ill-formed.

We're not even using CopyTo directly: it's List<T> that does it as an optimization. We're just calling an AddRange method on List<T>. So that's my inclination as well.

If CopyTo called by List<T>.AddRange is unreliable, ToArray in LINQ would also be unreliable because it also calls ICollection<T>.CopyTo on the result array.

RikkiGibson · 2026-09-02T23:06:00Z

+                var list = CreateAndPopulateList(node, elementType, node.Elements,
+                    // Array/Span collections cannot have with-elements.  So we can create a List<T> in an optimal
+                    // fashion depending on our analysis of the elements.
+                    rewrittenReceiver: null);


Refreshing myself on this. The 'rewrittenReceiver' is basically the result of the 'with()' element. We know we don't have that so we just pass null here and let 'CreateAndPopulateList' automatically cook up a receiver (e.g. list instance). Right?

I have't deep-dived about this and just copied from createArray. It should be the same situation for creating list.

RikkiGibson · 2026-09-02T23:15:20Z

+                """;
+
+            var comp = CreateCompilation(new[] { source, s_collectionExtensionsWithSpan }, options: TestOptions.ReleaseExe, targetFramework: TargetFramework.Net80);
+            comp.MakeMemberMissing(WellKnownMember.System_Runtime_InteropServices_CollectionsMarshal__AsSpan_T);


It would be good to verify the behavior when both this and the Span->ReadOnlySpan operator are missing.
It looks like we would give an error when only the operator is missing, but not when both are missing.

Added a test path for this.

RikkiGibson · 2026-09-02T23:17:56Z

+            comp.VerifyEmitDiagnostics(
+                // (15,36): error CS0656: Missing compiler required member 'System.Span`1.op_Implicit'
+                //     ReadOnlySpan<int> result = [..e1, ..e2];
+                Diagnostic(ErrorCode.ERR_MissingPredefinedMember, "[..e1, ..e2]").WithArguments("System.Span`1", "op_Implicit").WithLocation(15, 36)


This seems reasonable. Any well formed Span type would have a conversion op to ReadOnlySpan.

It would also be an option to fallback to ToArray+ReadOnlySpan..ctor here, but, it feels like that conversion op is fairly comparable to the ctor in terms of importance, if you don't have it you simply can't convert Spans to ReadOnlySpans, that's such a core functionality.

RikkiGibson

LGTM! Let us know if you are planning to make any further changes per any of the PR feedback, and then we can merge

…iter_CollectionExpression.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>

Copilot

🔵 Needs a closer look

Low-level compiler lowering and emitted IL changes warrant maintainer review, and CI checks are still in progress.

Review details

Files reviewed: 6/6 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot

🟢 Approval recommended

The focused optimization includes appropriate fallback and missing-member coverage with no unresolved correctness issues.

Review details

Files reviewed: 6/6 changed files
Comments generated: 0 new
Review effort level: Balanced

huoyaoyuan · 2026-09-03T02:15:47Z

I have no further changes to make. You can check if there's still pending comment.

huoyaoyuan added 4 commits August 24, 2026 15:36

Add Span->ROSpan cast to WellKnownMember

6f90c68

Use CollectionsMarshal.AsSpan for spread

66c06e6

Update test codegen

430db72

Prefer single spread optimization over List

56d4a2f

Copilot AI lite review requested due to automatic review settings August 24, 2026 07:40

huoyaoyuan requested a review from a team as a code owner August 24, 2026 07:40

Copilot started reviewing on behalf of huoyaoyuan August 24, 2026 07:40 View session

dotnet-policy-service Bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 24, 2026

Copilot AI reviewed Aug 24, 2026

View reviewed changes

jjonescz added the Area-Compilers label Aug 24, 2026

CyrusNajmabadi reviewed Aug 24, 2026

View reviewed changes

huoyaoyuan added 2 commits August 26, 2026 15:02

Add comment

a5b6544

Add dedicated tests

76f8bad

Copilot AI review requested due to automatic review settings August 28, 2026 03:58

Copilot started reviewing on behalf of huoyaoyuan August 28, 2026 03:58 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated

Remove unnecesssary Slice

fcc8dd7

Copilot AI review requested due to automatic review settings August 28, 2026 05:12

Copilot started reviewing on behalf of huoyaoyuan August 28, 2026 05:12 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs Outdated

""");

}

[Fact]

Copilot AI review requested due to automatic review settings August 28, 2026 13:26

huoyaoyuan force-pushed the spread-ros branch from 80b2b57 to fcc8dd7 Compare August 28, 2026 13:27

Copilot started reviewing on behalf of huoyaoyuan September 2, 2026 09:49 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

333fred requested a balanced review from Copilot September 2, 2026 18:02

Copilot started reviewing on behalf of 333fred September 2, 2026 18:03 View session

333fred approved these changes Sep 2, 2026

View reviewed changes

Copilot AI reviewed Sep 2, 2026

View reviewed changes

RikkiGibson self-requested a review September 2, 2026 19:00

RikkiGibson reviewed Sep 2, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs Outdated

RikkiGibson reviewed Sep 2, 2026

View reviewed changes

RikkiGibson approved these changes Sep 2, 2026

View reviewed changes

Update src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewr…

6bd6c6f

…iter_CollectionExpression.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>

Copilot AI review requested due to automatic review settings September 3, 2026 01:45

Copilot started reviewing on behalf of huoyaoyuan September 3, 2026 01:46 View session

Update tests

3cd5512

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 3, 2026 01:50

Copilot started reviewing on behalf of huoyaoyuan September 3, 2026 01:51 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

RikkiGibson approved these changes Sep 3, 2026

View reviewed changes

RikkiGibson enabled auto-merge (squash) September 3, 2026 02:50

RikkiGibson merged commit 6a0c2f2 into dotnet:main Sep 3, 2026
21 of 22 checks passed

dotnet-policy-service Bot added this to the Next milestone Sep 3, 2026

huoyaoyuan deleted the spread-ros branch September 3, 2026 03:19

This was referenced Sep 3, 2026

[release/11.0.1xx] Source code updates from dotnet/roslyn dotnet/dotnet#9006

Merged

[main] Source code updates from dotnet/roslyn dotnet/dotnet#9040

Merged

dibarbet mentioned this pull request Sep 4, 2026

Update roslyn to 5.12.0-1.26453.19 dotnet/vscode-csharp#9731

Merged

dotnet-bot mentioned this pull request Sep 8, 2026

[Automated] PRs inserted in VS build main-12208.01 #85205

Closed

		@@ -17138,35 +17138,37 @@ static MyCollection<object> F2(MyCollection<object> c)
		verifier.VerifyIL("Program.F2",

                               """);
                       }
+                      [Fact]

		// Use CollectionsMarshal.AsSpan(list) to avoid copying the underlying array.
		// The array is exclusively used by the list and can be safely acquired.

Uh oh!

Conversation

huoyaoyuan commented Aug 24, 2026 • edited by dotnet-policy-service Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

azure-pipelines Bot commented Aug 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

huoyaoyuan commented Aug 24, 2026 •

edited by dotnet-policy-service Bot

Loading

RikkiGibson Sep 2, 2026 •

edited

Loading

huoyaoyuan commented Sep 3, 2026 •

edited

Loading