Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async foreach binds to IAsyncDisposable.DisposeAsync() rather than pattern based DisposeAsync() #59955

Closed
cston opened this issue Mar 4, 2022 · 2 comments · Fixed by #60022
Closed

Comments

@cston
Copy link
Member

cston commented Mar 4, 2022

The following prints IAsyncDisposable.DisposeAsync():

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

await foreach (var i in new AsyncEnumerable())
{
}

struct AsyncEnumerable : IAsyncEnumerable<int>
{
    public AsyncEnumerator GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();
    IAsyncEnumerator<int> IAsyncEnumerable<int>.GetAsyncEnumerator(CancellationToken token) => new AsyncEnumerator();
}

struct AsyncEnumerator : IAsyncEnumerator<int>
{
    public int Current => 0;
    public async ValueTask<bool> MoveNextAsync() => false;
    public async ValueTask DisposeAsync() { Console.WriteLine("DisposeAsync()"); }
    async ValueTask IAsyncDisposable.DisposeAsync() { Console.WriteLine("IAsyncDisposable.DisposeAsync()"); }
}

See sharplab.io.

From async-streams spec:

The compiler will bind to the pattern-based APIs if they exist, preferring those over using the interface (the pattern may be satisfied with instance methods or extension methods).

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 4, 2022
@jcouv jcouv self-assigned this Mar 4, 2022
@jcouv jcouv added this to the Compiler.Next milestone Mar 4, 2022
@jcouv jcouv added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 4, 2022
@bernd5
Copy link
Contributor

bernd5 commented Mar 6, 2022

@jcouv I were really surprised that just DisposeAsync is not preferring pattern based methods...
I found no clear statement about it in the spec - but for consistency reasons I agree that this is maybe a bug...

Your sample is a bit wrong - because the non interface GetAsyncEnumerator method is not invocable without a parameter. In this case the compiler must call the interface based method (the enumerator is of type IAsyncEnumerator<int>).

But if you change that - the compiler still uses the explicit interface implementation of DisposeAsync. This is not really a big problem because it is executed with a constrained call - no Boxing:

IL_00f0: constrained. AsyncEnumerator
IL_00f6: callvirt instance valuetype [System.Private.CoreLib]System.Threading.Tasks.ValueTask[System.Private.CoreLib]System.IAsyncDisposable::DisposeAsync()

This behaviour is required by the spec for foreach in general - not just async foreach.

See fixed code: link

Perhaps we should just fix the spec - and make it clear.

@jcouv
Copy link
Member

jcouv commented Mar 7, 2022

Re: "I found no clear statement about it in the spec"

I've added a note to OP with the statement from async-streams spec that clarifies expectations:

The compiler will bind to the pattern-based APIs if they exist, preferring those over using the interface (the pattern may be satisfied with instance methods or extension methods).

The usings spec says:

In the situation where a type can be implicitly converted to IDisposable and also fits the disposable pattern, then IDisposable will be preferred. While this takes the opposite approach of foreach (pattern preferred over interface) it is necessary for backwards compatibility.

Tagging @chsienki in case the precedence order as implemented was intended (TL;DR: foreach generally picks pattern-based first, then interface; async-streams spec agree; but implementation does reverse).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants