Can I have a review here? @RikkiGibson ?

I am concerned that the result of ToArray(), which we are doing a covariant conversion on, will be ultimately converted to Span and throw at runtime. See the following test which throws during execution in this branch

[Fact]
public void SingleSpread_SpanToSpan_Covariant()
{
    var source = """
        using System;

        class C
        {
            static void Main()
            {
                Span<string> span1 = ["a", "b", "c"];
                span1.Report();
                Span<object> span2 = [..span1];
                span2.Report();
            }
        }
        """;

    var verifier = CompileAndVerify(new[] { source, s_collectionExtensionsWithSpan }, expectedOutput: IncludeExpectedOutput("[a, b, c], [a, b, c],"), targetFramework: TargetFramework.Net80, verify: Verification.Skipped);
    verifier.VerifyDiagnostics();
            verifier.VerifyIL("C.Main", """
        TODO2: update baseline
        """);
}

I think this optimization needs to be limited to cases where the collection-expr type is a "readonly" type, such as ReadOnlySpan<T>, or an IEnumerable<T> converted from a private <>z__ReadOnlyArray<T>.

We can't use this optimization when the collection-expr is of an array type, for similar reasons as the Span case above. Today the array's runtime type is the same as the element type. If we start returning a TBase[] which throws when assigning a TBase into it, it's a behavioral breaking change. e.g. I think Array_06 is not a change we can make, since it is converting Derived[] to Base[], and not further converting the value to a "readonly collection type".

Could you please also add a test which does something like Base[] arr = [.. derivedArray]; arr[0] = new Base();, or something similar, to demonstrate that particular behavior that I think we should preserve.

Taking another look this afternoon.

I pushed 9d61d77 to refactor VisitArrayOrSpanCollectionExpression.

This is a pure refactor as much as possible. I went into the original code, wrapped some sections in local functions, and moved some things around from one function to another, in order to make each path more focused on a specific task. I also tried to eliminate locals when they did not provide significant value, in order to make the set of data flowing from one function to another easier to follow.

@jcouv for a new review pass

@CyrusNajmabadi I tried merging the PR branch for #80568 into this PR branch locally. I found the conflicts fairly straightforward to handle, including that there were no conflicts in the tests. See the conflict resolutions in RikkiGibson@1d3ce15.

@RikkiGibson Can we merge now (assuming CI passes after rerun)?

It looks like the failing test jobs were "dead letter" work items in Helix. I suspect this is due to some infrastructure issue. Will plan to merge if the rerun passes. Thanks

Thanks for the contribution @DoctorKrolic!