[Suggestion] Rename AsyncMethodKind.TaskReturning

[noahfalk]

I got a bit confused by the naming and comments here

Hopefully I understand it correctly now, but if not please correct me. It appears we are using the term "Task Returning" to mean different things in different places:

AsyncMethodKind.TaskReturning - The MethodDesc variant whose entrypoint returns a task and it corresponds to a C# method not marked with the MethodImpl::Async attribute.
// Task-returning methods - Any C# method returning a Task regardless if it has the MethodImpl::Async attribute

I think if you renamed AsyncMethodKind.TaskReturning -> AsyncMethodKind.CompilerAsync it would remove the naming collision and it would also contrast nicely: RuntimeAsync vs CompilerAsync.

[jakobbotsch]

I think if you renamed AsyncMethodKind.TaskReturning -> AsyncMethodKind.CompilerAsync it would remove the naming collision and it would also contrast nicely: RuntimeAsync vs CompilerAsync.

Compiler async is quite confusing to me, since in the context of the runtime/VM the only compiler I see is the JIT/interpreter's compiler. But IIUC, here it would refer to something that originally produced the IL that we are seeing? Such a compiler doesn't even need to exist.

Originally these were the names:
https://github.com/dotnet/runtimelab/blame/d95f6eadeeb696071635d9662fbe33f8a0e0d645/src/coreclr/vm/method.hpp#L58-L64

I quite liked those names, but the enum grew and things changed so it wouldn't really map anymore.

[VSadov]

AsyncMethodKind.TaskReturning - The MethodDesc variant whose entrypoint returns a task and it corresponds to a C# method marked with the MethodImpl::Async attribute.

No. AsyncMethodKind.TaskReturning is a method that returns Task, ValueTask or generic version of that and the method's definition in metadata is NOT marked as MethodImpl::Async

Basically, these methoddescs represent ordinary Task-returning methods. The IL may contain a state machine produced by C#/VB/?? or may contain whatever else code.

The IL for these methods is compiled as-is. To participate in runtime async such methods get paired with a thunk that has runtime-async calling convention.

I think TaskReturning is appropriate as the part that they return tasks is what makes them special, while NotAsync are not special at all.

[VSadov]

Perhaps it is NotAsync that is confusing and should be called something else. NotAsync is for ordinary methods not interesting to runtime async in any way.
Ordinary could probably be a better name, but I think we have other classifications that use Ordinary so it may be confusing that these are also ordinary but in async sense.

[noahfalk]

No. AsyncMethodKind.TaskReturning is a method that returns Task, ValueTask or generic version of that and the method's definition in metadata is NOT marked as MethodImpl::Async

Oops I had the right idea in my head but then still wrote the wrong text. I stink. I edited it in the original post so hopefully its correct now.

I think TaskReturning is appropriate as the part that they return tasks is what makes them special, while NotAsync are not special at all.

The key thing that I am suggesting we fix is that "TaskReturning" should have only one definition. I discovered this function which I think really exemplifies it:

    inline bool IsTaskReturningMethod() const
    {
        ...
        return asyncKind == AsyncMethodKind::RuntimeAsync ||
            asyncKind == AsyncMethodKind::TaskReturning;
    }

The meaning of "TaskReturning" in the method name isn't the same as the meaning of "TaskReturning" in the enum value. Right now AsyncMethodKind::RuntimeAsync is task-returning (IsTaskReturningMethod()==true) and also isn't task-returning (asyncKind != AsyncMethodKind::TaskReturning).

IMO "TaskReturning" as used in the method name feels intuitive. Its what I would guess the term meant if I saw it in a comment or a document and thats the one I'd recommend we keep.

If folks don't like CompilerAsync I'd have no qualms calling it anything else. CompileAsync was just the first thought that came to mind.