[naot] [Runtime async] Support for covariant override of Task -> Task<T>#126768
[naot] [Runtime async] Support for covariant override of Task -> Task<T>#126768eduardo-vp wants to merge 6 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Mirrors the CoreCLR runtime-async fix for covariant overrides (Task base overridden by Task<T> derived) into the NativeAOT type system by introducing a return-dropping async thunk and wiring it into virtual/interface resolution, and re-enables the previously NativeAOT-disabled test.
Changes:
- Add
ReturnDroppingAsyncThunkand a corresponding IL stub emitter that calls theT-returning async variant and discards the result. - Update NativeAOT async-aware virtual/interface method resolution to select the return-dropping thunk when the async slot is
void-returning but the resolved override isT-returning. - Re-enable the covariant-return async test project for NativeAOT.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/async/covariant-return/covariant-returns.csproj | Re-enables building the test under NativeAOT. |
| src/coreclr/vm/asyncthunks.cpp | Adds a comment noting parity with the managed type system emitter. |
| src/coreclr/tools/Common/TypeSystem/IL/Stubs/AsyncThunks.cs | Adds IL emission for the return-dropping async thunk. |
| src/coreclr/tools/Common/TypeSystem/IL/NativeAotILProvider.cs | Routes ReturnDroppingAsyncThunk to the new IL emitter. |
| src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Async.cs | Wraps resolved async variants with a return-dropping thunk for Task -> Task<T> async-slot mismatches; adds caching. |
| src/coreclr/tools/Common/Compiler/AsyncMethodVariant.Mangling.cs | Adds prefix mangling support for ReturnDroppingAsyncThunk. |
| src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs | Introduces ReturnDroppingAsyncThunk type and updates async-thunk classification helpers. |
| codestream.EmitLdArg(localArg++); | ||
| } | ||
|
|
||
| codestream.Emit(ILOpcode.call, emitter.NewToken(asyncVariantTarget)); |
There was a problem hiding this comment.
EmitReturnDroppingThunk says it matches the VM and the comment notes CALLVIRT, but this stub currently emits ILOpcode.call (line 424). call bypasses virtual dispatch and doesn't match CoreCLR's EmitCALLVIRT; use callvirt for the instance case (and only use call if the thunk can be static).
| codestream.Emit(ILOpcode.call, emitter.NewToken(asyncVariantTarget)); | |
| ILOpcode callOpcode = asyncVariantTarget.Signature.IsStatic ? ILOpcode.call : ILOpcode.callvirt; | |
| codestream.Emit(callOpcode, emitter.NewToken(asyncVariantTarget)); |
| // } | ||
|
|
||
| int localArg = 0; | ||
| codestream.EmitLdArg(localArg++); |
There was a problem hiding this comment.
Argument loading in EmitReturnDroppingThunk unconditionally does EmitLdArg(0) and then loops over sig.Length, which is correct for instance methods but will push one extra arg for static signatures. Either assert this thunk is never static, or mirror EmitAsyncMethodThunk and only load this when !sig.IsStatic.
| codestream.EmitLdArg(localArg++); | |
| if (!sig.IsStatic) | |
| { | |
| codestream.EmitLdArg(localArg++); | |
| } |
| public static bool IsAsyncThunk(this MethodDesc method) | ||
| { | ||
| return method.IsAsyncVariant() ^ method.IsAsync; | ||
| return (method.IsAsyncVariant() ^ method.IsAsync) || method.IsReturnDroppingAsyncThunk(); | ||
| } |
There was a problem hiding this comment.
ReturnDroppingAsyncThunk is used as the resolved target for async-variant virtual calls, but it isn't covered by IsAsyncCall (currently based on IsAsyncVariant() / IsAsync). That risks the thunk being compiled/called without CORINFO_CALLCONV_ASYNCCALL in non-runtime-async scenarios, causing an ABI mismatch. Ensure async-call detection includes IsReturnDroppingAsyncThunk() (or similar).
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Mirrors work in #125900 for Native AOT.
Fixes #126685.