Fix duplicated methods generated in proxies #2264
Conversation
| 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 Distinct. But its contract does not guarantee we will takes the base type methods rather than "duplicated" methods from the interfaces. (Its current implementation will keep the methods from the base type, but we should not rely on this.)
By explicitly adding the base methods to the hashset first, it is guaranteed that this is dups from interfaces which will be ignored.
| 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 InterfaceWithEqualsGethashcodeTests test to fail.
| method.ReturnType, | ||
| parameters.ToArray(param => param.ParameterType)); | ||
| if (explicitImplementation) | ||
| methodBuilder.SetImplementationFlags(MethodImplAttributes.Managed | MethodImplAttributes.IL); |
There was a problem hiding this comment.
Copied from NHibernateProxyBuilder.ImplementGetLazyInitializer explicit implementation, I do not actually know if it is needed.
| /// 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.
|
Your change seems to me a good change in principle, but I have not yet thoroughly reviewed it, just to let you know. |
I've just moved the logic out of the comparer. |
|
Would it be fair to say that the |
|
Yes, that is it. |
Indeed, I agree with the change. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fixes #2085