[Rgen] Implement the generation of the smart enums. #21562

mandel-macaque · 2024-10-31T17:41:08Z

Add the code generation for smart enumerators. This means the following changes:

Refactor the way we register the diff pipelines.
Add code that generates the Extension class for the enums.
Add code that will generate the Library.g.cs partial class.

Add the code generation for smart enumerators. This means the following changes: 1. Refactor the way we register the diff pipelines. 2. Add code that generates the Extension class for the enums. 3. Add code that will generate the Library.g.cs partial class.

mandel-macaque · 2024-10-31T18:00:04Z

src/Makefile

@@ -460,6 +460,7 @@ $($(2)_DOTNET_BUILD_DIR)/$(4)/Microsoft.$(1)%dll $($(2)_DOTNET_BUILD_DIR)/$(4)/M
 		$(DOTNET_FLAGS) \
 		/analyzer:$(ROSLYN_GENERATOR_COMMON) \
 		/analyzer:$(ROSLYN_GENERATOR) \
+		/generatedfilesout:$($(2)_DOTNET_BUILD_DIR)/generated-sources \


Allows to see the generator API diff :)

stephen-hawley

Love all the tests.

stephen-hawley · 2024-10-31T18:02:18Z

src/ObjCBindings/FieldAttribute.cs

+		public string SymbolName { get; set; }
+
+		/// <summary>
+		/// Get/Set the library that contains the symbol..


I'd like this to be clear as to whether it is:

the name of the library

a path to the library and whether or not it is absolute and/or relative and to what it is relative (RPATH?)

stephen-hawley · 2024-10-31T18:05:46Z

src/ObjCBindings/FieldAttribute.cs

+		/// Create a new FieldAttribute for the given symbol in the provided library and customizing the flags.
+		/// <param name="symbolName">The name of the symbol.</param>
+		/// <param name="libraryName">The name of the library that contains the symbol.</param>
+		/// <param name="flags">The flags to customize the field.</param>


Is it a requirement that the enum should have FlagsAttribute on it? If so can we put a check in the constructor to catch incorrect usage such as:

[Field<StringComparison>]

No, it does not need to have a flag. The default flat will be use in that case. 'default (T)' in C# on an enumerator will always return the enumerator value that is mapped to 0. We can add a code analyzer for exactly that, coming in the next PR.

stephen-hawley · 2024-10-31T18:09:13Z

src/ObjCBindings/FieldAttribute.cs

+		public FieldAttribute (string symbolName, string libraryName, T? flags)
+		{
+			SymbolName = symbolName;
+			LibraryName = libraryName;


LibraryName is a nullable string. I think you'd be better off with this constructor:

public FieldAttribute (string symbolName, string? libraryName = null, T? flags = default (T)) { SymbolName = symbolName; LibraryName = libraryName; Flags = flags }

Then you can eliminate the other two constructors entirely.

The issue with that is that it does not allow to do the following:

[Field<Method>("Foo", Method.Appearance)] public int Test { get; }

The compiler will complain with:

Error CS1503 : Argument 2: cannot convert from 'TestCSharp.AttrsTest.Method' to 'string?'

So for the user to only provide the flags they will have to use one of the following:

[Field<Method>("Foo", flags:Method.Appearance)] public int Test { get; } [Field<Method>("Foo", null, Method.Appearance)] public int Test { get; }

I wanted to provide an API in which we have to write the least amount.

stephen-hawley · 2024-10-31T18:17:56Z

src/rgen/Microsoft.Macios.Generator/BindingSourceGeneratorGenerator.cs

+						SourceText.From (code, Encoding.UTF8));
+				} else {
+					// add to the diagnostics and continue to the next possible candidate
+					foreach (Diagnostic diagnostic in diagnostics) {


nit: foreach (var diagnostic...)
or if you don't care about setting a breakpoint:

diagnostics.ForEach (context.ReportDiagnostic);

Moved that to an extension method as per rolf comments.

stephen-hawley · 2024-10-31T18:20:41Z

src/rgen/Microsoft.Macios.Generator/BindingSourceGeneratorGenerator.cs

+	}
+
+	/// <summary>
+	/// Code generator that emmits the static classes that contain the pointers to the library used


emmits -> emits

stephen-hawley · 2024-10-31T18:47:27Z

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

+			emitter = null;
+			break;
+		}
+


I like this pattern.

stephen-hawley · 2024-10-31T18:55:15Z

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

+	public string SymbolName => "Libraries";
+	INamedTypeSymbol? LibrarySymbol { get; } = context.Compilation.GetTypeByMetadataName ("ObjCRuntime.Libraries");
+
+	bool IsSymbolPreset (string className)


Did you mean IsSymbolPresent?

Yes, I'll fix that.

stephen-hawley · 2024-10-31T19:00:14Z

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

+	public IEnumerable<string> UsingStatements { get; } = [];
+	public bool TryEmit ([NotNullWhen (false)] out ImmutableArray<Diagnostic>? diagnostics)
+	{
+		diagnostics = null;


yes, work in progress

stephen-hawley · 2024-10-31T19:01:33Z

src/rgen/Microsoft.Macios.Generator/Extensions/NamedTypeSymbolExtensions.cs

+					fieldBucket.Add ((Symbol: fieldSymbol, FieldData: fieldData));
+				} else {
+					// TODO: diagnostics
+				}


stephen-hawley · 2024-10-31T19:02:29Z

src/rgen/Microsoft.Macios.Generator/Extensions/TypeSymbolExtensions.cs

+			if (string.IsNullOrEmpty (attrName))
+				continue;
+			if (!attributes.TryAdd (attrName, attributeData)) {
+				// TODO: diagnostics


I assume that the name and signature of this method will change to a TryGet pattern?

I need to fix this. attrs that we are interested do not allow to be referenced multiple types on a symbol. If that happens we introduced and unexpected bug.

rolfbjarne

This is probably for a different PR, but I think it would be a good idea to add unit tests for memory usage as well (using something like BenchmarkDotNet), because it's really easy to end up using a lot of string memory unnecessarily (as can be seen in bgen, which uses multiple GBs of memory).

src/ObjCBindings/FieldAttribute.cs

src/rgen/Microsoft.Macios.Generator/Attributes/FieldData.cs

src/rgen/Microsoft.Macios.Generator/BindingSourceGeneratorGenerator.cs

tests/rgen/Microsoft.Macios.Generator.Tests/SmartEnum/Data/ExpectedAVCaptureDeviceTypeEnum.cs

rolfbjarne · 2024-11-01T11:54:20Z

tests/rgen/Microsoft.Macios.Generator.Tests/SmartEnum/SmartEnumTests.cs

+		// Run generators and retrieve all results.
+		var runResult = RunGenerators (compilation);
+
+		// bad formed bindings should not generate code


Maybe assert that an error is reported too?

Errors will come from the Analyzer which will have tests for it. Coming in the next PR.

src/rgen/Microsoft.Macios.Generator/BindingSourceGeneratorGenerator.cs

rolfbjarne · 2024-11-01T12:03:54Z

src/rgen/Microsoft.Macios.Generator/Context/RootBindingContext.cs

+			if (libraryName [0] == '+') {
+				switch (libraryName) {
+				case "+CoreImage":
+					CurrentPlatform.TryGetCoreImageMap (out libraryName);
+					break;
+				case "+CoreServices":
+					CurrentPlatform.TryGetCoreServicesMap (out libraryName);
+					break;
+				case "+PDFKit":
+					libraryName = "PdfKit";
+					CurrentPlatform.TryGetPDFKitMap (out libraryPath);
+					break;
+				}
+			} else {


I don't see any tests for this logic.

src/rgen/Microsoft.Macios.Generator/Context/RootBindingContext.cs

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

github-actions · 2024-11-01T15:29:59Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

github-actions · 2024-11-01T21:16:30Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

vs-mobiletools-engineering-service2 · 2024-11-01T21:46:38Z

🔥 [CI Build] Build failed 🔥

Build failed for the job 'Build macOS tests'

Pipeline on Agent
Hash: 255afa1d114d6afa7dc3bb000202464c3d4f49c1 [PR build]

github-actions · 2024-11-01T21:54:32Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

…pos.

…amarin/xamarin-macios into dev/mandel/rgen-smart-enums-generation

github-actions · 2024-11-01T22:58:48Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

…ter.

github-actions · 2024-11-13T23:01:34Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

dalexsoto

Looking good, love the extensive testing!

rolfbjarne · 2024-11-18T10:16:21Z

src/rgen/Microsoft.Macios.Generator/Availability/SymbolAvailability.cs

+	/// <summary>
+	/// MacOS platform availability. If null, the default versions are used.
+	/// </summary>
+	public PlatformAvailability? MacOSX { get; }


Nitpick: the correct platform name is macOS (or MacOS) - it's not really MacOSX anymore, it would be MacOSXV this year.

rolfbjarne · 2024-11-18T10:24:38Z

src/rgen/Microsoft.Macios.Generator/Availability/SymbolAvailability.cs

+	SymbolAvailability (PlatformAvailability? iOsAvailability, PlatformAvailability? tvOsAvailability,
+		PlatformAvailability? macCatalystAvailability, PlatformAvailability? macOSXAvailability)
+	{
+		iOS = iOsAvailability;
+		TvOS = tvOsAvailability;
+		MacCatalyst = macCatalystAvailability;
+		MacOSX = macOSXAvailability;
+	}


If this class could be implemented in a way that doesn't hardcode the platforms, then that would be great!

Over the years I've added 3 platforms, and removed 1 (although that's not quite completed yet), so I've firmly believe the fewer places that need updating whenever the platforms we support change, the better.

100% agree, it is annoying to implement it in a way without hardcoding due to the fact that we are trying to use a read only structure. I'll try to implement a smarter way in a smaller PR.

rolfbjarne · 2024-11-18T10:25:09Z

src/rgen/Microsoft.Macios.Generator/Availability/SymbolAvailabilityBuilder.cs

+		readonly HashSet<ApplePlatform> supportedPlatforms =
+			[ApplePlatform.iOS, ApplePlatform.TVOS, ApplePlatform.MacOSX, ApplePlatform.MacCatalyst];


This can be static.

Suggested change

readonly HashSet<ApplePlatform> supportedPlatforms =

[ApplePlatform.iOS, ApplePlatform.TVOS, ApplePlatform.MacOSX, ApplePlatform.MacCatalyst];

static readonly HashSet<ApplePlatform> supportedPlatforms =

[ApplePlatform.iOS, ApplePlatform.TVOS, ApplePlatform.MacOSX, ApplePlatform.MacCatalyst];

rolfbjarne · 2024-11-18T10:25:34Z

src/rgen/Microsoft.Macios.Generator/Availability/SymbolAvailabilityBuilder.cs

+			PlatformAvailability? iOS =
+				platforms.ContainsKey (ApplePlatform.iOS) ? platforms [ApplePlatform.iOS].ToImmutable () : null;
+			PlatformAvailability? tvOS =
+				platforms.ContainsKey (ApplePlatform.TVOS) ? platforms [ApplePlatform.TVOS].ToImmutable () : null;
+			PlatformAvailability? macCatalyst =
+				platforms.ContainsKey (ApplePlatform.MacCatalyst)
+					? platforms [ApplePlatform.MacCatalyst].ToImmutable ()
+					: null;
+			PlatformAvailability? macOS =
+				platforms.ContainsKey (ApplePlatform.MacOSX) ? platforms [ApplePlatform.MacOSX].ToImmutable () : null;
+			return new (iOS, tvOS, macCatalyst, macOS);


Can this be implemented as a loop over supportedPlatforms to avoid hardcoding the platforms?

rolfbjarne · 2024-11-18T16:47:04Z

src/rgen/Microsoft.Macios.Generator/BindingSourceGeneratorGenerator.cs

+				if (emitter.TryEmit (out var diagnostics)) {
+					// only add file when we do generate code
+					var code = sb.ToString ();
+					context.AddSource ($"{emitter.SymbolNamespace}/{emitter.SymbolName}.g.cs",


Suggested change

context.AddSource ($"{emitter.SymbolNamespace}/{emitter.SymbolName}.g.cs",

context.AddSource ($"{Path.Combine (emitter.SymbolNamespace, emitter.SymbolName)}.g.cs",

rolfbjarne · 2024-11-18T16:47:14Z

src/rgen/Microsoft.Macios.Generator/BindingSourceGeneratorGenerator.cs

+		if (emitter.TryEmit (out var diagnostics)) {
+			// only add file when we do generate code
+			var code = sb.ToString ();
+			context.AddSource ($"{emitter.SymbolNamespace}/{emitter.SymbolName}.g.cs",


Path.Combine

…rted dict.

github-actions · 2024-11-19T02:55:42Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

vs-mobiletools-engineering-service2 · 2024-11-19T04:10:52Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: 29ae4cd40dc96cdfb457774d3aee2caa64b4be2b [PR build]

vs-mobiletools-engineering-service2 · 2024-11-19T04:12:10Z

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

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

Pipeline on Agent
Hash: 29ae4cd40dc96cdfb457774d3aee2caa64b4be2b [PR build]

vs-mobiletools-engineering-service2 · 2024-11-19T04:14:11Z

❗ API diff for current PR / commit (Breaking changes)

.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)

iOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
tvOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
MacCatalyst: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
macOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)

✅ 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: 29ae4cd40dc96cdfb457774d3aee2caa64b4be2b [PR build]

vs-mobiletools-engineering-service2 · 2024-11-19T04:15:08Z

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

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

Pipeline on Agent
Hash: 29ae4cd40dc96cdfb457774d3aee2caa64b4be2b [PR build]

vs-mobiletools-engineering-service2 · 2024-11-19T04:17:06Z

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

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

Pipeline on Agent
Hash: 29ae4cd40dc96cdfb457774d3aee2caa64b4be2b [PR build]

vs-mobiletools-engineering-service2 · 2024-11-19T05:54:08Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 101 tests passed 🎉

Tests counts

✅ 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 3 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 40 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 7 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 29ae4cd40dc96cdfb457774d3aee2caa64b4be2b [PR build]

mandel-macaque requested a review from rolfbjarne as a code owner October 31, 2024 17:41

mandel-macaque commented Oct 31, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

stephen-hawley requested changes Oct 31, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rolfbjarne requested changes Nov 1, 2024

View reviewed changes

mandel-macaque and others added 2 commits November 1, 2024 11:26

Apply suggestions from code review

b71341a

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

Auto-format source code

1ce09d0

This comment has been minimized.

Sign in to view

mandel-macaque and others added 2 commits November 1, 2024 17:12

Add code to ensure that fields and library names are valid identifiers.

52b2558

Auto-format source code

255afa1

Fix tests that broke due to code contributions in the review.

c325a14

mandel-macaque and others added 2 commits November 1, 2024 17:50

Use a static class for the diagnostics.

56a4e51

Auto-format source code

792e794

mandel-macaque and others added 4 commits November 1, 2024 18:30

Fix how we get attrs to allow them to appear more than once. Minor ty…

d3ba13f

…pos.

Clean using.

aa993b6

Merge branch 'dev/mandel/rgen-smart-enums-generation' of github.com:x…

92bae18

…amarin/xamarin-macios into dev/mandel/rgen-smart-enums-generation

Auto-format source code

1ed746b

This comment has been minimized.

Sign in to view

mandel-macaque and others added 2 commits November 13, 2024 17:57

Add more tests and some xml docs for new developers that migh join la…

8c09fac

…ter.

Auto-format source code

4f3781d

This comment has been minimized.

Sign in to view

dalexsoto approved these changes Nov 14, 2024

View reviewed changes

dalexsoto requested a review from rolfbjarne November 14, 2024 23:23

rolfbjarne approved these changes Nov 18, 2024

View reviewed changes

mandel-macaque and others added 2 commits November 18, 2024 16:27

Address the final reviews. Move away from hardcoded platforms to a so…

d1c22ab

…rted dict.

Auto-format source code

29ae4cd

mandel-macaque merged commit 6bfecec into main Nov 19, 2024
12 checks passed

mandel-macaque deleted the dev/mandel/rgen-smart-enums-generation branch November 19, 2024 11:55

		readonly HashSet<ApplePlatform> supportedPlatforms =
		[ApplePlatform.iOS, ApplePlatform.TVOS, ApplePlatform.MacOSX, ApplePlatform.MacCatalyst];

	context.AddSource ($"{emitter.SymbolNamespace}/{emitter.SymbolName}.g.cs",
	context.AddSource ($"{Path.Combine (emitter.SymbolNamespace, emitter.SymbolName)}.g.cs",

[Rgen] Implement the generation of the smart enums. #21562

[Rgen] Implement the generation of the smart enums. #21562

Conversation

mandel-macaque commented Oct 31, 2024

Choose a reason for hiding this comment

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.

stephen-hawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandel-macaque Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandel-macaque Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandel-macaque Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

rolfbjarne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 1, 2024

⚠️ Your code has been reformatted. ⚠️

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 1, 2024

⚠️ Your code has been reformatted. ⚠️

vs-mobiletools-engineering-service2 commented Nov 1, 2024

🔥 [CI Build] Build failed 🔥

github-actions bot commented Nov 1, 2024

⚠️ Your code has been reformatted. ⚠️

github-actions bot commented Nov 1, 2024

⚠️ Your code has been reformatted. ⚠️

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.

github-actions bot commented Nov 13, 2024

⚠️ Your code has been reformatted. ⚠️

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.

dalexsoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024

⚠️ Your code has been reformatted. ⚠️

vs-mobiletools-engineering-service2 commented Nov 19, 2024

💻 [CI Build] Windows Integration Tests passed 💻

vs-mobiletools-engineering-service2 commented Nov 19, 2024

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

mandel-macaque Nov 1, 2024 •

edited

Loading

mandel-macaque Nov 1, 2024 •

edited

Loading

mandel-macaque Nov 1, 2024 •

edited

Loading