Skip to content

Move runtime async method validation into initial binding#78310

Merged
333fred merged 15 commits intodotnet:features/runtime-asyncfrom
333fred:initial-binding-validation
May 2, 2025
Merged

Move runtime async method validation into initial binding#78310
333fred merged 15 commits intodotnet:features/runtime-asyncfrom
333fred:initial-binding-validation

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Apr 24, 2025

We now do method construction and validation for runtime async helpers up front in initial binding, rather than doing it in RuntimeAsyncRewriter. I've also renamed the APIs as per dotnet/runtime#114310 (comment) (though I haven't added ConfigureAwait support yet, that will be the next PR). We now validate:

  • The helpers come from System.Runtime.CompilerServices.AsyncHelpers, defined in corelib. This means that I now need a fairly extensive corelib mock to be able to compile. When we have a testing runtime that defines these helpers, we can remove the giant mock and use the real one.
  • We properly error when expected helpers aren't present.
  • We properly check to make sure that constraints are satisfied when doing generic substitution in one of the runtime helpers.
  • Runtime async is not turned on if the async method does not return Task, Task<T>, ValueTask, or ValueTask<T>.

Relates to test plan #75960

@333fred 333fred requested a review from a team as a code owner April 24, 2025 23:39
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 24, 2025
@333fred 333fred requested review from RikkiGibson and jcouv April 24, 2025 23:39
internal bool RuntimeSupportsAsyncMethods
=> RuntimeSupportsFeature(SpecialMember.System_Runtime_CompilerServices_RuntimeFeature__Async)
|| _overrideRuntimeSupportsAsyncMethods;
=> GetSpecialType(InternalSpecialType.System_Runtime_CompilerServices_AsyncHelpers) is { TypeKind: TypeKind.Class, IsStatic: true };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the runtime feature entry from the design doc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the resolution?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was removed.

&& !ReferenceEquals(methodReturn, GetSpecialType(InternalSpecialType.System_Threading_Tasks_ValueTask))
&& !ReferenceEquals(methodReturn, GetSpecialType(InternalSpecialType.System_Threading_Tasks_ValueTask_T)))
{
return false;
Copy link
Member

@jcouv jcouv Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a corresponding update to the design doc? Or should we have a follow-up comment to make the void-returning method scenario work at some point? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the design doc, the runtime-side is very clear that only Task/ValueTask methods can be runtime async.

@jcouv jcouv self-assigned this Apr 25, 2025
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with review pass (iteration 10)

@333fred
Copy link
Member Author

333fred commented Apr 29, 2025

@jcouv addressed feedback. @RikkiGibson for review please

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 12). Consider updating the design doc (removing runtime feature flag) in same PR

@RikkiGibson RikkiGibson self-assigned this Apr 30, 2025
@RikkiGibson
Copy link
Member

It looks like the following tests need to be updated

Microsoft.CodeAnalysis.CSharp.UnitTests.MissingSpecialMember.AllWellKnownTypesBeforeCSharp7()
    in /_/src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs:line 926
Microsoft.CodeAnalysis.VisualBasic.UnitTests.Symbols.CorLibrary.CorTypes.PresentCorLib()
    in /_/src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/CorLibrary/CorTypes.vb:line 77

@333fred
Copy link
Member Author

333fred commented Apr 30, 2025

LGTM Thanks (iteration 12). Consider updating the design doc (removing runtime feature flag) in same PR

The doc is maintained in main.

break;

default:
Debug.Fail($"Unexpected RuntimeAsyncAwaitMethod: {RuntimeAsyncAwaitMethod.Name}");
Copy link
Member

@RikkiGibson RikkiGibson Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking my understanding: it looks like we expect this condition to never occur. i.e. compiler will never produce a BoundAwaitableInfo whose RuntimeAsyncAwaitMethod.Name is not one of the above cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's correct.

Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some minor comments/suggestions

return false;
}

Debug.Assert((runtimeAwaitHelper is { Arity: 1 } && maybeResultType is { }) || runtimeAwaitHelper.TypeParameters.Length == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe slightly more uniform/compact check would be:

Suggested change
Debug.Assert((runtimeAwaitHelper is { Arity: 1 } && maybeResultType is { }) || runtimeAwaitHelper.TypeParameters.Length == 0);
Debug.Assert(runtimeAwaitHelper.Arity == (maybeResultType is null ? 0 : 1));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeWithAnnotations also has its own HasType which distinguishes default values from "meaningful" values, which could be used instead of System.Nullable, but, there's no deep need to do that.

&& AwaiterImplementsINotifyCompletion(awaiterType, node, diagnostics)
&& GetGetResultMethod(getAwaiter, node, expression.Type!, diagnostics, out getResult, out getAwaiterGetResultCall);
&& GetGetResultMethod(getAwaiter, node, expression.Type!, diagnostics, out getResult, out getAwaiterGetResultCall)
&& (!isRuntimeAsyncEnabled || getRuntimeAwaitAwaiter(awaiterType, out runtimeAsyncAwaitMethod, expression.Syntax, diagnostics));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking my understanding on the sequence of checks here.

  • if runtime async, and got runtime await helper, use that
  • otherwise, need an "ordinary" GetAwaiterMethod to proceed--if we didn't get one just return false.
  • then check for IsCompleted, GetResult, ..
  • finally if runtime async enabled, then get a RuntimeAwaitAwaiter?

Is the idea that in the 'RuntimeAwaitAwaiter' case, we still need to use the type's GetAwaiter method etc? That seems to make sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that in the 'RuntimeAwaitAwaiter' case, we still need to use the type's GetAwaiter method etc?

Precisely. This the lowering mechanism in https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Runtime%20Async%20Design.md#await-any-other-type, as opposed to the rest of the document which talks about await Task

}

var methodReturn = method.ReturnType.OriginalDefinition;
if (!ReferenceEquals(methodReturn, GetSpecialType(InternalSpecialType.System_Threading_Tasks_Task))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if there a way to say something like methodReturn.InternalSpecialType is InternalSpecialType.Task or Task_T or ValueTask or ValueTask_T. No need to try and dig up a way to do that though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think we can, since this is special types, not well-known types.

@333fred
Copy link
Member Author

333fred commented Apr 30, 2025

@jcouv @RikkiGibson for another look, a few small changes in the last commit.

@333fred
Copy link
Member Author

333fred commented May 2, 2025

@jcouv, since the last 2 commits were only minor changes, I'm going to go ahead and merge this now; if you have any comments on these last 2 bits, I can address them in the next PR.

@333fred 333fred merged commit e272201 into dotnet:features/runtime-async May 2, 2025
24 checks passed
@333fred 333fred deleted the initial-binding-validation branch May 2, 2025 17:37
333fred added a commit that referenced this pull request Aug 19, 2025
* Handle basic await scenarios (#76121)

Add initial handling of expressions that return `Task`, `Task<T>`, `ValueTask`, `ValueTask<T>`.

* Add RuntimeAsyncMethodGenerationAttribute (#77543)

Adds control for whether to use runtime async. The flowchart is as follows:

1. The flag `System.Runtime.CompilerServices.RuntimeFeature.Async` must be present.
2. Assuming that flag is present, we look for the presence of `System.Runtime.CompilerServices.RuntimeAsyncMethodGenerationAttribute` on the method. If that attribute is present, we use the preference expressed in the attribute. The preference does not carry to nested contexts, such as local functions or lambdas.
3. If the attribute is not present, we look for `features:runtime-async=on` on the command line. If that is present, then the feature is on by default. Otherwise, the feature is off.

* Semantic search info

* Implement custom awaitable support (#78071)

This adds support for awaiting task-like types that are not natively supported by runtime async. Closes #77897.

* Move runtime async method validation into initial binding (#78310)

We now do method construction and validation for runtime async helpers up front in initial binding, rather than doing it in `RuntimeAsyncRewriter`. I've also renamed the APIs as per dotnet/runtime#114310 (comment) (though I haven't added ConfigureAwait support yet, that will be the next PR). We now validate:

* The helpers come from `System.Runtime.CompilerServices.AsyncHelpers`, defined in corelib. This means that I now need a fairly extensive corelib mock to be able to compile. When we have a testing runtime that defines these helpers, we can remove the giant mock and use the real one.
* We properly error when expected helpers aren't present.
* We properly check to make sure that constraints are satisfied when doing generic substitution in one of the runtime helpers.
* Runtime async is not turned on if the async method does not return `Task`, `Task<T>`, `ValueTask`, or `ValueTask<T>`.

Relates to test plan #75960

* React to main changes #78246 and #78231.

* Switch MethodImplAttributes.Async to 0x2000 (#78536)

It was changed in dotnet/runtime#114310 as 0x400 is a thing in framework.

* Ensure return local is the correct type for runtime async (#78603)

* Add test demonstrating current behavior

* Ensure return local is the correct type in async scenarios

* Ensure method is actually async when doing local rewrite

* Exception Handler support (#78773)

* Update EH tests to run with runtime async

* Handle non-null exception filter prologues in the spill sequencer

* Add more testing to show current incorrect behavior

* Unskip ConditionalFacts that do not need to be skipped.

* Handle ensuring that the method remains valid, even when there is an `await` in a finally section. Add signifcant testing of `await using`.

* Fix baselines

* Support `await foreach` and add runtime async verification to existing tests.

* Remove unnecessary generic

* Failing tests, add async void test suggestion

* CI failures

* Add additional test

* Test fixes

* Remove implemented PROTOTYPE, add assertion on behavior.

* Update to SpillSequenceSpiller after some more debugging and tightening the assertion

* Fix nullref

* Enable nullable for VisitCatchBlock

* Support using a simple overload resolution for finding Await helpers from the BCL (#79135)

* Support using a simple overload resolution for finding Await helpers from the BCL

This PR removes special knowledge of what `Await` helpers correspond to what types, and instead implements a very simple form of overload resolution. We immediately bail on any conflict or error and fall back to attempting to use `AwaitAwaiter` or `UnsafeAwaitAwaiter` when such scenarios are detected. I've also updated the rules to better reflect what is actually implementable.

* Create the full BoundCall in initial binding.

* PR feedback.

* Baseline struct lifting tests (#79505)

* Extract expectedOutput constants, minor touchups

* Rename expected -> expectedOutput

* Include new testing with placeholder baselines

* Progress

* First ILVerify pass

* Initial baseline IL run.

* Further baseline additions and skips based on missing corelib apis.

* Clone async void tests and have them use async Task, and validate existing code spit for these under runtime async

* Update baselines after .NET 10 intake

* Delete the stub

* Remove long dynamic baseline and leave a prototype.

* Feedback.

* BOM

* Remove unused references parameter

* Block `await dynamic`

* Block hoisted variables from runtime async for now

* Update test baselines for block

* Block arglist in runtime async

* Add IL baseline

* Handle an additional branch beyond the end of the method case.

* Move prototype comments to issues.

* Remove entry point prototypes

* Add assert and comment

* Add back assert

* Report obsolete/experimental diagnostics on await helpers.

* Fix ref safety analysis build error.

---------

Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
Co-authored-by: Jan Jones <janjones@microsoft.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: David Barbet <dabarbet@microsoft.com>
Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
Co-authored-by: akhera99 <ankitakhera@microsoft.com>
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
Co-authored-by: Todd Grunke <toddgrun@microsoft.com>
Co-authored-by: Youssef1313 <youssefvictor00@gmail.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com>
Co-authored-by: Amadeusz Wieczorek <amwieczo@microsoft.com>
Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
Co-authored-by: Jared Parsons <jared@paranoidcoding.org>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com>
Co-authored-by: Etienne Baudoux <etbaudou@microsoft.com>
Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com>
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
Co-authored-by: Maryam Ariyan <maariyan@microsoft.com>
Co-authored-by: Andrew Hall <andrha@microsoft.com>
Co-authored-by: Arun Chander <arkalyan@microsoft.com>
Co-authored-by: Kauwai Lucchesi <53876126+kauwai@users.noreply.github.com>
Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: PaddiM8 <hi@bakk.dev>
Co-authored-by: Matteo Prosperi <maprospe@microsoft.com>
Co-authored-by: Julien Couvreur <julien.couvreur@gmail.com>
Co-authored-by: Matteo Prosperi <41970398+matteo-prosperi@users.noreply.github.com>
Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
Co-authored-by: Tomas Matousek <tomat@microsoft.com>
Co-authored-by: Deepak Rathore (ALLYIS INC) <v-deerathore@microsoft.com>
Co-authored-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
Co-authored-by: Evgeny Tvorun <evgenyt@microsoft.com>
Co-authored-by: Victor Pogor <victor.pogor@outlook.com>
Co-authored-by: Ella Hathaway <67609881+ellahathaway@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Jason Malinowski <jason@jason-m.com>
Co-authored-by: DoctorKrolic <mapmyp03@gmail.com>
Co-authored-by: John Douglas Leitch <JohnLeitch@outlook.com>
Co-authored-by: Matt Thalman <mthalman@microsoft.com>
Co-authored-by: Bernd Baumanns <baumannsbernd@gmail.com>
Co-authored-by: Thomas Shephard <thomas@thomas-shephard.com>
Co-authored-by: DoctorKrolic <70431552+DoctorKrolic@users.noreply.github.com>
Co-authored-by: David Barbet <dibarbet@gmail.com>
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
Co-authored-by: tmat <tomas.matousek@microsoft.com>
Co-authored-by: gel@microsoft.com <gel@microsoft.com>
Co-authored-by: Gen Lu <genlu@users.noreply.github.com>
Co-authored-by: Oleg Tkachenko <olegtk@microsoft.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
Co-authored-by: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Stuart Lang <stuart.b.lang@gmail.com>
Co-authored-by: RaymondY <yangrongwei@hotmail.com>
Co-authored-by: Gobind Singh <gobindsingh2004@gmail.com>
Co-authored-by: David Kean <davkean@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Runtime Async untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants