Skip to content

Conversation

@VSadov
Copy link
Member

@VSadov VSadov commented Dec 13, 2025

Fixes: #121764
Fixes: #121762

Following up on TODOs. Contributes to: #115094

Added scenarios for other ways to get reflection info on methods.
Checks that we only see the formal definitions, never thunks/stubs.

@VSadov VSadov added this to the 11.0.0 milestone Dec 13, 2025
Copilot AI review requested due to automatic review settings December 13, 2025 03:28
@dotnet-policy-service
Copy link
Contributor

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

@VSadov
Copy link
Member Author

VSadov commented Dec 13, 2025

@MichalStrehovsky - are the new scenarios AOT-compatible?

Copy link
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

This pull request removes reflection-related TODOs from the RuntimeAsync feature and adds comprehensive tests for async method reflection behavior. The changes enable RuntimeAsync by default and verify that async method variants are properly handled by the reflection APIs.

Key changes:

  • Added three new test cases (CurrentMethod, FromStack, EnumerateAll) to validate reflection behavior with async methods
  • Enabled RuntimeAsync by default (configuration value changed from 0 to 1)
  • Removed completed TODOs and fixed a typo in C++ comments

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/tests/async/reflection/reflection.csproj New test project file with proper structure and TestLibrary reference
src/tests/async/reflection/reflection.cs Added three new test methods to verify MethodBase.GetCurrentMethod(), StackFrame.GetMethod(), and Type.GetMethods() behavior with async methods
src/tests/async/Directory.Build.targets Removed property group that was disabling tests for non-nativeaot builds, enabling the tests to run
src/coreclr/vm/runtimehandles.cpp Fixed typo: "varinat" → "variant" and removed completed TODO comment
src/coreclr/vm/commodule.cpp Removed completed TODO comment about async variant support
src/coreclr/inc/clrconfigvalues.h Changed RuntimeAsync default value from 0 to 1, enabling the feature by default
Comments suppressed due to low confidence (2)

src/tests/async/reflection/reflection.cs:346

  • The Assert.Equal parameters are in the wrong order. According to xUnit conventions, the expected value should be the first parameter and the actual value should be the second parameter. Currently it's asserting Equal(actual[i], expected[i]) when it should be Equal(expected[i], actual[i]).
    src/tests/async/reflection/reflection.cs:341
  • The array initializer has inconsistent indentation. The first element "Boolean Equals(System.Object)" is indented with 12 spaces, but the remaining elements (lines 333-341) are indented with 17 spaces. All elements should have consistent indentation.

@VSadov
Copy link
Member Author

VSadov commented Dec 14, 2025

NativeAOT seems to have a bug here

How did reflection get a RuntimeMethodHandle for an async variant?
   at System.Diagnostics.DebugProvider.Fail(String, String) + 0x36
   at System.Diagnostics.Debug.Fail(String, String) + 0x32
   at Internal.Runtime.TypeLoader.TypeLoaderEnvironment.TryGetRuntimeMethodHandleComponents(RuntimeMethodHandle, RuntimeTypeHandle&, QMethodDefinition&, RuntimeTypeHandle[]&) + 0x6c
   at Internal.Reflection.Augments.ReflectionAugments.GetMethodFromHandle(RuntimeMethodHandle) + 0x65
   at async!<BaseAddress>+0x3055aa
   at async!<BaseAddress>+0x2fd22e
   at System.Runtime.CompilerServices.AsyncHelpers.RuntimeAsyncTask`1.DispatchContinuations() + 0x13e
   at System.Threading.ThreadPoolWorkQueue.Dispatch() + 0x196
   at System.Threading.WindowsThreadPool.DispatchCallback(IntPtr, IntPtr, IntPtr) + 0x7d
Expected: 100

@MichalStrehovsky
Copy link
Member

NativeAOT seems to have a bug here

Yep, MethodBase.GetCurrentMethod is annotated as RequiresUnreferencedCode, so it can be broken with native AOT (we generate a warning, and therefore the API might not work), but this is fixable. I filed #122546 on it, let's ActiveIssue this new test on that.

@MichalStrehovsky - are the new scenarios AOT-compatible?

StackFrame.GetMethod is also annotated as RequiresUnreferencedCode so it may not work (and will not). However, we have DiagnosticMethodInfo.Create(StackFrame) API that is trim safe. On CoreCLR it simply builds on top of StackFrame.GetMethod. If you update the test to use that API, we can ActiveIssue it on #122547. If it stays as StackFrame.GetMethod, it's unsuspportable and the test should not run on native AOT.

@VSadov
Copy link
Member Author

VSadov commented Dec 16, 2025

Thanks!

@VSadov VSadov enabled auto-merge (squash) December 16, 2025 02:21
@VSadov VSadov disabled auto-merge December 16, 2025 03:25
@VSadov VSadov enabled auto-merge (squash) December 16, 2025 03:28
@VSadov VSadov merged commit 831d3cc into dotnet:main Dec 16, 2025
102 of 104 checks passed
@VSadov VSadov deleted the refTODOs branch December 16, 2025 14:09
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.

[RuntimeAsync] TODO in runtimehandles.cpp [RuntimeAsync] TODO in ModuleBuilder_GetMemberRefOfMethodInfo

3 participants