[sharpie] Fix platform type mapping for types inside generic type arguments. Fixes #24892.#24911
Conversation
…uments The PlatformTypeMappingMassager did not recurse into child nodes (such as type arguments inside Action<> or Func<>) because the VisitMemberType and VisitSimpleType overrides did not call their base implementations. This meant that types like NSURLResponse inside Action<NSData, NSURLResponse> were never mapped to their .NET equivalents (NSUrlResponse). Fix by calling base.VisitMemberType/VisitSimpleType before processing the type itself, so children (type arguments) are mapped first. Added a test case with a block parameter containing mapped types. Fixes #24892 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ping The PlatformTypeMappingMassager now correctly recurses into generic type arguments, which means protocol names like NSSecureCoding inside generic constraints (e.g. NSObject<NSCopying, NSSecureCoding>) are now properly mapped to their .NET equivalents (INSSecureCoding). Update the expected test output to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Sharpie’s PlatformTypeMappingMassager so that platform type mapping is applied recursively inside generic type arguments (for example within Action<>/Func<> block signatures), ensuring native types like NSURLResponse get correctly mapped to their .NET equivalents (e.g. NSUrlResponse). This addresses the generation issue reported in #24892.
Changes:
- Update
PlatformTypeMappingMassagerto callbase.VisitMemberType/base.VisitSimpleTypeso child nodes (type arguments) are visited and mapped. - Extend the PlatformTypeMapping test header and expected outputs with a block parameter that includes a mapped type inside an
Action<>. - Update ObjC generics expected outputs to reflect the now-recursive type mapping behavior in generic arguments.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/sharpie/Sharpie.Bind/Massagers/PlatformTypeMappingMassager.cs | Ensures AST traversal recurses into generic arguments so nested types are mapped. |
| tests/sharpie/Tests/Massagers/PlatformTypeMapping.h | Adds an Objective-C method with a block parameter containing NSURLResponse to exercise nested mapping. |
| tests/sharpie/Tests/Massagers/PlatformTypeMapping.macosx.cs | Updates expected output to include Action<..., NSUrlResponse> and adds using System;. |
| tests/sharpie/Tests/Massagers/PlatformTypeMapping.iphoneos.cs | Updates expected output to include Action<..., NSUrlResponse> and adds using System;. |
| tests/sharpie/Tests/ObjCGenerics.macosx.cs | Updates expected output for generic type arguments affected by recursive mapping. |
| tests/sharpie/Tests/ObjCGenerics.iphoneos.cs | Updates expected output for generic type arguments affected by recursive mapping. |
You can also share your feedback on Copilot code review. Take the survey.
| NSObject<NSCopying, NSSecureCoding> ValueTypeProperty { get; } | ||
| NSObject<NSCopying, INSSecureCoding> ValueTypeProperty { get; } | ||
|
|
||
| // -(ValueType _Nullable)getValueTypeMethod; | ||
| [NullAllowed, Export ("getValueTypeMethod")] | ||
| [Verify (MethodToProperty)] | ||
| NSObject<NSCopying, NSSecureCoding> ValueTypeMethod { get; } | ||
| NSObject<NSCopying, INSSecureCoding> ValueTypeMethod { get; } | ||
|
|
||
| // -(void)setValueTypeMethod:(ValueType _Nullable)obj; | ||
| [Export ("setValueTypeMethod:")] | ||
| void SetValueTypeMethod ([NullAllowed] NSObject<NSCopying, NSSecureCoding> obj); | ||
| void SetValueTypeMethod ([NullAllowed] NSObject<NSCopying, INSSecureCoding> obj); | ||
| } |
There was a problem hiding this comment.
@rolfbjarne This is interesting, so we have some inconsistencies in protocols?
While INSSecureCoding is correct, why is NSCopying not getting picked to be INSCopying?
|
@dalexsoto Good catch — this is a pre-existing inconsistency in the collision detection logic in
Both map to the same native name
No collision → the mapping The |
|
I'll ask Copilot to fix this too, in a different pull request. |
✅ [CI Build #33b540c] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #33b540c] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #33b540c] Build passed (Build macOS tests) ✅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 #33b540c] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
The PlatformTypeMappingMassager did not recurse into child nodes (such as type
arguments inside Action<> or Func<>) because the VisitMemberType and
VisitSimpleType overrides did not call their base implementations. This meant
that types like NSURLResponse inside Action<NSData, NSURLResponse> were never
mapped to their .NET equivalents (NSUrlResponse).
Fix by calling base.VisitMemberType/VisitSimpleType before processing the type
itself, so children (type arguments) are mapped first.
Added a test case with a block parameter containing mapped types.
Fixes #24892.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com