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,71 @@ 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(list.Type.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T), TypeCompareKind.AllIgnoreOptions));

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 would expec this to be reference equal with the original definition.

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 assertion expression is copied from createArray.

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.

There is no harm in tightening assertions that are currently too loose.


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).Slice(0, list.Count) 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.

BoundLocal temp = _factory.StoreToTemp(list, out var assignment);
var sideEffects = ArrayBuilder<BoundExpression>.GetInstance();
sideEffects.Add(assignment);

// 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);
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.

same is not null.

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

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.

Duplicate assertions with line 395.


var listSpanValue = _factory.Call(null, asSpanMethod.Construct(elementType.Type), temp);
var getCount = ((PropertySymbol)_factory.WellKnownMember(WellKnownMember.System_Collections_Generic_List_T__Count)).AsMember((NamedTypeSymbol)list.Type);
var count = _factory.Property(temp, getCount);

Comment thread
Copilot 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.


var sliceMethod = _factory.WellKnownMethod(isReadOnlySpan ? WellKnownMember.System_ReadOnlySpan_T__Slice_Int_Int : WellKnownMember.System_Span_T__Slice_Int_Int).AsMember(spanType);
var result = _factory.Call(listSpanValue, sliceMethod, _factory.Literal(0), count);
return new BoundSequence(
node.Syntax,
[temp.LocalSymbol],
sideEffects.ToImmutableAndFree(),
result,
spanType);
}
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,18 +459,13 @@ 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17138,35 +17138,37 @@ static MyCollection<object> F2(MyCollection<object> c)
verifier.VerifyIL("Program.F2",

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.

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.

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 didn't find any dedicated test for this path, only indirect usage via collection builder. I can add more dedicated tests.

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.

Yes please

"""
{
// Code size 79 (0x4f)
.maxstack 2
// Code size 97 (0x61)
.maxstack 3
.locals init (System.Collections.Generic.List<object> V_0,
System.Collections.Generic.IEnumerator<object> V_1,
object V_2)
System.Collections.Generic.List<object> V_1,
System.Collections.Generic.IEnumerator<object> V_2,
object V_3,
System.ReadOnlySpan<object> V_4)
IL_0000: newobj "System.Collections.Generic.List<object>..ctor()"
IL_0005: stloc.0
IL_0006: ldarga.s V_0
IL_0008: call "System.Collections.Generic.IEnumerator<object> MyCollection<object>.GetEnumerator()"
IL_000d: stloc.1
IL_000d: stloc.2
.try
{
IL_000e: br.s IL_001e
IL_0010: ldloc.1
IL_0010: ldloc.2
IL_0011: callvirt "object System.Collections.Generic.IEnumerator<object>.Current.get"
IL_0016: stloc.2
IL_0016: stloc.3
IL_0017: ldloc.0
IL_0018: ldloc.2
IL_0018: ldloc.3
IL_0019: callvirt "void System.Collections.Generic.List<object>.Add(object)"
IL_001e: ldloc.1
IL_001e: ldloc.2
IL_001f: callvirt "bool System.Collections.IEnumerator.MoveNext()"
IL_0024: brtrue.s IL_0010
IL_0026: leave.s IL_0032
}
finally
{
IL_0028: ldloc.1
IL_0028: ldloc.2
IL_0029: brfalse.s IL_0031
IL_002b: ldloc.1
IL_002b: ldloc.2
IL_002c: callvirt "void System.IDisposable.Dispose()"
IL_0031: endfinally
}
Expand All @@ -17175,10 +17177,18 @@ .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_0049: call "MyCollection<object> MyCollectionBuilder.Create<object>(System.ReadOnlySpan<object>)"
IL_004e: ret
IL_003f: stloc.1
IL_0040: ldloc.1
IL_0041: call "System.Span<object> System.Runtime.InteropServices.CollectionsMarshal.AsSpan<object>(System.Collections.Generic.List<object>)"
IL_0046: call "System.ReadOnlySpan<object> System.Span<object>.op_Implicit(System.Span<object>)"
IL_004b: stloc.s V_4
IL_004d: ldloca.s V_4
IL_004f: ldc.i4.0
IL_0050: ldloc.1
IL_0051: callvirt "int System.Collections.Generic.List<object>.Count.get"
IL_0056: call "System.ReadOnlySpan<object> System.ReadOnlySpan<object>.Slice(int, int)"
IL_005b: call "MyCollection<object> MyCollectionBuilder.Create<object>(System.ReadOnlySpan<object>)"
IL_0060: ret
}
""");
}
Expand Down Expand Up @@ -37319,6 +37329,121 @@ .locals init (System.Collections.Generic.List<int> V_0,
""");
}

[Fact]
public void IEnumerableToSpan_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)
{
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 50 (0x32)
.maxstack 3
.locals init (System.Span<int> V_0, //result
System.Collections.Generic.List<int> V_1,
System.Span<int> V_2)
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: stloc.1
IL_0014: ldloc.1
IL_0015: call "System.Span<int> System.Runtime.InteropServices.CollectionsMarshal.AsSpan<int>(System.Collections.Generic.List<int>)"
IL_001a: stloc.2
IL_001b: ldloca.s V_2
IL_001d: ldc.i4.0
IL_001e: ldloc.1
IL_001f: callvirt "int System.Collections.Generic.List<int>.Count.get"
IL_0024: call "System.Span<int> System.Span<int>.Slice(int, int)"
IL_0029: stloc.0
IL_002a: ldloca.s V_0
IL_002c: call "void CollectionExtensions.ReportSpan<int>(in System.Span<int>)"
IL_0031: 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 55 (0x37)
.maxstack 3
.locals init (System.ReadOnlySpan<int> V_0, //result
System.Collections.Generic.List<int> V_1,
System.ReadOnlySpan<int> V_2)
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: stloc.1
IL_0014: ldloc.1
IL_0015: call "System.Span<int> System.Runtime.InteropServices.CollectionsMarshal.AsSpan<int>(System.Collections.Generic.List<int>)"
IL_001a: call "System.ReadOnlySpan<int> System.Span<int>.op_Implicit(System.Span<int>)"
IL_001f: stloc.2
IL_0020: ldloca.s V_2
IL_0022: ldc.i4.0
IL_0023: ldloc.1
IL_0024: callvirt "int System.Collections.Generic.List<int>.Count.get"
IL_0029: call "System.ReadOnlySpan<int> System.ReadOnlySpan<int>.Slice(int, int)"
IL_002e: stloc.0
IL_002f: ldloca.s V_0
IL_0031: call "void CollectionExtensions.Report<int>(in System.ReadOnlySpan<int>)"
IL_0036: ret
}
""");
}

[Fact]
public void List_Spread_Mutation()
{
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/Core/Portable/WellKnownMember.cs
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ internal enum WellKnownMember
System_Span_T__get_Item,
System_Span_T__get_Length,
System_Span_T__Slice_Int_Int,
System_Span_T__op_Implicit_ReadOnlySpan_T,

Comment thread
huoyaoyuan marked this conversation as resolved.
Resolved
System_ReadOnlySpan_T__ctor_Pointer,
System_ReadOnlySpan_T__ctor_Array,
Expand Down
Loading
Loading