Fix duplicated methods generated in proxies #2264
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.Serialization; | ||
| using System.Security; | ||
| using NHibernate.Proxy.DynamicProxy; | ||
| using NHibernate.Util; | ||
|
|
||
| namespace NHibernate.Proxy | ||
|
|
@@ -101,11 +102,19 @@ internal static IEnumerable<MethodInfo> GetProxiableMethods(System.Type type) | |
|
|
||
| internal static IEnumerable<MethodInfo> GetProxiableMethods(System.Type type, IEnumerable<System.Type> interfaces) | ||
| { | ||
| if (type.IsInterface || type == typeof(object) || type.GetInterfaces().Length == 0) | ||
| { | ||
| return GetProxiableMethods(type) | ||
| .Concat(interfaces.SelectMany(i => i.GetMethods())) | ||
| .Distinct(); | ||
| } | ||
|
|
||
| var proxiableMethods = new HashSet<MethodInfo>(GetProxiableMethods(type), new MethodInfoComparer(type)); | ||
| foreach (var interfaceMethod in interfaces.SelectMany(i => i.GetMethods())) | ||
| { | ||
| proxiableMethods.Add(interfaceMethod); | ||
| } | ||
|
There was a problem hiding this comment. I could have kept the previous code, just giving the comparer to |
||
|
|
||
| return proxiableMethods; | ||
| } | ||
|
|
||
|
|
@@ -136,23 +145,43 @@ internal static MethodBuilder GetObjectDataMethodBuilder(TypeBuilder typeBuilder | |
|
|
||
| internal static MethodBuilder GenerateMethodSignature(string name, MethodInfo method, TypeBuilder typeBuilder) | ||
| { | ||
| var explicitImplementation = method.DeclaringType.IsInterface; | ||
| if (explicitImplementation && | ||
| (typeBuilder.BaseType == typeof(object) || | ||
| #pragma warning disable 618 | ||
| typeBuilder.BaseType == typeof(ProxyDummy)) && | ||
|
There was a problem hiding this comment. Old dynamic proxy legacy. Not handling this causes its |
||
| #pragma warning restore 618 | ||
| (IsEquals(method) || IsGetHashCode(method))) | ||
| { | ||
| // If we are building a proxy for an interface, and it defines an Equals or GetHashCode, they must | ||
| // be implicitly implemented for overriding object methods. | ||
| // (Ideally we should check the method actually comes from the interface declared for the proxy.) | ||
| explicitImplementation = false; | ||
| } | ||
|
|
||
| var methodAttributes = explicitImplementation | ||
| ? MethodAttributes.Private | MethodAttributes.Final | MethodAttributes.HideBySig | | ||
| MethodAttributes.SpecialName | MethodAttributes.NewSlot | MethodAttributes.Virtual | ||
| : MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Virtual; | ||
|
|
||
| if (method.IsSpecialName) | ||
| methodAttributes |= MethodAttributes.SpecialName; | ||
|
|
||
| var parameters = method.GetParameters(); | ||
| var implementationName = explicitImplementation | ||
| ? $"{method.DeclaringType.FullName}.{name}" | ||
| : name; | ||
| var methodBuilder = | ||
| typeBuilder.DefineMethod( | ||
| implementationName, | ||
| methodAttributes, | ||
| CallingConventions.HasThis, | ||
| method.ReturnType, | ||
| parameters.ToArray(param => param.ParameterType)); | ||
| if (explicitImplementation) | ||
| methodBuilder.SetImplementationFlags(MethodImplAttributes.Managed | MethodImplAttributes.IL); | ||
|
There was a problem hiding this comment. Copied from |
||
|
|
||
| var typeArgs = method.GetGenericArguments(); | ||
| if (typeArgs.Length > 0) | ||
| { | ||
| var typeNames = GenerateTypeNames(typeArgs.Length); | ||
|
|
@@ -182,6 +211,18 @@ internal static MethodBuilder GenerateMethodSignature(string name, MethodInfo me | |
| return methodBuilder; | ||
| } | ||
|
|
||
| private static bool IsGetHashCode(MethodBase method) | ||
| { | ||
| return method.Name == "GetHashCode" && method.GetParameters().Length == 0; | ||
| } | ||
|
|
||
| private static bool IsEquals(MethodBase method) | ||
| { | ||
| if (method.Name != "Equals") return false; | ||
| var parameters = method.GetParameters(); | ||
| return parameters.Length == 1 && parameters[0].ParameterType == typeof(object); | ||
| } | ||
|
|
||
| internal static void GenerateInstanceOfIgnoresAccessChecksToAttribute( | ||
| AssemblyBuilder assemblyBuilder, | ||
| string assemblyName) | ||
|
|
@@ -253,5 +294,72 @@ private static string[] GenerateTypeNames(int count) | |
|
|
||
| return result; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Method equality for the proxy building purpose: we want to equate an interface method to a base type | ||
| /// method which implements it. This implies the base type method has the same signature and there is no | ||
| /// explicit implementation of the interface method in the base type. | ||
| /// </summary> | ||
| private class MethodInfoComparer : IEqualityComparer<MethodInfo> | ||
|
There was a problem hiding this comment. This comparer is just an optimization to generate only one method if the interface method is implemented implicitly. Right? There was a problem hiding this comment. Initially I wanted to handle it fully in the method using that comparer, but it was too complex. Yes it is for generating only one method if the interface is implemented implicitly. No it is not an optimization from my viewpoint. Not doing so, while implementing the base method and the interface method both publicly, is #2085 bug. Granted, the other changes in the method generation is enough to fix it alone, while this change is not enough for explicit implementation cases. But then, not doing so would trigger again #2052. This bug was re-qualified as just a "possible breaking change" bound to the switch to the static proxy, but if we can avoid that breaking change, that would be better. Otherwise those who read the Id through an implicitly implemented interface will trigger somewhat undue lazy loadings. This really can cause major performance issues. |
||
| { | ||
| private readonly Dictionary<System.Type, Dictionary<MethodInfo, MethodInfo>> _interfacesMap; | ||
|
|
||
| public MethodInfoComparer(System.Type baseType) | ||
| { | ||
| _interfacesMap = BuildInterfacesMap(baseType); | ||
| } | ||
|
|
||
| private static Dictionary<System.Type, Dictionary<MethodInfo, MethodInfo>> BuildInterfacesMap(System.Type type) | ||
| { | ||
| return type.GetInterfaces() | ||
| .Distinct() | ||
| .ToDictionary(i => i, i => ToDictionary(type.GetInterfaceMap(i))); | ||
| } | ||
|
|
||
| private static Dictionary<MethodInfo, MethodInfo> ToDictionary(InterfaceMapping interfaceMap) | ||
| { | ||
| var map = new Dictionary<MethodInfo, MethodInfo>(interfaceMap.InterfaceMethods.Length); | ||
| for (var i = 0; i < interfaceMap.InterfaceMethods.Length; i++) | ||
| { | ||
| map.Add(interfaceMap.InterfaceMethods[i], interfaceMap.TargetMethods[i]); | ||
| } | ||
|
|
||
| return map; | ||
| } | ||
|
|
||
| public bool Equals(MethodInfo x, MethodInfo y) | ||
| { | ||
| if (x == y) | ||
| return true; | ||
| if (x == null || y == null) | ||
| return false; | ||
| if (x.Name != y.Name) | ||
| return false; | ||
|
|
||
| // If they have the same declaring type, one cannot be the implementation of the other. | ||
| if (x.DeclaringType == y.DeclaringType) | ||
| return false; | ||
| // If they belong to two different interfaces or to two different concrete types, one cannot be the | ||
| // implementation of the other. | ||
| if (x.DeclaringType.IsInterface == y.DeclaringType.IsInterface) | ||
| return false; | ||
|
|
||
| var interfaceMethod = x.DeclaringType.IsInterface ? x : y; | ||
| // If the interface is not implemented by the base type, the method cannot be implemented by the | ||
| // base type method. | ||
| if (!_interfacesMap.TryGetValue(interfaceMethod.DeclaringType, out var map)) | ||
| return false; | ||
|
|
||
| var baseMethod = x.DeclaringType.IsInterface ? y : x; | ||
| return map[interfaceMethod] == baseMethod; | ||
| } | ||
|
|
||
| public int GetHashCode(MethodInfo obj) | ||
| { | ||
| // Hashing by name only, putting methods with the same name in the same bucket, in order to keep | ||
| // this method fast. | ||
| return obj.Name.GetHashCode(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.