Add GetEcmaDefinition extension method for MethodDesc in crossgen2

[Copilot]

• Create a new extension method file R2RMethodExtensions.cs in ILCompiler.ReadyToRun/Compiler that adds a GetEcmaDefinition() extension method for MethodDesc
• The extension method returns EcmaMethod? and handles AsyncMethodVariant by returning its Target property
• Handle PInvokeTargetNativeMethod by mapping to its Target property in the extension method
• Update places in CorInfoImpl.ReadyToRun.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in ReadyToRunCompilationModuleGroupBase.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in ReadyToRunTableManager.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in ReadyToRunCodegenNodeFactory.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in InstanceEntryPointTableNode.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in InstrumentationDataTableNode.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in InliningInfoNode.cs that cast GetTypicalMethodDefinition() to EcmaMethod to use the new extension method
• Update places in ProfileDataManager.cs that check GetTypicalMethodDefinition() is EcmaMethod to use the new extension method
• Update places in ModuleTokenResolver.cs that pattern match GetTypicalMethodDefinition() is EcmaMethod to use the new extension method
• Update places in ReadyToRunCodegenCompilation.cs that pattern match GetTypicalMethodDefinition() is EcmaMethod to use the new extension method
• Update places in ReadyToRunVisibilityRootProvider.cs that pattern match GetTypicalMethodDefinition() is EcmaMethod to use the new extension method
• Build the crossgen2 to verify compilation
• Run code review and address any issues
• Run security scan (no issues found)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>MethodDefinition is overloaded in crossgen to mean "noninstantiated method" and "method with metadata"</issue_title>
<issue_description>Crossgen2 seems to use GetMethodDefinition and GetTypicalMethodDefinition as both "get the uninstantiated version of this method" and "get the EcmaMethod that has info about its metadata". In many places in crossgen, we cast the return value of GetTypicalMethodDefinition to EcmaMethod, assuming that this is valid for all methods that have metadata. The way we constructed Async methods invalidates that assumption, as AsyncMethodVariant is the definition and can be returned from GetTypicalMethodDefinition.

It seems like a crossgen2-specific extension method for the functionality would be the best solution to keep the base type system simple while solving the problem.</issue_description>

<agent_instructions>Find a good location under the ILCompiler.ReadyToRun tree to hold this functionality, or create a new file under that tree. Update places in the ILCompiler project that cast GetTypicalMethodDefinition to EcmaMethod and use this extension method instead</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@jkotas I see ~10 hits in crossgen when I search for `EcmaMethod.*GetTypicalMethodDefinition`. There are some more hits that this search does not pick up. It does not seem like that many.

It is not clear to me how many of these places would work well with the proposed method. It depends on what the caller is interested in exactly. It can be method signature, method attributes and impl flags, method IL, method token or method containing module, ... .

For example, I do not think the proposed replacements would work well for async thunks here:

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

Line 781 in eeaef45

if (MethodBeingCompiled.GetTypicalMethodDefinition() is EcmaMethod ecmaMethod)

. It does not make sense to interrogate actual method IL for async thunks. Instead, I think this check should be skipped for async thunks.

I would start with one or more extension methods to replace these problematic casts in crossgen. Implementation of these extension methods can special case AsyncVariant methods and return EcmaMethod for them. We use this coding style a lot to keep the core managed type system object model simple. There are ~40 extension methods for MethodDesc.

If it is not clear how some of these places should work, it may be useful to ask rhetorical question what would the code look like if we were to start emitting instantiating and unboxing stubs in crossgen2 too. Instantiating and unboxing stubs are similar to AsyncVariantMethods as pointed out before. </comment_new>
<comment_new>@MichalStrehovsky
I agree with what Jan said, this feels like a crossgen2 extension method situation that would be used on a case-by-case basis.

It's not that "The managed type system overloads MethodDefinition to mean "noninstantiated method" and "method with metadata"". It's crossgen that is doing the overloading because so far it could get away with it. In ILC we more often have methods that are not ECMA based and the code is written to ignore or look through those as needed, often using extension methods or is EcmaMethod checks.</comment_new>
<comment_new>@jtschuster
That makes sense to me, the best solution looks to be an extension method for MethodDesc that returns EcmaMethod? to get the definition in metadata. It should be only added to the ILCompiler.ReadyToRun project.</comment_new>

• Fixes MethodDefinition is overloaded in crossgen to mean "noninstantiated method" and "method with metadata" #121991

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[Copilot]

Pull request overview

This PR adds a new extension method GetEcmaDefinition() for MethodDesc in the ReadyToRun compiler to cleanly separate "getting the uninstantiated method" from "getting the method with ECMA metadata". This solves an issue where GetTypicalMethodDefinition() was overloaded to mean both concepts, which broke down with the introduction of AsyncMethodVariant.

Key changes:

• Created R2RMethodExtensions.cs with the GetEcmaDefinition() extension method that handles special wrapper types like AsyncMethodVariant and PInvokeTargetNativeMethod
• Replaced ~15 instances across the codebase where GetTypicalMethodDefinition() was cast to EcmaMethod with calls to the new extension method
• Improved code clarity by using pattern matching in many locations

Reviewed changes

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

Show a summary per file

File	Description
R2RMethodExtensions.cs	New extension method file containing GetEcmaDefinition() to unwrap method wrappers and return the underlying EcmaMethod
ILCompiler.ReadyToRun.csproj	Added the new R2RMethodExtensions.cs file to the project
CorInfoImpl.ReadyToRun.cs	Replaced multiple casts of GetTypicalMethodDefinition() to EcmaMethod with calls to GetEcmaDefinition(), including removal of explicit PInvokeTargetNativeMethod handling
ReadyToRunCompilationModuleGroupBase.cs	Updated token translation check to use GetEcmaDefinition()
ReadyToRunTableManager.cs	Updated module extraction logic to use GetEcmaDefinition()
ReadyToRunCodegenNodeFactory.cs	Updated module extraction in debug code to use GetEcmaDefinition()
InstanceEntryPointTableNode.cs	Updated signature building to use GetEcmaDefinition()
InstrumentationDataTableNode.cs	Updated method-to-int conversion to use GetEcmaDefinition()
InliningInfoNode.cs	Updated inlining info collection to use GetEcmaDefinition() and removed redundant cast
ProfileDataManager.cs	Updated PGO data synthesis check to use GetEcmaDefinition() with null check
ModuleTokenResolver.cs	Updated module token retrieval to use GetEcmaDefinition() with pattern matching
ReadyToRunCodegenCompilation.cs	Updated cross-module inlineable check to use GetEcmaDefinition()
ReadyToRunVisibilityRootProvider.cs	Updated visibility checks to use GetEcmaDefinition() in two locations

[jkotas]

Is this what we want here?

If we inline async thunk but not the method body itself, we do not want to create an artificial dependency on the method body itself.

[jkotas]

InstanceEntryPointTableNode seems to use this to build a hashtable of all methods stored in a R2R module. I would expect that we want to capture whether the method is the async call variant or not as part of the hashcode key.

[jkotas]

The implementation of the extension method looks fine. I am not sure whether all uses of it are right. Is your expectation that number of the uses that you are introducing in this PR will need to be revisited?

[jtschuster]

The implementation of the extension method looks fine. I am not sure whether all uses of it are right. Is your expectation that number of the uses that you are introducing in this PR will need to be revisited?

Your point about the InliningInfoNode makes me think these changes should wait until we hit issues in the compiler once we start generating async thunks. I'm not familiar enough with the code to know if these changes might cause subtle issues.