[Runtime Async] Support for covariant override of Task- returning base with a Task<T>- returning derived method.#125900
Merged
VSadov merged 21 commits intodotnet:mainfrom Apr 8, 2026
Merged
Conversation
Contributor
|
Tagging subscribers to this area: @agocke |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR’s Runtime Async “variant/thunk” machinery to correctly support covariant overrides where a base virtual returns Task and an override returns Task<T> by introducing an additional void-returning async thunk that drops the T result, and adds a regression test covering the scenario.
Changes:
- Add a new “return dropping” async thunk variant used to bridge
Task<T>overrides back toTask-based async variants in base slots. - Refactor async-variant lookup from “matching/other” semantics to explicit
OrdinaryvsAsynclookup and thread that through associated-method/parallel-method resolution. - Add an async test case for
Task→Task<T>covariant override behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/async/covariant-return/covariant-returns.csproj | Adds a new Runtime Async test project for covariant-return scenarios. |
| src/tests/async/covariant-return/covariant-returns.cs | Adds a regression test for Task base overridden by Task<T> and derived async override cases. |
| src/coreclr/vm/zapsig.cpp | Updates associated-MethodDesc resolution to use explicit Ordinary/Async variant lookup. |
| src/coreclr/vm/stubmgr.cpp | Adjusts debugger thunk step-through logic to use the renamed variant accessor. |
| src/coreclr/vm/runtimehandles.cpp | Ensures reflection normalizes away async variants via GetOrdinaryVariant. |
| src/coreclr/vm/methodtablebuilder.h | Removes builder-time “other variant” linkage and switches to lookup-based selection. |
| src/coreclr/vm/methodtablebuilder.cpp | Adds a third declared-method pass for return-dropping async thunks in covariant override cases; updates async-variant resolution logic. |
| src/coreclr/vm/methodtable.h | Splits GetParallelMethodDesc into overloads (no-lookup + lookup). |
| src/coreclr/vm/methodtable.cpp | Implements new overload structure and updates async-variant mapping logic. |
| src/coreclr/vm/method.hpp | Introduces ReturnDroppingThunk flag, MatchesAsyncVariantLookup, new variant accessors, and updated associated-method APIs. |
| src/coreclr/vm/method.cpp | Extends return-kind classification to capture generic element type length for signature rewriting. |
| src/coreclr/vm/memberload.cpp | Updates associated-MethodDesc creation call sites for new signature/lookup behavior. |
| src/coreclr/vm/jitinterface.cpp | Updates JIT query for “other async variant” to use GetAsyncVariant/GetOrdinaryVariant. |
| src/coreclr/vm/genmeth.cpp | Updates associated-MethodDesc logic to use explicit MatchesAsyncVariantLookup checks and new parameter ordering. |
| src/coreclr/vm/class.cpp | Removes special-case skipping of async variants during covariant return validation. |
| src/coreclr/vm/asyncthunks.cpp | Adds return-dropping thunk IL emission and refactors token selection into a helper. |
Comments suppressed due to low confidence (1)
src/coreclr/vm/method.hpp:1748
GetAsyncOtherVariantNoCreatestill referencesHasAsyncOtherVariant()andAsyncVariantLookup::AsyncOtherVariant, butHasAsyncOtherVariant/AsyncOtherVariantare no longer defined after the enum/API rename. This will break the build and also leaves existing callers (e.g. DAC) without a valid no-create path. Update/remove this helper to use the newOrdinary/Asynclookup model (potentially adding anGetOrdinaryVariantNoCreateor a newGetOtherVariantNoCreatethat handles ReturnDroppingThunk).
MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/debug/daccess/dacdbiimpl.cpp:1289
- GetNativeCodeInfo attempts to map async thunk MethodDescs to the non-thunk variant (so the debugger can report code info for the actual implementation). Replacing GetAsyncOtherVariantNoCreate() with GetOrdinaryVariantNoCreate() breaks this for cases where the ordinary method is itself the thunk (e.g., MethodImpl.Async), and for ReturnDroppingThunk it will also point at the wrong target. Consider switching back to “other variant” semantics (choose Async vs Ordinary based on pMethodDesc->IsAsyncMethod() and whether it is a ReturnDroppingThunk) while keeping the no-create behavior.
if (pMethodDesc != NULL && pMethodDesc->IsAsyncThunkMethod())
{
MethodDesc* pAsyncVariant = pMethodDesc->GetOrdinaryVariantNoCreate();
if (pAsyncVariant != NULL)
{
pMethodDesc = pAsyncVariant;
}
Member
Author
|
I think this is ready to be reviewed. |
This was referenced Mar 24, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/coreclr/vm/method.cpp:2402
ClassifyMethodReturnKindonly writes*elementTypeLengthon theGenericTaskReturningMethodpath. Since this is an out-parameter, initialize it (e.g., set to 0 near the start of the function) so callers don’t accidentally read an uninitialized value in non-generic/non-task cases.
MethodReturnKind ClassifyMethodReturnKind(SigPointer sig, Module* pModule, ULONG* offsetOfAsyncDetails, ULONG* elementTypeLength, bool *isValueTask)
{
// Without runtime async, every declared method is classified as a NormalMethod.
// Thus code that handles runtime async scenarios becomes unreachable.
if (!g_pConfig->RuntimeAsync())
{
return MethodReturnKind::NormalMethod;
}
PCCOR_SIGNATURE initialSig = sig.GetPtr();
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/debug/daccess/dacdbiimpl.cpp:1289
- GetNativeCodeInfo resolves a MethodDesc from a metadata token, so when it finds an async thunk MethodDesc it needs to map to the other (non-thunk) variant that contains the actual body for correct breakpoint/code-region reporting. Switching to GetOrdinaryVariantNoCreate() does not do that for ordinary Task/ValueTask-returning thunks (it returns the same thunk). Consider selecting the non-thunk counterpart explicitly (e.g., for an ordinary thunk, use the async variant; for an async-variant thunk, use the ordinary variant), and keep this in the no-create path.
if (pMethodDesc != NULL && pMethodDesc->IsAsyncThunkMethod())
{
MethodDesc* pAsyncVariant = pMethodDesc->GetOrdinaryVariantNoCreate();
if (pAsyncVariant != NULL)
{
pMethodDesc = pAsyncVariant;
}
jakobbotsch
reviewed
Mar 25, 2026
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| @@ -0,0 +1,9 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <!-- Async covariant return NYI in NativeAOT --> | |||
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contributes to: #124238
(Note: this does not deal with the
Task<T>->SomeCustomTypereturn override. That will need a completely different fix, - no special thunks needed in that case).== The problem:
When we have
Task-returning method overriden withTask<T>-returning method, in the paralell "async space" we see avoid-returning base logically overridden with aT-returning derived. That cannot work as-is in a virtual call as the caller/calee would be in disagreement whether there is a need for the return buffer and for a return slot in the continuation.We need a thunk that could override the void-returning base and bridge into derived methods that are now in the T-returning virtual slot.
There are some additional inconveniences from the part that method descs/slots are added very early in the type system construction when we may not know exact types returned by the base/derived.
However, since
Task<T>has uncomplicated base hierarchy, we can figure with good certainty that we may haveTask<T>->Taskcase.== Implementation strategy.
Introduce a special kind of an async variant that has the same signature as the regular
T-returning async variant, except that it isvoid-returning. The body of this new variant simply calls theT-returning async variant and then ignores the return.In a case when we may have
Task->Task<T>return override, we introduce 2 async variants for the overriding method.I.E. we may end up with 3 method descs/bodies that represent the same method definition in IL:
--
Task<T>-returning variant,-- regular
T-returning Async variant. Either this or the one above has user code. and-- special
void-returnig Async variant. Always a thunk.Since we can have 3 variants, the existing
Matching/Othervariant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.Matching/Otherwas already somewhat confusing when getting deeper into helper calls as we need extra context of what we are Other from or Matching. With 3 variants theOtherbecomes even more confusing.One observation - we never need to lookup for the return-dropping thunk, thus we do not need a lookup value for it. Noone should call this thunk directly. The only purpose of the thunk is to override
void-returning base. The model allows for adding a lookup value that matches return-dropping thunk, just it seems there is no need.When matching overrides for async variants we require that derived/base signatures are completely equivalent - including return types.
This way the
void-returning thunk will match with thevoid-returning base and theT-returning variant will be the base of the new chain of overrides. - Exactly what we need.