Avoid CollectionsMarshal.AsSpan for derived List<T> in Linq

[xtqqczze]

CollectionsMarshal.AsSpan relies on the exact internal layout of List<T>. Passing a subclass is unsafe because a derived list may reimplement IEnumerable<T> with altered enumeration semantics.

Related: #96574, #85288

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

[xtqqczze]

@MihuBot

[stephentoub]

Where are we currently using AsSpan on a List<T> that's possibly a derived type?

[jkotas]

Isn't this something for JIT to fix instead?

cc @EgorBo

[EgorBo]

I see no difference locally from this change

[stephentoub]

a derived list may reimplement IEnumerable with altered enumeration semantics.

This has long been the case, even on .NET Framework, e.g. on .NET Framework, this doesn't throw:

using System;
using System.Collections.Generic;
using System.Linq;

internal class Program
{
    static void Main()
    {
        foreach (var item in new MyFailingList().Where(i => i % 2 == 0))
        {
            Console.WriteLine(item);
        }
    }
}

class MyFailingList : List<int>, IEnumerable<int>
{
    public MyFailingList()
    {
        Add(1);
        Add(2);
        Add(3);
    }

    IEnumerator<int> IEnumerable<int>.GetEnumerator()
    {
        throw new InvalidOperationException("This is a failing enumerator.");
    }
}

That has nothing to do with AsSpan, which isn't used there.

[xtqqczze]

This has long been the case, even on .NET Framework

Is it a breaking change to change Linq to respect the IEnumerable<T> implementation? Take the following:

using System;
using System.Collections.Generic;
using System.Linq;

internal class Program
{
    private static IEnumerable<int> _en = new MyFailingList();

    static void Main()
    {
        foreach (var item in _en)
        {
            Console.WriteLine(item); // throws
        }
        foreach (var item in _en.Where(i => i % 2 == 0))
        {
            Console.WriteLine(item); // does not throw
        }
    }
}

class MyFailingList : List<int>, IEnumerable<int>
{
    public MyFailingList()
    {
        Add(1);
        Add(2);
        Add(3);
    }

    IEnumerator<int> IEnumerable<int>.GetEnumerator()
    {
        throw new InvalidOperationException("This is a failing enumerator.");
    }
}

[stephentoub]

Is it a breaking change to change Linq to respect the IEnumerable implementation?

Yes. That doesn't mean we couldn't, but it's been this way for 20 years and I've not heard any concerns raised, so I don't think we should. Note, as well, this isn't just LINQ. If you just do:

foreach (var item in new MyFailingList()) { }

in the cited example, it will not throw, because the C# compiler will bind to the public GetEnumerator method on List rather than to its IEnumerable implementation. I don't see a good argument for changing behavior here at this point.

[teo-tsirpanis]

There is an implicit contract in collection types supporting both IEnumerable<T> and IReadOnlyList<T>, that they must behave equivalently, or in other words, that foreach (var x in xs) { } and for (int i = 0; i < xs.Length; i++) { var x = xs[i]; } have no functional differences. People can go out of their way and write a hostile and non-conformant implementation of an interface, but the rest of the ecosystem does not have to be defensive and account for them.

[xtqqczze]

There is an implicit contract in collection types supporting both IEnumerable<T> and IReadOnlyList<T>, that they must behave equivalently, or in other words, that foreach (var x in xs) { } and for (int i = 0; i < xs.Length; i++) { var x = xs[i]; } have no functional differences. People can go out of their way and write a hostile and non-conformant implementation of an interface, but the rest of the ecosystem does not have to be defensive and account for them.

A derived type of List<T> that also reimplements both IEnumerable<T> and IReadOnlyList<T> can have those two interfaces behave equivalently when enumerated. However, this equivalence does not extend if someone bypasses the interface and directly enumerates over the backing array, this may yield different results if the logical view and physical storage diverge.

[stephentoub]

A derived type of List that also reimplements both IEnumerable and IReadOnlyList can have those two interfaces behave equivalently when enumerated.

But a developer has no ability to control the List's public APIs, e.g. its indexer, its public GetEnumerator, etc. It's also expected that those are in sync behaviorally with any such interface implementations, which then effectively means that no one should ever be changing the behavior of those interfaces on a derived type.

[teo-tsirpanis]

A derived type of List<T> that also reimplements both IEnumerable<T> and IReadOnlyList<T> can have those two interfaces behave equivalently when enumerated.

This violates another implicit contract; that implict and explicit implementations of an interface have to behave equivalently (if not being identical), and the Liskov substitution principle for that matter. Since List<T>'s implicit interface implementations are not virtual, it is undefined behavior for a subclass to alter the explicit ones.

[xtqqczze]

A derived type of List that also reimplements both IEnumerable and IReadOnlyList can have those two interfaces behave equivalently when enumerated.

But a developer has no ability to control the List's public APIs, e.g. its indexer, its public GetEnumerator, etc. It's also expected that those are in sync behaviorally with any such interface implementations, which then effectively means that no one should ever be changing the behavior of those interfaces on a derived type.

The derived type could reimplement these interfaces explicitly, hiding members inherited from List, for example:

https://github.com/microsoft/CollectServiceFabricData/blob/a7e559e95191fa2366251aea68191b8f14300098/src/CollectSFDataDll/Common/SynchronizedList.cs#L238-L250

[stephentoub]

The derived type could reimplement these interfaces explicitly, hiding members inherited from List, for example:

I'm not talking about the interface methods; I'm talking about List's public APIs, which are not virtual and cannot be overridden.

[xtqqczze]

The derived type could reimplement these interfaces explicitly, hiding members inherited from List, for example:

I'm not talking about the interface methods; I'm talking about List's public APIs, which are not virtual and cannot be overridden.

I meant an explicit interface implementation that hides inherited non-virtual members, using new, as in the following example:

• https://github.com/microsoft/CollectServiceFabricData/blob/a7e559e95191fa2366251aea68191b8f14300098/src/CollectSFDataDll/Common/SynchronizedList.cs#L238-L250

See sharplab for how this works.

[stephentoub]

I meant an explicit interface implementation that hides inherited non-virtual members

I understand what you're saying. And I'm saying code that queries for List<T> and then uses its public APIs will not be impacted by any interface implementations on the derived type. Such a derived type therefore can't expect its deviating behaviors in those interface implementations to be used.

[jeffhandley]

@xtqqczze With the discussion above, I'm closing this PR without merge. If you think it should be further considered, please file an issue that captures what challenge is present with the current implementation. Thank you for the effort and PR regardless.