-
Notifications
You must be signed in to change notification settings - Fork 547
[RGen] Generate the StrongDelegates property for a WeakDelegate. #23232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WeakDelegate properties are marched as a ArgumentSemantic.Weak which in a normal case would mean that we do not mark them as dirty, but that is not the case for eak delegates since we do know we provide a strong delegate in the class. For example bgen will generate the following for a weak delegate: ```csharp [BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)] object? __mt_WeakDataSource_var; [BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)] public virtual NSObject? WeakDataSource { [Export ("dataSource", ArgumentSemantic.Weak)] get { global::UIKit.UIApplication.EnsureUIThread (); NSObject? ret; if (IsDirectBinding) { ret = Runtime.GetNSObject (global::ObjCRuntime.Messaging.NativeHandle_objc_msgSend (this.Handle, Selector.GetHandle ("dataSource")), false)!; } else { ret = Runtime.GetNSObject (global::ObjCRuntime.Messaging.NativeHandle_objc_msgSendSuper (this.SuperHandle, Selector.GetHandle ("dataSource")), false)!; } MarkDirty (); __mt_WeakDataSource_var = ret; return ret!; } [Export ("setDataSource:", ArgumentSemantic.Weak)] set { global::UIKit.UIApplication.EnsureUIThread (); var value__handle__ = value.GetHandle (); if (IsDirectBinding) { global::ObjCRuntime.Messaging.void_objc_msgSend_NativeHandle (this.Handle, Selector.GetHandle ("setDataSource:"), value__handle__); } else { global::ObjCRuntime.Messaging.void_objc_msgSendSuper_NativeHandle (this.SuperHandle, Selector.GetHandle ("setDataSource:"), value__handle__); } GC.KeepAlive (value); MarkDirty (); __mt_WeakDataSource_var = value; } } ``` And the followign for the strong delegate: ```csharp [BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)] public IQLPreviewControllerDataSource DataSource { get { return (WeakDataSource as IQLPreviewControllerDataSource)!; } set { var rvalue = value as NSObject; if (!(value is null) && rvalue is null) throw new ArgumentException ("The object passed of type " + value.GetType () + " does not derive from NSObject"); WeakDataSource = rvalue; } } ``` With this commit the same happens for rgen. This is not the complete fix because we will need to still generate the strong delegate in rgen, that will happen in follow up change (needs a new flag/api to mark the property).
This commit allows to generate the strong delegate for a weak delegate in a bindings. The API provides two ways to configure the strong property: 1. StrongDelegateType: provide a Type for the new property. 2. StrongDelegateName: Allow to choose the generated property name. If empty the name will be the name of the WeakDelefate with the Weak prefix removed. We have some tests updates: 1. Remove the warnings from the tests. Some are due to the API being experimenta, other due to a rename. We will remove all errors in a comming PR. 2. Remove the strong delegate property from the test source since it is now generated. This completes the work for support properties (so far).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for generating a “strong” wrapper property for any weak delegate in the binding generator. It introduces two new configuration options (StrongDelegateType
and StrongDelegateName
), implements the ToStrongDelegate
transformation, updates the emitter to output the new property, and adjusts many tests to account for the generated code.
- Added
StrongDelegateType
/StrongDelegateName
inExportData
andExportAttribute
to configure strong-delegate generation - Implemented
Property.ToStrongDelegate()
and updatedClassEmitter
to emit the new property - Updated tests (
GeneralPropertyTests
,*ExpectedPropertyTests.cs
, etc.) to cover and reflect the new strong-delegate behavior and silenced APL0003 warnings
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/.../GeneralPropertyTests.cs | New theory testing ToStrongDelegate under various flags |
tests/.../*ExpectedPropertyTests.cs | Updated expected generated code for array & strong props |
src/.../Property.Generator.cs | Added ToStrongDelegate method |
src/.../ClassEmitter.cs | Emits the strong-delegate property block |
src/.../ExportData.cs & ExportAttribute.cs | Added StrongDelegateType /StrongDelegateName fields |
src/.../TypeInfo.cs | Renamed ToNonNullable() → WithNullable(bool) |
Comments suppressed due to low confidence (3)
tests/rgen/Microsoft.Macios.Generator.Tests/DataModel/PropertyTests/GeneralPropertyTests.cs:472
- Add a test case for when
StrongDelegateName
is provided to ensure the generated property uses the custom name instead of just stripping the "Weak" prefix.
[InlineData (true, true, true, true)] // isProperty, isWeakDelegate, hasStrongDelegateType, shouldChange
src/rgen/Microsoft.Macios.Generator/DataModel/TypeInfo.cs:566
- The original
ToNonNullable()
method was removed; for backward compatibility it may be worth marking it[Obsolete]
and forwarding toWithNullable(false)
to avoid breaking existing code.
public TypeInfo WithNullable (bool isNullable)
src/ObjCBindings/ExportAttribute.cs:82
- [nitpick] There is a missing space before the
{
inStrongDelegateName{
; update toStrongDelegateName {
to match code style guidelines.
public string? StrongDelegateName { get; set; } = null;
|
||
// update the return type, all the rest is the same | ||
return this with { | ||
Name = ExportPropertyData.Value.StrongDelegateName ?? Name.Remove (0, 4 /* "Weak".Length */), |
Copilot
AI
Jun 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a hard-coded 4
for the "Weak" prefix length is brittle; consider using "Weak".Length
as a constant or checking Name.StartsWith("Weak")
before removing to avoid mis-removal if the pattern changes.
Name = ExportPropertyData.Value.StrongDelegateName ?? Name.Remove (0, 4 /* "Weak".Length */), | |
Name = ExportPropertyData.Value.StrongDelegateName ?? (Name.StartsWith("Weak") ? Name.Remove(0, "Weak".Length) : Name), |
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.
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.
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.
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.
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.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #0ef187d] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #0ef187d] Build passed (Detect API changes) ✅Pipeline on Agent |
❗ API diff for current PR / commit (Breaking changes).NET ( ❗ 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 #0ef187d] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #0ef187d] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #0ef187d] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #0ef187d] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #0ef187d] 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.
🔥 [CI Build #0ef187d] Test results 🔥Test results❌ Tests failed on VSTS: test results 1 tests crashed, 0 tests failed, 108 tests passed. Failures❌ monotouch tests (macOS) [attempt 2]🔥 Failed catastrophically on VSTS: test results - monotouch_macos (no summary found). Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Mac OS failures are due to the bot disconnecting during the test execution. They are not related to the code changes. |
This commit allows to generate the strong delegate for a weak delegate in a bindings. The API provides two ways to configure the strong property:
We have some tests updates:
This completes the work for support properties (so far).