[bgen] Propagate nullability information for generic type arguments. Fixes #16860#25541
[bgen] Propagate nullability information for generic type arguments. Fixes #16860#25541rolfbjarne wants to merge 18 commits into
Conversation
…ixes #16860. When a property has a generic type like Action<NSObject?, NSError?>, the C# compiler emits a [NullableAttribute(byte[])] where each byte encodes the nullability for the outer type and each generic argument in depth-first order. Previously, bgen only looked at the first byte (the outer type) and ignored nullability for generic type arguments. This change: - Adds GetNullabilityBytes() to AttributeManager to extract the full byte array - Adds a FormatType overload in TypeManager that processes the byte array to annotate each generic type argument with ? where appropriate - Uses the new method when rendering property type declarations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…onsumption. - Use braces for all if/else/for blocks in new code. - Add complex test samples with many generic args, value types, and alternating nullability patterns. - Fix bug where value types incorrectly consumed nullability bytes. - Only apply nullability for void-returning delegates (Action<>) in the non-wrap path to avoid Func<> covariance issues with trampolines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tead. 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.
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.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
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.
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.
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.
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 MarkdownFormatter had the old and new text swapped in two places: - DiffModification wrapped 'old' with addition markers and 'new' with removal markers (backwards compared to HtmlFormatter). - The Diff method assigned the wrong Clean() arguments to the - and + lines, causing the old line to show added content and vice versa. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ [PR Build #60617e8] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #60617e8] 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 |
✅ [PR Build #60617e8] Build passed (Build macOS tests) ✅Pipeline on Agent |
There was a problem hiding this comment.
Pull request overview
This PR updates bgen and related tooling to correctly propagate C# nullable-reference-type annotations (?) into generated signatures for generic type arguments (notably Action<T1,...> completion handlers), addressing #16860.
Changes:
- bgen: extract full
[Nullable]byte arrays and use them to render?on generic type arguments in generated C# signatures (with a guard to avoidFunc<>trampoline mismatches). - api-tools: render full nullability in mono-api-info outputs and improve markdown/html diff formatting for nullability-only changes.
- tests: add a new bgen test input + assertions covering generic-argument nullability patterns (including value-type skipping).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/api-tools/mono-api-info/mono-api-info.cs | Formats member type names with full nullability (incl. generic args) and omits nullable metadata attributes from output. |
| tools/api-tools/mono-api-html/MultiplexedFormatter.cs | Ensures placeholder </> tokens are replaced per-formatter before diff rendering. |
| tools/api-tools/mono-api-html/MarkdownFormatter.cs | Fixes add/remove handling in modification diffs and handles nullability-only diff markers. |
| tools/api-tools/mono-api-html/ApiChange.cs | Treats placeholder-escaped > as a valid separator when detecting ? suffixes. |
| tests/bgen/tests/generic-type-nullability.cs | Adds binding input covering nullable/non-nullable generic args, value types, and mixed patterns. |
| tests/bgen/BGenTests.cs | Adds a new test validating that generated output includes ? on the correct generic arguments. |
| src/Foundation/NSUrlSessionHandler.cs | Updates redirection completion handler to accept nullable NSUrlRequest? and passes null directly. |
| src/foundation.cs | Updates the delegate signature for redirection to Action<NSUrlRequest?> (scoped #nullable enable). |
| src/bgen/TypeManager.cs | Adds formatting that consumes nullability-byte arrays to render nullable generic arguments. |
| src/bgen/Models/AsyncMethodInfo.cs | Updates async-method analysis to infer nullable NSError? from the outer parameter’s nullability bytes. |
| src/bgen/Generator.cs | Threads nullability-byte info into parameter/property signature rendering (with Func<> covariance guard). |
| src/bgen/AttributeManager.cs | Adds GetNullabilityBytes to retrieve the full [Nullable] byte array. |
| if (nullabilityBytes is null || nullabilityBytes.Length <= 1) { | ||
| return FormatTypeUsedIn (usedIn?.Namespace, type); | ||
| } |
| // Reference types consume one byte from the array | ||
| byte currentByte = index < nullabilityBytes.Length ? nullabilityBytes [index] : (byte) 0; | ||
| index++; | ||
|
|
| if (nullabilityBytes is not null && nullabilityBytes.Length > 1) { | ||
| // Walk the type arguments depth-first to find the byte index for the last param. | ||
| // Value types don't consume bytes, reference types do. | ||
| // byte[0] is for the Action<> itself, then each reference type arg consumes one byte. | ||
| int byteIndex = 1; // start after the outer type byte | ||
| var genericArgs = lastType.GetGenericArguments (); | ||
| for (int i = 0; i < genericArgs.Length; i++) { | ||
| if (!genericArgs [i].IsValueType) { | ||
| if (i == genericArgs.Length - 1) { | ||
| // This is the last generic argument (NSError) | ||
| IsNSErrorNullable = byteIndex < nullabilityBytes.Length && nullabilityBytes [byteIndex] == 2; | ||
| } | ||
| byteIndex++; | ||
| } | ||
| } | ||
| } else { | ||
| IsNSErrorNullable = generator.AttributeManager.IsNullable (lastParam); | ||
| } |
🔥 [CI Build #60617e8] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 5 tests failed, 202 tests passed. Failures❌ fsharp tests2 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ xcframework tests2 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ Tests on macOS Sequoia (15) tests1 tests failed, 4 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Summary
When properties have generic delegate types (like
Action<NSObject?, NSError?>), bgen now correctly propagates the?nullability annotations on the generic type arguments in the generated C# code.Previously, bgen only read the first byte of the
[NullableAttribute(byte[])]array (for the outer type's nullability), ignoring the remaining bytes that encode nullability for generic type arguments. This caused properties likeAction<UIViewController?, NSError?>to be rendered asAction<UIViewController, NSError>— losing the nullability annotations.Changes
src/bgen/AttributeManager.cs: AddedGetNullabilityBytes()method that extracts the full byte array from[NullableAttribute].src/bgen/TypeManager.cs: AddedFormatType(Type?, Type?, byte[]?)overload and a recursiveFormatTypeUsedInhelper that consumes nullability bytes in depth-first traversal order, correctly skipping value types (which don't have bytes in the array).src/bgen/Generator.cs: Updated property rendering to pass nullability bytes. The "wrap" path applies nullability for all delegate types. The "non-wrap" path only applies for void-returning delegates (Action<>) to avoidFunc<>covariance issues with trampoline signatures.tests/bgen/BGenTests.cs: AddedGenericTypeNullabilitytest with 10 assertions.tests/bgen/tests/generic-type-nullability.cs: Test input covering nullable args, non-nullable args, value types, many args, mixed patterns, and alternating nullability.Key design decisions
NullableAttributearray. The fix correctly identifies value types and skips them during traversal.Func<>covariance guard: Trampolines use non-nullable generic args. This is fine forAction<in T>(contravariant) but breaks forFunc<out T>(covariant). The non-wrap path only applies nullability for void-returning delegates.Fixes #16860
🤖 Pull request created by Copilot