Skip to content

Remove TODO for async devirtualization#126825

Merged
MichalStrehovsky merged 1 commit intomainfrom
MichalStrehovsky-patch-2
Apr 13, 2026
Merged

Remove TODO for async devirtualization#126825
MichalStrehovsky merged 1 commit intomainfrom
MichalStrehovsky-patch-2

Conversation

@MichalStrehovsky
Copy link
Copy Markdown
Member

The issue is fixed so this shouldn't be needed.

The issue is fixed so this shouldn't be needed.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes the temporary async-variant exemption around a debug-time devirtualization “unknown” failure check in the CoreCLR JIT interface tooling, now that async devirtualization support is considered fixed.

Changes:

  • Removes the TODO referencing async-variant devirtualization work.
  • Tightens the DEBUG assertion to trigger on any CORINFO_DEVIRTUALIZATION_UNKNOWN failure, including async variants.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126825

Note

This review was generated by GitHub Copilot using multiple AI models (Claude Opus 4.6, Claude Sonnet 4.6, GPT-5.3-Codex). All models independently reached the same conclusion.

Holistic Assessment

Motivation: The original `&& !decl.IsAsyncVariant()` guard was a workaround for #124620, where async variant virtual method resolution was not yet implemented and would fail with `UNKNOWN` detail. Now that `AsyncAwareVirtualMethodResolutionAlgorithm` properly handles async variants, the workaround is dead code that weakens a correctness diagnostic.

Approach: A one-line removal of the suppression condition is the correct and minimal change. No behavioral impact in release builds; in debug builds the assert now covers async variants too, which improves bug detection.

Summary: ✅ LGTM. The change is correct, safe, and well-scoped.


Detailed Findings

✅ Algorithm coverage is complete

`AsyncAwareVirtualMethodResolutionAlgorithm` (in `CompilerTypeSystemContext.Async.cs`) overrides every virtual resolution method that `DevirtualizationManager.ResolveVirtualMethod` dispatches through:

  • Non-interface path: `FindVirtualFunctionTargetMethodOnObjectType` — overridden; decomposes async variants before base resolution, re-wraps result.
  • Interface path (variance): `ResolveInterfaceMethodTargetWithVariance` → `ResolveVariantInterfaceMethodToVirtualMethodOnType` — overridden and async-aware.
  • Interface path (DIM): `ResolveInterfaceMethodToDefaultImplementationOnType` — overridden and async-aware.
  • All other variant resolution methods (`ResolveInterfaceMethodToStaticVirtualMethodOnType`, `ResolveVariantInterfaceMethodToDefaultImplementationOnType`, etc.) — all overridden with the same decompose/resolve/re-wrap pattern.

The `defaultInterfaceDispatchDeclMethod` lookup in `DevirtualizationManager` (`iface == declMethod.OwningType`) also works correctly for async variants because `AsyncMethodVariant` inherits from `MethodDelegator`, which delegates `OwningType` to the wrapped method — so the owning interface type is preserved.

✅ Remaining UNKNOWN paths are not async-specific

`ResolveVirtualMethod` does have paths where `impl` can be null while `devirtualizationDetail` stays `UNKNOWN` (e.g., interface method not found in `RuntimeInterfaces`, or `FindVirtualFunctionTargetMethodOnObjectType` returning null for non-interface methods). In both cases, the async-aware overrides decompose the async variant and call the base algorithm for the underlying non-async method. If the base algorithm still fails, it is a genuine resolution failure unrelated to async — and the assert should fire for those.

✅ Blast radius is minimal

The assert is `#if DEBUG` only. Release builds are completely unaffected. If the assert fires unexpectedly, the consequence is a `Console.Error.WriteLine` + `Debug.Assert` crash in debug tooling — the method still safely returns `false`, so no codegen correctness issue.

✅ No orphaned code

`IsAsyncVariant()` is used in 10+ other locations across the codebase and is not orphaned by this removal. No additional cleanup needed.

Generated by Code Review for issue #126825 ·

@MichalStrehovsky
Copy link
Copy Markdown
Member Author

/ba-g deadletter

@MichalStrehovsky MichalStrehovsky merged commit 33be435 into main Apr 13, 2026
127 of 131 checks passed
@MichalStrehovsky MichalStrehovsky deleted the MichalStrehovsky-patch-2 branch April 13, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants