[RGen] Convert native values to managed enum values for the delegate execution. #22918
Conversation
…execution. There was a bug in the previous implementation in which we were: 1. Not lowering the enum to its native type correctly. 2. Not castin the lower type to the managed enum type for the trampoline execution. This is similar to the issue we had with booleans in which I confused the direction of the conversion. We are doing low -> managed. Managed -> low is needed in other aread of the generation, not in the delegate execution.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug in the enum conversion logic for delegate execution by updating the conversion functions and associated tests.
- Updates conversion method calls from CastToNative to CastEnumToNative and CastNativeToEnum.
- Enhances test coverage with new scenarios for native-to-enum conversion.
- Adds documentation and a new TypeSyntax for UIntPtr to support the updated conversion logic.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/rgen/Microsoft.Macios.Generator.Tests/Emitters/BindingSyntaxFactoryTrampolineTests.cs | Updated casting expressions in test cases to use the new conversion patterns. |
| tests/rgen/Microsoft.Macios.Generator.Tests/Emitters/BindingSyntaxFactoryObjCRuntimeTests.cs | Revised tests to validate the newly introduced CastEnumToNative and CastNativeToEnum functions. |
| src/rgen/Microsoft.Macios.Generator/Emitters/BindingSyntaxFactory.Trampoline.cs | Replaced CastToNative with CastEnumToNative and CastNativeToEnum, added a helper GetNativeEnumLowLevel, and improved documentation. |
| src/rgen/Microsoft.Macios.Generator/Emitters/BindingSyntaxFactory.ObjCRuntime.cs | Renamed conversion functions and added methods for casting native representations back to enum types along with updated documentation. |
| src/rgen/Microsoft.Macios.Generator/Emitters/BindingSyntaxFactory.KnownTypes.cs | Introduced a TypeSyntax entry for System.UIntPtr for consistency with enum conversion logic. |
Comments suppressed due to low confidence (1)
src/rgen/Microsoft.Macios.Generator/Emitters/BindingSyntaxFactory.Trampoline.cs:324
- [nitpick] Confirm that switching from CastToNative to CastNativeToEnum here correctly implements the intended low-to-managed enum conversion for delegate parameters, and that this change does not inadvertently affect other enum handling paths.
=> CastNativeToEnum (parameter)!;
| @@ -28,17 +28,17 @@ static partial class BindingSyntaxFactory { | |||
| /// <param name="parameter">The parameter whose casting we need to generate. The type info has to be | |||
| /// and enum and be marked as native. If it is not, the method returns null</param> | |||
| /// <returns>The cast C# expression.</returns> | |||
| internal static CastExpressionSyntax? CastToNative (in Parameter parameter) | |||
| => CastToNative (parameter.Name, parameter.Type); | |||
| internal static CastExpressionSyntax? CastEnumToNative (in Parameter parameter) | |||
There was a problem hiding this comment.
[nitpick] Update the XML documentation for CastEnumToNative to clearly indicate that this method is intended for enum-specific native conversions, ensuring consistency with its usage throughout the codebase.
Copilot uses AI. Check for mistakes.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #83b056a] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #83b056a] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ 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 |
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.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #83b056a] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #83b056a] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #83b056a] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #83b056a] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💻 [CI Build #d615955] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
🚀 [CI Build #d615955] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 115 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
There was a bug in the previous implementation in which we were:
This is similar to the issue we had with booleans in which I confused the direction of the conversion. We are doing low -> managed. Managed -> low is needed in other aread of the generation, not in the delegate execution.