Skip to content

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jul 21, 2025

In order to make the next PR easier to read, I've started by baselining the existing struct lifting tests with current runtime async behavior; this is an entirely mechanical change and I've made no attempt to validate that the resulting code is good (it likely isn't in most cases). The next PR should implement proper struct lifting, and having a good set of baselines will make it easier to observe the changes in IL behavior, rather than trying to understand the entire IL all at once.

Relates to test plan #75960

Copy link
Member Author

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

At about 2/3rds of the way through updating the tests, I got fed up with adding new things to the runtime-async stub of corelib. 10p5 has the necessary support, so I have a PR (out)[https://github.com/jaredpar/basic-reference-assemblies/pull/73/] to update Basic.Reference.Assemblies with 10p6 and will unskip the relevant tests when we have that and can consume them here.

@333fred 333fred marked this pull request as ready for review July 21, 2025 23:25
@333fred 333fred requested a review from a team as a code owner July 21, 2025 23:25
@333fred 333fred requested review from RikkiGibson and jcouv July 21, 2025 23:25
…sting code spit for these under runtime async
@jcouv
Copy link
Member

jcouv commented Jul 22, 2025

        verifier.VerifyIL("Test.G()", """

This doesn't look like it's validating the intended method. Same comment applies in other places below


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:1437 in c4e8111. [](commit_id = c4e8111, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 22, 2025

        verifier.VerifyIL("Test.G()", """

Same here


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:1907 in c4e8111. [](commit_id = c4e8111, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 22, 2025

        verifier.VerifyIL("Test.G()", """

Same here


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:2085 in c4e8111. [](commit_id = c4e8111, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 22, 2025

            ILVerifyMessage = $$"""

I probably asked in a previous PR, but I can't find an issue in runtime repo for addressing these ILVerify errors. What's the link?
Personally, I'd link it everywhere, but I could settle for at least having it in first occurrence of each specific error in the file (ie. one for Task and one for Task scenario)


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:2385 in c4e8111. [](commit_id = c4e8111, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 22, 2025

public async Task Run()

Consider refactoring the test code, so that we have easier to review IL blobs. In this case, I'd probably extract the logic for arr1 and arr2 to helper methods. This will separate concerns.
This applies to some other tests as well


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:4671 in c4e8111. [](commit_id = c4e8111, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Jul 22, 2025

        verifier.VerifyIL("c1.Goo1()", """

Consider also verifying S1.Goo1 (involves ref)


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:7803 in c4e8111. [](commit_id = c4e8111, deletion_comment = False)

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 (commit 8)

333fred added 2 commits July 24, 2025 12:28
* upstream/main: (224 commits)
  Mark SyntaxTokenParser as non-experimental (dotnet#79521)
  Add test
  Fix behavior
  Fix issue where we weren't properly adding elastic trivia to newly generated members
  Cleanup test methods
  inline test strings
  Tweak build break fix
  Tweak build break fix
  Revert "Fix main build break AGAIN (dotnet#79559)"
  [main] Source code updates from dotnet/dotnet (dotnet#79563)
  [main] Source code updates from dotnet/dotnet (dotnet#79483)
  Projects to deploy (dotnet#79430)
  Fix main build break AGAIN
  Fix main build break
  Add Enum static extensions (dotnet#79546)
  Also downgrade System.Numerics.Vectors for VS
  Update logic
  Convert ValueTaskFactory methods to static extensions (dotnet#79541)
  Add back
  Fix issue with 'use explicit type' and nullable tuples
  ...
@333fred 333fred requested review from a team as code owners July 24, 2025 19:28
@dotnet-policy-service dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Jul 24, 2025
Copy link
Contributor

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

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 (commit 15)

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.

I did only a cursory review of CodeGenAsyncSpillTests.cs, but did fully review the others. Changes LGTM.

@333fred 333fred enabled auto-merge (squash) July 25, 2025 17:45
@333fred 333fred merged commit 46064a0 into dotnet:features/runtime-async Jul 25, 2025
23 of 24 checks passed
@333fred 333fred deleted the struct-lifting-tests branch July 25, 2025 19:45
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 <[email protected]>
Co-authored-by: Jan Jones <[email protected]>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Cyrus Najmabadi <[email protected]>
Co-authored-by: David Barbet <[email protected]>
Co-authored-by: Ankita Khera <[email protected]>
Co-authored-by: David Wengier <[email protected]>
Co-authored-by: Rikki Gibson <[email protected]>
Co-authored-by: Cyrus Najmabadi <[email protected]>
Co-authored-by: Rikki Gibson <[email protected]>
Co-authored-by: akhera99 <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: Todd Grunke <[email protected]>
Co-authored-by: Youssef1313 <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: Tomáš Matoušek <[email protected]>
Co-authored-by: Amadeusz Wieczorek <[email protected]>
Co-authored-by: Charles Stoner <[email protected]>
Co-authored-by: Jared Parsons <[email protected]>
Co-authored-by: Sam Harwell <[email protected]>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Jason Malinowski <[email protected]>
Co-authored-by: Etienne Baudoux <[email protected]>
Co-authored-by: AlekseyTs <[email protected]>
Co-authored-by: Jan Jones <[email protected]>
Co-authored-by: Maryam Ariyan <[email protected]>
Co-authored-by: Andrew Hall <[email protected]>
Co-authored-by: Arun Chander <[email protected]>
Co-authored-by: Kauwai Lucchesi <[email protected]>
Co-authored-by: Bill Wagner <[email protected]>
Co-authored-by: PaddiM8 <[email protected]>
Co-authored-by: Matteo Prosperi <[email protected]>
Co-authored-by: Julien Couvreur <[email protected]>
Co-authored-by: Matteo Prosperi <[email protected]>
Co-authored-by: Carlos Sánchez López <[email protected]>
Co-authored-by: Tomas Matousek <[email protected]>
Co-authored-by: Deepak Rathore (ALLYIS INC) <[email protected]>
Co-authored-by: Emmanuel Ferdman <[email protected]>
Co-authored-by: Evgeny Tvorun <[email protected]>
Co-authored-by: Victor Pogor <[email protected]>
Co-authored-by: Ella Hathaway <[email protected]>
Co-authored-by: Viktor Hofer <[email protected]>
Co-authored-by: Jason Malinowski <[email protected]>
Co-authored-by: DoctorKrolic <[email protected]>
Co-authored-by: John Douglas Leitch <[email protected]>
Co-authored-by: Matt Thalman <[email protected]>
Co-authored-by: Bernd Baumanns <[email protected]>
Co-authored-by: Thomas Shephard <[email protected]>
Co-authored-by: DoctorKrolic <[email protected]>
Co-authored-by: David Barbet <[email protected]>
Co-authored-by: Chris Sienkiewicz <[email protected]>
Co-authored-by: tmat <[email protected]>
Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Gen Lu <[email protected]>
Co-authored-by: Oleg Tkachenko <[email protected]>
Co-authored-by: Matt Mitchell <[email protected]>
Co-authored-by: Djuradj Kurepa <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Stuart Lang <[email protected]>
Co-authored-by: RaymondY <[email protected]>
Co-authored-by: Gobind Singh <[email protected]>
Co-authored-by: David Kean <[email protected]>
JoeRobich added a commit to dotnet/roslyn-tools that referenced this pull request Aug 29, 2025
46 PRs before and 36 after
```diff
+    * [Cache diagnostic analyzer computation](dotnet/roslyn#80045)
+    * [Remove parameter always passed the same value](dotnet/roslyn#80042)
     * [Update doc for IMethodSymbol.IsExtensionMethod](dotnet/roslyn#80016)
     * [Don't cache known-broken compositions](dotnet/roslyn#80021)
     * [Cleanup methods in DiagAnalyzerService](dotnet/roslyn#80013)
     * [Simplify processing of errors reported by the build](dotnet/roslyn#79964)
     * [Additional cleanup of the DiagnosticAnalyzerServier](dotnet/roslyn#80005)
     * [Fix Code Lens around source generated files](dotnet/roslyn#79992)
     * [Remove superflous DiagService api that can be achieved with existing apis](dotnet/roslyn#80007)
     * [Generate `init` accessor for required properties inside `readonly struct`s](dotnet/roslyn#80004)
     * [Remove existing low level diag oop code now that it's all handled at higher levels.](dotnet/roslyn#79994)
     * [Allow large InlineHint ArrayBuilder pooling](dotnet/roslyn#79857)
     * [Reduce allocations obtaining classified spans in ClassifierHelper](dotnet/roslyn#79856)
     * [Compute span diagnostics in oop](dotnet/roslyn#79991)
     * [Allow Razor cohosting to work with non-Razor SDK projects](dotnet/roslyn#79953)
     * [Move computation of deprioritized analyzers to oop](dotnet/roslyn#79989)
     * [EnC: Fix symbol mapping of delegates with indexed name](dotnet/roslyn#79837)
     * [Emit telemetry 'durations' with known radix point '.'](dotnet/roslyn#79988)
     * [Move logic up into DiagService](dotnet/roslyn#79985)
     * [Move the StateManager type up to the DiagnosticService from the DiagnosticIncrementalANalyzer](dotnet/roslyn#79984)
     * [Immediately remote diagnostics call to OOP](dotnet/roslyn#79983)
     * [Build Microsoft.CodeAnalysis.SemanticSearch.Extension ref assembly for use in semantic search queries](dotnet/roslyn#79972)
     * [Only cache compilation if we have the same set of analyzers](dotnet/roslyn#79978)
     * [Delete unused property](dotnet/roslyn#79963)
     * [Update 'use expr body' to be a purely syntactic analyzer](dotnet/roslyn#79979)
     * [Mark 'Use expr body' as a syntax-only fixer](dotnet/roslyn#79971)
     * [♻️ MSBuildWorkspaceDirectory - Fallback to AppContext.BaseDirectory when Assembly Location is empty](dotnet/roslyn#79934)
     * [Merge runtime async support into main](dotnet/roslyn#79833)
     * [Implement "Simplify property accessor" feature](dotnet/roslyn#79754)
-    * Merge main to runtime async branch (PR: [#79961](dotnet/roslyn#79961))
     * [Redo how and when we report source generator telemetry](dotnet/roslyn#79951)
     * [Allow MEF components to supply assembly path resolvers](dotnet/roslyn#79218)
     * [Allow Razor to hook up the source generator in misc files](dotnet/roslyn#79891)
     * [Block ENC for extension blocks](dotnet/roslyn#79883)
     * [Upgrade servicehub.client to fix test source discovery](dotnet/roslyn#79899)
     * [Update package restore error message.](dotnet/roslyn#79876)
-    * Merge main (PR: [#79834](dotnet/roslyn#79834))
-    * Merge main (PR: [#79830](dotnet/roslyn#79830))
     * [Baseline struct lifting tests](dotnet/roslyn#79505)
-    * Merge main to runtime async branch (PR: [#79582](dotnet/roslyn#79582))
-    * Merge main (PR: [#79424](dotnet/roslyn#79424))
-    * Merge main (PR: [#78994](dotnet/roslyn#78994))
-    * Merge main to runtime async branch (PR: [#78740](dotnet/roslyn#78740))
-    * Merge main to runtime async branch (PR: [#78517](dotnet/roslyn#78517))
-    * Merge main to runtime async branch (PR: [#78114](dotnet/roslyn#78114))
-    * Merge main to runtime async branch (PR: [#77700](dotnet/roslyn#77700))
-    * Merge main to runtime async branch (PR: [#77533](dotnet/roslyn#77533))
-    * Merge main to runtime async branch (PR: [#77265](dotnet/roslyn#77265))
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants