Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Unreleased

Bugfixes:
- Prevent member name collision when proxy implements same generic interface more than twice (@stakx, #88)
- Fix incorrect replication (reversed order) of custom modifiers (modopts and modreqs) on the CLR, does not work yet on Mono (@stakx, #277)
- Fix COM interface proxy error case throwing exceptions trying to release null pointer from QueryInterface (@stakx, #281)

Expand Down
46 changes: 46 additions & 0 deletions src/Castle.Core.Tests/BasicInterfaceProxyTestCase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,52 @@ public void Should_implement_explicitly_duplicate_interface_members()
Assert.IsNotNull(method2);
}

[Test]
public void Should_choose_noncolliding_member_names_when_implementing_same_generic_interface_several_times()
{
var boolInterfaceType = typeof(IGenericWithNonGenericMethod<bool>);
var intInterfaceType = typeof(IGenericWithNonGenericMethod<int>);
var nestedGenericBoolInterfaceType = typeof(IGenericWithNonGenericMethod<IGenericWithNonGenericMethod<bool>>);

var proxy = generator.CreateInterfaceProxyWithoutTarget(
boolInterfaceType,
new[] { intInterfaceType, nestedGenericBoolInterfaceType });

var type = proxy.GetType().GetTypeInfo();

var boolMethod = type.GetRuntimeInterfaceMap(boolInterfaceType).TargetMethods[0];
Assert.AreEqual("DoSomething", boolMethod.Name);

var intMethod = type.GetRuntimeInterfaceMap(intInterfaceType).TargetMethods[0];
Assert.AreEqual("IGenericWithNonGenericMethod`1[Int32].DoSomething", intMethod.Name);

var nestedGenericBoolMethod = type.GetRuntimeInterfaceMap(nestedGenericBoolInterfaceType).TargetMethods[0];
Assert.AreEqual("IGenericWithNonGenericMethod`1[IGenericWithNonGenericMethod`1[Boolean]].DoSomething", nestedGenericBoolMethod.Name);
}

[Test]
public void Should_choose_noncolliding_member_names_when_implementing_same_generic_interface_with_two_type_args_several_times()
{
var boolIntInterfaceType = typeof(IGenericWithNonGenericMethod<bool, int>);
var intBoolInterfaceType = typeof(IGenericWithNonGenericMethod<int, bool>);
var intNestedGenericBoolInterfaceType = typeof(IGenericWithNonGenericMethod<int, IGenericWithNonGenericMethod<bool>>);

var proxy = generator.CreateInterfaceProxyWithoutTarget(
boolIntInterfaceType,
new[] { intBoolInterfaceType, intNestedGenericBoolInterfaceType });

var type = proxy.GetType().GetTypeInfo();

var boolIntMethod = type.GetRuntimeInterfaceMap(boolIntInterfaceType).TargetMethods[0];
Assert.AreEqual("DoSomething", boolIntMethod.Name);

var intBoolMethod = type.GetRuntimeInterfaceMap(intBoolInterfaceType).TargetMethods[0];
Assert.AreEqual("IGenericWithNonGenericMethod`2[Int32,Boolean].DoSomething", intBoolMethod.Name);

var intNestedGenericBoolMethod = type.GetRuntimeInterfaceMap(intNestedGenericBoolInterfaceType).TargetMethods[0];
Assert.AreEqual("IGenericWithNonGenericMethod`2[Int32,IGenericWithNonGenericMethod`1[Boolean]].DoSomething", intNestedGenericBoolMethod.Name);
}

private ParameterInfo[] GetMyTestMethodParams(Type type)
{
MethodInfo methodInfo = type.GetMethod("MyTestMethod", BindingFlags.Instance | BindingFlags.Public);
Expand Down
26 changes: 26 additions & 0 deletions src/Castle.Core.Tests/Interfaces/IGenericWithNonGenericMethod.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2004-2010 Castle Project - http://www.castleproject.org/
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

namespace Castle.DynamicProxy.Tests.Interfaces
{
public interface IGenericWithNonGenericMethod<T>
{
void DoSomething();
}

public interface IGenericWithNonGenericMethod<T1, T2>
{
void DoSomething();
}
}
26 changes: 25 additions & 1 deletion src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace Castle.DynamicProxy.Generators
using System;
using System.Diagnostics;
using System.Reflection;
using System.Text;

using Castle.DynamicProxy.Internal;

Expand Down Expand Up @@ -101,7 +102,30 @@ internal override void SwitchToExplicitImplementation()
Attributes |= MethodAttributes.SpecialName;
}

name = string.Format("{0}.{1}", Method.DeclaringType.Name, Method.Name);
var nameBuilder = new StringBuilder();
AppendTypeNameTo(nameBuilder, Method.DeclaringType);
nameBuilder.Append('.');
nameBuilder.Append(Method.Name);
name = nameBuilder.ToString();
}

private static void AppendTypeNameTo(StringBuilder stringBuilder, Type type)
{
stringBuilder.Append(type.Name);
if (type.GetTypeInfo().IsGenericType)
{
stringBuilder.Append('[');
var genericTypeArguments = type.GetGenericArguments();
for (int i = 0, n = genericTypeArguments.Length; i < n; ++i)
Copy link

@ghost ghost Jul 4, 2017

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just basing that on a rudimentary find of usages for MetaMethod.

image

Copy link
Member Author

@stakx stakx Jul 4, 2017

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.

Copy link

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?

Copy link
Member

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.

{
if (i > 0)
{
stringBuilder.Append(',');
}
AppendTypeNameTo(stringBuilder, genericTypeArguments[i]);
}
stringBuilder.Append(']');
}
}

private MethodAttributes ObtainAttributes()
Expand Down