[Runtime Async] Support for covariant override of Task- returning base with a Task<T>- returning derived method.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.

[VSadov]

Contributes to: #124238

(Note: this does not deal with the Task<T> -> SomeCustomType return override. That will need a completely different fix, - no special thunks needed in that case).

== The problem:

When we have Task-returning method overriden with Task<T>-returning method, in the paralell "async space" we see a void-returning base logically overridden with a T-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 have Task<T> -> Task case.

== 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 is void-returning. The body of this new variant simply calls the T-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/Other variant lookups must be replaced with explicit lookups that specifically tell what kind of variant is desired.
  Matching/Other was 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 the Other becomes 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 the void-returning base and the T-returning variant will be the base of the new chain of overrides. - Exactly what we need.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

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 to Task-based async variants in base slots.
• Refactor async-variant lookup from “matching/other” semantics to explicit Ordinary vs Async lookup 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

• GetAsyncOtherVariantNoCreate still references HasAsyncOtherVariant() and AsyncVariantLookup::AsyncOtherVariant, but HasAsyncOtherVariant/AsyncOtherVariant are 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 new Ordinary/Async lookup model (potentially adding an GetOrdinaryVariantNoCreate or a new GetOtherVariantNoCreate that handles ReturnDroppingThunk).

 MethodDesc* GetAsyncOtherVariantNoCreate(BOOL allowInstParam = TRUE)
{
_ASSERTE(HasAsyncOtherVariant());
return FindOrCreateAssociatedMethodDesc(this, GetMethodTable(), FALSE, GetMethodInstantiation(), allowInstParam, FALSE, FALSE, AsyncVariantLookup::AsyncOtherVariant);
}

[Copilot]

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;
}

[VSadov]

I think this is ready to be reviewed.

[Copilot]

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

• ClassifyMethodReturnKind only writes *elementTypeLength on the GenericTaskReturningMethod path. 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();

[Copilot]

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;
}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

[MichalStrehovsky]

Disabling tests without filing a bug is not cool. I only found out there may be work to do because you forgot to disable on ReadyToRun and I saw the bugs #126682 and #126683 that got filed for it.