[bgen] Propagate nullability information for generic type arguments. Fixes #16860 by rolfbjarne · Pull Request #25541 · dotnet/macios

rolfbjarne · 2026-05-27T12:56:36Z

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 like Action<UIViewController?, NSError?> to be rendered as Action<UIViewController, NSError> — losing the nullability annotations.

Changes

src/bgen/AttributeManager.cs: Added GetNullabilityBytes() method that extracts the full byte array from [NullableAttribute].
src/bgen/TypeManager.cs: Added FormatType(Type?, Type?, byte[]?) overload and a recursive FormatTypeUsedIn helper 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 avoid Func<> covariance issues with trampoline signatures.
tests/bgen/BGenTests.cs: Added GenericTypeNullability test 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

Value types skip byte consumption: The C# compiler does NOT emit bytes for value types in the NullableAttribute array. The fix correctly identifies value types and skips them during traversal.
Func<> covariance guard: Trampolines use non-nullable generic args. This is fine for Action<in T> (contravariant) but breaks for Func<out T> (covariant). The non-wrap path only applies nullability for void-returning delegates.
Wrap path is unrestricted: The wrap path generates self-contained code without trampoline interaction, so it can safely apply nullability for any delegate type.

Fixes #16860

🤖 Pull request created by Copilot

…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>

rolfbjarne · 2026-06-03T18:56:41Z

/azp run

azure-pipelines · 2026-06-03T18:57:09Z

Azure Pipelines successfully started running 3 pipeline(s).

rolfbjarne · 2026-06-04T07:33:21Z

/azp run

azure-pipelines · 2026-06-04T07:33:51Z

Azure Pipelines successfully started running 3 pipeline(s).

Pass wrapNullabilityBytes to FormatType calls in the getter body so that cast/construction expressions match the declared property type when it has nullable generic arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

rolfbjarne · 2026-06-25T12:36:32Z

/review

github-actions · 2026-06-25T12:37:22Z

✅ .NET for Apple Platforms PR Reviewer completed successfully!

github-actions

Review Summary

This PR implements proper nullability propagation for generic type arguments in bgen—fixing a long-standing gap where Action<NSObject?, NSError?> was rendered as Action<NSObject, NSError>. The implementation is well-architected with comprehensive test coverage.

✅ Strengths

Thorough test coverage: 10 test cases covering nullable args, non-nullable args, value types, mixed patterns, and alternating nullability
Consistent depth-first traversal: Both bgen and mono-api-info use the same depth-first byte-consumption algorithm
Well-documented: Excellent inline comments explaining the byte array encoding and the Func<> covariance issue
Parallel fixes: api-tools updated to match, ensuring API diff reports are correct

Issues Found

⚠️ Warnings (3)

Error handling: Missing null check in AttributeManager.GetNullabilityBytes could throw NullReferenceException
Performance: AsyncMethodInfo.cs has O(n2) type tree traversal when computing NSError nullability for complex generic types
Breaking change: MarkdownFormatter.DiffModification swaps removal/addition order—confirm this is intentional

💡 Suggestions (4)

Code organization: Extract duplicate "is-void-returning-delegate" check to a helper method (appears 3 times)
Error handling: Out-of-bounds nullability array access silently defaults to oblivious rather than failing fast
Code duplication: Nullability suffix detection logic duplicated between ApiChange.cs and Helpers.cs
Documentation: Add comment explaining why NullableAttribute is now skipped in mono-api-info.cs

CI Status

✅ Most checks passing. Some builds/tests still in progress.

Verdict

⚠️ Needs Minor Changes — The core logic is sound, but address the null-check issue and the DiffModification swap before merging. The performance concern in AsyncMethodInfo is worth investigating but may be acceptable given that most completion handlers have 2-3 args.

Generated by .NET for Apple Platforms PR Reviewer for issue #25541 · 71.4 AIC · ⌖ 9.49 AIC · ⊞ 5.1K
_{Comment /review to run again}

Clarify that the original code had old/new reversed (additions were rendered as removals and vice versa in markdown output). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

The element type for wrapped arrays is a simple non-generic type (e.g. NSObject), so FormatType ignores the bytes anyway. Passing them is misleading and would produce incorrect byte indexing if the element type were ever generic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

vs-mobiletools-engineering-service2 · 2026-06-25T13:16:06Z

✅ [PR Build #2512b80] Build passed (Detect API changes) ✅

Pipeline on Agent
Hash: 2512b809e424a504f84cf0cfda01d414dd0fc6f6 [PR build]

vs-mobiletools-engineering-service2 · 2026-06-25T13:18:03Z

✅ [PR Build #2512b80] Build passed (Build packages) ✅

Pipeline on Agent
Hash: 2512b809e424a504f84cf0cfda01d414dd0fc6f6 [PR build]

vs-mobiletools-engineering-service2 · 2026-06-25T13:28:21Z

✅ API diff for current PR / commit

NET (empty diffs)

iOS: vsdrops gist ( No breaking changes )
tvOS: vsdrops gist ( No breaking changes )
MacCatalyst: vsdrops gist ( No breaking changes )
macOS: vsdrops gist ( No breaking changes )

✅ API diff vs stable

NET (empty diffs)

iOS: vsdrops gist ( No breaking changes )
tvOS: vsdrops gist ( No breaking changes )
MacCatalyst: vsdrops gist ( No breaking changes )
macOS: vsdrops gist ( No breaking changes )

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 2512b809e424a504f84cf0cfda01d414dd0fc6f6 [PR build]

vs-mobiletools-engineering-service2 · 2026-06-25T13:37:04Z

✅ [PR Build #2512b80] Build passed (Build macOS tests) ✅

Pipeline on Agent
Hash: 2512b809e424a504f84cf0cfda01d414dd0fc6f6 [PR build]

rolfbjarne and others added 3 commits May 27, 2026 14:16

[bgen] Remove null-forgiving operator usage, use pattern matching ins…

bce4d13

…tead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rolfbjarne added the copilot label May 27, 2026

This comment has been minimized.

Sign in to view

Merge branch 'main' into dev/rolf/bgen-generic-actions

54f622b

This comment has been minimized.

Sign in to view

Merge branch 'main' into dev/rolf/bgen-generic-actions

c0a4994

This comment has been minimized.

Sign in to view

rolfbjarne requested a review from Copilot June 25, 2026 12:24

Copilot started reviewing on behalf of rolfbjarne June 25, 2026 12:24 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

github-actions Bot reviewed Jun 25, 2026

View reviewed changes

Add comment explaining DiffModification fix

f576008

Clarify that the original code had old/new reversed (additions were rendered as removals and vice versa in markdown output). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rolfbjarne requested a review from Copilot June 25, 2026 12:46

Copilot started reviewing on behalf of rolfbjarne June 25, 2026 12:46 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread src/bgen/Generator.cs Outdated

rolfbjarne requested a review from Copilot June 25, 2026 12:59

This comment has been minimized.

Sign in to view

Copilot started reviewing on behalf of rolfbjarne June 25, 2026 13:00 View session

Uh oh!

Conversation

rolfbjarne commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Key design decisions

Uh oh!

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.

rolfbjarne commented Jun 3, 2026

Uh oh!

azure-pipelines Bot commented Jun 3, 2026

Uh oh!

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.

rolfbjarne commented Jun 4, 2026

Uh oh!

azure-pipelines Bot commented Jun 4, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rolfbjarne commented Jun 25, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

✅ Strengths

Issues Found

CI Status

Verdict

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Jun 25, 2026

rolfbjarne commented May 27, 2026 •

edited

Loading

github-actions Bot commented Jun 25, 2026 •

edited

Loading