From 3056e08fc1aa4b39059c0b4483b1705c0578fc6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Sun, 3 Nov 2019 19:22:56 +0100 Subject: [PATCH 1/4] Fix duplicated methods generated in proxies Fix #2085 --- .../StaticProxyFactoryFixture.cs | 83 +++++++++++- src/NHibernate/Proxy/ProxyBuilderHelper.cs | 122 +++++++++++++++--- 2 files changed, 186 insertions(+), 19 deletions(-) diff --git a/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs b/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs index 1977b3f4049..c9183e757b6 100644 --- a/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs +++ b/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs @@ -70,6 +70,30 @@ public PublicExplicitInterfaceTestClass() } } + [Serializable] + public class PublicExplicitInterfaceWithSameMembersTestClass : IPublic + { + public virtual int Id { get; set; } + public virtual string Name { get; set; } + + int IPublic.Id { get; set; } + string IPublic.Name { get; set; } + + public PublicExplicitInterfaceWithSameMembersTestClass() + { + // Check access to properties from the default constructor do not fail once proxified + Id = -1; + Assert.That(Id, Is.EqualTo(-1)); + Name = "Unknown"; + Assert.That(Name, Is.EqualTo("Unknown")); + IPublic pub = this; + pub.Id = -2; + Assert.That(pub.Id, Is.EqualTo(-2)); + pub.Name = "Unknown2"; + Assert.That(pub.Name, Is.EqualTo("Unknown2")); + } + } + [Serializable] public abstract class AbstractTestClass : IPublic { @@ -241,8 +265,8 @@ public void VerifyProxyForClassWithInterface() // Check interface and implicit implementations do both call the delegated state var state = new PublicInterfaceTestClass { Id = 5, Name = "State" }; proxy.HibernateLazyInitializer.SetImplementation(state); - var pub = (IPublic) proxy; var ent = (PublicInterfaceTestClass) proxy; + IPublic pub = ent; Assert.That(pub.Id, Is.EqualTo(5), "IPublic.Id"); Assert.That(ent.Id, Is.EqualTo(5), "entity.Id"); Assert.That(pub.Name, Is.EqualTo("State"), "IPublic.Name"); @@ -276,8 +300,8 @@ public void VerifyProxyForClassWithExplicitInterface() Assert.That(proxy, Is.Not.Null); Assert.That(proxy, Is.InstanceOf()); Assert.That(proxy, Is.InstanceOf()); - - // Check interface and implicit implementations do both call the delegated state + + // Check explicit implementation IPublic state = new PublicExplicitInterfaceTestClass(); state.Id = 5; state.Name = "State"; @@ -285,7 +309,7 @@ public void VerifyProxyForClassWithExplicitInterface() var entity = (IPublic) proxy; Assert.That(entity.Id, Is.EqualTo(5), "Id"); Assert.That(entity.Name, Is.EqualTo("State"), "Name"); - + entity.Id = 10; entity.Name = "Test"; Assert.That(entity.Id, Is.EqualTo(10), "entity.Id"); @@ -297,6 +321,57 @@ public void VerifyProxyForClassWithExplicitInterface() #endif } + [Test] + public void VerifyProxyForClassWithExplicitInterfaceWithSameMembers() + { + var factory = new StaticProxyFactory(); + factory.PostInstantiate( + typeof(PublicExplicitInterfaceWithSameMembersTestClass).FullName, + typeof(PublicExplicitInterfaceWithSameMembersTestClass), + new HashSet {typeof(INHibernateProxy)}, + null, null, null, true); +#if NETFX + VerifyGeneratedAssembly( + () => + { +#endif + var proxy = factory.GetProxy(1, null); + Assert.That(proxy, Is.Not.Null); + Assert.That(proxy, Is.InstanceOf()); + Assert.That(proxy, Is.InstanceOf()); + + // Check interface and implicit implementations do both call the delegated state + var state = new PublicExplicitInterfaceWithSameMembersTestClass(); + IPublic pubState = state; + state.Id = 5; + state.Name = "State"; + pubState.Id = 10; + pubState.Name = "State2"; + proxy.HibernateLazyInitializer.SetImplementation(state); + var entity = (PublicExplicitInterfaceWithSameMembersTestClass) proxy; + IPublic pubEntity = entity; + Assert.That(entity.Id, Is.EqualTo(5), "Id member"); + Assert.That(entity.Name, Is.EqualTo("State"), "Name member"); + Assert.That(pubEntity.Id, Is.EqualTo(10), "Id from interface"); + Assert.That(pubEntity.Name, Is.EqualTo("State2"), "Name from interface"); + + entity.Id = 15; + entity.Name = "Test"; + pubEntity.Id = 20; + pubEntity.Name = "Test2"; + Assert.That(entity.Id, Is.EqualTo(15), "entity.Id"); + Assert.That(state.Id, Is.EqualTo(15), "state.Id"); + Assert.That(entity.Name, Is.EqualTo("Test"), "entity.Name"); + Assert.That(state.Name, Is.EqualTo("Test"), "state.Name"); + Assert.That(pubEntity.Id, Is.EqualTo(20), "pubEntity.Id"); + Assert.That(pubState.Id, Is.EqualTo(20), "pubState.Id"); + Assert.That(pubEntity.Name, Is.EqualTo("Test2"), "pubEntity.Name"); + Assert.That(pubState.Name, Is.EqualTo("Test2"), "pubState.Name"); +#if NETFX + }); +#endif + } + [Test] public void VerifyProxyForRefOutClass() { diff --git a/src/NHibernate/Proxy/ProxyBuilderHelper.cs b/src/NHibernate/Proxy/ProxyBuilderHelper.cs index 9fdb85266df..6457d3db24b 100644 --- a/src/NHibernate/Proxy/ProxyBuilderHelper.cs +++ b/src/NHibernate/Proxy/ProxyBuilderHelper.cs @@ -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,12 @@ internal static IEnumerable GetProxiableMethods(System.Type type) internal static IEnumerable GetProxiableMethods(System.Type type, IEnumerable interfaces) { - var proxiableMethods = - GetProxiableMethods(type) - .Concat(interfaces.SelectMany(i => i.GetMethods())) - .Distinct(); - + var proxiableMethods = new HashSet(GetProxiableMethods(type), new MethodInfoComparer(type)); + foreach (var interfaceMethod in interfaces.SelectMany(i => i.GetMethods())) + { + proxiableMethods.Add(interfaceMethod); + } + return proxiableMethods; } @@ -134,25 +136,48 @@ internal static MethodBuilder GetObjectDataMethodBuilder(TypeBuilder typeBuilder return methodBuilder; } + private static readonly System.Type[] _equalsParametersTypes = { typeof(object) }; + internal static MethodBuilder GenerateMethodSignature(string name, MethodInfo method, TypeBuilder typeBuilder) { - //TODO: Should we use attributes of base method? - var methodAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Virtual; + var explicitImplementation = method.DeclaringType.IsInterface; + if (explicitImplementation && + (typeBuilder.BaseType == typeof(object) || +#pragma warning disable 618 + typeBuilder.BaseType == typeof(ProxyDummy)) && +#pragma warning restore 618 + (method.Name == "Equals" && method.GetParameters().Select(p => p.ParameterType).SequenceEqual(_equalsParametersTypes) || + method.Name == "GetHashCode" && method.GetParameters().Length == 0)) + { + // 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 methodBuilder = typeBuilder.DefineMethod( - name, - methodAttributes, - CallingConventions.HasThis, - method.ReturnType, - parameters.ToArray(param => param.ParameterType)); + 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); var typeArgs = method.GetGenericArguments(); - if (typeArgs.Length > 0) { var typeNames = GenerateTypeNames(typeArgs.Length); @@ -253,5 +278,72 @@ private static string[] GenerateTypeNames(int count) return result; } + + /// + /// 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. + /// + private class MethodInfoComparer : IEqualityComparer + { + private readonly System.Type _baseType; + private readonly HashSet _baseTypeInterfaces; + private readonly Dictionary> _interfacesMap; + private readonly bool _baseIsObjectOrInterface; + + public MethodInfoComparer(System.Type baseType) + { + _baseType = baseType; + _baseIsObjectOrInterface = _baseType.IsInterface || _baseType == typeof(object); + _baseTypeInterfaces = new HashSet(baseType.GetInterfaces()); + _interfacesMap = new Dictionary>(_baseTypeInterfaces.Count); + } + + public bool Equals(MethodInfo x, MethodInfo y) + { + if (_baseIsObjectOrInterface) + return x == 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 (!_baseTypeInterfaces.Contains(interfaceMethod.DeclaringType)) + return false; + + if (!_interfacesMap.TryGetValue(interfaceMethod.DeclaringType, out var map)) + { + var interfaceMap = _baseType.GetInterfaceMap(interfaceMethod.DeclaringType); + map = new Dictionary(interfaceMap.InterfaceMethods.Length); + for (var i = 0; i < interfaceMap.InterfaceMethods.Length; i++) + { + map.Add(interfaceMap.InterfaceMethods[i], interfaceMap.TargetMethods[i]); + } + _interfacesMap.Add(interfaceMethod.DeclaringType, map); + } + 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(); + } + } } } From 50a54b7e614a795ed558e7ad086982d67bb8ff1d Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Mon, 4 Nov 2019 13:37:05 +1300 Subject: [PATCH 2/4] Extract IsEquals and IsGetHashCode methods to make CodeFactor happy --- src/NHibernate/Proxy/ProxyBuilderHelper.cs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/NHibernate/Proxy/ProxyBuilderHelper.cs b/src/NHibernate/Proxy/ProxyBuilderHelper.cs index 6457d3db24b..065ecc535c0 100644 --- a/src/NHibernate/Proxy/ProxyBuilderHelper.cs +++ b/src/NHibernate/Proxy/ProxyBuilderHelper.cs @@ -136,8 +136,6 @@ internal static MethodBuilder GetObjectDataMethodBuilder(TypeBuilder typeBuilder return methodBuilder; } - private static readonly System.Type[] _equalsParametersTypes = { typeof(object) }; - internal static MethodBuilder GenerateMethodSignature(string name, MethodInfo method, TypeBuilder typeBuilder) { var explicitImplementation = method.DeclaringType.IsInterface; @@ -146,8 +144,7 @@ internal static MethodBuilder GenerateMethodSignature(string name, MethodInfo me #pragma warning disable 618 typeBuilder.BaseType == typeof(ProxyDummy)) && #pragma warning restore 618 - (method.Name == "Equals" && method.GetParameters().Select(p => p.ParameterType).SequenceEqual(_equalsParametersTypes) || - method.Name == "GetHashCode" && method.GetParameters().Length == 0)) + (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. @@ -207,6 +204,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) From 47518ede4dd689df393d39c8470d95d45a53a665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Mon, 4 Nov 2019 09:11:45 +0100 Subject: [PATCH 3/4] Strengthen the tests --- .../StaticProxyFactoryFixture.cs | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs b/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs index c9183e757b6..da3c5cde84f 100644 --- a/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs +++ b/src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs @@ -24,6 +24,12 @@ internal interface IInternal { int Id { get; } } + + public interface IWithEqualsAndGetHashCode + { + bool Equals(object that); + int GetHashCode(); + } [Serializable] public class InternalInterfaceTestClass : IInternal @@ -242,6 +248,39 @@ public void VerifyProxyForClassWithAdditionalInterface() #endif } + [Test] + public void VerifyProxyForInterfaceWithEqualsAndGetHashCode() + { + var factory = new StaticProxyFactory(); + factory.PostInstantiate( + typeof(IWithEqualsAndGetHashCode).FullName, + typeof(object), + new HashSet {typeof(IWithEqualsAndGetHashCode), typeof(INHibernateProxy)}, + null, null, null, false); + +#if NETFX + VerifyGeneratedAssembly( + () => + { +#endif + var proxy = factory.GetProxy(1, null); + Assert.That(proxy, Is.Not.Null); + Assert.That(proxy, Is.InstanceOf()); + var proxyType = proxy.GetType(); + var proxyMap = proxyType.GetInterfaceMap(typeof(IWithEqualsAndGetHashCode)); + Assert.That( + proxyMap.TargetMethods, + Has.One.Property("Name").EqualTo("Equals").And.Property("IsPublic").EqualTo(true), + "Equals is not implicitly implemented"); + Assert.That( + proxyMap.TargetMethods, + Has.One.Property("Name").EqualTo("GetHashCode").And.Property("IsPublic").EqualTo(true), + "GetHashCode is not implicitly implemented"); +#if NETFX + }); +#endif + } + [Test] public void VerifyProxyForClassWithInterface() { @@ -261,6 +300,24 @@ public void VerifyProxyForClassWithInterface() Assert.That(proxy, Is.Not.Null); Assert.That(proxy, Is.InstanceOf()); Assert.That(proxy, Is.InstanceOf()); + var proxyType = proxy.GetType(); + var proxyMap = proxyType.GetInterfaceMap(typeof(IPublic)); + Assert.That( + proxyMap.TargetMethods, + Has.One.EqualTo(proxyType.GetProperty(nameof(PublicInterfaceTestClass.Name)).GetMethod), + "Name getter does not implement IPublic"); + Assert.That( + proxyMap.TargetMethods, + Has.One.EqualTo(proxyType.GetProperty(nameof(PublicInterfaceTestClass.Name)).SetMethod), + "Name setter does not implement IPublic"); + Assert.That( + proxyMap.TargetMethods, + Has.One.EqualTo(proxyType.GetProperty(nameof(PublicInterfaceTestClass.Id)).GetMethod), + "Id setter does not implement IPublic"); + Assert.That( + proxyMap.TargetMethods, + Has.One.EqualTo(proxyType.GetProperty(nameof(PublicInterfaceTestClass.Id)).SetMethod), + "Id setter does not implement IPublic"); // Check interface and implicit implementations do both call the delegated state var state = new PublicInterfaceTestClass { Id = 5, Name = "State" }; @@ -300,6 +357,11 @@ public void VerifyProxyForClassWithExplicitInterface() Assert.That(proxy, Is.Not.Null); Assert.That(proxy, Is.InstanceOf()); Assert.That(proxy, Is.InstanceOf()); + var proxyType = proxy.GetType(); + Assert.That(proxyType.GetMethod($"get_{nameof(IPublic.Name)}"), Is.Null, "get Name is implicitly implemented"); + Assert.That(proxyType.GetMethod($"set_{nameof(IPublic.Name)}"), Is.Null, "set Name is implicitly implemented"); + Assert.That(proxyType.GetMethod($"get_{nameof(IPublic.Id)}"), Is.Null, "get Id is implicitly implemented"); + Assert.That(proxyType.GetMethod($"set_{nameof(IPublic.Id)}"), Is.Null, "set Id is implicitly implemented"); // Check explicit implementation IPublic state = new PublicExplicitInterfaceTestClass(); @@ -339,6 +401,24 @@ public void VerifyProxyForClassWithExplicitInterfaceWithSameMembers() Assert.That(proxy, Is.Not.Null); Assert.That(proxy, Is.InstanceOf()); Assert.That(proxy, Is.InstanceOf()); + var proxyType = proxy.GetType(); + var proxyMap = proxyType.GetInterfaceMap(typeof(IPublic)); + Assert.That( + proxyMap.TargetMethods, + Has.None.EqualTo(proxyType.GetProperty(nameof(PublicExplicitInterfaceWithSameMembersTestClass.Name)).GetMethod), + "class Name getter does implement IPublic"); + Assert.That( + proxyMap.TargetMethods, + Has.None.EqualTo(proxyType.GetProperty(nameof(PublicExplicitInterfaceWithSameMembersTestClass.Name)).SetMethod), + "class Name setter does implement IPublic"); + Assert.That( + proxyMap.TargetMethods, + Has.None.EqualTo(proxyType.GetProperty(nameof(PublicExplicitInterfaceWithSameMembersTestClass.Id)).GetMethod), + "class Id setter does implement IPublic"); + Assert.That( + proxyMap.TargetMethods, + Has.None.EqualTo(proxyType.GetProperty(nameof(PublicExplicitInterfaceWithSameMembersTestClass.Id)).SetMethod), + "class Id setter does implement IPublic"); // Check interface and implicit implementations do both call the delegated state var state = new PublicExplicitInterfaceWithSameMembersTestClass(); From 5940ff97210938d59377023f6871e8b93cccebb7 Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Thu, 7 Nov 2019 01:00:27 +1300 Subject: [PATCH 4/4] Simplify MethodInfoComparer --- src/NHibernate/Proxy/ProxyBuilderHelper.cs | 49 ++++++++++++---------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/NHibernate/Proxy/ProxyBuilderHelper.cs b/src/NHibernate/Proxy/ProxyBuilderHelper.cs index 065ecc535c0..a9868383878 100644 --- a/src/NHibernate/Proxy/ProxyBuilderHelper.cs +++ b/src/NHibernate/Proxy/ProxyBuilderHelper.cs @@ -102,6 +102,13 @@ internal static IEnumerable GetProxiableMethods(System.Type type) internal static IEnumerable GetProxiableMethods(System.Type type, IEnumerable 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(GetProxiableMethods(type), new MethodInfoComparer(type)); foreach (var interfaceMethod in interfaces.SelectMany(i => i.GetMethods())) { @@ -295,24 +302,33 @@ private static string[] GenerateTypeNames(int count) /// private class MethodInfoComparer : IEqualityComparer { - private readonly System.Type _baseType; - private readonly HashSet _baseTypeInterfaces; private readonly Dictionary> _interfacesMap; - private readonly bool _baseIsObjectOrInterface; public MethodInfoComparer(System.Type baseType) { - _baseType = baseType; - _baseIsObjectOrInterface = _baseType.IsInterface || _baseType == typeof(object); - _baseTypeInterfaces = new HashSet(baseType.GetInterfaces()); - _interfacesMap = new Dictionary>(_baseTypeInterfaces.Count); + _interfacesMap = BuildInterfacesMap(baseType); } - public bool Equals(MethodInfo x, MethodInfo y) + private static Dictionary> BuildInterfacesMap(System.Type type) { - if (_baseIsObjectOrInterface) - return x == y; + return type.GetInterfaces() + .Distinct() + .ToDictionary(i => i, i => ToDictionary(type.GetInterfaceMap(i))); + } + + private static Dictionary ToDictionary(InterfaceMapping interfaceMap) + { + var map = new Dictionary(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) @@ -327,22 +343,13 @@ public bool Equals(MethodInfo x, MethodInfo y) // 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 (!_baseTypeInterfaces.Contains(interfaceMethod.DeclaringType)) + if (!_interfacesMap.TryGetValue(interfaceMethod.DeclaringType, out var map)) return false; - if (!_interfacesMap.TryGetValue(interfaceMethod.DeclaringType, out var map)) - { - var interfaceMap = _baseType.GetInterfaceMap(interfaceMethod.DeclaringType); - map = new Dictionary(interfaceMap.InterfaceMethods.Length); - for (var i = 0; i < interfaceMap.InterfaceMethods.Length; i++) - { - map.Add(interfaceMap.InterfaceMethods[i], interfaceMap.TargetMethods[i]); - } - _interfacesMap.Add(interfaceMethod.DeclaringType, map); - } var baseMethod = x.DeclaringType.IsInterface ? y : x; return map[interfaceMethod] == baseMethod; }