dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.93k stars 4.02k forks source link

Collection expression target-type not being used for nested spread expressions. #69277

Open CyrusNajmabadi opened 1 year ago

CyrusNajmabadi commented 1 year ago

The following fails to compile:

List<int> c = [1, .. b ? [2] : []];

With the error: error CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'collection expressions' and 'collection expressions'

This is expected to work (and is called out in teh spec a nice shorthand for conditionally adding elements). The outer target type should influence the inner spread. Since List<int> c = b ? [2] : []; isd legal, this should work.

Note: as per our list of optimizations, while this technically would mean creating sublists just to spread, we should optimize the above to just be emitted as if (b) __result.Add(2);

CyrusNajmabadi commented 1 year ago

I'm realizing we did not spec this well (or left it incomplete). because it's not exactly 'target typing'. teh spec says it wants this to work, but also defines 'spreads' as a translation to:

foreach (var v in <Expr>)
    result.Add(v);

Under that formalization, this does not fall out. Nor would it be desirable to just translate the above to

foreach (var v in (TargetType)<expr>)
    result.Add(v);

as such a conversion might be illegal.

So, we either want this to say something like "it is conditionally target typed if it otherwise does not have an iteration type". Or instead say that the translation is to:

result.AddRange(<expr>)

With the latter formalization, this would fall out, since AddRange would target type to something normally like IEnumerable<T>, which would normally work. However, this would not work for Span/Arrays. I don't have a good sense of the best way to spec this out to make this fall out cleanly. @cston @jnm2 for thoughts.

CyrusNajmabadi commented 1 year ago

From discussion offline, what we think we should do is that if the spread fails to have a type, we should rebind with ROS<target-element-type> as the target type.

In this case, that would mean that b ? [2] : [] would rebind with ROS<int> as the target-type. This woudl then both correctly conditiona-expression-target-type this expression, it would allow the compiler to properly stack alloc the results if the literals pass the stack-alloc heuristics (Which they should here).

CyrusNajmabadi commented 1 year ago

a similar thing we should look into is:

foreach (int i in [1, 2, 3])

this should target type the foreach expression with ROS<foreach-iteration-type (in this case: ROS<int>). Similar to above, this will allow this to bind properly, and shoudl allow emitting efficiently.

CyrusNajmabadi commented 1 year ago

@cston do we need any language/work here so that these 'temporary collection expressions' (ones not assigned to an lvalue) can be appropriately stack-allocated'ed? or does that just fall out?

cston commented 1 year ago

do we need any language/work here so that these 'temporary collection expressions' (ones not assigned to an lvalue) can be appropriately stack-allocated'ed?

The compiler will need to special-case these temporary ReadOnlySpan<T> instances so they are allocated on the stack.