Fix async disposal

[jnm2]

I think some of these deviations might have originally been misconceptions on my part, so I apologize. I've had time to do thorough testing with the a recent GA version of Roslyn (5.9.0-1.26423.113 e34a38d2ae1fc26406a317517196e55c68ff83ab), under langversion 8.

The commits go step-by-step, either making minor corrections or doing refactorings which don't change the meaning, one at a time. This helped me be sure I fully understood what was changing and why. I hope the commit messages bring similar benefit to the reviewers.

[jskeet]

Thanks for these Joseph (and for splitting up the changes by commit) - I think I understand them all and basically agree, but with a few nits about text that I believe should be informative.

Would welcome @BillWagner's input as well though. (I think it's Bill who was leading changes in this area recently... I could be wrong.)

[jskeet]

Note for other reviewers: this means that a DisposeAsync(int x = 0) { } method will be found. Validated with this sample code:

public class AsyncCollection
{
    public AsyncEnumerator GetAsyncEnumerator() => new AsyncEnumerator();
}

public class AsyncEnumerator
{
    public ValueTask<bool> MoveNextAsync() => ValueTask.FromResult(false);
    public int Current => 1;
    public ValueTask DisposeAsync(int x = 0)
    {
        Console.WriteLine("Got to DisposeAsync");
        return ValueTask.CompletedTask;
    }
}

...

await foreach (var x in new AsyncCollection())
{
    Console.WriteLine($"Value {x}");
}
Console.WriteLine("Done");

Output:

Got to DisposeAsync
Done

[jnm2]

Another consequence of my update here is to correctly fall back to the interface rather than stopping with a failure when the match is ambiguous:

using System;
using System.Threading.Tasks;

await foreach (var x in new AsyncCollection())
{
    Console.WriteLine($"Value {x}");
}
Console.WriteLine("Done");

public class AsyncCollection
{
    public AsyncEnumerator GetAsyncEnumerator() => new AsyncEnumerator();
}

public class AsyncEnumerator : IAsyncDisposable
{
    public ValueTask<bool> MoveNextAsync() => ValueTask.FromResult(false);
    public int Current => 1;

    public ValueTask DisposeAsync(int x = 0)
    {
        Console.WriteLine("Got to DisposeAsync(int)");
        return ValueTask.CompletedTask;
    }

    public ValueTask DisposeAsync(string x = "")
    {
        Console.WriteLine("Got to DisposeAsync(string)");
        return ValueTask.CompletedTask;
    }

    ValueTask IAsyncDisposable.DisposeAsync()
    {
        Console.WriteLine("Got to IAsyncDisposable.DisposeAsync");
        return ValueTask.CompletedTask;
    }
}

A version of this can also be built where the async enumerator is an interface type which derives from IAsyncDisposable and inherits from other interfaces which also have DisposeAsync, thus causing lookup ambiguity another way.

[jskeet]

Again, this sounds like it should be normative rather than informative.

[jnm2]

I'm willing to make the update, but I want to quickly check whether it's necessary. I believe this is already the behavior described by the text above. Based on other language rules, the Dispose() lookup on E is already not going to find things on the underlying type of E. And, the finally block in the lowering would have to have .Value.Dispose() or equivalent, instead of .Dispose(). Given that it's already implied, I thought a note could make sense to explain one consequence of the text above it. But I'm happy going either way.

[BillWagner]

I think a note is fine, if we add xrefs to the other clauses cited in the comment.

[jnm2]

Can you see any additional xrefable clauses besides "member lookup"?

[BillWagner]

I had a few comments, but this a great improvement.

[BillWagner]

Editorial note: This repeats the text in line 1391 (asynchronous foreach). If possible, it'd be nice to de-duplicate it, and provide an xref. If de-dup is readable, at least an xref (both ways) should be added so we keep them in sync in the future.

[jnm2]

I tried a dedup! Commit 0e1abef. What do you think?

[BillWagner]

I think a note is fine, if we add xrefs to the other clauses cited in the comment.