From 8ba420f040a2987aea55bb9c103606971427cb3d Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 24 May 2017 06:40:34 +0100 Subject: [PATCH 1/5] Change C# reflection to avoid using expression trees This should work on Unity, Mono and .NET 3.5 as far as I'm aware. It won't work on platforms where reflection itself is prohibited, but that's a non-starter basically. --- .../Reflection/OneofAccessor.cs | 2 +- .../Reflection/ReflectionUtil.cs | 86 ++++++++++++------- 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs b/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs index 8714ab18efe7..975962187959 100644 --- a/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs +++ b/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs @@ -52,7 +52,7 @@ internal OneofAccessor(PropertyInfo caseProperty, MethodInfo clearMethod, OneofD throw new ArgumentException("Cannot read from property"); } this.descriptor = descriptor; - caseDelegate = ReflectionUtil.CreateFuncIMessageT(caseProperty.GetGetMethod()); + caseDelegate = ReflectionUtil.CreateFuncIMessageInt32(caseProperty.GetGetMethod()); this.descriptor = descriptor; clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod); diff --git a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs index df820ca36bd8..0d5ab7cd3f6f 100644 --- a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs +++ b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs @@ -31,8 +31,6 @@ #endregion using System; -using System.Collections.Generic; -using System.Linq.Expressions; using System.Reflection; namespace Google.Protobuf.Reflection @@ -56,52 +54,74 @@ internal static class ReflectionUtil /// Creates a delegate which will cast the argument to the appropriate method target type, /// call the method on it, then convert the result to object. /// - internal static Func CreateFuncIMessageObject(MethodInfo method) - { - ParameterExpression parameter = Expression.Parameter(typeof(IMessage), "p"); - Expression downcast = Expression.Convert(parameter, method.DeclaringType); - Expression call = Expression.Call(downcast, method); - Expression upcast = Expression.Convert(call, typeof(object)); - return Expression.Lambda>(upcast, parameter).Compile(); - } + internal static Func CreateFuncIMessageObject(MethodInfo method) => + GetReflectionHelper(method.DeclaringType, method.ReturnType).CreateFuncIMessageObject(method); /// /// Creates a delegate which will cast the argument to the appropriate method target type, /// call the method on it, then convert the result to the specified type. /// - internal static Func CreateFuncIMessageT(MethodInfo method) - { - ParameterExpression parameter = Expression.Parameter(typeof(IMessage), "p"); - Expression downcast = Expression.Convert(parameter, method.DeclaringType); - Expression call = Expression.Call(downcast, method); - Expression upcast = Expression.Convert(call, typeof(T)); - return Expression.Lambda>(upcast, parameter).Compile(); - } + internal static Func CreateFuncIMessageInt32(MethodInfo method) => + GetReflectionHelper(method.DeclaringType, typeof(object)).CreateFuncIMessageInt32(method); /// /// Creates a delegate which will execute the given method after casting the first argument to /// the target type of the method, and the second argument to the first parameter type of the method. /// - internal static Action CreateActionIMessageObject(MethodInfo method) - { - ParameterExpression targetParameter = Expression.Parameter(typeof(IMessage), "target"); - ParameterExpression argParameter = Expression.Parameter(typeof(object), "arg"); - Expression castTarget = Expression.Convert(targetParameter, method.DeclaringType); - Expression castArgument = Expression.Convert(argParameter, method.GetParameters()[0].ParameterType); - Expression call = Expression.Call(castTarget, method, castArgument); - return Expression.Lambda>(call, targetParameter, argParameter).Compile(); - } + internal static Action CreateActionIMessageObject(MethodInfo method) => + GetReflectionHelper(method.DeclaringType, method.GetParameters()[0].ParameterType).CreateActionIMessageObject(method); /// /// Creates a delegate which will execute the given method after casting the first argument to /// the target type of the method. /// - internal static Action CreateActionIMessage(MethodInfo method) + internal static Action CreateActionIMessage(MethodInfo method) => + GetReflectionHelper(method.DeclaringType, typeof(object)).CreateActionIMessage(method); + + /// + /// Creates a reflection helper for the given type arguments. Currently these are created on demand + /// rather than cached; this will be "busy" when initially loading a message's descriptor, but after that + /// they can be garbage collected. We could cache them by type if that proves to be important, but creating + /// an object is pretty cheap. + /// + private static IReflectionHelper GetReflectionHelper(Type t1, Type t2) => + (IReflectionHelper) Activator.CreateInstance(typeof(ReflectionHelper<,>).MakeGenericType(t1, t2)); + + // Non-generic interface allowing us to use an instance of ReflectionHelper without statically + // knowing the types involved. + private interface IReflectionHelper + { + Func CreateFuncIMessageInt32(MethodInfo method); + Action CreateActionIMessage(MethodInfo method); + Func CreateFuncIMessageObject(MethodInfo method); + Action CreateActionIMessageObject(MethodInfo method); + } + + private class ReflectionHelper : IReflectionHelper { - ParameterExpression targetParameter = Expression.Parameter(typeof(IMessage), "target"); - Expression castTarget = Expression.Convert(targetParameter, method.DeclaringType); - Expression call = Expression.Call(castTarget, method); - return Expression.Lambda>(call, targetParameter).Compile(); - } + public Func CreateFuncIMessageInt32(MethodInfo method) + { + var del = (Func) method.CreateDelegate(typeof(Func)); + return message => del((T1) message); + } + + public Action CreateActionIMessage(MethodInfo method) + { + var del = (Action) method.CreateDelegate(typeof(Action)); + return message => del((T1) message); + } + + public Func CreateFuncIMessageObject(MethodInfo method) + { + var del = (Func) method.CreateDelegate(typeof(Func)); + return message => del((T1) message); + } + + public Action CreateActionIMessageObject(MethodInfo method) + { + var del = (Action) method.CreateDelegate(typeof(Action)); + return (message, arg) => del((T1) message, (T2) arg); + } + } } } \ No newline at end of file From aa59eaa77b8ede7b0ed120e9bda8a92348ede946 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Thu, 16 Nov 2017 16:19:38 +0000 Subject: [PATCH 2/5] Introduce a compatiblity shim to support .NET 3.5 delegate creation --- .../Compatibility/MethodInfoExtensions.cs | 47 +++++++++++++++++++ .../Reflection/ReflectionUtil.cs | 4 ++ 2 files changed, 51 insertions(+) create mode 100644 csharp/src/Google.Protobuf/Compatibility/MethodInfoExtensions.cs diff --git a/csharp/src/Google.Protobuf/Compatibility/MethodInfoExtensions.cs b/csharp/src/Google.Protobuf/Compatibility/MethodInfoExtensions.cs new file mode 100644 index 000000000000..7b946cb631fa --- /dev/null +++ b/csharp/src/Google.Protobuf/Compatibility/MethodInfoExtensions.cs @@ -0,0 +1,47 @@ +#region Copyright notice and license +// Protocol Buffers - Google's data interchange format +// Copyright 2017 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +#endregion + +#if NET35 +using System; +using System.Reflection; + +namespace Google.Protobuf.Compatibility +{ + // .NET Core (at least netstandard1.0) doesn't have Delegate.CreateDelegate, and .NET 3.5 doesn't have + // MethodInfo.CreateDelegate. Proxy from one to the other on .NET 3.5... + internal static class MethodInfoExtensions + { + internal static Delegate CreateDelegate(this MethodInfo method, Type type) => + Delegate.CreateDelegate(type, method); + } +} +#endif diff --git a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs index 0d5ab7cd3f6f..c2771d723f07 100644 --- a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs +++ b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs @@ -33,6 +33,10 @@ using System; using System.Reflection; +#if NET35 +using Google.Protobuf.Compatibility; +#endif + namespace Google.Protobuf.Reflection { /// From 8e23d4e49c69385537869ed1332d52317c904d59 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Thu, 16 Nov 2017 18:20:01 +0000 Subject: [PATCH 3/5] Work around an "old runtime" issue with reflection For oneofs, to get the case, we need to call the property that returns the enum value. We really want it as an int, and modern runtimes allow us to create a delegate which returns an int from the method. (I suspect that the MS runtime has always allowed that.) Old versions of Mono (e.g. used by Unity3d) don't allow that, so we have to convert the enum value to an int via boxing. It's ugly, but it should work. --- .../Reflection/ReflectionUtil.cs | 56 +++++++++++++++++-- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs index c2771d723f07..3a5b3bd9df14 100644 --- a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs +++ b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs @@ -30,6 +30,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #endregion +using Google.Protobuf.Compatibility; using System; using System.Reflection; @@ -63,10 +64,12 @@ internal static Func CreateFuncIMessageObject(MethodInfo metho /// /// Creates a delegate which will cast the argument to the appropriate method target type, - /// call the method on it, then convert the result to the specified type. + /// call the method on it, then convert the result to the specified type. The method is expected + /// to actually return an enum (because of where we're calling it - for oneof cases). Sometimes that + /// means we need some extra work to perform conversions. /// internal static Func CreateFuncIMessageInt32(MethodInfo method) => - GetReflectionHelper(method.DeclaringType, typeof(object)).CreateFuncIMessageInt32(method); + GetReflectionHelper(method.DeclaringType, method.ReturnType).CreateFuncIMessageInt32(method); /// /// Creates a delegate which will execute the given method after casting the first argument to @@ -80,7 +83,7 @@ internal static Action CreateActionIMessageObject(MethodInfo m /// the target type of the method. /// internal static Action CreateActionIMessage(MethodInfo method) => - GetReflectionHelper(method.DeclaringType, typeof(object)).CreateActionIMessage(method); + GetReflectionHelper(method.DeclaringType, typeof(object)).CreateActionIMessage(method); /// /// Creates a reflection helper for the given type arguments. Currently these are created on demand @@ -103,10 +106,24 @@ private interface IReflectionHelper private class ReflectionHelper : IReflectionHelper { + public Func CreateFuncIMessageInt32(MethodInfo method) { - var del = (Func) method.CreateDelegate(typeof(Func)); - return message => del((T1) message); + // On pleasant runtimes, we can create a Func from a method returning + // an enum based on an int. That's the fast path. + if (CanConvertEnumFuncToInt32Func) + { + var del = (Func) method.CreateDelegate(typeof(Func)); + return message => del((T1) message); + } + else + { + // On some runtimes (e.g. old Mono) the return type has to be exactly correct, + // so we go via boxing. Reflection is already fairly inefficient, and this is + // only used for one-of case checking, fortunately. + var del = (Func) method.CreateDelegate(typeof(Func)); + return message => (int) (object) del((T1) message); + } } public Action CreateActionIMessage(MethodInfo method) @@ -127,5 +144,34 @@ public Action CreateActionIMessageObject(MethodInfo method) return (message, arg) => del((T1) message, (T2) arg); } } + + // Runtime compatibility checking code - see ReflectionHelper.CreateFuncIMessageInt32 for + // details about why we're doing this. + + // Deliberately not inside the generic type. We only want to check this once. + private static bool CanConvertEnumFuncToInt32Func { get; } = CheckCanConvertEnumFuncToInt32Func(); + + private static bool CheckCanConvertEnumFuncToInt32Func() + { + try + { + MethodInfo method = typeof(ReflectionUtil).GetMethod(nameof(SampleEnumMethod)); + // If this passes, we're in a reasonable runtime. + method.CreateDelegate(typeof(Func)); + return true; + } + catch (ArgumentException) + { + return false; + } + } + + public enum SampleEnum + { + X + } + + // Public to make the reflection simpler. + public static SampleEnum SampleEnumMethod() => SampleEnum.X; } } \ No newline at end of file From 7b19d2088278e06a7b6a9720ddbfd5ec484ab14f Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Tue, 27 Mar 2018 08:44:42 +0100 Subject: [PATCH 4/5] Add extra C# file to Makefile.am --- Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile.am b/Makefile.am index 8e85484b9c83..41b68b2b4f2d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -133,6 +133,7 @@ csharp_EXTRA_DIST= \ csharp/src/Google.Protobuf/Collections/ProtobufEqualityComparers.cs \ csharp/src/Google.Protobuf/Collections/ReadOnlyDictionary.cs \ csharp/src/Google.Protobuf/Collections/RepeatedField.cs \ + csharp/src/Google.Protobuf/Compatibility/MethodInfoExtensions.cs \ csharp/src/Google.Protobuf/Compatibility/PropertyInfoExtensions.cs \ csharp/src/Google.Protobuf/Compatibility/StreamExtensions.cs \ csharp/src/Google.Protobuf/Compatibility/TypeExtensions.cs \ From 9c05c353414cc4439085151362756536511c2f0d Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Tue, 27 Mar 2018 10:16:53 +0100 Subject: [PATCH 5/5] Address review comments --- .../Reflection/ReflectionUtil.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs index 3a5b3bd9df14..1afaa90c9e53 100644 --- a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs +++ b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs @@ -56,32 +56,40 @@ internal static class ReflectionUtil internal static readonly Type[] EmptyTypes = new Type[0]; /// - /// Creates a delegate which will cast the argument to the appropriate method target type, + /// Creates a delegate which will cast the argument to the type that declares the method, /// call the method on it, then convert the result to object. /// + /// The method to create a delegate for, which must be declared in an IMessage + /// implementation. internal static Func CreateFuncIMessageObject(MethodInfo method) => GetReflectionHelper(method.DeclaringType, method.ReturnType).CreateFuncIMessageObject(method); /// - /// Creates a delegate which will cast the argument to the appropriate method target type, + /// Creates a delegate which will cast the argument to the type that declares the method, /// call the method on it, then convert the result to the specified type. The method is expected /// to actually return an enum (because of where we're calling it - for oneof cases). Sometimes that /// means we need some extra work to perform conversions. /// + /// The method to create a delegate for, which must be declared in an IMessage + /// implementation. internal static Func CreateFuncIMessageInt32(MethodInfo method) => GetReflectionHelper(method.DeclaringType, method.ReturnType).CreateFuncIMessageInt32(method); /// /// Creates a delegate which will execute the given method after casting the first argument to - /// the target type of the method, and the second argument to the first parameter type of the method. + /// the type that declares the method, and the second argument to the first parameter type of the method. /// + /// The method to create a delegate for, which must be declared in an IMessage + /// implementation. internal static Action CreateActionIMessageObject(MethodInfo method) => GetReflectionHelper(method.DeclaringType, method.GetParameters()[0].ParameterType).CreateActionIMessageObject(method); /// /// Creates a delegate which will execute the given method after casting the first argument to - /// the target type of the method. + /// type that declares the method. /// + /// The method to create a delegate for, which must be declared in an IMessage + /// implementation. internal static Action CreateActionIMessage(MethodInfo method) => GetReflectionHelper(method.DeclaringType, typeof(object)).CreateActionIMessage(method); @@ -174,4 +182,4 @@ public enum SampleEnum // Public to make the reflection simpler. public static SampleEnum SampleEnumMethod() => SampleEnum.X; } -} \ No newline at end of file +}