[linker] Only generate NSObject factories for generic types. Fixes #25232. - #26510
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.
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.
The generic NSObject factory optimization allows additional protocol-only frameworks to be trimmed from fully linked CoreCLR apps. Update the expected native library lists accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use the platform assembly name generated by CreatePreparer, including the required .dll suffix, so ManagedRegistrarStep processes the test assembly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
…ate protocol'
The VB iOS/MacCatalyst 'SceneDelegate' template subclasses the
'UIWindowSceneDelegate' Model class (VB doesn't support consuming C#'s
default interface implementations, so implementing the corresponding
interface directly would require implementing dozens of members).
The linker/trimmer strips the 'IUIWindowSceneDelegate' interface
implementation from the 'UIWindowSceneDelegate' Model class itself once
trimmed (since nothing calls through it directly), which means the
native registrar can no longer see that 'SceneDelegate' transitively
conforms to the 'UIWindowSceneDelegate' protocol. At runtime this causes
UIKit to reject the class with:
NSInternalInconsistencyException: 'representation's delegateClass
must conform to UISceneDelegate protocol'
Fix this by adding an explicit '<Adopts ("UIWindowSceneDelegate")>'
attribute to the class, which tells the registrar to declare the
protocol conformance regardless of what the (trimmed) base class
implements.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d41380a-e6ee-470e-b837-68411050a4f6
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
The “only generate NSObject factories for generic types” behavior isn’t applied consistently (ManagedRegistrarLookupTablesStep still unconditionally generates the NSObject factory), so the fix appears incomplete for some build flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Lite
Treat a missing static constructor as having no interface dynamic dependencies, so the test remains focused on the behavior it verifies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d41380a-e6ee-470e-b837-68411050a4f6
✅ 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 #ebb8d3f] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 264 tests passed 🎉 Tests counts✅ assembly-processing: 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