Treat ValueTask<TResult> as an asynchronous return type #70
Conversation
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
==========================================
+ Coverage 81.26% 81.54% +0.28%
==========================================
Files 38 38
Lines 2856 2943 +87
Branches 184 188 +4
==========================================
+ Hits 2321 2400 +79
- Misses 440 444 +4
- Partials 95 99 +4
Continue to review full report at Codecov.
|
sharwell
force-pushed
the
support-valuetask
branch
from
86945af
to
a770626
Compare
| class ClassName | ||
| { | ||
| ValueTask<int> FirstMethod() { return new ValueTask<int>(3); } | ||
| ValueTask<int> SecondMethodAsync() { return new ValueTask<int>(3); } |
There was a problem hiding this comment.
How about a test for the non-generic ValueTask type as well?
There was a problem hiding this comment.
It's not available in the System.Threading.Tasks.Extensions package and requires the test to compile against .NET Core. The test project doesn't currently support this to my knowledge.
There was a problem hiding this comment.
The non-generic ValueTask struct certainly is available from that same package, and it does not require .NET Core as a target.
In my Nerdbank.Streams project I use ValueTask in many places. Hitting F12 on it reveals it is defined here:
.nuget\packages\system.threading.tasks.extensions\4.5.0\ref\netstandard1.0\System.Threading.Tasks.Extensions.dll
There was a problem hiding this comment.
👍 I was going by these pages:
https://apisof.net/catalog/System.Threading.Tasks.ValueTask%3CTResult%3E
https://apisof.net/catalog/System.Threading.Tasks.ValueTask
Looks like it hasn't been updated for the 4.5.0 release of these packages.
There was a problem hiding this comment.
Ya, apisof.net has been the most accurate reference for this info (much better than msdn/docs). But it does appear to be outdated now. :(
| @@ -25,7 +25,8 @@ public static bool HasAsyncSignature(this IMethodSymbol symbol, bool treatAsyncV | |||
| if (!symbol.IsAsync) | |||
| { | |||
| // This check conveniently covers Task and Task<T> by ignoring the `1 in Task<T>. | |||
| if (!string.Equals(nameof(Task), symbol.ReturnType?.Name, StringComparison.Ordinal)) | |||
| if (!string.Equals(nameof(Task), symbol.ReturnType?.Name, StringComparison.Ordinal) | |||
| && !string.Equals("ValueTask", symbol.ReturnType?.Name, StringComparison.Ordinal)) | |||
There was a problem hiding this comment.
Why not allow any type whose GetAwaitInfo suggests the type is awaitable?
There was a problem hiding this comment.
❓ Is there an easy way to do this using existing Roslyn APIs? Otherwise I was planning to leave it as a follow-up change.
There was a problem hiding this comment.
I'm pretty sure it's been there since the earliest versions. Let me find it...
There was a problem hiding this comment.
Ok, never mind. The API I was thinking of is SemanticModel.GetAwaitExpressionInfo which accepts an await expression rather than a type that may be awaited on.
There was a problem hiding this comment.
I'll file a follow-up issue to go based on AsyncMethodBuilderAttribute:
There was a problem hiding this comment.
Rather than base it on whether a method's return type is compatible with the async method declaration modifier, why not just expect/require an Async suffix anytime the returned type is awaitable? For example, people may return custom awaitables from non-async methods that nevertheless dictate that the calling pattern for the method is to await the result. SwitchToMainThreadAsync is an example of this. We have several other custom awaitables returned from various Async-suffixed methods as well.
There was a problem hiding this comment.
@sharwell there is http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Shared/Extensions/ISymbolExtensions.cs,882
Or you can roll your own:
internal static bool IsAwaitable(this ITypeSymbol type)
{
return type.TryFindFirstMethod("GetAwaiter", x => x.Parameters.Length == 0, out var method) &&
method.ReturnType is ITypeSymbol returnType &&
returnType.TryFindFirstMethod("GetResult", x => x.Parameters.Length == 0, out _);
}The above does not check for extension methods and there can be more checks on the awaiter type.
sharwell
force-pushed
the
support-valuetask
branch
from
5473b23
to
ca7bcad
Compare
Fixes #68