From dd95d17b5f19faa2b6efd0a5fa15442c6c96ecb9 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Fri, 1 Dec 2017 15:18:20 +0100 Subject: [PATCH] [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 | 129 ++++++++++++------ tests/generator/BGenTests.cs | 41 ++++++ tests/generator/BGenTool.cs | 2 +- tests/generator/generator-tests.csproj | 4 + tests/generator/tests/is-direct-binding.cs | 18 +++ tests/introspection/ApiCtorInitTest.cs | 11 +- tests/introspection/Mac/MacApiCtorInitTest.cs | 5 + tests/xtro-sharpie/ObjCProtocolCheck.cs | 5 + 8 files changed, 171 insertions(+), 44 deletions(-) create mode 100644 tests/generator/tests/is-direct-binding.cs diff --git a/src/generator.cs b/src/generator.cs index d03084f928..2e17a3cfab 100644 --- a/src/generator.cs +++ b/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 trueCode, Func 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); - indent++; - GenerateInvoke (false, mi, minfo, sel, argsArray, needs_temp, category_type); - indent--; - print ("} else {"); - indent++; - GenerateInvoke (true, mi, minfo, sel, argsArray, needs_temp, category_type); - indent--; - print ("}"); + + WriteIsDirectBindingCondition (sw, ref indent, is_direct_binding, + 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); return null; }, + () => { GenerateInvoke (true, mi, minfo, sel, argsArray, needs_temp, category_type); return null; } + ); if (null_handle) { indent--; @@ -5731,7 +5766,8 @@ public partial class Generator : IMemberGatherer { bool is_static_class = AttributeManager.HasAttribute (type) || is_category_class; bool is_partial = AttributeManager.HasAttribute (type); bool is_model = AttributeManager.HasAttribute (type); - bool is_protocol = AttributeManager.HasAttribute (type); + var protocol = AttributeManager.GetCustomAttribute (type); + bool is_protocol = protocol != null; bool is_abstract = AttributeManager.HasAttribute (type); bool is_sealed = AttributeManager.HasAttribute (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 (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 (type); - if (!type.IsEnum && !core_image_filter) - print ("[Register(\"{0}\", {1})]", objc_type_name, AttributeManager.HasAttribute (type) || is_model ? "false" : "true"); + if (!type.IsEnum && !core_image_filter) { + if (is_model || AttributeManager.HasAttribute (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) - sw.WriteLine ("\t\t\t{0}", init_binding_type); - if (debug) - sw.WriteLine ("\t\t\tConsole.WriteLine (\"{0}.ctor ()\");", TypeName); - 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}"); + var indentation = 3; + WriteIsDirectBindingCondition (sw, ref indentation, is_direct_binding, is_direct_binding_value, + () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSend (this.Handle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime), + () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSendSuper (this.SuperHandle, global::{2}.{0}), \"init\");", initSelector, ns.Messaging, ns.CoreObjCRuntime)); + 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) {"); - sw.WriteLine ("\t\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSend_IntPtr (this.Handle, {0}, coder.Handle), \"initWithCoder:\");", initWithCoderSelector, ns.Messaging); - sw.WriteLine ("\t\t\t} else {"); - sw.WriteLine ("\t\t\t\tInitializeHandle (global::{1}.IntPtr_objc_msgSendSuper_IntPtr (this.SuperHandle, {0}, coder.Handle), \"initWithCoder:\");", initWithCoderSelector, ns.Messaging); - sw.WriteLine ("\t\t\t}"); + var indentation = 3; + WriteIsDirectBindingCondition (sw, ref indentation, is_direct_binding, is_direct_binding_value, + () => string.Format ("InitializeHandle (global::{1}.IntPtr_objc_msgSend_IntPtr (this.Handle, {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)); 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) - sw.WriteLine ("\t\t\t{0}", init_binding_type); + if (is_direct_binding_value != null) + 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) - sw.WriteLine ("\t\t\t{0}", init_binding_type); + if (is_direct_binding_value != null) + sw.WriteLine ("\t\t\tIsDirectBinding = {0};", is_direct_binding_value); WriteMarkDirtyIfDerived (sw, type); sw.WriteLine ("\t\t}"); sw.WriteLine (); diff --git a/tests/generator/BGenTests.cs b/tests/generator/BGenTests.cs index bd1b924bbc..7370209f5e 100644 --- a/tests/generator/BGenTests.cs +++ b/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 ((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); diff --git a/tests/generator/BGenTool.cs b/tests/generator/BGenTool.cs index 9bca236985..1d8c3cb624 100644 --- a/tests/generator/BGenTool.cs +++ b/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 () diff --git a/tests/generator/generator-tests.csproj b/tests/generator/generator-tests.csproj index 5b5e3031a6..5aecc5833e 100644 --- a/tests/generator/generator-tests.csproj +++ b/tests/generator/generator-tests.csproj @@ -64,6 +64,10 @@ + + + + \ No newline at end of file diff --git a/tests/generator/tests/is-direct-binding.cs b/tests/generator/tests/is-direct-binding.cs new file mode 100644 index 0000000000..329a11b695 --- /dev/null +++ b/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 { + } +} \ No newline at end of file diff --git a/tests/introspection/ApiCtorInitTest.cs b/tests/introspection/ApiCtorInitTest.cs index d13d5451c3..30641065d0 100644 --- a/tests/introspection/ApiCtorInitTest.cs +++ b/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": diff --git a/tests/introspection/Mac/MacApiCtorInitTest.cs b/tests/introspection/Mac/MacApiCtorInitTest.cs index a17280a2f9..7354353b38 100644 --- a/tests/introspection/Mac/MacApiCtorInitTest.cs +++ b/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": diff --git a/tests/xtro-sharpie/ObjCProtocolCheck.cs b/tests/xtro-sharpie/ObjCProtocolCheck.cs index 18f6f90ef9..d204543dc3 100644 --- a/tests/xtro-sharpie/ObjCProtocolCheck.cs +++ b/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;