Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,22 @@ BoundExpression createImmutableArray(BoundCollectionExpression node, NamedTypeSy
return null;
}

BoundExpression? tryCreateOptimizedArray(BoundCollectionExpression node, ArrayTypeSymbol arrayType, bool targetsReadOnlyCollection)
{
BoundExpression? array = null;
if (TryOptimizeSingleSpreadToArray_NoConversionApplied(node, targetsReadOnlyCollection, arrayType) is { } optimizedArray)
{
array = optimizedArray;
}
else if (ShouldUseKnownLength(node, out _))
{
array = CreateAndPopulateArray(node, arrayType);
}
// https://github.com/dotnet/roslyn/issues/68785: Emit Enumerable.TryGetNonEnumeratedCount() and avoid intermediate List<T> at runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this comment is needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return array;
}

BoundExpression createSpan(BoundCollectionExpression node, NamedTypeSymbol spanType, bool isReadOnlySpan)
{
Debug.Assert(isReadOnlySpan
Expand All @@ -365,16 +381,56 @@ BoundExpression createSpan(BoundCollectionExpression node, NamedTypeSymbol spanT
return spanValue;

var arrayType = getBackingArrayType(spanType);
var arrayValue = createArray(node, arrayType, targetsReadOnlyCollection: isReadOnlySpan);

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return wrapArrayInSpan(optimizedArrayValue, spanType, isReadOnlySpan);

var elementType = spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];

// The span initializer has an unknown length, so we'll create an intermediate List<T> instance.
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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Debug.Assert(list.Type is { });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just say is not null?

Debug.Assert((object)list.Type.OriginalDefinition == _compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T));

if (_factory.WellKnownMethod(WellKnownMember.System_Runtime_InteropServices_CollectionsMarshal__AsSpan_T, isOptional: true) is { } asSpanMethod)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RikkiGibson RikkiGibson Sep 2, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// We can either get the same array type as the target span type or an array of more derived type.
// In the second case reference conversion would happen automatically since we still construct the span
// 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);
var listSpanValue = _factory.Call(null, asSpanMethod.Construct(elementType.Type), list);
Comment thread
huoyaoyuan marked this conversation as resolved.
Resolved

if (isReadOnlySpan)
{
// CollectionsMarshal.AsSpan only has overload to return writable span.
// Cast to ReadOnlySpan when needed.
var implicitOperator = _factory.WellKnownMethod(WellKnownMember.System_Span_T__op_Implicit_ReadOnlySpan_T).AsMember((NamedTypeSymbol)listSpanValue.Type);
listSpanValue = _factory.Call(null, implicitOperator, listSpanValue);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, but docs ould be good.


return listSpanValue;
}
Comment thread
huoyaoyuan marked this conversation as resolved.
Resolved
else
{
// CollectionsMarshal.AsSpan is not present. Use list.ToArray() as fallback.
var listToArray = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_Collections_Generic_List_T__ToArray)).AsMember((NamedTypeSymbol)list.Type);
var array = _factory.Call(list, listToArray);
return wrapArrayInSpan(array, spanType, isReadOnlySpan);
}

BoundExpression wrapArrayInSpan(BoundExpression arrayValue, NamedTypeSymbol spanType, bool isReadOnlySpan)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's capturing the instance _factory from this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass in 'this'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var wellKnownMember = isReadOnlySpan ? WellKnownMember.System_ReadOnlySpan_T__ctor_Array : WellKnownMember.System_Span_T__ctor_Array;
var spanConstructor = _factory.WellKnownMethod(wellKnownMember).AsMember(spanType);

// We can either get the same array type as the target span type or an array of more derived type.
// In the second case reference conversion would happen automatically since we still construct the span
// of the base type, while usually such conversion requires stloc+ldloc with the local of the base type
assertTypesAreCompatible(_compilation, arrayValue.Type!, spanConstructor.Parameters[0].Type, isReadOnlySpan);
return _factory.New(spanConstructor, arrayValue);
}

[Conditional("DEBUG")]
static void assertTypesAreCompatible(CSharpCompilation compilation, TypeSymbol arrayType, TypeSymbol constructorParameterType, bool isReadOnlySpan)
Expand All @@ -388,25 +444,20 @@ static void assertTypesAreCompatible(CSharpCompilation compilation, TypeSymbol a
BoundExpression createArray(BoundCollectionExpression node, ArrayTypeSymbol arrayType, bool targetsReadOnlyCollection)
{
BoundExpression array;
if (TryOptimizeSingleSpreadToArray_NoConversionApplied(node, targetsReadOnlyCollection, arrayType) is { } optimizedArray)
if (tryCreateOptimizedArray(node, arrayType, targetsReadOnlyCollection) is { } optimizedArray)
{
array = optimizedArray;
}
else if (ShouldUseKnownLength(node, out _))
{
array = CreateAndPopulateArray(node, arrayType);
}
else
{
// The array initializer has an unknown length, so we'll create an intermediate List<T> instance.
// https://github.com/dotnet/roslyn/issues/68785: Emit Enumerable.TryGetNonEnumeratedCount() and avoid intermediate List<T> at runtime.
var list = CreateAndPopulateList(node, arrayType.ElementTypeWithAnnotations, 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);

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

var listToArray = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_Collections_Generic_List_T__ToArray)).AsMember((NamedTypeSymbol)list.Type);
array = _factory.Call(list, listToArray);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17175,8 +17175,8 @@ .locals init (System.Collections.Generic.List<object> V_0,
IL_0034: box "int"
IL_0039: callvirt "void System.Collections.Generic.List<object>.Add(object)"
IL_003e: ldloc.0
IL_003f: callvirt "object[] System.Collections.Generic.List<object>.ToArray()"
IL_0044: newobj "System.ReadOnlySpan<object>..ctor(object[])"
IL_003f: call "System.Span<object> System.Runtime.InteropServices.CollectionsMarshal.AsSpan<object>(System.Collections.Generic.List<object>)"
IL_0044: call "System.ReadOnlySpan<object> System.Span<object>.op_Implicit(System.Span<object>)"
IL_0049: call "MyCollection<object> MyCollectionBuilder.Create<object>(System.ReadOnlySpan<object>)"
IL_004e: ret
}
Expand Down Expand Up @@ -37319,6 +37319,236 @@ .locals init (System.Collections.Generic.List<int> V_0,
""");
}

[Fact]
Comment thread
huoyaoyuan marked this conversation as resolved.
Resolved
public void IEnumerableToSpan_Spreads()
{
var source = """
using System;
Comment thread
Copilot marked this conversation as resolved.
Resolved
using System.Collections.Generic;

class C
{
static void Main()
{
List<int> list1 = [1, 2, 3];
List<int> list2 = [4, 5, 6];
M(list1, list2);
}

static void M(IEnumerable<int> e1, IEnumerable<int> e2)
{
Span<int> result = [..e1, ..e2];
result.ReportSpan();
}
}
""";

var verifier = CompileAndVerify(new[] { source, s_collectionExtensionsWithSpan }, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput("[1, 2, 3, 4, 5, 6], "), targetFramework: TargetFramework.Net80);
verifier.VerifyDiagnostics();
verifier.VerifyIL("C.M", """
{
// Code size 33 (0x21)
.maxstack 3
.locals init (System.Span<int> V_0) //result
IL_0000: newobj "System.Collections.Generic.List<int>..ctor()"
IL_0005: dup
IL_0006: ldarg.0
IL_0007: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_000c: dup
IL_000d: ldarg.1
IL_000e: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_0013: call "System.Span<int> System.Runtime.InteropServices.CollectionsMarshal.AsSpan<int>(System.Collections.Generic.List<int>)"
IL_0018: stloc.0
IL_0019: ldloca.s V_0
IL_001b: call "void CollectionExtensions.ReportSpan<int>(in System.Span<int>)"
IL_0020: ret
}
""");
}

[Fact]
public void IEnumerableToSpan_Spreads_MissingCollectionsMarshal()
{
var source = """
using System;
using System.Collections.Generic;

class C
{
static void Main()
{
List<int> list1 = [1, 2, 3];
List<int> list2 = [4, 5, 6];
M(list1, list2);
}

static void M(IEnumerable<int> e1, IEnumerable<int> e2)
{
Span<int> result = [..e1, ..e2];
result.ReportSpan();
}
}
""";

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

var verifier = CompileAndVerify(comp, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput("[1, 2, 3, 4, 5, 6], "));
verifier.VerifyDiagnostics();
verifier.VerifyIL("C.M", """
{
// Code size 39 (0x27)
.maxstack 4
.locals init (System.Span<int> V_0) //result
IL_0000: ldloca.s V_0
IL_0002: newobj "System.Collections.Generic.List<int>..ctor()"
IL_0007: dup
IL_0008: ldarg.0
IL_0009: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_000e: dup
IL_000f: ldarg.1
IL_0010: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_0015: callvirt "int[] System.Collections.Generic.List<int>.ToArray()"
IL_001a: call "System.Span<int>..ctor(int[])"
IL_001f: ldloca.s V_0
IL_0021: call "void CollectionExtensions.ReportSpan<int>(in System.Span<int>)"
IL_0026: ret
}
""");
}

[Fact]
public void IEnumerableToReadOnlySpan_Spreads()
{
var source = """
using System;
using System.Collections.Generic;

class C
{
static void Main()
{
List<int> list1 = [1, 2, 3];
List<int> list2 = [4, 5, 6];
M(list1, list2);
}

static void M(IEnumerable<int> e1, IEnumerable<int> e2)
{
ReadOnlySpan<int> result = [..e1, ..e2];
result.Report();
}
}
""";

var verifier = CompileAndVerify(new[] { source, s_collectionExtensionsWithSpan }, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput("[1, 2, 3, 4, 5, 6], "), targetFramework: TargetFramework.Net80);
verifier.VerifyDiagnostics();
verifier.VerifyIL("C.M", """
{
// Code size 38 (0x26)
.maxstack 3
.locals init (System.ReadOnlySpan<int> V_0) //result
IL_0000: newobj "System.Collections.Generic.List<int>..ctor()"
IL_0005: dup
IL_0006: ldarg.0
IL_0007: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_000c: dup
IL_000d: ldarg.1
IL_000e: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_0013: call "System.Span<int> System.Runtime.InteropServices.CollectionsMarshal.AsSpan<int>(System.Collections.Generic.List<int>)"
IL_0018: call "System.ReadOnlySpan<int> System.Span<int>.op_Implicit(System.Span<int>)"
IL_001d: stloc.0
IL_001e: ldloca.s V_0
IL_0020: call "void CollectionExtensions.Report<int>(in System.ReadOnlySpan<int>)"
IL_0025: ret
}
""");
}

[Fact]
public void IEnumerableToReadOnlySpan_Spreads_MissingImplicitCast()
{
var source = """
using System;
using System.Collections.Generic;

class C
{
static void Main()
{
List<int> list1 = [1, 2, 3];
List<int> list2 = [4, 5, 6];
M(list1, list2);
}

static void M(IEnumerable<int> e1, IEnumerable<int> e2)
{
ReadOnlySpan<int> result = [..e1, ..e2];
result.Report();
}
}
""";

var comp = CreateCompilation(new[] { source, s_collectionExtensionsWithSpan }, targetFramework: TargetFramework.Net80);
comp.MakeMemberMissing(WellKnownMember.System_Span_T__op_Implicit_ReadOnlySpan_T);
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
}

[Fact]
public void IEnumerableToReadOnlySpan_Spreads_MissingCollectionsMarshal()
{
var source = """
using System;
using System.Collections.Generic;

class C
{
static void Main()
{
List<int> list1 = [1, 2, 3];
List<int> list2 = [4, 5, 6];
M(list1, list2);
}

static void M(IEnumerable<int> e1, IEnumerable<int> e2)
{
ReadOnlySpan<int> result = [..e1, ..e2];
result.Report();
}
}
""";

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test path for this.


var verifier = CompileAndVerify(comp, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput("[1, 2, 3, 4, 5, 6], "));
verifier.VerifyDiagnostics();
verifier.VerifyIL("C.M", """
{
// Code size 39 (0x27)
.maxstack 4
.locals init (System.ReadOnlySpan<int> V_0) //result
IL_0000: ldloca.s V_0
IL_0002: newobj "System.Collections.Generic.List<int>..ctor()"
IL_0007: dup
IL_0008: ldarg.0
IL_0009: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_000e: dup
IL_000f: ldarg.1
IL_0010: callvirt "void System.Collections.Generic.List<int>.AddRange(System.Collections.Generic.IEnumerable<int>)"
IL_0015: callvirt "int[] System.Collections.Generic.List<int>.ToArray()"
IL_001a: call "System.ReadOnlySpan<int>..ctor(int[])"
IL_001f: ldloca.s V_0
IL_0021: call "void CollectionExtensions.Report<int>(in System.ReadOnlySpan<int>)"
IL_0026: ret
}
""");
}

[Fact]
public void List_Spread_Mutation()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -988,6 +988,7 @@ public void AllWellKnownTypeMembers()
case WellKnownMember.System_Span_T__get_Length:
case WellKnownMember.System_Span_T__Slice_Int:
case WellKnownMember.System_Span_T__Slice_Int_Int:
case WellKnownMember.System_Span_T__op_Implicit_ReadOnlySpan_T:
case WellKnownMember.System_ReadOnlySpan_T__ctor_Pointer:
case WellKnownMember.System_ReadOnlySpan_T__ctor_Array:
case WellKnownMember.System_ReadOnlySpan_T__ctor_Array_Start_Length:
Expand Down
Loading