-
Notifications
You must be signed in to change notification settings - Fork 466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent member name collision when proxy implements same generic interface more than twice #285
Prevent member name collision when proxy implements same generic interface more than twice #285
Conversation
91a5ea2
to
77cfec0
Compare
@@ -213,10 +213,37 @@ public void Should_implement_explicitly_duplicate_interface_members() | |||
MethodInfo method = type.GetMethod("Foo", BindingFlags.Instance | BindingFlags.Public); | |||
Assert.IsNotNull(method); | |||
Assert.AreSame(method, type.GetTypeInfo().GetRuntimeInterfaceMap(typeof(IIdenticalOne)).TargetMethods[0]); | |||
MethodInfo method2 = type.GetMethod("IIdenticalTwo.Foo", BindingFlags.Instance | BindingFlags.Public); | |||
MethodInfo method2 = type.GetMethod($"{typeof(IIdenticalTwo).FullName}.Foo", BindingFlags.Instance | BindingFlags.Public); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I chose not to hardcode the full type name here is because it includes things like DLL version numbers and public keys, which I wasn't sure whether they are the same on Mono and the CLR, and they're irrelevant to the test anyway.
As you suspected, I'm not a fan of those member names. It appears they also won't be CLS compliant with the spaces: https://docs.microsoft.com/en-us/dotnet/standard/language-independence-and-language-independent-components#naming, I'm also not sure dots would be compliant but they don't seem to be mentioned. However, I'm not sure CLS compiliance really matters since no one is going to manually call them from language compiled code. I'd hate to see a stacktrace with them though, would be awfully confusing. What do the CodeDom identifiers look like? I'd be happy with using just the parameter short names (i.e. not namespace or assembly-qualified) which is what is already done for the first bit (e.g.
The docs for
Unless I missed it I can't see any property in the BCL on a type to get the unqualified name as well as its generic parameters, but should be easy to implement. |
I only mentioned this as a potential route to take, I haven't actually tried it out just yet. The general idea is that CodeDom can be used to create type names that look more like C# rather than IL (e.g.,
I like that suggestion. The resulting member names won't be guaranteed to be unique (e.g. if you use two types Shall I go ahead and change the PR accordingly? |
Completely in agreement, not even sure if .NET Core has CodeDom.
Agreed, it'll be exactly like the base type right now, you'd be silly to inherit/implement two
Yep, don't forget the changelog. Thanks. |
When a proxy is made to implement the same generic interface more than twice, and that generic interface contains a method that does not use any generic type parameters, this can be result in a name collision. This would happen e.g. with a proxy implementing `IObserver<T>` three times. DynamicProxy does check for member name collisions, and when such a one is detected, it switches to explicit interface implementation and prefixes the member name with the generic interface type name. For the above example, this will add the following methods to the proxy: * OnCompleted * IObserver`1.OnCompleted * IObserver`1.OnCompleted It is clearly not sufficient to exclude the concrete type arguments and use only the number of type parameters. This commit changes the naming of explicitly implemented interface methods such that they are unique by including the generic type arguments. For example: * OnCompleted * IObserver`1[Boolean].OnCompleted * IObserver`1[Int32].OnCompleted
77cfec0
to
55192ae
Compare
I think something might still be missing here: This PR only takes care of method name collisions, but does nothing about events and properties. I'll try to see if a similar change needs to be made to the other types overriding |
{ | ||
stringBuilder.Append('['); | ||
var genericTypeArguments = type.GetGenericArguments(); | ||
for (int i = 0, n = genericTypeArguments.Length; i < n; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method might introduce a performance hit on generation. It is recursive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. The recursion depth will be proportional to the nesting depth of the (generic) type for which it generates a name. Non-generic types, however, will not trigger any recursion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fixes the original problem, but should we flag performance as a feature and make it part of the proxy options with a sensible default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the performance will be affected. This only gets run when SwitchToExplicitImplementation
is called once per MetaMethod
and it is then cached in its name
field. The StringBuilder
will perform pretty similar to the existing string.Format
and users are rarely going to have more than a single level of generic nesting, and even then the cost here is negligible compared to the actual type generation or other work DP does.
Looks good so far, just need to address properties and events. |
5047f76
to
b3ea5a3
Compare
@jonorossi - Done. I've extracted the logic for creating explicit implementation member names into a separate helper class This opens up an opportunity to minimize unnecessary heap allocations. @fir3pho3nixx, like you showed in your review before, generating names for explicitly implemented members is possibly on a hot code path, so it should be reasonably efficient. It would be silly, for instance, having to instantiate 3 |
@fir3pho3nixx - Thanks for checking up on my use of If my understanding of the TPL and I am almost certain that won't be an issue here, for the simple reason that our code in question is entirely synchronous. It should be impossible that code execution would switch from one thread to another all of a sudden and thus end up "stealing" the original thread's Unless I'm mistaken with any or all of the above, |
@stakx - I agree. It is all sync so it should be fine. |
Agreed. In general
What concerns me is that With that said the .NET Framework does actually have an internal In this case it appears there is enough evidence that this will likely improve performance by some margin, but the performance improvement is currently unknown and likely negligible compared to the reflection calls to get the type names along with generic types; and this is all small compared to the reflection emit time. It is best to avoid premature optimization (especially premature micro-optimisations) without measuring/benchmarking especially when the readability is hindered. For example, you could add a fast path to Let me know your thoughts, and if you disagree. |
@jonorossi: Your response makes perfect sense to me. I've heard similar things about the relatively large overhead of My overall impression is that using |
01d4085
to
d5c941f
Compare
Thanks for making this thread interesting. Did not know threadstatic performance issue existed(or was fixed), just generally avoided it like herpes altogether :) Learn't |
@stakx thanks for making the change, and for getting this fix in. Merging. |
After #89, this is a second attempt at fixing #88.
When a proxy is made to implement the same generic interface more than twice, and that generic interface contains a method that does not use any generic type parameters, this can result in a member name collision. This would happen e.g. with a proxy implementing
IObserver<T>
three times.DynamicProxy already checks for member name collisions, and when it detects one, it switches to explicit interface implementation and prefixes member names with the interface's type name. For the above example, this will add the following methods to the proxy:
Because the actual type arguments are omitted and only their count is included, the current collision checks are not sufficient. This commit makes explicitly implemented methods' names unique by including the full type names of the generic type arguments; for instance:
(I've omitted some detail above for brevity, using
…
.)This is about the simplest possible solution to #88, but I'm not sure if you are OK with these kinds of member names. They are unique, but also long and ugly (which could be partly mitigated by using
System.CodeDom
to produce more C#-like type names), and they contain whitespace and other special characters; I have no idea whether these are always safe for proxy consumers.Feel free to request modifications or to close this PR if it's not acceptable.