Avoid CollectionsMarshal.AsSpan for derived List<T> in Linq#118682
Avoid CollectionsMarshal.AsSpan for derived List<T> in Linq#118682xtqqczze wants to merge 1 commit intodotnet:mainfrom
CollectionsMarshal.AsSpan for derived List<T> in Linq#118682Conversation
|
Tagging subscribers to this area: @dotnet/area-system-linq |
`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.
398d6e7 to
297fdf3
Compare
|
Where are we currently using |
| if (source.GetType() == typeof(List<TSource>)) // avoid accidentally bypassing a derived type's reimplementation of IEnumerable<T> | ||
| { | ||
| return new ListSelectIterator<TSource, TResult>(list, selector); | ||
| return new ListSelectIterator<TSource, TResult>(Unsafe.As<List<TSource>>(source), selector); |
There was a problem hiding this comment.
Isn't this something for JIT to fix instead?
cc @EgorBo
There was a problem hiding this comment.
I see no difference locally from this change
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 |
Is it a breaking change to change Linq to respect the 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.");
}
} |
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. |
* `Task.WaitAll` * `Task.Whenall` * `string.oin` Related: dotnet#118682
* `Task.WaitAll` * `Task.Whenall` * `string.Join` Related: dotnet#118682
|
There is an implicit contract in collection types supporting both |
A derived type of |
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. |
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 |
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 See sharplab for how this works. |
I understand what you're saying. And I'm saying code that queries for |
|
@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. |
CollectionsMarshal.AsSpanrelies on the exact internal layout ofList<T>. Passing a subclass is unsafe because a derived list may reimplementIEnumerable<T>with altered enumeration semantics.Related: #96574, #85288