Replicate custom attributes on constructor parameters #355

stakx · 2018-05-02T14:23:44Z

This is an attempt at resolving #341.

The gist of this PR is to change code in the following location where custom attribute reproduction happens (currently only for ParamArrayAttribute):

Core/src/Castle.Core/DynamicProxy/Generators/BaseProxyGenerator.cs

Lines 240 to 249 in 980752c

    
           if (baseConstructorParams != null && baseConstructorParams.Length != 0) 
        
           { 
        
           	var last = baseConstructorParams.Last(); 
        
           	if (last.ParameterType.GetTypeInfo().IsArray && last.IsDefined(typeof(ParamArrayAttribute))) 
        
           	{ 
        
           		var parameter = constructor.ConstructorBuilder.DefineParameter(args.Length, ParameterAttributes.None, last.Name); 
        
           		var info = AttributeUtil.CreateInfo<ParamArrayAttribute>(); 
        
           		parameter.SetCustomAttribute(info.Builder); 
        
           	} 
        
           }

This gets changed to replicate all custom attributes of all parameters.

There's a catch, however: Because constructors in the generated proxy type have different signatures (additional parameters e.g. for the interceptor array), the constructors' signatures do not match with those in the base class. This means that custom attribute inheritance would never work with these constructor parameters, which is why we need to disregard [AttributeTargets(Inherited = true)] and reproduce even those attributes.

Can you think of any scenario(s) where replicating additional, "inheritable" custom attributes on ctors could cause negative side effects in downstream libraries?

Finally, take note that AFAIK even the updated code does not yet reproduce custom modifiers (modopts, modreqs) because that would have to happen in a different code location and because that's not what #341 is about. Eventually, we should reproduce custom modifiers as well, otherwise we might get confronted with something like #339, but for ctors.

stakx

Just a couple of explanatory notes.

src/Castle.Core/DynamicProxy/Internal/AttributeUtil.cs

stakx · 2018-05-02T14:31:14Z

src/Castle.Core/DynamicProxy/Generators/BaseProxyGenerator.cs

-					var info = AttributeUtil.CreateInfo<ParamArrayAttribute>();
-					parameter.SetCustomAttribute(info.Builder);
+					var parameterBuilder = constructor.ConstructorBuilder.DefineParameter(offset + i, baseConstructorParams[i].Attributes, baseConstructorParams[i].Name);
+					foreach (var attribute in baseConstructorParams[i].GetCustomAttributesToBeReproducedOnProxyCtor())


For regular methods, we'd call baseMethodParams[i].GetNonInheritableAttributes(), but we cannot do that here because no attributes would ever be returned by it due to the non-matching ctor signatures. We therefore need a new method that does all the work AttributeUtil.GetNonInheritableAttributes() does, except for excluding inheritable custom attributes.

stakx · 2018-05-02T14:31:58Z

src/Castle.Core/DynamicProxy/Internal/AttributeUtil.cs

@@ -153,7 +153,7 @@ public static IEnumerable<CustomAttributeInfo> GetNonInheritableAttributes(this
 #else
 				var attributeType = attribute.AttributeType;
 #endif
-				if (ShouldSkipAttributeReplication(attributeType))
+				if (ShouldSkipAttributeReplication(attributeType, ignoreInheritance: false))


ignoreInheritance: false will result in the same behavior that was there before this parameter was added.

jonorossi · 2018-05-02T14:46:30Z

src/Castle.Core/DynamicProxy/Internal/AttributeUtil.cs

@@ -204,7 +224,7 @@ public static IEnumerable<CustomAttributeInfo> GetNonInheritableAttributes(this
 				var attributeType = attribute.AttributeType;
 #endif

-				if (ShouldSkipAttributeReplication(attributeType))
+				if (ShouldSkipAttributeReplication(attributeType, ignoreInheritance))


Could you use (parameter.Member as ConstructorInfo) != null rather than those new methods.

Not sure why I wrote that inheritance could be the problem in the issue, because obviously constructors are not part of an class inheritance contract and must be redefined, but maybe I was thinking it could be this code excluding them because they are marked inheritable 😉.

Could you please clarify which new methods you'd like to avoid and where a condition such as parameter.Member is ConstructorInfo should go?

I'll quickly lay out how I ended up with my proposed changes:

Your check by itself would not be sufficient. For a ctor parameter attribute to get replicated, it must be of a public type, and it must not be in AttributesToAvoidReplicating. Since ShouldSkipAttributeReplication already performs these checks and has a fitting name, I thought I'd reuse it.

But to prevent ShouldSkipAttributeReplication from doing too much for ctor parameters, it needs the additional parameter to decide when to bypass checking AttributeUsageAttribute.Inherited. It has no access to a ParameterInfo, so parameter.Member is ConstructorInfo cannot go in here.

parameter.Member is ConstructorInfo can go in the original GetNonInheritableAttributes and so avoid the two new forwarding methods. However, doing this makes GetNonInheritableAttributes return inheritable attributes in some circumstances, which might seem like a bug, given its name.

Yep, I would like to avoid the 2 new forwarding methods as they seem unnecessary. I was thinking you could determine if you are handling a constructor in GetNonInheritableAttributes, pretty much thinking:

bool ignoreInheritance = (parameter.Member is ConstructorInfo);

Since constructors in .NET are not inherited (not that you'd want them to be), I don't see any problem returning all attributes via this named method because no constructor parameters can ever be inherited.

Ah, gotcha. Makes sense now. Done.

CHANGELOG.md

src/Castle.Core/DynamicProxy/Internal/AttributeUtil.cs

This takes the unit test from castleproject#345 and applies some necessary changes: * Bring it in line with the recently changed directory and namespace structure of the test project. * The LINQ query fails with a `NullReferenceException` on .NET Core. Therefore rewrite the test such that this test bug doesn't surface. It's still the same test in spirit.

The only custom attribute that DynamicProxy currently reproduces on a proxy class' parameters is [ParamArray] (if present on the last parameter of the base class' ctor). This commit changes this so DynamicProxy replicates *all* non- inheritable custom attributes on ctor parameters. Regarding "non-inheritable": Because proxy ctors have a different signature than the proxied class' ctors, parameters can never inherit custom attributes from the base ctor. This means that inheritability of custom attributes needs to be ignored in the case of proxy ctors. Also some minor cleanups in `AttributeUtil` while we're looking at it.

stakx · 2018-05-14T13:04:27Z

Squashed a little and rebased on top of current master, should be good to go.

jonorossi · 2018-05-14T13:17:44Z

All good, merging.

src/Castle.Core.Tests/DynamicProxy.Tests/ConstructorWithAttributesOnParametersTestCase.cs

stakx commented May 2, 2018

View reviewed changes

stakx mentioned this pull request May 2, 2018

Release 4.3.0 #354

Closed

jonorossi reviewed May 2, 2018

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

stakx force-pushed the ctor-params-attributes branch from 87e9b22 to 391e8a3 Compare May 2, 2018 16:29

stakx mentioned this pull request May 2, 2018

Added unit test for attributes on constructor parameters #345

Closed

jonorossi reviewed May 13, 2018

View reviewed changes

src/Castle.Core/DynamicProxy/Internal/AttributeUtil.cs Outdated Show resolved Hide resolved

jonorossi reviewed May 13, 2018

View reviewed changes

src/Castle.Core/DynamicProxy/Internal/AttributeUtil.cs Outdated Show resolved Hide resolved

stakx force-pushed the ctor-params-attributes branch from 4370cdc to befc287 Compare May 13, 2018 17:25

gliljas and others added 3 commits May 14, 2018 14:41

Update the changelog

38b1357

stakx force-pushed the ctor-params-attributes branch from befc287 to 38b1357 Compare May 14, 2018 12:43

jonorossi merged commit 1bc7fae into castleproject:master May 14, 2018

jonorossi added this to the vNext milestone May 14, 2018

stakx deleted the ctor-params-attributes branch May 14, 2018 13:18

jonorossi mentioned this pull request May 14, 2018

Class proxies lose custom attributes on constructor parameters #341

Closed

gliljas reviewed Jun 4, 2018

View reviewed changes

src/Castle.Core.Tests/DynamicProxy.Tests/ConstructorWithAttributesOnParametersTestCase.cs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replicate custom attributes on constructor parameters #355

Replicate custom attributes on constructor parameters #355

stakx commented May 2, 2018 •

edited

Loading

stakx left a comment

stakx May 2, 2018

stakx May 2, 2018

jonorossi May 2, 2018

stakx May 2, 2018 •

edited

Loading

jonorossi May 13, 2018

stakx May 13, 2018

stakx commented May 14, 2018

jonorossi commented May 14, 2018

	if (baseConstructorParams != null && baseConstructorParams.Length != 0)
	{
	var last = baseConstructorParams.Last();
	if (last.ParameterType.GetTypeInfo().IsArray && last.IsDefined(typeof(ParamArrayAttribute)))
	{
	var parameter = constructor.ConstructorBuilder.DefineParameter(args.Length, ParameterAttributes.None, last.Name);
	var info = AttributeUtil.CreateInfo<ParamArrayAttribute>();
	parameter.SetCustomAttribute(info.Builder);
	}
	}

Replicate custom attributes on constructor parameters #355

Replicate custom attributes on constructor parameters #355

Conversation

stakx commented May 2, 2018 • edited Loading

stakx left a comment

Choose a reason for hiding this comment

stakx May 2, 2018

Choose a reason for hiding this comment

stakx May 2, 2018

Choose a reason for hiding this comment

jonorossi May 2, 2018

Choose a reason for hiding this comment

stakx May 2, 2018 • edited Loading

Choose a reason for hiding this comment

jonorossi May 13, 2018

Choose a reason for hiding this comment

stakx May 13, 2018

Choose a reason for hiding this comment

stakx commented May 14, 2018

jonorossi commented May 14, 2018

stakx commented May 2, 2018 •

edited

Loading

stakx May 2, 2018 •

edited

Loading