-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Fix TypeLoadException when async Task<T> overrides Task #124062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
674b5ee
86c98da
f90f25b
7be6836
89130b3
e719797
ab7f774
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Threading.Tasks; | ||
| using Xunit; | ||
|
|
||
| namespace AsyncCovariantReturnTest; | ||
|
|
||
| public static class Program | ||
| { | ||
| [Fact] | ||
| public static void TestEntryPoint() | ||
| { | ||
| // This test validates that async methods with covariant-like return type signatures | ||
| // do not trigger TypeLoadException during type loading. | ||
| // | ||
| // Background: The C# compiler allows overriding a Task method with an async Task<T> method | ||
| // because the async keyword changes the method's semantics. The compiler generates | ||
| // async variant methods with different signatures to support the async calling convention. | ||
| // These variant methods have the unwrapped return type (T instead of Task<T>). | ||
| // | ||
| // The issue was that the runtime's covariant return type validator was incorrectly | ||
| // treating these synthetic async variant methods as invalid overrides, causing a | ||
| // TypeLoadException even though the code is valid and compiles successfully. | ||
| // | ||
| // This test ensures that such valid code can be loaded without throwing. | ||
|
|
||
|
agocke marked this conversation as resolved.
|
||
| GC.KeepAlive(new Derived()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a plan to add actual test coverage for covariant returns? The test here inherits whatever is the global runtime-async setting, but I believe it might be useful to have real coverage under tests/async that tests: actually calling the method, pairs of "base is runtime-async, override is not" and inverted. This test has a minimal value because it surgically only checks type load and nothing else. We don't get much bang for our RequiresProcessIsolation buck. |
||
| } | ||
| } | ||
|
|
||
| public abstract class Base | ||
| { | ||
| public abstract Task HandleAsync(); | ||
| } | ||
|
|
||
| public class Derived : Base | ||
| { | ||
| // This override is valid C# code that compiles successfully. | ||
| // The C# compiler allows overriding Task with async Task<T> because the async keyword | ||
| // changes how the method is compiled. However, the runtime generates async variant methods | ||
| // with different signatures (returning T instead of Task<T>) to support the async calling | ||
|
agocke marked this conversation as resolved.
|
||
| // convention. The covariant return type validator should skip these synthetic methods. | ||
| public override async Task<string> HandleAsync() | ||
| { | ||
| await Task.Yield(); | ||
| return string.Empty; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <RequiresProcessIsolation>true</RequiresProcessIsolation> | ||
| </PropertyGroup> | ||
|
Comment on lines
+2
to
+4
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this require process isolation? |
||
| <ItemGroup> | ||
| <Compile Include="$(MSBuildProjectName).cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <ProjectReference Include="$(TestLibraryProjectPath)" /> | ||
| </ItemGroup> | ||
| </Project> | ||
Uh oh!
There was an error while loading. Please reload this page.