[generator] Properly set the IsDirectBinding value. (#3063)

* [generator] Properly set the IsDirectBinding value.

Properly set the IsDirectBinding value to false for models and synthetic types.

This also means we can now stop excluding models when testing if the
IsDirectBinding value is correct.

Also set IsDirectBinding value to true for sealed wrapper types, since those
will always be direct bindings since they can't be subclassed.

https://gist.github.com/rolfbjarne/24028bf944db848fed4083c460d0ec71

* [tests] Add introspection exclusion for XM.

* [introspection] Add back exclusions for Classic, since we can't modify/fix Classic assemblies anymore.

* [generator] Print the correct protocol name with the protocol attribute.

Fixes this test failure:

    [FAIL] Foundation.NSUrlDownloadDelegate : ConformsToProtocol(null) failed

because our binding code claimed that our `NSUrlDownloadDelegate` class
implemented the `NSUrlDownloadDelegate` protocol, but since the
`NSUrlDownloadDelegate` protocol doesn't exist (it's `NSURLDownloadDelegate` -
different case), we'd verify against a null protocol (and return true from
`ConformsToProtocol(null)`, which would fail the test).

* [xtro] Only treat interfaces as protocols.

Unfortunately we add [Protocol] to [Model]s as well as on interfaces, but we
must not process those in xtro, since they don't correspond with the actual protocol.

src/generator.cs

@@ -53,6 +53,16 @@ using System.ComponentModel;
 using XamCore.ObjCRuntime;
 using XamCore.Foundation;
 
+public static class GeneratorExtensions
+{
+	public static StreamWriter Write (this StreamWriter sw, char c, int count)
+	{
+		for (int i = 0; i < count; i++)
+			sw.Write (c);
+		return sw;
+	}
+}
+
 public static class ReflectionExtensions {
 	public static BaseTypeAttribute GetBaseTypeAttribute (Type type)
 	{
@@ -910,7 +920,8 @@ public partial class Generator : IMemberGatherer {
 	//
 	// Properties and definitions to support binding third-party Objective-C libraries
 	//
-	string init_binding_type;
+	string is_direct_binding_value; // An expression that calculates the IsDirectBinding value. Might not be a constant expression. This will be added to every constructor for a type.
+	bool? is_direct_binding; // If a constant value for IsDirectBinding is known, it's stored here. Will be null if no constant value is known.
 
 	// Whether to use ZeroCopy for strings, defaults to false
 	public bool ZeroCopyStrings;
@@ -2199,8 +2210,6 @@ public partial class Generator : IMemberGatherer {
 			marshal_types.Add (TypeManager.AURenderEventEnumerator);
 		}
 
-		init_binding_type = String.Format ("IsDirectBinding = GetType ().Assembly == global::{0}.this_assembly;", ns.Messaging);
-
 		m = GetOutputStream ("ObjCRuntime", "Messaging");
 		Header (m);
 		print (m, "namespace {0} {{", ns.ObjCRuntime);
@@ -3030,6 +3039,40 @@ public partial class Generator : IMemberGatherer {
 			sw.Write ('\t');
 		sw.WriteLine ("[CompilerGenerated]");
 	}
+
+	static void WriteIsDirectBindingCondition (StreamWriter sw, ref int tabs, bool? is_direct_binding, string is_direct_binding_value, Func<string> trueCode, Func<string> falseCode)
+	{
+		if (is_direct_binding_value != null)
+			sw.Write ('\t', tabs).WriteLine ("IsDirectBinding = {0};", is_direct_binding_value);
+		
+		// If we don't know the IsDirectBinding value, we need the condition
+		if (!is_direct_binding.HasValue) {
+			sw.Write ('\t', tabs).WriteLine ("if (IsDirectBinding) {");
+			tabs++;
+		}
+
+		// We need the true part if we don't know, or if we know it's true
+		if (is_direct_binding != false) {
+			var code = trueCode ();
+			if (!string.IsNullOrEmpty (code))
+				sw.Write ('\t', tabs).WriteLine (code);
+		}
+		
+		if (!is_direct_binding.HasValue)
+			sw.Write ('\t', tabs - 1).WriteLine ("} else {");
+
+		// We need the false part if we don't know, or if we know it's false
+		if (is_direct_binding != true) {
+			var code = falseCode ();
+			if (!string.IsNullOrEmpty (code))
+				sw.Write ('\t', tabs).WriteLine (code);
+		}
+		
+		if (!is_direct_binding.HasValue) {
+			tabs--;
+			sw.Write ('\t', tabs).WriteLine ("}");
+		}
+	}
 	
 	public void print_generated_code ()
 	{
@@ -4196,26 +4239,18 @@ public partial class Generator : IMemberGatherer {
 					GenerateInvoke (false, mi, minfo, sel, argsArray, needs_temp, category_type);
 				}
 			} else {
-				if (BindThirdPartyLibrary && mi.Name == "Constructor"){
-					print (init_binding_type);
-				}
-				
 				var may_throw = ShouldMarshalNativeExceptions (mi);
 				var null_handle = may_throw && mi.Name == "Constructor";
 				if (null_handle) {
 					print ("try {");
 					indent++;
 				}
-				
+
-				print ("if (IsDirectBinding) {{", type.Name);
+				WriteIsDirectBindingCondition (sw, ref indent, is_direct_binding,
-				indent++;
+							       mi.Name == "Constructor" ? is_direct_binding_value : null, // We only need to print the is_direct_binding value in constructors
-				GenerateInvoke (false, mi, minfo, sel, argsArray, needs_temp, category_type);
+				                               () => { GenerateInvoke (false, mi, minfo, sel, argsArray, needs_temp, category_type); return null; },
-				indent--;
+				                               () => { GenerateInvoke (true, mi, minfo, sel, argsArray, needs_temp, category_type); return null; }
-				print ("} else {");
+							      );
-				indent++;
-				GenerateInvoke (true, mi, minfo, sel, argsArray, needs_temp, category_type);
-				indent--;
-				print ("}");
 				
 				if (null_handle) {
 					indent--;
@@ -5731,7 +5766,8 @@ public partial class Generator : IMemberGatherer {
 			bool is_static_class = AttributeManager.HasAttribute<StaticAttribute> (type) || is_category_class;
 			bool is_partial = AttributeManager.HasAttribute<PartialAttribute> (type);
 			bool is_model = AttributeManager.HasAttribute<ModelAttribute> (type);
-			bool is_protocol = AttributeManager.HasAttribute<ProtocolAttribute> (type);
+			var protocol = AttributeManager.GetCustomAttribute<ProtocolAttribute> (type);
+			bool is_protocol = protocol != null;
 			bool is_abstract = AttributeManager.HasAttribute<AbstractAttribute> (type);
 			bool is_sealed = AttributeManager.HasAttribute<SealedAttribute> (type);
 			string class_visibility = type.IsInternal () ? "internal" : "public";
@@ -5748,7 +5784,6 @@ public partial class Generator : IMemberGatherer {
 				if (is_model && base_type == TypeManager.System_Object)
 					ErrorHelper.Show (new BindingException (1060, "The {0} protocol is decorated with [Model], but not [BaseType]. Please verify that [Model] is relevant for this protocol; if so, add [BaseType] as well, otherwise remove [Model].", type.FullName));
 
-				var protocol = AttributeManager.GetCustomAttribute<ProtocolAttribute> (type);
 				GenerateProtocolTypes (type, class_visibility, TypeName, protocol.Name ?? objc_type_name, protocol);
 			}
 
@@ -5762,22 +5797,37 @@ public partial class Generator : IMemberGatherer {
 
 			bool core_image_filter = false;
 			string class_mod = null;
+
+			is_direct_binding_value = null;
+			is_direct_binding = null;
+			if (BindThirdPartyLibrary)
+				is_direct_binding_value = string.Format ("GetType ().Assembly == global::{0}.this_assembly", ns.Messaging);
 			if (is_static_class || is_category_class || is_partial) {
 				base_type = TypeManager.System_Object;
 				if (!is_partial)
 					class_mod = "static ";
 			} else {
 				if (is_protocol)
-					print ("[Protocol]");
+					print ("[Protocol({0})]", !string.IsNullOrEmpty (protocol.Name) ? $"Name = \"{protocol.Name}\"" : string.Empty);
 				core_image_filter = AttributeManager.HasAttribute<CoreImageFilterAttribute> (type);
-				if (!type.IsEnum && !core_image_filter)
+				if (!type.IsEnum && !core_image_filter) {
-					print ("[Register(\"{0}\", {1})]", objc_type_name, AttributeManager.HasAttribute<SyntheticAttribute> (type) || is_model ? "false" : "true");
+					if (is_model || AttributeManager.HasAttribute<SyntheticAttribute> (type)) {
+						is_direct_binding = false;
+						is_direct_binding_value = "false";
+					}
+					print ("[Register(\"{0}\", {1})]", objc_type_name, is_direct_binding == false ? "false" : "true");
+				}
 				if (is_abstract || need_abstract.ContainsKey (type))
 					class_mod = "abstract ";
 				else if (is_sealed)
 					class_mod = "sealed ";
 			} 
 			
+			if (is_sealed && is_direct_binding == null) {
+				is_direct_binding = true;
+				is_direct_binding_value = "true";
+			}
+				
 			if (is_model){
 				if (is_category_class)
 					ErrorHelper.Show (new BindingException (1022, true, "Category classes can not use the [Model] attribute"));
@@ -5942,6 +5992,8 @@ public partial class Generator : IMemberGatherer {
 							sw.WriteLine ("\t\t[Export (\"init\")]");
 							sw.WriteLine ("\t\t{0} {1} () : base (NSObjectFlag.Empty)", v, TypeName);
 							sw.WriteLine ("\t\t{");
+							if (is_direct_binding_value != null)
+								sw.WriteLine ("\t\t\tIsDirectBinding = {0};", is_direct_binding_value);
 							if (debug)
 								sw.WriteLine ("\t\t\tConsole.WriteLine (\"{0}.ctor ()\");", TypeName);
 							sw.WriteLine ("\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSend (this.Handle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime);
@@ -5955,15 +6007,11 @@ public partial class Generator : IMemberGatherer {
 							sw.WriteLine ("\t\t[Export (\"init\")]");
 							sw.WriteLine ("\t\t{0} {1} () : base (NSObjectFlag.Empty)", v, TypeName);
 							sw.WriteLine ("\t\t{");
-							if (BindThirdPartyLibrary)
+							var indentation = 3;
-								sw.WriteLine ("\t\t\t{0}", init_binding_type);
+							WriteIsDirectBindingCondition (sw, ref indentation, is_direct_binding, is_direct_binding_value,
-							if (debug)
+							                               () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSend (this.Handle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime),
-								sw.WriteLine ("\t\t\tConsole.WriteLine (\"{0}.ctor ()\");", TypeName);
+							                               () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSendSuper (this.SuperHandle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime));
-							sw.WriteLine ("\t\t\tif (IsDirectBinding) {");
+
-							sw.WriteLine ("\t\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSend (this.Handle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime);
-							sw.WriteLine ("\t\t\t} else {");
-							sw.WriteLine ("\t\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSendSuper (this.SuperHandle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime);
-							sw.WriteLine ("\t\t\t}");
 							WriteMarkDirtyIfDerived (sw, type);
 							sw.WriteLine ("\t\t}");
 							sw.WriteLine ();
@@ -5982,16 +6030,13 @@ public partial class Generator : IMemberGatherer {
 							sw.WriteLine ("\t\t{0} {1} (NSCoder coder) : base (NSObjectFlag.Empty)", UnifiedAPI ? v : "public", TypeName);
 							sw.WriteLine ("\t\t{");
 							if (nscoding) {
-								if (BindThirdPartyLibrary)
-									sw.WriteLine ("\t\t\t{0}", init_binding_type);
 								if (debug)
 									sw.WriteLine ("\t\t\tConsole.WriteLine (\"{0}.ctor (NSCoder)\");", TypeName);
 								sw.WriteLine ();
-								sw.WriteLine ("\t\t\tif (IsDirectBinding) {");
+								var indentation = 3;
-								sw.WriteLine ("\t\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSend_IntPtr (this.Handle, {0}, coder.Handle), \"initWithCoder:\");", initWithCoderSelector, ns.Messaging);
+								WriteIsDirectBindingCondition (sw, ref indentation, is_direct_binding, is_direct_binding_value,
-								sw.WriteLine ("\t\t\t} else {");
+								                               () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSend_IntPtr (this.Handle, {0}, coder.Handle), \"initWithCoder:\");", initWithCoderSelector, ns.Messaging),
-								sw.WriteLine ("\t\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSendSuper_IntPtr (this.SuperHandle, {0}, coder.Handle), \"initWithCoder:\");", initWithCoderSelector, ns.Messaging);
+								                               () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSendSuper_IntPtr (this.SuperHandle, {0}, coder.Handle), \"initWithCoder:\");", initWithCoderSelector, ns.Messaging));
-								sw.WriteLine ("\t\t\t}");
 								WriteMarkDirtyIfDerived (sw, type);
 							} else {
 								sw.WriteLine ("\t\t\tthrow new InvalidOperationException (\"Type does not conform to NSCoding\");");
@@ -6005,8 +6050,8 @@ public partial class Generator : IMemberGatherer {
 						sw.WriteLine ("\t\t[EditorBrowsable (EditorBrowsableState.Advanced)]");
 						sw.WriteLine ("\t\t{0} {1} (NSObjectFlag t) : base (t)", UnifiedAPI ? "protected" : "public", TypeName);
 						sw.WriteLine ("\t\t{");
-						if (BindThirdPartyLibrary)
+						if (is_direct_binding_value != null)
-							sw.WriteLine ("\t\t\t{0}", init_binding_type);
+							sw.WriteLine ("\t\t\tIsDirectBinding = {0};", is_direct_binding_value);
 						WriteMarkDirtyIfDerived (sw, type);
 						sw.WriteLine ("\t\t}");
 						sw.WriteLine ();
@@ -6015,8 +6060,8 @@ public partial class Generator : IMemberGatherer {
 					sw.WriteLine ("\t\t[EditorBrowsable (EditorBrowsableState.Advanced)]");
 					sw.WriteLine ("\t\t{0} {1} (IntPtr handle) : base (handle)", UnifiedAPI ? "protected internal" : "public", TypeName);
 					sw.WriteLine ("\t\t{");
-					if (BindThirdPartyLibrary)
+					if (is_direct_binding_value != null)
-						sw.WriteLine ("\t\t\t{0}", init_binding_type);
+						sw.WriteLine ("\t\t\tIsDirectBinding = {0};", is_direct_binding_value);
 					WriteMarkDirtyIfDerived (sw, type);
 					sw.WriteLine ("\t\t}");
 					sw.WriteLine ();

tests/generator/BGenTests.cs

@@ -475,6 +475,47 @@ namespace GeneratorTests
 			Assert.AreEqual (12, getINativeObjectCalls, "Preserve attribute count"); // If you modified code that generates PreserveAttributes please update the preserve count
 		}
 
+		[Test]
+		public void IsDirectBinding ()
+		{
+			var bgen = BuildFile (Profile.iOS, "tests/is-direct-binding.cs");
+
+			var callsMethod = new Func<MethodDefinition, string, bool> ((method, name) => {
+				return method.Body.Instructions.Any ((ins) => {
+					switch (ins.OpCode.Code) {
+					case Code.Call:
+					case Code.Calli:
+					case Code.Callvirt:
+						var mr = ins.Operand as MethodReference;
+						return mr.Name == name;
+					default:
+						return false;
+					}
+				});
+			});
+
+			// The normal constructor should get the IsDirectBinding value, and call both objc_msgSend and objc_msgSendSuper
+			var cConstructor = bgen.ApiAssembly.MainModule.GetType ("NS", "C").Methods.First ((v) => v.IsConstructor && !v.HasParameters && !v.IsStatic);
+			Assert.That (callsMethod (cConstructor, "set_IsDirectBinding"), "C: set_IsDirectBinding");
+			Assert.That (callsMethod (cConstructor, "get_IsDirectBinding"), "C: get_IsDirectBinding");
+			Assert.That (callsMethod (cConstructor, "IntPtr_objc_msgSend"), "C: objc_msgSend");
+			Assert.That (callsMethod (cConstructor, "IntPtr_objc_msgSendSuper"), "C: objc_msgSendSuper");
+
+			// The constructor for a model should not get the IsDirectBinding value, because it's always 'false'. Neither should it call objc_msgSend, only objc_msgSendSuper
+			var pConstructor = bgen.ApiAssembly.MainModule.GetType ("NS", "P").Methods.First ((v) => v.IsConstructor && !v.HasParameters && !v.IsStatic);
+			Assert.That (callsMethod (pConstructor, "set_IsDirectBinding"), "P: set_IsDirectBinding");
+			Assert.That (!callsMethod (pConstructor, "get_IsDirectBinding"), "P: get_IsDirectBinding");
+			Assert.That (!callsMethod (pConstructor, "IntPtr_objc_msgSend"), "P: objc_msgSend");
+			Assert.That (callsMethod (pConstructor, "IntPtr_objc_msgSendSuper"), "P: objc_msgSendSuper");
+
+			// The constructor for a sealed class should not get the IsDirectBinding value, because it's always true. Neither should it call objc_msgSendSuper, only objc_msgSend.
+			var sConstructor = bgen.ApiAssembly.MainModule.GetType ("NS", "S").Methods.First ((v) => v.IsConstructor && !v.HasParameters && !v.IsStatic);
+			Assert.That (callsMethod (sConstructor, "set_IsDirectBinding"), "S: set_IsDirectBinding");
+			Assert.That (!callsMethod (sConstructor, "get_IsDirectBinding"), "S: get_IsDirectBinding");
+			Assert.That (callsMethod (sConstructor, "IntPtr_objc_msgSend"), "S: objc_msgSend");
+			Assert.That (!callsMethod (sConstructor, "IntPtr_objc_msgSendSuper"), "S: objc_msgSendSuper");
+		}
+
 		BGenTool BuildFile (Profile profile, params string [] filenames)
 		{
 			return BuildFile (profile, true, filenames);

tests/generator/BGenTool.cs

@@ -259,7 +259,7 @@ namespace Xamarin.Tests
 		void LoadAssembly ()
 		{
 			if (assembly == null)
-				assembly = AssemblyDefinition.ReadAssembly (Path.Combine (TmpDirectory, Path.GetFileNameWithoutExtension (ApiDefinitions [0]) + ".dll"));
+				assembly = AssemblyDefinition.ReadAssembly (Path.Combine (TmpDirectory, Path.GetFileNameWithoutExtension (ApiDefinitions [0]).Replace ('-', '_') + ".dll"));
 		}
 
 		void EnsureTempDir ()

tests/generator/generator-tests.csproj

@@ -64,6 +64,10 @@
   </ItemGroup>
   <ItemGroup>
     <None Include="packages.config" />
+    <None Include="tests\is-direct-binding.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Folder Include="tests\" />
   </ItemGroup>
   <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
 </Project>

tests/generator/tests/is-direct-binding.cs

@@ -0,0 +1,18 @@
+using System;
+using Foundation;
+
+namespace NS {
+	[BaseType (typeof (NSObject))]
+	interface C {
+	}
+
+	[Sealed]
+	[BaseType (typeof (NSObject))]
+	interface S {
+	}
+
+	[Model][Protocol]
+	[BaseType (typeof (NSObject))]
+	interface P {
+	}
+}

tests/introspection/ApiCtorInitTest.cs

@@ -56,7 +56,8 @@ namespace Introspection {
 		{
 			if (type.ContainsGenericParameters)
 				return true;
-			
+
+#if !XAMCORE_2_0
 			// skip delegate (and other protocol references)
 			foreach (object ca in type.GetCustomAttributes (false)) {
 				if (ca is ProtocolAttribute)
@@ -64,8 +65,16 @@ namespace Introspection {
 				if (ca is ModelAttribute)
 					return true;
 			}
+#endif
 
 			switch (type.Name) {
+			case "JSExport":
+				// This is interesting: Apple defines a private JSExport class - if you try to define your own in an Objective-C project you get this warning at startup:
+				//     objc[334]: Class JSExport is implemented in both /Applications/Xcode91.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks/JavaScriptCore.framework/JavaScriptCore (0x112c1e430) and /Users/rolf/Library/Developer/CoreSimulator/Devices/AC5323CF-225F-44D9-AA18-A37B7C28CA68/data/Containers/Bundle/Application/DEF9EAFC-CB5C-454F-97F5-669BBD00A609/jsexporttest.app/jsexporttest (0x105b49df0). One of the two will be used. Which one is undefined.
+				// Due to how we treat models, we'll always look the Objective-C type up at runtime (even with the static registrar),
+				// see that there's an existing JSExport type, and use that one instead of creating a new type.
+				// This is problematic, because Apple's JSExport is completely unusable, and will crash if you try to do anything.
+				return true;
 #if !XAMCORE_2_0
 			case "AVAssetResourceLoader": // We have DisableDefaultCtor in XAMCORE_2_0 but can't change in compat because of backwards compat
 			case "AVAssetResourceLoadingRequest":

tests/introspection/Mac/MacApiCtorInitTest.cs

@@ -38,6 +38,11 @@ namespace Introspection {
 		protected override bool Skip (Type type)
 		{
 			switch (type.FullName) {
+#if !XAMCORE_4_0
+			case "AppKit.NSDraggingInfo":
+			case "MonoMac.AppKit.NSDraggingInfo": // binding mistakes.
+				return true;
+#endif
 			// Random failures on build machine
 			case "QuickLookUI.QLPreviewPanel":
 			case "MonoMac.QuickLookUI.QLPreviewPanel":

tests/xtro-sharpie/ObjCProtocolCheck.cs

@@ -41,6 +41,11 @@ namespace Extrospection {
 			if (!type.HasCustomAttributes)
 				return;
 
+			if (!type.IsInterface) {
+				// Only interfaces map to protocols, but unfortunately we add [Protocol] to generated model classes too, so we need to skip those.
+				return;
+			}
+
 			string pname = null;
 			bool informal = false;