Properly handle generic ValueTask await adapter

[RenderMichael]

Fixes #4639

[manfred-brands]

Thanks for doing this.
Looks good.

[manfred-brands]

@RenderMichael Any idea why the tests after 3 hours are still running?

[RenderMichael]

Not sure why it manifested as a timeout, but there's a bunch of behavior which seemed to rely on the old implementation, in other works for ValueTask<> to be handled by CSharpPatternBasedAwaitAdapter.

I'll look deeper into this later today.

[RenderMichael]

Ah never mind, I was completely misinterpreting those results. taskType.GetGenericTypeDefinition() was throwing for non-generic types, I forgot to put a taskType.IsGenericType before it. Fixed, the failing test passes for me locally.

I guess the failure was at an unfortunate location where it caused an infinite wait instead of an exception bubbling up.

[OsirisTerje]

Good there are tests then!

@RenderMichael Btw, are there enough tests to cover the changes done here ?

[OsirisTerje]

This is the coverage of the current code for the ValueTaskAwaitAdapter, and the GenericAdapter is not part of any tests. And with the change in the PR, this part of the code is being changed. Shouldn't we have a unit test around this code?

[RenderMichael]

@OsirisTerje I’m happy to add a unit test here, note that while there’s no explicit test coverage, as you can see from the CI failures, this was implicitly tested by the rest of the suite.

[OsirisTerje]

@jnm2 wrote the AsyncAdapters back in 2018, and he added a series of tests, AsyncExecutionApiAdapterTests, AwaitableReturnTypeTests, and more - search for "await" in the test explorer. @jnm2 might explain more here, but I got a feeling we should have added more tests here when ValueTask was introduced/changed. And the old #2774 is still open.

@RenderMichael Nice if you can have a look at the tests I mention above, and see if you could add to those.

The issues related to ValueTask are old, so this is not new, but when we start changing things, that's when I at least start to be a bit worried :-)

About implicitly tests, I am not sure if that was implicit or a side-effect, if it was implicit, it should have shown up in the coverage. UPDATE: I tested on the old code, not your PR, so this is not relevant :-) Sorry.

@stevenaw @manfred-brands Comments ?

[OsirisTerje]

@RenderMichael Can you share the code you use to verify that it is working. I am trying with the repro code for the #4588 issue and can't get it to go into the ValueTask awaiter.
I'm using the workaround for the compile error : await Assert.ThatAsync(async () => await Method(), Is.EqualTo(""));
where Method is: public static ValueTask<string> Method() => new("");

[OsirisTerje]

Thanks! LGTM :-)

[RenderMichael]

@OsirisTerje I don't think there's a test we can write which will fail without this PR, since the C# pattern-based adapter catches this scenario anyways, so I had to use to the debugger to watch the flow.

The tests I added passed even before this PR, but they hit the code paths (and the other tests hit the negative code paths which caused an exception previously).

[stevenaw]

Thanks for noticing and fixing this @RenderMichael ! LGTM

[jnm2]

I apologize for missing #4543. What is the reason that we don't use CSharpPatternBasedAwaitAdapter for ValueTask?

[RenderMichael]

What is the reason that we don't use CSharpPatternBasedAwaitAdapter for ValueTask

Benchmark code

using BenchmarkDotNet.Attributes;

using NUnit.Framework.Internal;
using NUnit.TestData;

namespace NUnit.Framework;

[MemoryDiagnoser(false)]
public class AwaitableBenchmarks
{
    public IEnumerable<object> ValueData()
    {
        yield return Task.Yield();
        yield return ValueTask.FromResult(30);
        yield return ValueTask.FromException<object>(new Exception());
        yield return new AwaitableReturnTypeFixture.CustomAwaitable();
        yield return new AwaitableReturnTypeFixture.CustomAwaitableWithImplicitOnCompleted();
        yield return new AwaitableReturnTypeFixture.CustomAwaitableWithImplicitUnsafeOnCompleted();
    }

    [Benchmark]
    [ArgumentsSource(nameof(ValueData))]
    public AwaitAdapter? After(object maybeTask)
    {
        var awaiter = ValueTaskAwaitAdapter.TryCreate(maybeTask)
            ?? CSharpPatternBasedAwaitAdapter.TryCreate(maybeTask)
            ?? FSharpAsyncAwaitAdapter.TryCreate(maybeTask);

        if (awaiter is not null)
        {
            return awaiter;
        }

        throw new InvalidOperationException();
    }

    [Benchmark(Baseline = true)]
    [ArgumentsSource(nameof(ValueData))]
    public AwaitAdapter? Before(object maybeTask)
    {
        var awaiter = CSharpPatternBasedAwaitAdapter.TryCreate(maybeTask)
            ?? FSharpAsyncAwaitAdapter.TryCreate(maybeTask);

        if (awaiter is not null)
        {
            return awaiter;
        }

        throw new InvalidOperationException();
    }
}

Method	maybeTask	Mean	Error	StdDev	Ratio	RatioSD	Allocated	Alloc Ratio
After		634.9 ns	12.32 ns	16.86 ns	0.65	0.04	296 B	1.32
Before		964.9 ns	19.30 ns	44.34 ns	1.00	0.00	224 B	1.00
After	30	596.2 ns	11.38 ns	17.37 ns	0.59	0.03	288 B	1.33
Before	30	993.7 ns	19.83 ns	54.28 ns	1.00	0.00	216 B	1.00
After	NUnit(...)table [57]	938.2 ns	18.77 ns	28.66 ns	1.02	0.06	240 B	1.00
Before	NUnit(...)table [57]	924.4 ns	18.22 ns	32.39 ns	1.00	0.00	240 B	1.00
After	NUnit(...)leted [80]	926.4 ns	10.37 ns	8.66 ns	0.98	0.03	240 B	1.00
Before	NUnit(...)leted [80]	945.7 ns	18.42 ns	24.59 ns	1.00	0.00	240 B	1.00
After	NUnit(...)leted [86]	942.3 ns	18.42 ns	22.62 ns	1.03	0.03	240 B	1.00
Before	NUnit(...)leted [86]	916.5 ns	17.82 ns	19.81 ns	1.00	0.00	240 B	1.00
After	Syste(...)table [46]	908.2 ns	17.76 ns	24.31 ns	1.03	0.03	208 B	1.00
Before	Syste(...)table [46]	884.8 ns	17.68 ns	23.60 ns	1.00	0.00	208 B	1.00

I'm not versed enough in BenchmarkDotNet to give a better parameter name than the default ToString() it gives for maybeTask, but the first two cases are ValueTask<>s and the rest are custom awaiters. Note that this check happens after we check for Task, Task<TResult>, any task derived from Task, as well as the non-generic ValueTask (so in practice, I can't imagine the "custom awaiter test fixture" scenario happens very often)

I can't speak for why it was initially implemented, but it noticeably speeds up the ValueTask<> check (in exchange for higher allocations) without meaningfully regressing the other cases.