Optimize single spread collection expression for List<T>

[DoctorKrolic]

Closes: #71217

So, perhaps heuristic should instead be: use ToList() if spread operand implements ICollection<T> or the compile-time type lacks a struct enumerator.

[DoctorKrolic]

The original motivation for verifying multiple target frameworks was to test AddRange code path. Since now there is no difference between codegen on .NET Framework and modern .NET, I don't see value in having multiple TFMs to verify against

[DoctorKrolic]

I didin't want to mix single-spread and AddRange tests together and I wanted to be sure, that AddRange code paths are still being verified. Therefore I modified existing AddRange tests, but added an equivalent SingleSpread tests on top for these cases

[DoctorKrolic]

This test for missing members doesn't make sense with new codegen, thus modified

[DoctorKrolic]

All changed tests can now use standard TFM, so IncludeExpectedOutput is no longer needed

[DoctorKrolic]

The diff is a mess, I don't know why is that. I just added a chunk of SingleSpread tests above SingleSpread_CustomCollection cases

[DoctorKrolic]

The unconstrained and struct-constrained cases are considered as boxing conversions, therefore they are verified by List_AddRange_IEnumerable_Constraint test

[DoctorKrolic]

Should we maybe use the actual use site info for thisand report it if we take the optimzal code path an the end? Unline the ICollection<T> check, the resulting conversion is kinda used in the final codegn, although implicitly

[RikkiGibson]

I guess the original code for the AddRange case was reporting the use-site diagnostic, but I think it's fine to drop it. The code we generate does not actually use this type in any way, it's just a hint for us to decide if AddRange/ToList is likely to be efficient.

[RikkiGibson]

LGTM except some minor comments

[RikkiGibson]

I guess the original code for the AddRange case was reporting the use-site diagnostic, but I think it's fine to drop it. The code we generate does not actually use this type in any way, it's just a hint for us to decide if AddRange/ToList is likely to be efficient.

[DoctorKrolic]

It looks like there may be legit bootstrap build failures

Unfortunatelly yes, bootstrap failure seem to consistently reproduce in CI and I was able to reproduce the failure locally. Also it is a "bad" type of failure where no Debug.Assert was hit and the error with its stack trace look like this:

ID=MSBuild 5396 TID=38: Error Error: 'EndOfStreamException' 'Reached end of stream before end of read.' occurred during 'Reading response for Microsoft.CodeAnalysis.Features (net7.0)'
Stack trace:
   at Microsoft.CodeAnalysis.CommandLine.BuildProtocolConstants.<ReadAllAsync>d__4.MoveNext() in /_/src/Compilers/Core/CommandLine/BuildProtocol.cs:line 621
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.CodeAnalysis.CommandLine.BuildResponse.<ReadAsync>d__5.MoveNext() in /_/src/Compilers/Core/CommandLine/BuildProtocol.cs:line 322
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.CodeAnalysis.CommandLine.BuildServerConnection.<<RunServerBuildRequestAsync>g__tryRunRequestAsync|8_1>d.MoveNext() in /_/src/Compilers/Shared/BuildServerConnection.cs:line 312

There are multiple error like this, some have suspisious "Unnamed compilation" in the message, e.g. ID=MSBuild 7328 TID=47: Error Error: 'EndOfStreamException' 'Reached end of stream before end of read.' occurred during 'Reading response for Unnamed compilation 9780962c-3581-4fe9-b678-bc7725994f7d'

Let us know if you need assistance investigating the failures.

Given that the stack trace doesn't show anything related to the changes of the PR and no Debug.Assert was hit (therefore I cannot attach to the process at the point when something goes wrong) I cannot find out what is the point of failure myself and need external help(

[cston]

Please test the following, where M1() and M2() should use foreach, and M3() and M4() should use Enumerable.ToList<U>(IEnumerable<U>).

using System.Collections.Generic;
interface IMyEnumerable<T> : IEnumerable<T>
{
    new MyEnumerator<T> GetEnumerator();
}
interface IMyCollection<T> : ICollection<T>
{
    new MyEnumerator<T> GetEnumerator();
}
struct MyEnumerator<T>
{
    public T Current => default;
    public bool MoveNext() => false;
}
class Program
{
    static void Main()
    {
    }
#nullable enable
    static List<U> M1<T, U>(T c)
        where T : class, IMyEnumerable<U>
        where U : class
    {
        return [..c];
    }
    static List<U?> M2<T, U>(T c)
        where T : class, IMyEnumerable<U>
        where U : class
    {
        return [..c];
    }
    static List<U> M3<T, U>(T c)
        where T : class, IMyCollection<U>
        where U : class
    {
        return [..c];
    }
    static List<U?> M4<T, U>(T c)
        where T : class, IMyCollection<U>
        where U : class
    {
        return [..c];
    }
}

[cston]

Unfortunatelly yes, bootstrap failure seem to consistently reproduce in CI and I was able to reproduce the failure locally.

I ran build -bootstrap -config Debug and attached to VBCSCompiler.exe after the bootstrap build was created and started. There was an unhandled exception compiling Microsoft.CodeAnalysis.Features.dll compiling this code.

System.InvalidOperationException: Unexpected value 'PropertyAccess' of type 'Microsoft.CodeAnalysis.CSharp.BoundKind'

Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCore(Microsoft.CodeAnalysis.CSharp.BoundExpression expression, bool used) Line 357 C#
...

[cston]

Please add the test from #74769 (comment).

[DoctorKrolic]

@cston Please merge

[cston]

@cston Please merge

Sorry for the delay. I wanted to give @RikkiGibson a chance to review the last two commits.

[cston]

Thanks @DoctorKrolic.