[linker] Only generate NSObject factories for generic types. Fixes #25232. - #26510
[linker] Only generate NSObject factories for generic types. Fixes #25232.#26510rolfbjarne wants to merge 5 commits into
Conversation
Non-generic types are constructed through the trimmable registrar type map. Let ILLink retain generated interface implementations through their method overrides instead of broadly preserving every interface on each type. Add an assembly-preparer test covering generic and non-generic factory generation. Fixes #25232. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ef04d7c1-e5e3-4244-ac92-f7341ddac90e
There was a problem hiding this comment.
🔵 Needs a closer look
Pull request overview
This PR refines trimmable-static registrar factory generation to reduce unnecessary interface preservation and app size impact, aligning non-generic NSObject construction with the trimmable registrar type map while keeping factory methods only where type-map construction can’t work (generic NSObject types).
Changes:
- Generate
INSObjectFactory._Xamarin_ConstructNSObjectonly for genericNSObjecttypes in the trimmable-static registrar pipeline. - Remove the trimmable-static “preserve all interfaces” dynamic dependency from generated interface implementations and rely on the generated method overrides instead.
- Add assembly-preparer coverage for generic vs non-generic factory generation and update expected app-size baselines.
File summaries
| File | Description |
|---|---|
| tools/dotnet-linker/Steps/ManagedRegistrarStep.cs | Limits NSObject factory method generation to generic types under the trimmable-static registrar. |
| tools/dotnet-linker/AppBundleRewriter.cs | Removes the interface-preservation dynamic dependency for trimmable-static; keeps marking behavior for other registrar modes. |
| tests/assembly-preparer/ManagedRegistrarStepTests.cs | Adds test coverage verifying factory generation behavior for generic vs non-generic NSObject types. |
| tests/dotnet/UnitTests/expected/TVOS-NativeAOT-TrimmableStatic-size.txt | Updates expected app size after preservation reduction. |
| tests/dotnet/UnitTests/expected/MacOSX-NativeAOT-TrimmableStatic-size.txt | Updates expected app size after preservation reduction. |
| tests/dotnet/UnitTests/expected/MacOSX-CoreCLR-Interpreter-TrimmableStatic-size.txt | Updates expected app size after preservation reduction. |
| tests/dotnet/UnitTests/expected/MacCatalyst-NativeAOT-TrimmableStatic-size.txt | Updates expected app size after preservation reduction. |
| tests/dotnet/UnitTests/expected/iOS-NativeAOT-TrimmableStatic-size.txt | Updates expected app size after preservation reduction. |
Review details
Suppressed comments (1)
tests/assembly-preparer/ManagedRegistrarStepTests.cs:51
- 💡 Testing — For the same reason as the non-generic case, also assert that the generated
_Xamarin_ConstructINativeObjectmethod on the generic type has an override toObjCRuntime.INativeObject, since the trimmer-retention strategy depends on method overrides.
var factoryMethod = genericType.Methods.Single (v => v.Name == "_Xamarin_ConstructNSObject");
Assert.That (factoryMethod.Overrides.Select (v => v.DeclaringType.FullName), Does.Contain ("Foundation.INSObjectFactory"), "Generic method overrides");
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| Assert.That (nonGenericType.Interfaces.Select (v => v.InterfaceType.FullName), Does.Contain ("ObjCRuntime.INativeObject"), "Non-generic INativeObject interface"); | ||
| Assert.That (nonGenericType.Methods.Select (v => v.Name), Does.Contain ("_Xamarin_ConstructINativeObject"), "Non-generic INativeObject methods"); |
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.
|
✅ 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 #eb2252c] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 5 tests failed, 259 tests passed. Failures❌ assembly-processing tests1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (iOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (macOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (tvOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Sonoma (14): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Non-generic types are constructed through the trimmable registrar type map, so only generic
NSObjecttypes need to implementINSObjectFactory.Remove the broad interface-preservation dependency and rely on ILLink to retain generated interface implementations through their method overrides. This avoids preserving unrelated interface implementations and reduces app size.
Add assembly-preparer coverage for generic and non-generic factory generation, and update the expected app sizes.
Tests:
Fixes #25232
🤖 Pull request created by Copilot