Ability to obsolete IEnumerable.GetEnumerator used through Linq

[verdie-g]

Assume a class that reads rows from the network

class MyDbReader : IEnumerable<object>, IAsyncEnumerable<object>
{
    [Obsolete]
    public IEnumerator<object> GetEnumerator()
    {
        yield return new object();
    }

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    
    public async IAsyncEnumerator<object> GetAsyncEnumerator(CancellationToken cancellationToken = new())
    {
        yield return new object();
    }
}

the maintainer recently implemented IAsyncEnumerable after realizing that reading synchronously from the network using the IEnumerable was causing a thread pool starvation. They also added the Obsolete attribute on the GetEnumerator hoping this would guide the users to use GetAsyncEnumerator. Though, if used through a foreach or Linq, no warnings are emitted:

MyDbReader r = new();
_ = r.GetEnumerator(); // Method 'MyDbReader.GetEnumerator()' is obsolete
_ = r.Select(x => x); // No warnings.
foreach (var x in r) // No warnings
{
}

When GetEnumerator is marked as obsolete, would it possible to generate warnings on foreach and Linq?

[dotnet-policy-service]

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

[JakeYallop]

Would this be something fixed by dotnet/roslyn#73920 and #103430, at least for the foreach case?

[eiriktsarpalis]

I'm seeing warnings in the foreach case:

I don't think it would be possible to warn for the case of the Select since the compiler can't possibly reason about the enumerator being used (it isn't in your case).

[verdie-g]

I'm seeing warnings in the foreach case:

ha, indeed I see the warning in the build output but not in Rider. I'll redirect that issue to JetBrains (RIDER-113674).

I don't think it would be possible to warn for the case of the Select since the compiler can't possibly reason about the enumerator being used

In the expression x.Select, in the case where x is not an IEnumerable (e.g. a MyDbReader), I think an analyzer could check if the GetEnumerator is obsolete, right?

But yes it would miss such case:

IEnumerable<object> e = new MyDbReader();
_ = e.Select(x => x);

[eiriktsarpalis]

I think an analyzer could check if the GetEnumerator is obsolete, right?

It could, but like I said technically calling Select doesn't call GetEnumerator. In my view it doesn't seem like a particularly common pattern for us to invest in adding an analyzer.

[verdie-g]

technically calling Select doesn't call GetEnumerator

Hmm true.

In my view it doesn't seem like a particularly common pattern for us to invest in adding an analyzer.

With the introduction of IAsyncEnumerable in .NET Core 3, I feel like this could a great feature to migrate from one to the other. I've seen a bunch of libraries that mistakenly do sync-over-async because of their IEnumerable implementation (example in CassandraCsharpDriver).

[eiriktsarpalis]

My recommendation would be to use separate types for each of the interface implementations, or perhaps use a property or factory that explicitly gets the specific enumerator implementation

public IEnumerable<T> AsEnumerable();
public IAsyncEnumerable<T> AsAsyncEnumerable();