Fix covariant returns scenarios in task-returning methods

[Copilot]

Fixes covariant returns in runtime async scenarios where an override returns a type derived from Task/Task<T> rather than Task/Task<T> itself.

Problem

Task and Task<T> are not sealed, so a covariant override may return, say, MyTask : Task. Such a method is not task-returning as far as the runtime is concerned, so it got no Async variant and could not override the Async variant of the base method. As a result, when the call was made in a way that could become a runtime async call (e.g. await b.M1()), the base implementation ran instead of the override. The same code compiled without the runtime-async feature dispatched correctly.

Fix

• methodtablebuilder.cpp — when enumerating class methods, detect a virtual MethodImpl that requires covariant return type checking and whose decl is task-returning. Such a method is now treated as task-returning and gets exactly one Async variant, whose return type is the element type of the overridden method (void for Task, T for Task<T>). Type variables of the declaring type in T are substituted from the TypeSpec instantiation of the decl, so generic types/methods, closed and composed base instantiations are handled.

• New AsyncMethodFlags::CovariantForwardingThunk — this variant is always a thunk.
  The thunk's purpose is to switch to a different virtual slot and continue dispatching. The MyTask- returning method may be further overriden, thus the thunk needs to contain CALLVIRT to the non-async variant.

The forwarding thunk was chosen over async version so that only the covarainly overriding methods may need another variant and we can continue mostly ignoring non-Task returning methods for async purposes (only covariant overrides have a special case and check if base returns a Task).

• asyncthunks.cpp — EmitCovariantForwardingThunk emits the Async variant as CALLVIRT of the ordinary variant followed by AsyncHelpers.TransparentAwait/TransparentAwait<T> on the returned task.

• FindDeclMethodOnClassInHierarchy — use GetParallelMethodDesc instead of GetAsyncVariant(); the latter may create an InstantiatedMethodDesc and load types, which is not allowed while building a MethodTable (fires a contract violation on checked builds). Only the slot is needed.

• readytoruninfo.cpp — like return-dropping thunks, covariant forwarding thunks are VM-synthesized and share the token/signature shape of a regular async variant, so R2R lookup is skipped for them and the IL is generated transiently in the prestub.

• cDAC / data contract — the new flag is added to AsyncMethodFlags in the contract docs and readers, and the thunk is treated as diagnostics-hidden.

Tests

src/tests/async/covariant-return/covariant-returns.cs adds coverage for:

• overrides returning MyTask/MyTask<int>, awaited directly and observed as Task/Task<T> objects,
• covariant override of a covariant override, and calls on the derived static type,
• overridden methods annotated [RuntimeAsyncMethodGeneration(false)],
• generic methods, generic declaring types, Task<List<T>>/Task<T[]> element types, struct element types, closed (GBase<int>) and composed (GBase<List<U>>) base instantiations.

Not in scope

• NativeAOT / crossgen2 (AOT compilation of these thunks) is not fixed here. The test project is disabled for TestBuildMode == nativeaot; at runtime, R2R lookup for these thunks is bypassed in favor of transient IL. Full AOT support is a follow-up.

• Mono does not support runtime async; the test project is disabled there.

• Fixes [RuntimeAsync] Handle covariant returns scenarios in combination with task-returning methods. #124238

[Copilot]

Copilot wasn't able to review any files in this pull request.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tests/async/covariant-return/covariant-returns.cs:404

• PR metadata says "Fixes #124238", but the added coverage is currently marked [ActiveIssue] (and this PR doesn't include a runtime/product fix), so it doesn't actually fix the issue yet. Consider updating the PR title/description to reflect that this is a repro/coverage-only change until the runtime fix is included and the [ActiveIssue] attributes can be removed.

        [Fact]
        [ActiveIssue("https://github.com/dotnet/runtime/issues/124238")]
        public static void TestCustomTaskOverride()
        {

[VSadov]

@copilot lets try fixing the runtime behavior

• we need to detect the scenario and in this special case the derived methods should be classified as task-returning (not ordinary).
  Basically, the desired invariant is: if the base method is task-returning, then the derived is task-returning.
• the derived method will need to get an async thunk variant, just like other task-returning methods.
  The thunk would naturally forward to the actual non-async method. Since the defined method does not formally return a task, it cannot be async, so it is always a thunk.
• the return type of the async thunk should be the "element" type of the base method: void if it is just Task, T if it is Task<T>.
• the T can be a concrete type like "int" in the current test case. Lets make that work first.
  (do not get concerned with T being a generic type, just yet, will follow up with that later)

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/coreclr/vm/methodtablebuilder.cpp:2666

• PR description says the runtime fix is out of scope for this PR, but this change adds a runtime behavior change in MethodTableBuilder to synthesize async variants for covariant Task overrides. Please update the PR description/checklist (or split into separate PRs) so scope and CI expectations match the actual contents.

// Task and Task<T> are not sealed, thus a covariant override may return a type that derives from
// Task/Task<T>, while not being Task/Task<T> itself. Such a method is not Task-returning on its own,
// but the method that it overrides may well be. Since the overridden method has an Async variant,
// the override must have one as well, or it would not be able to override it.
//

src/coreclr/vm/methodtablebuilder.cpp:2704

• TryGetCovariantOverrideAsyncVariantReturnType bails out for MethodImpl declarations that are MemberRefs with a TypeSpec parent (instantiated generic type). That means covariant overrides of Task/Task-returning methods on closed generic base types won’t get an async variant and will likely keep the incorrect dispatch behavior this change is trying to address.

        // The signature of a member of an instantiated generic type may refer to the generic
        // parameters of that type. Such references do not have the same meaning in the scope of
        // the overriding method, so we cannot reuse the signature. That case is not supported yet.
        mdToken tkParent;
        if (FAILED(pMDInternalImport->GetParentToken(tkDecl, &tkParent)) ||
            (TypeFromToken(tkParent) == mdtTypeSpec))
        {
            return false;
        }

[Copilot]

Copilot review overview

🔵 Needs a closer look

The new suspension behavior and multidimensional-array signature path lack targeted regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Make abstract covariant forwarding variants concrete

Previously missed (1)

In code that hasn't changed since last review

Missing multidimensional-array dispatch regression test

src/​coreclr/​vm/​methodtablebuilder.cpp:2784

This newly added multidimensional-array signature path is not exercised by the regression suite: the generic tests cover T[] (ELEMENT_TYPE_SZARRAY) but never T[,] (ELEMENT_TYPE_ARRAY). Because this branch manually rewrites rank, size, and lower-bound payloads and failure silently suppresses the required async variant, add a covariant Task<T[,]>/MyTask<T[,]> dispatch test (or remove the unsupported branch until it is covered).

[VSadov]

@copilot resolve conflicts

[Copilot]

@copilot resolve conflicts

Resolved in merge commit 62161441a08. MethodDesc::GetAttrs() now preserves both fixes: return-dropping and covariant-forwarding thunks remain concrete even when their metadata method is abstract. This imports main’s #132971 fix as well.

Validated on Linux x64 Debug: clr+libs+host build passed; 24 filtered covariant tests and the full async runner (162 passed, zero failed) passed. Focused review and secret scan passed. Automated review was unavailable; CodeQL skipped C++/C# analysis because the databases were too large.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The tests never exercise suspension and resumption through the newly emitted forwarding thunk.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Decode array lower bounds as signed values

[Copilot]

Copilot review overview

🟡 Changes recommended

Valid custom-modified type signatures can still omit the required async variant and retain incorrect dispatch.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Handle custom modifiers in async override type signatures · New

[Copilot]

Fixed in 323720f9. The signature copier now preserves modreq/modopt tokens and recursively copies their modified types. Added an IL regression with a modified Task<T> argument: runtime-async dispatch failed before the fix and passes afterward (163 async tests passed).

Note

This reply was generated with GitHub Copilot.