[dotnet] preliminary cut for class-redirector #17951

stephen-hawley · 2023-03-30T20:17:34Z

This is the preliminary version of class-redirector addressing issue #16671

What it does:

reads in an XML configuration file that will have been generated by the static registrar.
writes static fields into a class named after each ObjC class backed by the equivalent C# class
writes an initialization method body that initialized each field based on the static registrar.
in the process it also makes a map from the C# class to a FieldDefinition for the class_ptr
For each class:
removes the class_ptr field
removes the initialization code for that field from that .cctor
removes that .cctor if it can
replaces all read references to the class_ptr to a read reference to the appropriate FieldDefinition

Tests:
right now there are tests that test the XML reading and writing
There will be future tests that test the class rewriting writ small before we release it on our code base

As it is, this will not affect our code if its merged in

To do:
need to modify the static registrar to generate the XML file
need to invoke this after the static registrar has run
make it localizable

What you should consider in this PR:

any boneheaded mistakes I made in style/spelling
overall structure
argument processing
error checking

github-actions · 2023-03-30T20:22:31Z

⚠️ 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).

mandel-macaque

Most of them are nits except for the choice of net6 over net7, why is that?

mandel-macaque · 2023-03-30T21:33:54Z

tools/class-redirector/class-redirector-tests/XmlTests.cs

+		var map = new CSToObjCMap ();
+		map.Add ("Foo", new ObjCNameIndex ("Bar", 2));
+		map.Add ("Baz", new ObjCNameIndex ("Zed", 3));


Because you are inheriting from the dictionary class, you can use a dict initializer:

Suggested change

var map = new CSToObjCMap ();

map.Add ("Foo", new ObjCNameIndex ("Bar", 2));

map.Add ("Baz", new ObjCNameIndex ("Zed", 3));

var map = new CSToObjCMap () {

["Foo"] = new ObjCNameIndex ("Bar", 2),

["Baz"] = new ObjCNameIndex ("Zed", 3),

};

mandel-macaque · 2023-03-30T21:35:03Z

tools/class-redirector/class-redirector-tests/XmlTests.cs

+  </Element>
+</CSToObjCMap>";
+
+		var reader = new StringReader (text);


Suggested change

var reader = new StringReader (text);

using var reader = new StringReader (text);

mandel-macaque · 2023-03-30T21:36:22Z

tools/class-redirector/class-redirector-tests/XmlTests.cs

+		if (map.TryGetValue ("Foo", out var nameIndex)) {
+			Assert.That (nameIndex.ObjCName, Is.EqualTo ("Bar"), "no bar name");
+			Assert.That (nameIndex.MapIndex, Is.EqualTo (2), "no bar index");
+		} else {
+			Assert.Fail ("no Foo value");
+		}


Maybe we can make things a little cleaner with the following:

Suggested change

if (map.TryGetValue ("Foo", out var nameIndex)) {

Assert.That (nameIndex.ObjCName, Is.EqualTo ("Bar"), "no bar name");

Assert.That (nameIndex.MapIndex, Is.EqualTo (2), "no bar index");

} else {

Assert.Fail ("no Foo value");

}

Assert.True(map.TryGetValue ("Foo", out var nameIndex));

Assert.That (nameIndex.ObjCName, Is.EqualTo ("Bar"), "no bar name");

Assert.That (nameIndex.MapIndex, Is.EqualTo (2), "no bar index");

mandel-macaque · 2023-03-30T21:37:12Z

tools/class-redirector/class-redirector-tests/XmlTests.cs

+		if (map.TryGetValue ("Baz", out var nameIndex1)) {
+			Assert.That (nameIndex1.ObjCName, Is.EqualTo ("Zed"), "no bar name");
+			Assert.That (nameIndex1.MapIndex, Is.EqualTo (3), "no bar index");
+		} else {
+			Assert.Fail ("no Foo value");
+		}


Same comment:

Suggested change

if (map.TryGetValue ("Baz", out var nameIndex1)) {

Assert.That (nameIndex1.ObjCName, Is.EqualTo ("Zed"), "no bar name");

Assert.That (nameIndex1.MapIndex, Is.EqualTo (3), "no bar index");

} else {

Assert.Fail ("no Foo value");

}

Assert.True(map.TryGetValue ("Baz", out var nameIndex1));

Assert.That (nameIndex1.ObjCName, Is.EqualTo ("Zed"), "no bar name");

Assert.That (nameIndex1.MapIndex, Is.EqualTo (3), "no bar index");

mandel-macaque · 2023-03-30T21:38:52Z

tools/class-redirector/class-redirector/Program.cs

+		static bool DirectoryIsWritable (string path)
+		{
+			var info = new DirectoryInfo (path);
+			return (info.Attributes & FileAttributes.ReadOnly) == 0;


Might be simpler to use the Enum.HasFlag method: https://learn.microsoft.com/en-us/dotnet/api/system.enum.hasflag?view=net-7.0

mandel-macaque · 2023-03-30T21:47:33Z

tools/class-redirector/class-redirector/Rewriter.cs

+				if (classMap.TryGetValue (cl.FullName, out var classPtrField)) {
+					PatchClassPtrUsage (cl, classPtrField!);
+				}


If we use net7 TryGetValue should have the attribute NotNullWhen in the newer versions and the '!' should not be needed. Mentioned before, but why net6?

mandel-macaque · 2023-03-30T21:51:45Z

tools/class-redirector/class-redirector/Rewriter.cs

+			var i = 0;
+			Instruction? stsfld = null;
+			while (i < il.Body.Instructions.Count) {
+				var instr = il.Body.Instructions [i];
+				// look for
+				// stsfld class_ptr
+				if (instr.OpCode == OpCodes.Stsfld && instr.Operand == class_ptr) {
+					stsfld = instr;
+					break;
+				}
+				i++;
+			}


We could use a for loop, why the use of a while loop? I am a person that makes mistakes, the for loop should help not to forget the index update:

Suggested change

var i = 0;

Instruction? stsfld = null;

while (i < il.Body.Instructions.Count) {

var instr = il.Body.Instructions [i];

// look for

// stsfld class_ptr

if (instr.OpCode == OpCodes.Stsfld && instr.Operand == class_ptr) {

stsfld = instr;

break;

}

i++;

}

int i;

Instruction? stsfld = null;

for (i=0; i < il.Body.Instructions.Count; i++) {

var instr = il.Body.Instructions [i];

// look for

// stsfld class_ptr

if (instr.OpCode == OpCodes.Stsfld && instr.Operand == class_ptr) {

stsfld = instr;

break;

}

}

You can still access the 'i' variable as you do later in the code. We ca even be little more evil (I do not like this option):

Suggested change

var i = 0;

Instruction? stsfld = null;

while (i < il.Body.Instructions.Count) {

var instr = il.Body.Instructions [i];

// look for

// stsfld class_ptr

if (instr.OpCode == OpCodes.Stsfld && instr.Operand == class_ptr) {

stsfld = instr;

break;

}

i++;

}

int i = 0;

Instruction? stsfld = null;

for (; i < il.Body.Instructions.Count; i++) {

var instr = il.Body.Instructions [i];

// look for

// stsfld class_ptr

if (instr.OpCode == OpCodes.Stsfld && instr.Operand == class_ptr) {

stsfld = instr;

break;

}

}

mandel-macaque · 2023-03-30T21:53:34Z

tools/class-redirector/class-redirector/Rewriter.cs

+			var instr = il.Body.Instructions [index]!;
+			var operand = instr.Operand?.ToString () ?? "";


No need for the '!' here, just fwd the '?' correct?

Suggested change

var instr = il.Body.Instructions [index]!;

var operand = instr.Operand?.ToString () ?? "";

var instr = il.Body.Instructions [index];

var operand = instr?.Operand?.ToString () ?? "";

I disagree here. When I use the ! I'm trying to say clearly "this cannot be and should never be null and I'm so confident that I will bet the stability of my code on that assertion" whereas the ?. says "well, if it's not there, empty string is OK, which it, in fact, is not. Every array element should have an instruction. Some instructions may or may not have operands.

mandel-macaque · 2023-03-30T21:55:26Z

tools/class-redirector/class-redirector/Rewriter.cs

+			var instr = il.Body.Instructions [index]!;
+			return instr.OpCode == OpCodes.Ldstr;


Same comment, no need to do the ! which is usually code smell regarding to nullability. We can do:

Suggested change

var instr = il.Body.Instructions [index]!;

return instr.OpCode == OpCodes.Ldstr;

var instr = il.Body.Instructions [index];

return instr?.OpCode == OpCodes.Ldstr;

Afaik OpCode is a ref class, so you are no dealing with a Nullable situation.

Unfortunately, Cecil is written with prior to nullable and Instruction is possible to be null (but should never be null in this situation, but the analyzer doesn't know that). In this case, the bad smell is in the analyzer in working with legacy code.

Oh yes, but you can move the ! to be a ? and the analyser won't complain and the output will be valid.

mandel-macaque · 2023-03-30T21:56:41Z

tools/common/CSToObjCMap.cs

+			foreach (var elem in elements) {
+				if (elem.Key is not null && elem.Value is not null)
+					map.Add (elem.Key, elem.Value);
+			}


Can't we add this filter for the key/value in the LINQ expression with a where clause?

I started out writing a LINQ expression and it was ugly and not as readable as I liked. This was much more readable. I could have done elements.Where (el => el.Key is not null && elem.Value is not null) but Dictionary doesn't have AddRange so I still need a for loop, or I'd have to do:

elements.Where (el => el.Key is not null && el.Value is not null).ForEach (el => map.Add(el.Key, el.Value));

And now I've reduced it to one line and just made it harder to read (IMHO) and harder to debug.

…xamarin-macios into class-redirector

vs-mobiletools-engineering-service2 · 2023-03-31T15:36:17Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

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

.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)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: cc9a08274209aaaa1973da69f02a08fc4774cbbc [PR build]

vs-mobiletools-engineering-service2 · 2023-03-31T15:37:04Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: cc9a08274209aaaa1973da69f02a08fc4774cbbc [PR build]

vs-mobiletools-engineering-service2 · 2023-03-31T15:45:45Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent xambot-1199.Ventura
Hash: cc9a08274209aaaa1973da69f02a08fc4774cbbc [PR build]

vs-mobiletools-engineering-service2 · 2023-03-31T15:47:15Z

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

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

Pipeline on Agent
Hash: cc9a08274209aaaa1973da69f02a08fc4774cbbc [PR build]

vs-mobiletools-engineering-service2 · 2023-03-31T15:47:59Z

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: cc9a08274209aaaa1973da69f02a08fc4774cbbc [PR build]

vs-mobiletools-engineering-service2 · 2023-03-31T19:02:19Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 225 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 25 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: cc9a08274209aaaa1973da69f02a08fc4774cbbc [PR build]

rolfbjarne · 2023-04-10T12:12:04Z

tools/class-redirector/class-redirector/Program.cs

+		static string [] xamarinDlls = new string [] {
+			"Microsoft.iOS.dll",
+			"Microsoft.macOS.dll",
+			"Microsoft.tvOS.dll",


Microsoft.MacCatalyst.dll?

rolfbjarne · 2023-04-10T14:37:54Z

tools/class-redirector/class-redirector/Rewriter.cs

+			if (index < 0)
+				return false;
+			var instr = il.Body.Instructions [index]!;
+			var operand = instr.Operand?.ToString () ?? "";


You're calling ToString on every instruction, which is unnecessary. You only need to call ToString on call instructions, which would be much more performant. This would mean moving the instr.OpCode == OpCodes.Call check to before the ToString call.

preliminary cut for class-redirector

4f12e19

stephen-hawley added the not-notes-worthy Ignore for release notes label Mar 30, 2023

stephen-hawley requested a review from rolfbjarne as a code owner March 30, 2023 20:17

Auto-format source code

3d1be29

mandel-macaque requested changes Mar 30, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

stephen-hawley added 2 commits March 31, 2023 09:55

style, update to net 7, typos, bugs

8ef86a6

Merge branch 'class-redirector' of https://github.com/stephen-hawley/…

cc9a082

…xamarin-macios into class-redirector

mandel-macaque approved these changes Apr 3, 2023

View reviewed changes

stephen-hawley merged commit bef5d47 into xamarin:main Apr 3, 2023

stephen-hawley deleted the class-redirector branch April 3, 2023 16:37

rolfbjarne reviewed Apr 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dotnet] preliminary cut for class-redirector #17951

[dotnet] preliminary cut for class-redirector #17951

stephen-hawley commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023

mandel-macaque left a comment

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

mandel-macaque Mar 30, 2023

stephen-hawley Mar 31, 2023

mandel-macaque Mar 30, 2023

stephen-hawley Mar 31, 2023

mandel-macaque Mar 31, 2023

mandel-macaque Mar 30, 2023

stephen-hawley Mar 31, 2023

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 Mar 31, 2023

vs-mobiletools-engineering-service2 commented Mar 31, 2023

vs-mobiletools-engineering-service2 commented Mar 31, 2023

vs-mobiletools-engineering-service2 commented Mar 31, 2023

vs-mobiletools-engineering-service2 commented Mar 31, 2023

vs-mobiletools-engineering-service2 commented Mar 31, 2023

rolfbjarne Apr 10, 2023

rolfbjarne Apr 10, 2023

	var reader = new StringReader (text);
	using var reader = new StringReader (text);

		var instr = il.Body.Instructions [index]!;
		var operand = instr.Operand?.ToString () ?? "";

		var instr = il.Body.Instructions [index]!;
		return instr.OpCode == OpCodes.Ldstr;

[dotnet] preliminary cut for class-redirector #17951

[dotnet] preliminary cut for class-redirector #17951

Conversation

stephen-hawley commented Mar 30, 2023 • edited Loading

github-actions bot commented Mar 30, 2023

⚠️ Your code has been reformatted. ⚠️

mandel-macaque 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

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

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 Mar 31, 2023

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

vs-mobiletools-engineering-service2 commented Mar 31, 2023

💻 [CI Build] Windows Integration Tests passed 💻

vs-mobiletools-engineering-service2 commented Mar 31, 2023

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Mar 31, 2023

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

vs-mobiletools-engineering-service2 commented Mar 31, 2023

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

vs-mobiletools-engineering-service2 commented Mar 31, 2023

🚀 [CI Build] Test results 🚀

Test results

Tests counts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-hawley commented Mar 30, 2023 •

edited

Loading