[xcode14.1] [ObjCRuntime] Change Runtime.GetNSObject<T> to create a n…

…ew instance if the existing one is of the wrong type. Fixes #13704. (#16502)

Objective-C has an optimization where creating an empty dictionary would
return the same instance every time (probably to a constant empty
dictionary).

This causes a problem with how we've bound generic dictionaries, because
all
empty dictionaries would have the same native handle, even if we'd bound
them
with different managed types.

This surfaces in unfortunate ways when we try to look up a managed
instance
given a native handle, we find that we already have a managed instance,
but
the managed instance is of the wrong type.

Example code:

    var a = new NSDictionary ();
    var b = new NSDictionary<NSString, NSString> ();
    var c = new NSDictionary<NSString, NSObject> ();
    Console.WriteLine ($"a: 0x{a.Handle:X}");
    Console.WriteLine ($"b: 0x{b.Handle:X}");
    Console.WriteLine ($"c: 0x{c.Handle:X}");

would produce something like:

    a: 0x0x7fff80821080
    b: 0x0x7fff80821080
    c: 0x0x7fff80821080

now for this code:

    Runtime.GetNSObject<NSDictionary<NSString, NSString>> (b.Handle)

it would throw an exception like this:

Unable to cast object of type
'Foundation.NSDictionary`2[[Foundation.NSString],[Foundation.NSObject]]'
to type
'Foundation.NSDictionary`2[[Foundation.NSString],[Foundation.NSString]]'

because we'll have the 'c' object (with type `NSDictionary<NSString,
Object>`)
in our dictionary of native handles -> managed instances, and that's not
compatible with the desired return type from GetNSObject
(`NSDictionary<NSString, NSString>`)

This likely happens with all the non-mutable collection types we have a
generic version of (NSArray, NSDictionary, NSOrderedSet, NSSet).

Fixes #16378.
Fixes #13704.

This PR is somewhat simpler to review by ignoring whitespace:
https://github.com/xamarin/xamarin-macios/pull/16491/files?w=1


Backport of #16491

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>

src/ObjCRuntime/Runtime.cs

@@ -1437,38 +1437,36 @@ static IntPtr CreateNSObject (IntPtr type_gchandle, IntPtr handle, NSObject.Flag
 				return null;
 
 			var obj = TryGetNSObject (ptr, evenInFinalizerQueue: false);
-			T? o;
-
-			if (obj is null) {
-				// Try to get the managed type that correspond to this exact native type
-				IntPtr p = Class.GetClassForObject (ptr);
-				// If unknown then we'll get the Class that Lookup to NSObject even if this is not NSObject.
-				// We can use this condition to fallback on the provided (generic argument) type
-				Type target_type;
-				if (p != NSObjectClass) {
-					target_type = Class.Lookup (p);
-					if (target_type == typeof(NSObject))
-						target_type = typeof(T);
-					else if (typeof (T).IsGenericType)
-						target_type = typeof(T);
-					else if (target_type.IsSubclassOf (typeof(T))) {
-						// do nothing, this is fine.
-					} else if (Messaging.bool_objc_msgSend_IntPtr (ptr, Selector.GetHandle ("isKindOfClass:"), Class.GetHandle (typeof (T)))) {
-						// If the instance itself claims it's an instance of the provided (generic argument) type,
-						// then we believe the instance. See bug #20692 for a test case.
-						target_type = typeof(T);
-					}
-				} else {
-					target_type = typeof(NSObject);
+
+			// First check if we got an object of the expected type
+			if (obj is T o)
+				return o;
+
+			// We either didn't find an object, or it was of the wrong type, so we need to create a new instance.
+
+			// Try to get the managed type that correspond to this exact native type
+			IntPtr p = Class.GetClassForObject (ptr);
+			// If unknown then we'll get the Class that Lookup to NSObject even if this is not NSObject.
+			// We can use this condition to fallback on the provided (generic argument) type
+			Type target_type;
+			if (p != NSObjectClass) {
+				target_type = Class.Lookup (p);
+				if (target_type == typeof(NSObject))
+					target_type = typeof(T);
+				else if (typeof (T).IsGenericType)
+					target_type = typeof(T);
+				else if (target_type.IsSubclassOf (typeof(T))) {
+					// do nothing, this is fine.
+				} else if (Messaging.bool_objc_msgSend_IntPtr (ptr, Selector.GetHandle ("isKindOfClass:"), Class.GetHandle (typeof (T)))) {
+					// If the instance itself claims it's an instance of the provided (generic argument) type,
+					// then we believe the instance. See bug #20692 for a test case.
+					target_type = typeof(T);
 				}
-				o = ConstructNSObject<T> (ptr, target_type, MissingCtorResolution.ThrowConstructor1NotFound);
 			} else {
-				o = obj as T;
-				if (o is null)
-					throw new InvalidCastException ($"Unable to cast object of type '{obj.GetType ().FullName}' to type '{typeof(T).FullName}'.");
+				target_type = typeof(NSObject);
 			}
 
-			return o;
+			return ConstructNSObject<T> (ptr, target_type, MissingCtorResolution.ThrowConstructor1NotFound);
 		}
 
 		static public T? GetNSObject<T> (IntPtr ptr, bool owns) where T : NSObject

tests/monotouch-test/ObjCRuntime/RegistrarTest.cs

@@ -2172,8 +2172,9 @@ public void TestCtors ()
 				// sure this is by design or by accident, but that's how we're behaving right now at least
 				ptr = Messaging.IntPtr_objc_msgSend (Class.GetHandle (typeof (D2)), Selector.GetHandle ("alloc"));
 				ptr = Messaging.IntPtr_objc_msgSend_long (ptr, Selector.GetHandle ("initWithBar:"), 2);
-				// Unable to cast object of type 'AppDelegate+D1' to type 'AppDelegate+D2'
-				Assert.Throws<InvalidCastException> (() => Runtime.GetNSObject<D2> (ptr), "b ex");
+				// Failed to marshal the Objective-C object 0x60000230a410 (type: MonoTouchFixtures_ObjCRuntime_RegistrarTest_D2). Could not find an existing managed instance for this object, nor was it possible to create a new managed instance (because the type 'MonoTouchFixtures.ObjCRuntime.RegistrarTest+D2' does not have a constructor that takes one NativeHandle argument
+				var ex = Assert.Throws<RuntimeException> (() => Runtime.GetNSObject<D2> (ptr), "b ex");
+				Assert.That (ex.Message, Does.Contain ("Could not find an existing managed instance for this object, nor was it possible to create a new managed instance (because the type 'MonoTouchFixtures.ObjCRuntime.RegistrarTest+D2' does not have a constructor that takes one"), "Exception message");
 				var obj = Runtime.GetNSObject<D1> (ptr);
 				Assert.AreEqual ("bar", obj.ctor1, "b ctor1");
 			} finally {

tests/monotouch-test/ObjCRuntime/RuntimeTest.cs

@@ -141,6 +141,23 @@ public void GetNSObject_Posing_Class ()
 			}
 		}
 
+		[Test]
+		public void GetNSObject_T_SameHandleDifferentInstances ()
+		{
+			using var a = new NSDictionary<NSString, NSObject> ();
+			using var b = new NSDictionary<NSString, NSString> ();
+			using var c = new NSDictionary ();
+
+			if (a.Handle != b.Handle || a.Handle != c.Handle)
+				Assert.Ignore ("The dictionaries are actually different");
+
+			var handle = a.Handle;
+
+			Assert.DoesNotThrow (() => Runtime.GetNSObject<NSDictionary> (handle), "A");
+			Assert.DoesNotThrow (() => Runtime.GetNSObject<NSDictionary<NSString, NSObject>> (handle), "B");
+			Assert.DoesNotThrow (() => Runtime.GetNSObject<NSDictionary<NSString, NSString>> (handle), "C");
+		}
+
 		[Test]
 		public void UsableUntilDead ()
 		{