[bgen] Preserve all public methods supporting protocol methos as events. Fixes #24236.#24566
Conversation
…ts. Fixes #24236. This has the same underlying cause as #24262, but the fix for #24262 was insufficient. The fix for #24262 ensured that if an event is listening for a protocol callback, then the corresponding protocol member implementation wouldn't be trimmed away. However, it didn't prevent protocol members from being trimmed if no event was listening for that protocol member to be called, which is a problem when the protocol member in question is a required protocol member, and native code would just call it without checking whether an implemented existed. For the bug in question, what happens is this: ```cs var mgr = new CBCentralManager (new DispatchQueue ("com.xamarin.tests.ios-plain")); // Uncomment to trigger bug, 'CBCentralManagerDelegate centralManagerDidUpdateState:' is a required protocol member // mgr.UpdatedState += (sender, e) => { // Console.WriteLine ("State: " + mgr.State); // }; mgr.DiscoveredPeripheral += (sender, e) => { }; mgr.ScanForPeripherals (); ``` In this case, there's an `ICBCentralManagerDelegate` instance assigned to `mgr.Delegate` (when an event handler was attached to the `DiscoveredPeripheral` event), but that `ICBCentralManagerDelegate` instance does not have an implementation for `ICBCentralManagerDelegate.UpdateState` (aka `CBCentralManagerDelegate centralManagerDidUpdateState:`), because it was trimmed away. This causes the bug when `CBCentralManager` calls its delegate's `centralManagerDidUpdateState:` selector. The fix is to always preserve all protocol members implemented by the internal class that implements the protocol for event handling purposes. Fixes #24236.
There was a problem hiding this comment.
Pull request overview
This PR fixes a trimming issue where required protocol methods could be removed by the linker when using event-based protocol callbacks in .NET for iOS/Mac. The fix ensures all protocol method implementations in internal delegate classes are preserved, even when no event handlers are attached.
Changes:
- Modified the binding generator to preserve all public methods of internal delegate classes using
DynamicDependencyattribute - Added comprehensive tests to validate the fix for both custom types and OS types (CoreBluetooth)
- Extended test infrastructure with new protocol methods and native implementations
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 |
|---|---|
| src/bgen/Generator.cs | Changed from per-method DynamicDependency to class-level preservation of all public methods in internal delegate classes |
| tests/bindings-test/ApiDefinition.cs | Added required and optional protocol methods to HitchhikerDelegate for testing |
| tests/test-libraries/libtest.h | Added @required and @optional markers to HitchhikerDelegate protocol methods |
| tests/test-libraries/libtest.m | Implemented native methods that call all protocol methods to test trimming behavior |
| tests/monotouch-test/ObjCRuntime/RegistrarTest.cs | Added tests for both custom types and OS types (CBCentralManager) to validate the fix |
| [EventArgs ("")] | ||
| [Export ("byeByeDolphins:")] | ||
| void ByeByeDolphins (Hitchhiker sender); |
There was a problem hiding this comment.
The ByeByeDolphins method is marked as @required in the Objective-C protocol (libtest.h line 370-371), but is missing the [Abstract] attribute in the C# binding. This inconsistency means the binding generator will treat it as optional instead of required.
For consistency with the native protocol definition and to properly test the fix, add the [Abstract] attribute before this method declaration.
✅ [PR Build #02ec481] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #02ec481] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #02ec481] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #02ec481] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #02ec481] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #02ec481] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #02ec481] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
💻 [CI Build #02ec481] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #02ec481] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 117 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
…ocol methos as events. Fixes #24236. (#24576) This has the same underlying cause as #24262, but the fix for #24262 was insufficient. The fix for #24262 ensured that if an event is listening for a protocol callback, then the corresponding protocol member implementation wouldn't be trimmed away. However, it didn't prevent protocol members from being trimmed if no event was listening for that protocol member to be called, which is a problem when the protocol member in question is a required protocol member, and native code would just call it without checking whether an implemented existed. For the bug in question, what happens is this: ```cs var mgr = new CBCentralManager (new DispatchQueue ("com.xamarin.tests.ios-plain")); // Uncomment to trigger bug, 'CBCentralManagerDelegate centralManagerDidUpdateState:' is a required protocol member // mgr.UpdatedState += (sender, e) => { // Console.WriteLine ("State: " + mgr.State); // }; mgr.DiscoveredPeripheral += (sender, e) => { }; mgr.ScanForPeripherals (); ``` In this case, there's an `ICBCentralManagerDelegate` instance assigned to `mgr.Delegate` (when an event handler was attached to the `DiscoveredPeripheral` event), but that `ICBCentralManagerDelegate` instance does not have an implementation for `ICBCentralManagerDelegate.UpdateState` (aka `CBCentralManagerDelegate centralManagerDidUpdateState:`), because it was trimmed away. This causes the bug when `CBCentralManager` calls its delegate's `centralManagerDidUpdateState:` selector. The fix is to always preserve all protocol members implemented by the internal class that implements the protocol for event handling purposes. Fixes #24236. Backport of #24566.
This has the same underlying cause as #24262, but the fix for #24262 was insufficient.
The fix for #24262 ensured that if an event is listening for a protocol callback, then the corresponding protocol member implementation wouldn't be trimmed away.
However, it didn't prevent protocol members from being trimmed if no event was listening for that protocol member to be called, which is a problem when the protocol member in question is a required protocol member, and native code would just call it without checking whether an implemented existed.
For the bug in question, what happens is this:
In this case, there's an
ICBCentralManagerDelegateinstance assigned tomgr.Delegate(when an event handler was attached to theDiscoveredPeripheralevent), but thatICBCentralManagerDelegateinstance does not have an implementation forICBCentralManagerDelegate.UpdateState(akaCBCentralManagerDelegate centralManagerDidUpdateState:), because it was trimmed away.This causes the bug when
CBCentralManagercalls its delegate'scentralManagerDidUpdateState:selector.The fix is to always preserve all protocol members implemented by the internal class that implements the protocol for event handling purposes.
Fixes #24236.