[CoreNFC] Add missing nullability on NSError parameters in callbacks. - #26531
Conversation
Helpers.GetName(TypeDefinition) only checked ProtocolAttribute's constructor arguments for the native protocol name, but [Protocol]'s Name is set via a named property, not a constructor argument (e.g. [Protocol (Name = "NFCISO15693Tag")]). This caused the lookup to fall back to the managed type name (NFCIso15693Tag), which doesn't match the native declaration's casing (NFCISO15693Tag), so NullabilityCheck's GetMethod lookup silently failed and no !missing-null-allowed! diagnostic was ever produced for members of such protocols. This is why NFCISO15693Tag.stayQuietWithCompletionHandler:'s missing nullable annotation on its block parameter was never caught, even though the sibling ConnectToTag/ConnectTo methods (bound with non-property-based native names) were correctly flagged. Fix GetName to also check the attribute's properties for 'Name', and add the now-detected StayQuiet diagnostic to the CoreNFC .ignore files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Count both the array and its element type when walking NullableAttribute positions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1fdf7050-96ec-4575-91b8-12824be5644a
Non-generic value types do not consume positions in NullableAttribute byte arrays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1fdf7050-96ec-4575-91b8-12824be5644a
There was a problem hiding this comment.
🟡 Changes recommended
xtro-sharpie’s CountNullablePositions still miscounts generic value types due to branch ordering, which can produce incorrect nullability diagnostics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates CoreNFC bindings to correctly surface nullable NSError parameters in completion callbacks (per Apple docs), and adjusts xtro-sharpie’s nullability accounting so it can correctly interpret nullable positions for arrays/value types when validating generated APIs.
Changes:
- Enable nullable context in
src/corenfc.csand annotateNSError(and related callback parameters) as nullable where native APIs can passnil. - Fix xtro-sharpie’s nullable-position counting for array types and value types.
- Refresh xtro-sharpie ignore baselines by removing entries/files that are no longer expected after the analysis fix and binding updates.
File summaries
| File | Description |
|---|---|
| tests/xtro-sharpie/xtro-sharpie/NullabilityCheck.cs | Updates nullable-position accounting (arrays/value types) used to validate block parameter nullability. |
| tests/xtro-sharpie/api-annotations-dotnet/macOS-HealthKit.ignore | Removes a now-unneeded ignore entry after nullability-position accounting changes. |
| tests/xtro-sharpie/api-annotations-dotnet/MacCatalyst-HealthKit.ignore | Removes a now-unneeded ignore entry after nullability-position accounting changes. |
| tests/xtro-sharpie/api-annotations-dotnet/iOS-HealthKit.ignore | Removes a now-unneeded ignore entry after nullability-position accounting changes. |
| tests/xtro-sharpie/api-annotations-dotnet/MacCatalyst-CoreNFC.ignore | Removes the ignore file now that CoreNFC callback nullability is corrected and/or diagnostics no longer apply. |
| tests/xtro-sharpie/api-annotations-dotnet/iOS-CoreNFC.ignore | Removes now-unneeded CoreNFC nullability ignore entries. |
| tests/xtro-sharpie/api-annotations-dotnet/MacCatalyst-AppClip.ignore | Removes the ignore file now that diagnostics no longer apply. |
| tests/xtro-sharpie/api-annotations-dotnet/iOS-AppClip.ignore | Removes the ignore file now that diagnostics no longer apply. |
| tests/xtro-sharpie/api-annotations-dotnet/iOS-ARKit.ignore | Removes the ignore file now that diagnostics no longer apply. |
| src/corenfc.cs | Enables nullable context and fixes nullable NSError (and related) parameters in completion callbacks, including NFCQueryNdefStatusCompletionHandler.error. |
Review details
- Files reviewed: 10/10 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.
Move the IsValueType check before GenericInstanceType handling in CountNullablePositions so generic value types (such as Nullable<T>) contribute zero nullable metadata positions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
rolfbjarne
added
ready-to-review
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.
✅ 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 #d16f27a] 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 |
Also a few nullability fixes in xtro to properly detect nullable types.
Fixes #21540.