Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add CancellationToken parameter to GetAsyncEnumerator #21397

Merged
merged 1 commit into from
Dec 7, 2018

Conversation

stephentoub
Copy link
Member

Per https://github.com/dotnet/corefx/issues/33338#issuecomment-444728011

@jcouv, presumably this (and the corresponding contract change in corefx) will lead to problems with the compiler targeting .NET Core 3 until it's updated with the new interface knowledge as well? Assuming yes, should we coordinate in some way / should I hold off on merging this until you've done the corresponding work on the compiler side?

cc: @MadsTorgersen, @terrajobst, @bartonjs, @kouvel, @tarekgh

@stephentoub stephentoub added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 6, 2018
@jcouv
Copy link
Member

jcouv commented Dec 6, 2018

I'm assuming that Core 3 preview 2 will release together with dev16 preview 2. If so, we can make the compiler and BCL changes together for preview 2. I expect to make the compiler-side change this week (targeting preview 2).

I will also add some tests on the compiler side for consuming the old interface. It may work because of pattern-based foreach.

@stephentoub
Copy link
Member Author

I expect to make the compiler-side change this week (targeting preview 2).

Ok, good. I just wanted to minimize the time that someone using a daily build of the compiler and a daily build of the runtime would be out-of-sync.

@stephentoub
Copy link
Member Author

@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness please
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

@jcouv
Copy link
Member

jcouv commented Dec 6, 2018

Regarding mixed preview1/preview2 scenarios, I would say it's ok to make the change in the BCL without waiting, because any types provided in source will win over types provided in BCL.

For example, I tested a preview1 compiler with a hypothetical BCL with CancellationToken (emulating preview2). Adding the preview1 interface in your program unblocks this scenario.

I don't think anyone uses daily compiler builds (they are trouble to install), so I'm not worried about preview2 compiler running on preview1 BCL, but the same mitigation would also work.

        [Fact]
        public void AsyncIteratorWithLocalDefinitionWithoutCancellationToken()
        {
            // This scenario illustrates using the dev16 preview1 compiler on Core preview2

            var core_preview2_src = @"
namespace System.Collections.Generic
{
    public interface IAsyncEnumerable<out T>
    {
        IAsyncEnumerator<T> GetAsyncEnumerator(System.Threading.CancellationToken token = default);
    }

    public interface IAsyncEnumerator<out T> : System.IAsyncDisposable
    {
        System.Threading.Tasks.ValueTask<bool> MoveNextAsync();
        T Current { get; }
    }
}
namespace System
{
    public interface IAsyncDisposable
    {
        System.Threading.Tasks.ValueTask DisposeAsync();
    }
}
";
            var core_preview2 = CreateCompilationWithTasksExtensions(core_preview2_src, assemblyName: "core_preview2");

            var source = @"
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
class C
{
    public static async IAsyncEnumerable<int> M()
    {
        yield return await Task.FromResult(2);
        await Task.Delay(1);
        yield return await Task.FromResult(8);
    }
    public static async Task Main(string[] args)
    {
        await foreach (var i in M())
        {
            Console.Write($""{i} "");
        }
    }
}";
            var comp = CreateCompilationWithTasksExtensions(new[] { source, AsyncStreamsTypes }, references: new[] { core_preview2.EmitToImageReference() }, TestOptions.ReleaseExe);
            comp.VerifyDiagnostics(
                // (6,9): warning CS0436: The type 'IAsyncEnumerator<T>' in '' conflicts with the imported type 'IAsyncEnumerator<T>' in 'core_preview2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. Using the type defined in ''.
                //         IAsyncEnumerator<T> GetAsyncEnumerator();
                Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "IAsyncEnumerator<T>").WithArguments("", "System.Collections.Generic.IAsyncEnumerator<T>", "core_preview2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", "System.Collections.Generic.IAsyncEnumerator<T>").WithLocation(6, 9),
                // (7,25): warning CS0436: The type 'IAsyncEnumerable<T>' in '' conflicts with the imported type 'IAsyncEnumerable<T>' in 'core_preview2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. Using the type defined in ''.
                //     public static async IAsyncEnumerable<int> M()
                Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "IAsyncEnumerable<int>").WithArguments("", "System.Collections.Generic.IAsyncEnumerable<T>", "core_preview2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", "System.Collections.Generic.IAsyncEnumerable<T>").WithLocation(7, 25),
                // (9,55): warning CS0436: The type 'IAsyncDisposable' in '' conflicts with the imported type 'IAsyncDisposable' in 'core_preview2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. Using the type defined in ''.
                //     public interface IAsyncEnumerator<out T> : System.IAsyncDisposable
                Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "IAsyncDisposable").WithArguments("", "System.IAsyncDisposable", "core_preview2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", "System.IAsyncDisposable").WithLocation(9, 55)
                );
            CompileAndVerify(comp, expectedOutput: @"2 8");
        }

@terrajobst
Copy link
Member

@jcouv @stephentoub @jaredpar @MadsTorgersen

At this point we can probably consider the API shape final (unless we find a major issue in preview 2). Any reason we shouldn't propose IAsyncEnumerable<T> and IAsyncEnumerator<T> for .NET Standard 2.1?

@stephentoub
Copy link
Member Author

At this point we can probably consider the API shape final (unless we find a major issue in preview 2). Any reason we shouldn't propose IAsyncEnumerable and IAsyncEnumerator for .NET Standard 2.1?

We should add them. As well as all of the supporting types, e.g. ManualResetValueTaskSourceCore, and the relevant extension methods, e.g. ConfigureAwait, WithCancellation (which still needs to be proposed./reviewed), etc.

@stephentoub stephentoub removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 7, 2018
@stephentoub stephentoub merged commit 811e34b into dotnet:master Dec 7, 2018
@stephentoub stephentoub deleted the aecancel branch December 7, 2018 18:56
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants