-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Move runtime async method validation into initial binding #78310
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
Move runtime async method validation into initial binding #78310
Conversation
…ually perform constraint validation on generically-constructed runtime helper methods.
// Keep in sync with VB's AssemblySymbol.RuntimeSupportsAsyncMethods | ||
internal bool RuntimeSupportsAsyncMethods | ||
=> RuntimeSupportsFeature(SpecialMember.System_Runtime_CompilerServices_RuntimeFeature__Async) | ||
|| _overrideRuntimeSupportsAsyncMethods; | ||
=> GetSpecialType(InternalSpecialType.System_Runtime_CompilerServices_AsyncHelpers) is { TypeKind: TypeKind.Class, IsStatic: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the runtime feature entry from the design doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the resolution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was removed.
src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/RuntimeAsyncRewriter.cs
Show resolved
Hide resolved
&& !ReferenceEquals(methodReturn, GetSpecialType(InternalSpecialType.System_Threading_Tasks_ValueTask)) | ||
&& !ReferenceEquals(methodReturn, GetSpecialType(InternalSpecialType.System_Threading_Tasks_ValueTask_T))) | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a corresponding update to the design doc? Or should we have a follow-up comment to make the void
-returning method scenario work at some point? #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the design doc, the runtime-side is very clear that only Task/ValueTask methods can be runtime async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (iteration 10)
@jcouv addressed feedback. @RikkiGibson for review please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 12). Consider updating the design doc (removing runtime feature flag) in same PR
It looks like the following tests need to be updated
|
The doc is maintained in |
break; | ||
|
||
default: | ||
Debug.Fail($"Unexpected RuntimeAsyncAwaitMethod: {RuntimeAsyncAwaitMethod.Name}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking my understanding: it looks like we expect this condition to never occur. i.e. compiler will never produce a BoundAwaitableInfo
whose RuntimeAsyncAwaitMethod.Name
is not one of the above cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some minor comments/suggestions
return false; | ||
} | ||
|
||
Debug.Assert((runtimeAwaitHelper is { Arity: 1 } && maybeResultType is { }) || runtimeAwaitHelper.TypeParameters.Length == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe slightly more uniform/compact check would be:
Debug.Assert((runtimeAwaitHelper is { Arity: 1 } && maybeResultType is { }) || runtimeAwaitHelper.TypeParameters.Length == 0); | |
Debug.Assert(runtimeAwaitHelper.Arity == (maybeResultType is null ? 0 : 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeWithAnnotations also has its own HasType
which distinguishes default values from "meaningful" values, which could be used instead of System.Nullable
, but, there's no deep need to do that.
@@ -286,7 +302,90 @@ private bool GetAwaitableExpressionInfo( | |||
TypeSymbol awaiterType = getAwaiter.Type!; | |||
return GetIsCompletedProperty(awaiterType, node, expression.Type!, diagnostics, out isCompleted) | |||
&& AwaiterImplementsINotifyCompletion(awaiterType, node, diagnostics) | |||
&& GetGetResultMethod(getAwaiter, node, expression.Type!, diagnostics, out getResult, out getAwaiterGetResultCall); | |||
&& GetGetResultMethod(getAwaiter, node, expression.Type!, diagnostics, out getResult, out getAwaiterGetResultCall) | |||
&& (!isRuntimeAsyncEnabled || getRuntimeAwaitAwaiter(awaiterType, out runtimeAsyncAwaitMethod, expression.Syntax, diagnostics)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking my understanding on the sequence of checks here.
- if runtime async, and got runtime await helper, use that
- otherwise, need an "ordinary" GetAwaiterMethod to proceed--if we didn't get one just return false.
- then check for IsCompleted, GetResult, ..
- finally if runtime async enabled, then get a RuntimeAwaitAwaiter?
Is the idea that in the 'RuntimeAwaitAwaiter' case, we still need to use the type's GetAwaiter method etc? That seems to make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that in the 'RuntimeAwaitAwaiter' case, we still need to use the type's GetAwaiter method etc?
Precisely. This the lowering mechanism in https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Runtime%20Async%20Design.md#await-any-other-type, as opposed to the rest of the document which talks about await Task
} | ||
|
||
var methodReturn = method.ReturnType.OriginalDefinition; | ||
if (!ReferenceEquals(methodReturn, GetSpecialType(InternalSpecialType.System_Threading_Tasks_Task)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there a way to say something like methodReturn.InternalSpecialType is InternalSpecialType.Task or Task_T or ValueTask or ValueTask_T
. No need to try and dig up a way to do that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think we can, since this is special types, not well-known types.
src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/RuntimeAsyncRewriter.cs
Show resolved
Hide resolved
@jcouv @RikkiGibson for another look, a few small changes in the last commit. |
…ndedSpecialType.ToString`.
@jcouv, since the last 2 commits were only minor changes, I'm going to go ahead and merge this now; if you have any comments on these last 2 bits, I can address them in the next PR. |
We now do method construction and validation for runtime async helpers up front in initial binding, rather than doing it in
RuntimeAsyncRewriter
. I've also renamed the APIs as per dotnet/runtime#114310 (comment) (though I haven't added ConfigureAwait support yet, that will be the next PR). We now validate:System.Runtime.CompilerServices.AsyncHelpers
, defined in corelib. This means that I now need a fairly extensive corelib mock to be able to compile. When we have a testing runtime that defines these helpers, we can remove the giant mock and use the real one.Task
,Task<T>
,ValueTask
, orValueTask<T>
.Relates to test plan #75960