[RuntimeAsync] Stop emitting modreqs into signatures of async variants.

[VSadov]

Re discussion in: #122672 (comment)

The modreqs were a somewhat convenient remnant from the experiment, but otherwise unnecessary.

The convenient part was that async variants for Task<int> M() and ValueTask<int> M() with modreqs would have distinct signatures, which also would not match regular non-async methods, since we used open type in the modreq. (it might be possible to declare such signature in IL, but I am not sure if we would load it).

Anyways, at this point modreqs are not strictly required, so with this change they will not be emitted.

That leaves the issue with signature ambiguities.

The issue arises in various places where we match method overrides with their virtual bases, match implementing methods with interfaces, and so on. Method equality/equivalence in these areas is based on matching MethodSignature instances. They are roughly {Name, Signature} tuples with some support for generic substitution and efficient compares using memoized string hashes.
Once we drop the modreqs from the signature of async variants, {Name, Signature} becomes potentially ambiguous, since we lose the information on:

• whether the signature is for an async variant or not
  (variants should not match with ordinary methods for overriding/implementing purposes)
• whether the variant is for a Task or ValueTask - returning methods.
  (Task<int> M() and ValueTask<int> M() can add int M() async variants into the same declared/lookup scope. They would need to be distinct when overriding/implementing)

To deal with the ambiguities, we keep that information by adding AsyncVariantKind to the MethodSignature as in:

    enum AsyncVariantKind
    {
        None      = 0,  // this is not a signature of an async variant method
        Task      = 1,  // this is a signature of an async variant for a Task[<T>] returning method
        ValueTask = 2   // this is a signature of an async variant for a ValueTask[<T>] returning method
    };

The matching kind is a prerequisite for two MethodSignatures to be considered Equivalent or ExactlyEqual for the purpose of overriding, hiding, implementing.

[Copilot]

Pull request overview

This pull request removes modreq emission from async variant method signatures in the RuntimeAsync feature. Instead of relying on modreqs to distinguish between Task and ValueTask async variants, the PR adds an AsyncVariantKind enum to the MethodSignature class to explicitly track whether a signature belongs to an async variant and whether it's for a Task or ValueTask returning method.

Changes:

• Added AsyncVariantKind enum to MethodSignature to track async variant type (None, Task, or ValueTask)
• Modified signature construction logic to directly use element types instead of modreqs
• Updated method signature comparison to check AsyncVariantKind matching
• Replaced IsValueTaskAsyncThunk() with IsAsyncVariantForValueTaskReturningMethod() for clearer semantics

Reviewed changes

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

File	Description
src/coreclr/vm/methodtablebuilder.h	Added AsyncVariantKind enum and updated MethodSignature constructors to accept and store async variant kind
src/coreclr/vm/methodtablebuilder.cpp	Implemented SameAsyncVariantKind() check, integrated it into signature equivalence checks, and simplified signature construction to remove modreqs
src/coreclr/vm/method.hpp	Added IsAsyncVariantForValueTask flag and IsAsyncVariantForValueTaskReturningMethod() method, updated comments to reflect removal of modreqs
src/coreclr/vm/asyncthunks.cpp	Removed IsValueTaskAsyncThunk() and replaced its usage with IsAsyncVariantForValueTaskReturningMethod()

[dotnet-policy-service]

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

[jkotas]

The issue arises in various places where we match method overrides with their virtual bases, match implementing methods with interfaces, and so on. Method equality/equivalence in these areas is based on matching MethodSignature instances.

I would expect all override resolution to be done using Task-returning variant. To figure out the override for async variant, go to Task-returning variant, resolve override for that, and then map it back to async variant. The signature of the async variant would not be used at all for override resolution if done this way. What prevents us from dong that?

I can see that adding AsyncVariantKind to the signature works for the common cases. I am not sure whether it is going to work for all corner-cases.

[VSadov]

The issue arises in various places where we match method overrides with their virtual bases, match implementing methods with interfaces, and so on. Method equality/equivalence in these areas is based on matching MethodSignature instances.

I would expect all override resolution to be done using Task-returning variant. To figure out the override for async variant, go to Task-returning variant, resolve override for that, and then map it back to async variant. The signature of the async variant would not be used at all for override resolution if done this way. What prevents us from dong that?

I had a few dead-ends starting on that direction.

It tend to bring complexity upon complexity. There is no single "choke-point" to do the "flip". At some point we compare signatures, so you'd need to make sure that all signatures came from non-async methoddescs, but you have to remember if we flipped either left or the right side or both. I think only when both sides were flipped or both were not, it is a "match".

There are patterns where we take a methoddesc's signature and go through type hierarchy to find/construct/collect whatever matches, then we must remember if we did the "flip", so that we would flip back the result, if the result actually has an async variant. (sometimes it does not, if derived substituted T for Task, there is a base for the Task-returning variant, but no base for the async variant, for example - that is completely ok).

There are places that start directly with signatures. It is possible to construct {Name, Signature} from metadata and it is cheap. It is known without consulting a methoddesc that these signatures are not for async variants. With this PR, the tuple simply remembers the fact as it defaults to not-async.

Basically, attempts with normalizing signatures to non-async form prior to matching and then switching results back accordingly were looking like they would end up a big and fragile change which may miss cases or flip twice by accident.
Even if made to work, I have some concerns about eventual perf impact.

It feels like ensuring that all areas that care about async variants should use a complete tuple, which records variant kind, would have fewer corner cases. The areas that care are basically just building the methodtable as it needs to match variants with bases/implementations, and it seems to be always using the complete tuple.

The other areas might not need to know about variants at all. Reflection is a good example.

Hiding variants in iterators and things like FindMethod unless explicitly asked for is a separate concern though.
The matching of overrides/implements is one area that actually needs to know about variants and how to match them, so removing modreqs has direct impact.

[VSadov]

I can see that adding AsyncVariantKind to the signature works for the common cases. I am not sure whether it is going to work for all corner-cases.

The main concern might be that cor signature and name could end up traveling around on their own and then using just that for method matching may cause unknown kind of troubles.

After thinking more about this, I started leaning back towards the idea that the modreq might actually be the most reliable approach.

Signature comparison is the natural unavoidable choke-point when matching methods. Since modreq decoration is reversible, an ambiguity cannot happen, and thus a much better assurance that some codepath will not slip through reviews and testing and end up equating signatures that should not be equal (or the other way around).

It also seems much cheaper than flipping variants back and forth, with the same end result - only things with the same None/Task/ValueTask decoration should match and form override/implements relationships.

If we stay with modreqs I'd change one part though.

We decorate nongeneric case as modreq(Task) void Foo(). Chances are low, but that is a clearly legal modreq. I'd change the encoding to modreq(Task') void Foo() - i.e. use an open type. That would still be reversible encoding, but I'd feel a lot better about rejecting it in actual metadata, if we do not reject that already.

[jkotas]

There are patterns where we take a methoddesc's signature and go through type hierarchy to find/construct/collect whatever matches, then we must remember if we did the "flip"

The unboxing stubs have the exact same problem. They do not even have different signature (since the type of "this" is implied). We are able to flip back and forth between them fine.

Basically, attempts with normalizing signatures to non-async form prior to matching

I do not think the normalization should be based on signatures. It should be based on the MethodDesc (or the temporary bmt...Method handles in the type loader).

After thinking more about this, I started leaning back towards the idea that the modreq

I think the signature flags in this PR are an improvement over modereqs, but it not going far enough

[VSadov]

The unboxing stubs have the exact same problem.

I've looked at how the stubs handle the same issue for a prior art, but could not find much.
I assume that unboxing variants are always struct methods and the byref-this variant never implements/overrides anything, thus the task of matching to the same variant in bases/interfaces might be trivial for stubs.

Maybe I did not look in right places, but I did not find where we would flip to another variant and back in order to match unboxing stubs with bases or interfaces.

[jkotas]

LGTM, I think this is still an improvement over the modopts.

[VSadov]

I took some time to step back and think if we can have a pattern that would be a better guarantee that there is no accidental matching by name+signature.
I do not have a good idea, though, other than deal with this issue ad-hoc as it arises.

Maybe it won't. Most common reasons for getting method information from metodtables (where async variants can be seen) are the methodtable builder itself and related areas, and reflection-like APIs. This PR deals with first and we already filtering out async variants in the second. There could be other areas/patterns though, so there is some risk.

One idea: It could be nice to have some kind of assert that we are not comparing signatures that we should not compare. Equating by signature a variant originated from Task-returning method vs. ValueTask-returning would almost always be a bug, for example. Maybe we could mark signatures somehow. We'd need 3-state marker. It would be ok if the tagging only happens in debug, if that helps.
I am not sure how to arrange that though, the format of the cor sig is quite rigid and compact (no random spare bits), so it is just an idea with no implementation.

[VSadov]

LGTM, I think this is still an improvement over the modopts.

Right. We do have improvements in this PR:

• we actually remove the modifiers.
• we add information about Task/ValueTask to the MethodDesc.
• we have some mechanism to deal with potential ambiguities in methodtable builder and related areas, even though it does not go as far as we'd like.

There might be a way for further improvements to make this more reliable or at least find a way to catch misuses.

[jkotas]

I agree. Would you like to merge this PR?

[VSadov]

Thanks!!