[RuntimeAsync] ilasm/ildasm support for the MethodImpl.Async (Take 2)

[hez2010]

We still define async as a keyword, but allow it to appear as an identifier.

cc @VSadov

Test code:

.assembly async {}
.module async.dll
.assemblyextern System.Runtime { }
.classprivateautoansibeforefieldinit async
extends[System.Runtime]System.Object
{
.methodprivatehidebysigstaticvaluetype[System.Runtime]System.Threading.Tasks.ValueTask`1<int32> async (
int32 async
) cilmanaged async
{
.entrypointret
}
}

Code after ilasm-ildasm round-trip:

.assemblyextern System.Console
{
.ver0:0:0:0
}
.assemblyextern System.Runtime
{
.ver0:0:0:0
}
.assembly'async'
{
.ver0:0:0:0
}
.module async.dll
// MVID: {33350b0f-ac29-4c00-af71-862deacba9aa}.imagebase0x00400000.file alignment 0x00000200.stackreserve0x00100000.subsystem0x0003// WINDOWS_CUI.corflags0x00000001// ILONLY// Image base: 0x000002C7087E0000// =============== CLASS MEMBERS DECLARATION ===================.classprivateautoansibeforefieldinit'async'extends[System.Runtime]System.Object
{
.methodprivatehidebysigstaticvaluetype[System.Runtime]System.Threading.Tasks.ValueTask`1<int32>
'async'(int32'async') cilmanaged async
{
.entrypoint// Code size 1 (0x1).maxstack8IL_0000: ret
} // end of method 'async'::'async'
} // end of class 'async'

I found we have the same problem for some other MethodImpl attributes such as nooptimization, aggressiveinlining, aggressiveoptimization etc. Should I fix them as well? @jkotas

Closes#115093

[Copilot]

Pull Request Overview

This PR extends the support for MethodImpl attributes to include the async keyword, treating it similarly to other attributes like nooptimization and aggressiveoptimization.

• Added the async keyword definition in the ilasm header file.
• Updated the disassembler and parser grammar files to handle async as a valid attribute.
• Introduced a new token ASYNC_ in the parser definition for method implementation attributes.

Reviewed Changes

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

File	Description
src/coreclr/inc/il_kywd.h	Added async attribute definition to the keyword table
src/coreclr/ildasm/dasm.cpp	Added check to output " async" when the method is async
src/coreclr/ilasm/prebuilt/asmparse.grammar	Updated grammar to accept async as an identifier
src/coreclr/ilasm/asmparse.y	Added async token handling to the parser productions

[dotnet-policy-service]

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

[jkotas]

I found we have the same problem for all other MethodImpl attributes
Should I fix them as well? @jkotas

Sounds reasonable to me - as long as it is non-breaking.

don't get escaped by Roslyn

Is there an example of where Roslyn does identifier escaping like that? I am not aware of any. It does not sound right for Roslyn to be aware of keywords in textual IL representation.

I see this as a problem specific to textual IL representation, so ilasm/ildasm are the right place to deal with it.

[hez2010]

Is there an example of where Roslyn does identifier escaping like that? I am not aware of any

For example, if you write void Foo(int noinlining) in C#, Roslyn will emit 'noinlining' as the parameter name instead of noinlining.
Or maybe I'm wrong, as I observed this behavior from ILSpy. So the behavior may come from ILSpy instead of Roslyn.

[jkotas]

Or maybe I'm wrong, as I observed this behavior from ILSpy. So the behavior may come from ILSpy instead of Roslyn.

Right, ildasm (and ILSpy?) encloses identifiers that happen to match IL keywords in quotes.

These identifiers are not enclosed in quotes in the metadata emitted by Roslyn. For example, the following program prints noinlining without quotes:

public class Program
{
static void Main()
{
Console.WriteLine(typeof(Program).GetMethod("Foo").GetParameters()[0].Name);
}
public void Foo(int noinlining) { }
}

[hez2010]

It seems that all existing keywords defined in il_kywd.h have the same problem.
For example, this won't compile as well:

.assembly test {}
.module test.dll
.assemblyextern System.Console { }
.assemblyextern System.Runtime { }
.classprivateautoansibeforefieldinit async
extends[System.Runtime]System.Object
{
.methodprivatehidebysigstaticvoid test (
int32 lpstr
) cilmanaged
{
.entrypointret
}
}

because we have lpstr defined as a keyword in ilasm. You can also try using any keyword defined in il_kywd.h as an identifier, like to, any, date, arm, value, request etc, and they won't compile as well.

So I believe the correct fix is to take the breaking change here, and audit the existing IL code and correctly escape them. i.e. modifying all async to 'async' for places like dotnet/dotnet#544 (comment).

I would propose a fix that:

1. Changing all identifiers that called async in existing IL code to 'async'
2. Revert Revert "[RuntimeAsync] ilasm/ildasm support for the MethodImpl.Async" #115621

[jkotas]

We still define async as a keyword, but allow it to appear as an identifier.
I found we have the same problem for all other MethodImpl attributes
Should I fix them as well? @jkotas

What's the problem with this solution?

It matches what Roslyn does. When C# introduces a new keyword, they do not break all code that happened to use this keyword as an identifier. They only break code where it introduces ambiguity. For example, public void Foo(int await) { } compiles fine even though await is a keyword.

We prefer option with less breaking changes when there are multiple equal options.

This PR does not need to solve it for all keywords. Switching just methodimpl related keywords to a less breaking plan is good enough for now.

[hez2010]

PTAL

[jkotas]

Can we add a test that validates that the unquoted keywords work correctly? (https://github.com/dotnet/runtime/tree/main/src/tests/baseservices/ilasm_ildasm/regression may be a good place)

[hez2010]

Tests have been added, but they will require the live-built ilasm to compile.

[jkotas]

they will require the live-built ilasm to compile.

Good point, that's less than ideal.

I found we have a few tests that run live-built ilasm/ildasm. Could you please add this test to https://github.com/dotnet/runtime/blob/main/src/tests/ilasm/MethodImplOptions.cs instead?

[hez2010]

Tests should pass. PTAL @jkotas@VSadov

[jkotas]

LGTM. Thank you!

@VSadov Could you please take a look as well?

[VSadov]

LGTM. Thanks!