From 567b82dc58e5dcd851fba662ceae417878aa0400 Mon Sep 17 00:00:00 2001 From: Andi McClure Date: Thu, 16 Feb 2017 17:39:32 -0500 Subject: [PATCH] Bump to cecil/master/d0cb2b78 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update cecil for parity with mono/2017-02 Bumping cecil requires updating `assembly-rename.patch` so that it will cleanly apply, and fixing `DirectoryAssemblyResolver.cs` so that `AssemblyResolverCoda.GetAssembly()` will compile. Then there's *all the other changes* in this patch, which are *unrelated* to the Cecil bump, but instead related to an "environmental" change on the PR builder: The xamarin-android repo [now supports auto-provisioning][0] dependencies, allowing Mono to be upgraded as part of the build preparation process. One of the xamarin-android PRs makes use of this, to install Mono 4.9 (to test building under/with Mono 4.9/Roslyn/etc.). The xamarin-android PR builder is the same machine as the Java.Interop PR builder. Meaning Java.Interop PRs may -- or may not, depending on global machine state -- be building with Mono 4.9. As It Turns Out™, building under Roslyn "breaks" things. :-D Gendarme now flags additional errors due to Roslyn IL differences, e.g. additional closure members which are apparently always null, and other IL differences. Then there's the larger changes `src/Java.Interop/Tests/Java.Interop/TestType.cs`: these are needed because using Roslyn *broke our unit tests*: System.ArgumentException : targetType must implement IJavaPeerable! at Java.Interop.JniRuntime+JniValueManager.CreatePeer (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions transfer, System.Type targetType) at Java.Interop.ProxyValueMarshaler.CreateGenericValue (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType) at Java.Interop.JniValueMarshaler`1[T].CreateValue (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType) at Java.Interop.JniValueMarshaler.CreateValue (System.IntPtr handle, System.Type targetType) at (wrapper dynamic-method) System.Object:lambda_method (System.Runtime.CompilerServices.Closure,intptr,intptr) Unfortunately, the `ArgumentException` doesn't provide much context, so -- relatedly -- update the error message so that it provides the type that doesn't implement `IJavaPeerable`: targetType `Java.InteropTests.TestType+<>c, Java.Interop-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null` must implement IJavaPeerable! This added context introduces a new question: WTF is `TestType.<>c`?! Funny story! Roslyn and `mcs` differ in how they implement lambda methods: `mcs` adds the lambda method into the enclosing class. Roslyn adds it to a *nested* `<>c` class: // C# Source: class Example { public static void Main () { Action a = () => {}; } } // mcs output equivalent class Example { static void
m__0 () { } public static void Main () { Action a =
m__0; } } // Roslyn output equivalent class Example { class <>c { public static <>c <>9 = new <>c (); public static Action <>9__0_0; internal void
b__0_0 () { } } public static void Main () { if (<>c.<>9__0_0 == null) <>c.<>9__0_0 = <>c.
.b__0_0; Action a = <>c.<>9__0_0; } } Aside: Roslyn, you're drunk. WTF?! Why does this matter? Because `TestType` uses `JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate()` on a `Delegate` instance, which does lots of `System.Linq.Expressions` voodoo, the short of which is that it creates a new method that uses `Delegate.Method.DeclaringType`: static Delegate GetEqualsThisHandler () { Func h = (jnienv, n_self, n_value) => { ... }; return JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate (h); } In `mcs` output, `h.Method.DeclaringType` is `typeof(TestType)`. In Roslyn output, `h.Method.DeclaringType` is `typeof(TestType.<>c)`. This breaks all manner of things, which is why the unit tests fail. The fix? Don't Do That™, specifically, don't use lambda methods here. Move the lambda method bodies out into normal methods, so that the `Delegate.Method.DeclaringType` for a delegate targeting those methods is of the appropriate type. [0]: https://github.com/xamarin/xamarin-android/commit/54a7a0299ef6489ce5cb861b43951f72c5f6a620 --- external/cecil | 2 +- .../DirectoryAssemblyResolver.cs | 2 +- .../JniRuntime.JniValueManager.cs | 2 +- src/Java.Interop/Java.Interop/JniType.cs | 2 + .../Tests/Java.Interop/TestType.cs | 52 +++++++++++-------- .../assembly-rename.patch | 40 +++++++------- xa-gendarme-ignore.txt | 7 +++ 7 files changed, 61 insertions(+), 46 deletions(-) diff --git a/external/cecil b/external/cecil index 172c907cd..d0cb2b783 160000 --- a/external/cecil +++ b/external/cecil @@ -1 +1 @@ -Subproject commit 172c907cd79a2e3e48df95f3f7425d46593274cb +Subproject commit d0cb2b7838f7c893325a2c636a56968455df582b diff --git a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs index 27300212c..04c06f8a4 100644 --- a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs +++ b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs @@ -44,7 +44,7 @@ public static class AssemblyResolverCoda { public static AssemblyDefinition GetAssembly (this IAssemblyResolver resolver, string fileName) { - return resolver.Resolve (Path.GetFileNameWithoutExtension (fileName)); + return resolver.Resolve (AssemblyNameReference.Parse (Path.GetFileNameWithoutExtension (fileName))); } } diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 08d0c1034..f6d5e4394 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -241,7 +241,7 @@ public virtual IJavaPeerable CreatePeer (ref JniObjectReference reference, JniOb targetType = GetPeerType (targetType); if (!typeof (IJavaPeerable).GetTypeInfo ().IsAssignableFrom (targetType.GetTypeInfo ())) - throw new ArgumentException ("targetType must implement IJavaPeerable!", "targetType"); + throw new ArgumentException ($"targetType `{targetType.AssemblyQualifiedName}` must implement IJavaPeerable!", "targetType"); var ctor = GetPeerConstructor (reference, targetType); if (ctor == null) diff --git a/src/Java.Interop/Java.Interop/JniType.cs b/src/Java.Interop/Java.Interop/JniType.cs index 8b633224b..35aa384fe 100644 --- a/src/Java.Interop/Java.Interop/JniType.cs +++ b/src/Java.Interop/Java.Interop/JniType.cs @@ -12,6 +12,8 @@ public sealed class JniType : IDisposable { public static unsafe JniType DefineClass (string name, JniObjectReference loader, byte[] classFileData) { + if (classFileData == null) + return null; fixed (byte* buf = classFileData) { var lref = JniEnvironment.Types.DefineClass (name, loader, (IntPtr) buf, classFileData.Length); return new JniType (ref lref, JniObjectReferenceOptions.CopyAndDispose); diff --git a/src/Java.Interop/Tests/Java.Interop/TestType.cs b/src/Java.Interop/Tests/Java.Interop/TestType.cs index 86dae01fa..987abf8fd 100644 --- a/src/Java.Interop/Tests/Java.Interop/TestType.cs +++ b/src/Java.Interop/Tests/Java.Interop/TestType.cs @@ -103,37 +103,43 @@ public bool PropogateFinallyBlockExecuted { static Delegate GetEqualsThisHandler () { - Func h = (jnienv, n_self, n_value) => { - var jvm = JniEnvironment.Runtime; - var r_self = new JniObjectReference (n_self); - var self = jvm.ValueManager.GetValue(ref r_self, JniObjectReferenceOptions.CopyAndDoNotRegister); - var r_value = new JniObjectReference (n_self); - var value = jvm.ValueManager.GetValue (ref r_value, JniObjectReferenceOptions.CopyAndDoNotRegister); - - try { - return self.EqualsThis (value); - } finally { - self.DisposeUnlessReferenced (); - value.DisposeUnlessReferenced (); - } - }; + Func h = _EqualsThis; return JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate (h); } + static bool _EqualsThis (IntPtr jnienv, IntPtr n_self, IntPtr n_value) + { + var jvm = JniEnvironment.Runtime; + var r_self = new JniObjectReference (n_self); + var self = jvm.ValueManager.GetValue(ref r_self, JniObjectReferenceOptions.CopyAndDoNotRegister); + var r_value = new JniObjectReference (n_self); + var value = jvm.ValueManager.GetValue (ref r_value, JniObjectReferenceOptions.CopyAndDoNotRegister); + + try { + return self.EqualsThis (value); + } finally { + self.DisposeUnlessReferenced (); + value.DisposeUnlessReferenced (); + } + } + static Delegate GetInt32ValueHandler () { - Func h = (jnienv, n_self) => { - var r_self = new JniObjectReference (n_self); - var self = JniEnvironment.Runtime.ValueManager.GetValue(ref r_self, JniObjectReferenceOptions.CopyAndDoNotRegister); - try { - return self.GetInt32Value (); - } finally { - self.DisposeUnlessReferenced (); - } - }; + Func h = _GetInt32Value; return JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate (h); } + static int _GetInt32Value (IntPtr jnienv, IntPtr n_self) + { + var r_self = new JniObjectReference (n_self); + var self = JniEnvironment.Runtime.ValueManager.GetValue(ref r_self, JniObjectReferenceOptions.CopyAndDoNotRegister); + try { + return self.GetInt32Value (); + } finally { + self.DisposeUnlessReferenced (); + } + } + static Delegate _GetStringValueHandler () { Func h = GetStringValueHandler; diff --git a/src/Xamarin.Android.Cecil/assembly-rename.patch b/src/Xamarin.Android.Cecil/assembly-rename.patch index ee98c4e5d..706e10080 100644 --- a/src/Xamarin.Android.Cecil/assembly-rename.patch +++ b/src/Xamarin.Android.Cecil/assembly-rename.patch @@ -1,39 +1,39 @@ diff --git a/Mono.Cecil.Cil/Symbols.cs b/Mono.Cecil.Cil/Symbols.cs -index 426c4a7..1bc138d 100644 +index 6ca158e..dbca57a 100644 --- a/Mono.Cecil.Cil/Symbols.cs +++ b/Mono.Cecil.Cil/Symbols.cs -@@ -186,7 +186,7 @@ namespace Mono.Cecil.Cil { - var cecil_name = typeof (SymbolProvider).Assembly.GetName (); +@@ -637,7 +637,7 @@ namespace Mono.Cecil.Cil { + var cecil_name = typeof (SymbolProvider).GetAssembly ().GetName (); var name = new SR.AssemblyName { -- Name = "Mono.Cecil." + symbol_kind, -+ Name = "Xamarin.Android.Cecil." + symbol_kind, +- Name = "Mono.Cecil." + symbolKind, ++ Name = "Xamarin.Android.Cecil." + symbolKind, Version = cecil_name.Version, }; -diff --git a/ProjectInfo.cs b/ProjectInfo.cs -index 8d427a7..59bc253 100644 ---- a/ProjectInfo.cs -+++ b/ProjectInfo.cs -@@ -10,7 +10,7 @@ - using System.Reflection; - using System.Runtime.InteropServices; - --[assembly: AssemblyProduct ("Mono.Cecil")] -+[assembly: AssemblyProduct ("Xamarin.Android.Cecil")] - [assembly: AssemblyCopyright ("Copyright © 2008 - 2015 Jb Evain")] - - [assembly: ComVisible (false)] diff --git a/Mono.Cecil/AssemblyInfo.cs b/Mono.Cecil/AssemblyInfo.cs -index 40cc0d4..41e159f 100644 +index 4d32f2b..9726954 100644 --- a/Mono.Cecil/AssemblyInfo.cs +++ b/Mono.Cecil/AssemblyInfo.cs -@@ -10,7 +10,7 @@ using System.Reflection; +@@ -12,7 +12,7 @@ using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; -[assembly: AssemblyTitle ("Mono.Cecil")] +[assembly: AssemblyTitle ("Xamarin.Android.Cecil")] + #if !PCL && !NET_CORE [assembly: Guid ("fd225bb4-fa53-44b2-a6db-85f5e48dcb54")] +diff --git a/ProjectInfo.cs b/ProjectInfo.cs +index 6bfdedf..95b940c 100644 +--- a/ProjectInfo.cs ++++ b/ProjectInfo.cs +@@ -10,7 +10,7 @@ + using System.Reflection; + using System.Runtime.InteropServices; + +-[assembly: AssemblyProduct ("Mono.Cecil")] ++[assembly: AssemblyProduct ("Xamarin.Android.Cecil")] + [assembly: AssemblyCopyright ("Copyright © 2008 - 2015 Jb Evain")] + #if !PCL diff --git a/xa-gendarme-ignore.txt b/xa-gendarme-ignore.txt index 1de90a469..f000fb528 100644 --- a/xa-gendarme-ignore.txt +++ b/xa-gendarme-ignore.txt @@ -24,6 +24,9 @@ R: Gendarme.Rules.Correctness.CheckParametersNullityInVisibleMethodsRule M: System.Boolean Java.Interop.JniPeerMembers::UsesVirtualDispatch(Java.Interop.IJavaPeerable,System.Type) M: Java.Interop.JniPeerMembers Java.Interop.JniPeerMembers::GetPeerMembers(Java.Interop.IJavaPeerable) +# I suspect a gendarme bug; I don't see what it's talking about +M: System.Void Java.Interop.JniArgumentValue::.ctor(Java.Interop.IJavaPeerable) + R: Gendarme.Rules.Correctness.EnsureLocalDisposalRule # We don't *want* to dispose the value! @@ -213,6 +216,10 @@ M: System.Void Java.Interop.NativeMethods::java_interop_jnienv_get_boolean_array M: System.Void Java.Interop.NativeMethods::java_interop_jnienv_set_boolean_array_region(System.IntPtr,System.IntPtr&,System.IntPtr,System.Int32,System.Int32,System.Boolean*) M: System.IntPtr Java.Interop.NativeMethods::java_interop_jnienv_get_primitive_array_critical(System.IntPtr,System.IntPtr,System.Boolean*) +R: Gendarme.Rules.Maintainability.AvoidAlwaysNullFieldRule +# I suspect that this is a Roslyn bug; mcs output doesn't produce this. +T: Java.Interop.JniRuntime/JniTypeManager/d__15 + R: Gendarme.Rules.Maintainability.AvoidLackOfCohesionOfMethodsRule # I'm not sure I understand "lack of cohesion" in this context, and the docs