[RGen] Complete the generation of the Async methods. #23276
Conversation
Put all the pieces together to generate the async calls for a method marked as Async.
There was a problem hiding this comment.
Pull Request Overview
This PR replaces stub async methods with TaskCompletionSource-based implementations and updates the generator to emit these bodies automatically.
- Updates expected test outputs for async methods (
CompleteRequestAsync,LoadFromHtmlAsync,LoadFromHtmlNoNameAsync) to useTaskCompletionSource<T>and handle NSError callbacks. - Changes
ClassEmitterto emit TCS-based method bodies instead of throwingNotImplementedException. - Adjusts
Method.ToAsyncto strip thepartialmodifier from generated async methods.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/tvOSExpectedMethodsTests.cs | Updated expected tvOS async methods to use TCS implementations |
| tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/ExpectedMethodsTests.cs | Updated expected async methods and added NSError exception handling for HTML loading |
| src/rgen/Microsoft.Macios.Generator/Emitters/ClassEmitter.cs | Generator now emits TaskCompletionSource creation, sync call invocation, and return of .Task |
| src/rgen/Microsoft.Macios.Generator/DataModel/Method.Generator.cs | Removed partial modifier from async method declarations |
| var tcsName = Nomenclator.GetTaskCompletionSourceName (); | ||
| methodBlock.WriteRaw ( | ||
| $@"{tcsType.GetIdentifierSyntax ()} {tcsName} = new (); | ||
| {ExpressionStatement( ExecuteSyncCall (method))} |
There was a problem hiding this comment.
The generator emits a single callback invocation but does not handle error parameters (e.g., NSError) by calling tcs.SetException. You should detect if the last callback argument is an NSError and emit conditional logic to set the exception when it's non-null.
| {ExpressionStatement( ExecuteSyncCall (method))} | |
| {ExpressionStatement( ExecuteSyncCall (method))} | |
| // Check if the last argument is an NSError and handle it | |
| if (callbackArguments.LastOrDefault() is NSError error && error != null) | |
| {{ | |
| {tcsName}.SetException(new Exception(error.LocalizedDescription)); | |
| }} | |
| else | |
| {{ | |
| {tcsName}.SetResult(default); | |
| }} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This comment is wrongs, what is more, tests show that we are exactly generating that code.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #342dec1] Build passed (Build macOS tests) ✅Pipeline on Agent |
✅ [PR Build #397f80b] Build passed (Detect API changes) ✅Pipeline on Agent |
❌ [CI Build #342dec1] Tests on macOS M1 - Mac Monterey (12) failed ❌Failed tests are:
Pipeline on Agent |
💻 [CI Build #342dec1] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #342dec1] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #342dec1] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
✅ API diff for current PR / commit.NET ( No breaking changes )✅ API diff vs stable.NET ( No breaking changes )ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🔥 [CI Build #342dec1] Test results 🔥Test results❌ Tests failed on VSTS: test results 4 tests crashed, 3 tests failed, 112 tests passed. Failures❌ dotnettests tests (iOS)🔥 Failed catastrophically on VSTS: test results - dotnettests_ios (no summary found). Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst)🔥 Failed catastrophically on VSTS: test results - dotnettests_maccatalyst (no summary found). Html Report (VSDrops) Download ❌ dotnettests tests (Multiple platforms)🔥 Failed catastrophically on VSTS: test results - dotnettests_multiple (no summary found). Html Report (VSDrops) Download ❌ framework tests🔥 Failed catastrophically on VSTS: test results - framework (no summary found). Html Report (VSDrops) Download ❌ monotouch tests (iOS)
Html Report (VSDrops) Download ❌ monotouch tests (tvOS)
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
🔥 [CI Build #397f80b] Build failed (Build packages) 🔥Build failed for the job 'Build packages' (with job status 'Failed') Pipeline on Agent |
Put all the pieces together to generate the async calls for a method marked as Async.