[RGen] Handle sealed property and methods correctly. #23663

mandel-macaque · 2025-08-21T17:38:56Z

From the review we noticed that sealed property and methods do not need the IsDirecBinding check. We update the struct to let use know if the method is sealed and generate the call accordingly. We just need to update some if conditions.

Tests have been updated to handle this case.

From the [review](#23656 (review)) we noticed that sealed property and methods do not need the IsDirecBinding check. We update the struct to let use know if the method is sealed and generate the call accordingly. We just need to update some if conditions. Tests have been updated to handle this case.

Copilot

Pull Request Overview

This PR optimizes code generation for sealed properties and methods in the RGen (binding generator) by eliminating unnecessary IsDirectBinding checks. When properties or methods are marked as sealed, they cannot be overridden, so the generator can call Objective-C messaging directly without checking for subclass overrides.

Key changes:

Added IsSealed property to both Method and Property data models to track sealed modifiers
Updated code generation logic to bypass IsDirectBinding checks for sealed members
Added test cases and expected output files for sealed property and method scenarios

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/rgen/Microsoft.Macios.Generator/DataModel/Method.cs	Added IsSealed property to track sealed modifier on methods
src/rgen/Microsoft.Macios.Generator/DataModel/Property.cs	Added IsSealed property to track sealed modifier on properties
src/rgen/Microsoft.Macios.Generator/Emitters/ClassEmitterExtensions.cs	Updated code generation logic to use direct messaging for sealed members
tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/MethodTests.cs	Added test case for sealed method
tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/PropertyTests.cs	Added test case for sealed property
Multiple expected output files	Updated with generated code for sealed members without IsDirectBinding checks

Comments suppressed due to low confidence (2)

Copilot · 2025-08-21T17:39:39Z

tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/PropertyTests.cs

+	[SupportedOSPlatform ("macos")]
+	[SupportedOSPlatform ("maccatalyst13.1")]
+	[Export<Property> ("sealedProperty")]
+	public sealed partial string SelaedProperty { get; set; }


The property name 'SelaedProperty' contains a spelling error. It should be 'SealedProperty'.

Suggested change

public sealed partial string SelaedProperty { get; set; }

public sealed partial string SealedProperty { get; set; }

Copilot · 2025-08-21T17:39:40Z

tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/tvOSExpectedPropertyTests.cs

+	[SupportedOSPlatform ("tvos")]
+	[SupportedOSPlatform ("maccatalyst13.1")]
+	[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
+	public sealed partial string SelaedProperty


The property name 'SelaedProperty' contains a spelling error. It should be 'SealedProperty'.

Suggested change

public sealed partial string SelaedProperty

public sealed partial string SealedProperty

vs-mobiletools-engineering-service2 · 2025-08-21T18:54:51Z

💻 [CI Build #5d3c39c] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: 5d3c39c1f281cd18b9f1cddb45aa1888c44a1574 [PR build]

rolfbjarne · 2025-08-22T11:58:30Z

tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/PropertyTests.cs

+	[SupportedOSPlatform ("macos")]
+	[SupportedOSPlatform ("maccatalyst13.1")]
+	[Export<Property> ("sealedProperty")]
+	public sealed partial string SelaedProperty { get; set; }


The [Sealed] attribute in bgen has more than one meaning, and the one that's important here is "don't export this method to Objective-C". This means the method must not have an [Export] attribute, and as an optimization, we can skip the IsDirectBinding check (because it won't call itself because it's not exported to Objective-C).

As such, I think it would be better to use something other than the sealed keyword to mark such APIs, because there's no reason it shouldn't be possible to export a sealed method to Objective-C (also it's a rather unintituive/obscure behavior).

Ideas:

Use a different attribute, say [Import ("selector")] or [Expose ("selector")] instead of [Export<T> ("selector")].

Use a property/flag on the [Export<T>] attribute, say [Export<T> ("selector", OnlyImport = true)]

I prefer to property flag, makes the api consistent

rolfbjarne · 2025-09-03T06:34:47Z

src/rgen/Microsoft.Macios.Generator/Emitters/ClassEmitterExtensions.cs

 				var (tempVar, tempDeclaration) = GetReturnValueAuxVariable (property.ReturnType);
 				// if the binding is a protocol, we need to call send directly
-				if (context.Changes.BindingType == BindingType.Protocol) {
+				if (context.Changes.BindingType == BindingType.Protocol || property.IsSealed) {


Suggested change

if (context.Changes.BindingType == BindingType.Protocol || property.IsSealed) {

if (context.Changes.BindingType == BindingType.Protocol || property.SkipRegistration) {

rolfbjarne · 2025-09-03T06:35:01Z

src/rgen/Microsoft.Macios.Generator/Emitters/ClassEmitterExtensions.cs

 				// perform the invocation
 				// if the binding is a protocol, we need to call send directly
-				if (context.Changes.BindingType == BindingType.Protocol) {
+				if (context.Changes.BindingType == BindingType.Protocol || property.IsSealed) {


Suggested change

if (context.Changes.BindingType == BindingType.Protocol || property.IsSealed) {

if (context.Changes.BindingType == BindingType.Protocol || property.SkipRegistration) {

rolfbjarne · 2025-09-03T06:36:38Z

src/rgen/Microsoft.Macios.Generator/DataModel/Method.cs

+	readonly bool isSealed;
+
+	/// <summary>
+	/// True if the method was marked as sealed.
+	/// </summary>
+	public bool IsSealed => isSealed;
+


No longer needed:

Suggested change

readonly bool isSealed;

/// <summary>

/// True if the method was marked as sealed.

/// </summary>

public bool IsSealed => isSealed;

rolfbjarne · 2025-09-03T06:36:50Z

src/rgen/Microsoft.Macios.Generator/DataModel/Method.cs

 		init {
 			modifiers = value;
 			isStatic = modifiers.Any (x => x.IsKind (SyntaxKind.StaticKeyword));
+			isSealed = modifiers.Any (x => x.IsKind (SyntaxKind.SealedKeyword));


Suggested change

isSealed = modifiers.Any (x => x.IsKind (SyntaxKind.SealedKeyword));

rolfbjarne · 2025-09-03T06:37:04Z

src/rgen/Microsoft.Macios.Generator/DataModel/Property.cs

+	readonly bool isSealed;
+
+	/// <summary>
+	/// True if the method was marked as sealed.
+	/// </summary>
+	public bool IsSealed => isSealed;
+


Suggested change

readonly bool isSealed;

/// <summary>

/// True if the method was marked as sealed.

/// </summary>

public bool IsSealed => isSealed;

rolfbjarne · 2025-09-03T06:37:16Z

src/rgen/Microsoft.Macios.Generator/DataModel/Property.cs

 		init {
 			modifiers = value;
 			isStatic = modifiers.Any (x => x.IsKind (SyntaxKind.StaticKeyword));
+			isSealed = modifiers.Any (x => x.IsKind (SyntaxKind.SealedKeyword));


Suggested change

isSealed = modifiers.Any (x => x.IsKind (SyntaxKind.SealedKeyword));

Ugh I forgot I added in more than one place. Cleaning.

rolfbjarne · 2025-09-03T06:38:33Z

tests/rgen/Microsoft.Macios.Generator.Tests/Classes/Data/PropertyTests.cs

+	[SupportedOSPlatform ("macos")]
+	[SupportedOSPlatform ("maccatalyst13.1")]
+	[Export<Property> ("sealedProperty")]
+	public sealed partial string SelaedProperty { get; set; }


Suggested change

public sealed partial string SelaedProperty { get; set; }

public sealed partial string SealedProperty { get; set; }

vs-mobiletools-engineering-service2 · 2025-09-03T15:15:39Z

✅ [CI Build #cb0fbb4] Build passed (Build packages) ✅

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T15:16:23Z

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

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T15:42:51Z

✅ API diff for current PR / commit

.NET ( No breaking changes )

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 ( No breaking changes )

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: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T15:46:22Z

✅ [CI Build #cb0fbb4] Build passed (Build macOS tests) ✅

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:00:53Z

💻 [CI Build #cb0fbb4] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:01:48Z

💻 [CI Build #cb0fbb4] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:02:51Z

✅ [CI Build #4befbdc] Build passed (Build macOS tests) ✅

Pipeline on Agent
Hash: 4befbdc6bb3d99398ac5ed86c87c59582bff6c5c [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:03:29Z

💻 [CI Build #cb0fbb4] Tests on macOS arm64 - Mac Sequoia (15) passed 💻

✅ All tests on macOS arm64 - Mac Sequoia (15) passed.

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:15:32Z

💻 [CI Build #4befbdc] Tests on macOS arm64 - Mac Sequoia (15) passed 💻

✅ All tests on macOS arm64 - Mac Sequoia (15) passed.

Pipeline on Agent
Hash: 4befbdc6bb3d99398ac5ed86c87c59582bff6c5c [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:18:47Z

💻 [CI Build #4befbdc] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: 4befbdc6bb3d99398ac5ed86c87c59582bff6c5c [PR build]

vs-mobiletools-engineering-service2 · 2025-09-03T16:23:31Z

💻 [CI Build #4befbdc] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: 4befbdc6bb3d99398ac5ed86c87c59582bff6c5c [PR build]

vs-mobiletools-engineering-service2 · 2025-09-04T18:12:41Z

🔥 [CI Build #cb0fbb4] Test results 🔥

Test results

❌ Tests failed on VSTS: test results

1 tests crashed, 0 tests failed, 112 tests passed.

Failures

❌ monotouch tests (macOS) [attempt 3]

🔥 Failed catastrophically on VSTS: test results - monotouch_macos (no summary found).

Html Report (VSDrops) Download

Successes

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 2 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 5 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 4 tests passed. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 44 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 15 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ monotouch (tvOS): All 10 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ windows: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. [attempt 2] Html Report (VSDrops) Download

Pipeline on Agent
Hash: cb0fbb49b5f25ea150741fca81ff6db7d4efb267 [PR build]

mandel-macaque · 2025-09-04T18:33:26Z

mono Mac tests are not touch by the code changes.

mandel-macaque requested review from Copilot and rolfbjarne August 21, 2025 17:39

Copilot AI reviewed Aug 21, 2025

View reviewed changes

mandel-macaque mentioned this pull request Aug 21, 2025

[RGen] Update the class validator to deal with properties and fields. #23656

Merged

Auto-format source code

5d3c39c

This comment has been minimized.

Sign in to view

rolfbjarne requested changes Aug 22, 2025

View reviewed changes

Merge branch 'main' into dev/mandel/rgen-sealed-methods

b462185

This comment has been minimized.

Sign in to view

rolfbjarne requested changes Sep 3, 2025

View reviewed changes

mandel-macaque and others added 2 commits September 3, 2025 10:45

Address reviews.

4befbdc

Auto-format source code

cb0fbb4

rolfbjarne approved these changes Sep 3, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

mandel-macaque merged commit 99eaf5f into main Sep 4, 2025
37 of 40 checks passed

mandel-macaque deleted the dev/mandel/rgen-sealed-methods branch September 4, 2025 18:37

	public sealed partial string SelaedProperty { get; set; }
	public sealed partial string SealedProperty { get; set; }

	if (context.Changes.BindingType == BindingType.Protocol \|\| property.IsSealed) {
	if (context.Changes.BindingType == BindingType.Protocol \|\| property.SkipRegistration) {

[RGen] Handle sealed property and methods correctly. #23663

[RGen] Handle sealed property and methods correctly. #23663

Uh oh!

Conversation

mandel-macaque commented Aug 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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.

vs-mobiletools-engineering-service2 commented Aug 21, 2025

💻 [CI Build #5d3c39c] Tests on macOS X64 - Mac Sonoma (14) passed 💻

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.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Sep 3, 2025

✅ [CI Build #cb0fbb4] Build passed (Build packages) ✅

Uh oh!

vs-mobiletools-engineering-service2 commented Sep 3, 2025

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

Uh oh!

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Sep 3, 2025

✅ API diff for current PR / commit