ParamArrayAttribute lost in proxy

[afscrome]

The only way to detect through reflection if a parameter is a parameter array is by checking if the parameter has the ParamArrayAttribute attribute defined on it. When creating a proxy object, the ParamArrayAttribute is lost on the method & indexer parameter arrays.

This can be demonstrated in the following code - I'd expect both assertions to pass, but they fail.

public abstract class Base
{
    public abstract void Method(params int[] args);
    public abstract object this[params int[] args] { get; set; }
}

[Test]
public void ProxyParameterArgumentTest()
{
    var generator = new ProxyGenerator();
    var proxy = generator.CreateClassProxy<Base>();

    var proxyType = proxy.GetType();

    var methodParam = proxyType.GetMethod("Method").GetParameters()[0];
    var indexParam = proxyType.GetProperty("Item").GetIndexParameters()[0];

    Assert.IsTrue(methodParam.IsDefined(typeof(ParamArrayAttribute), false), "Method arg is not params array");
    Assert.IsTrue(indexParam.IsDefined(typeof(ParamArrayAttribute), false), "Index arg is not params array");
}

[jonorossi]

@afscrome thanks for the detailed report. We've got some support for ParamArrayAttribute, however it looks like we've got a defect. Feel free to submit a pull request.

I've assigned this defect to v4.0 milestone.

[drieseng]

ProxyGenerator indeed does not "replicate" custom attributes for which AttributeUsageAttribute.Inherited is true. This check is performed in Castle.DynamicProxy.Internal.AttributeUtil.ShouldSkipAttributeReplication(Type).

Can you elaborate on why this is an issue for you ?

Note that if you use the following asserts, then the first will pass and the second still fails:

Assert.IsNotNull(Attribute.GetCustomAttribute(methodParam, typeof(ParamArrayAttribute), true));
Assert.IsNotNull(Attribute.GetCustomAttribute(indexParam, typeof(ParamArrayAttribute), true));

I'm inclined to think that's a bug in .NET.

[drieseng]

FYI, I submitted a bug against .NET Framework 4.6 (#2352748) and .NET Core (#6078) for the fact that inherited custom attributes are not returned for indexer arguments.

[afscrome]

Thanks for looking at this @drieseng.

My scenario is that I have a custom DLR language, part of which involves some custom overload resolution logic. I was using Moq in testing (which in turn uses DynamicProxy), and was getting some unexpected results because the overload resolution wasn't picking up the params array attribute ( https://github.com/afscrome/IronVelocity/blob/master/IronVelocity.Tests/Binders/SetIndexBinderTests.cs#L106 if you're interseted ). Looks like my problem is a combination of not doing the GetCustomAttribute check with inheritance, and the bugs you submitted.

That said, it does look like ParamArrayAttribute has some special treatment by the compiler, being automatically inserted into derived types even if it's not explicitly specified. Consider the following example:

    public abstract class Base
    {
        public abstract string Run(params int[] args);
    }

    public class Sub : Base
    {
        public override string Run(int[] args)
        {
            return null;
        }
    }

Even though the sub class doesn't have the params array explicitly attribute specified (through the params modifier), it still has the ParamArray attribute aded if you look in the IL code.

.method public hidebysig virtual instance string 
        Run(int32[] args) cil managed
{
  .param [1]
  .custom instance void [mscorlib]System.ParamArrayAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       7 (0x7)
  .maxstack  1
  .locals init ([0] string V_0)
  IL_0000:  nop
  IL_0001:  ldnull
  IL_0002:  stloc.0
  IL_0003:  br.s       IL_0005
  IL_0005:  ldloc.0
  IL_0006:  ret
} // end of method Sub::Run

Given that the C# compiler is replicating this attribute, should dynamic proxy also replicate it?

[drieseng]

Yeah, I noticed that myself (and doublechecked that it still applies to the Roslyn-based compilers).

To resolve your problem, a possible solution would be to only "skip replication" for custom attributes
that are inherited and allow multiple instances.

This would change line 237 to 244 of AttributeUtil.cs like this:

237             var attrs = attribute.GetCustomAttributes(typeof(AttributeUsageAttribute), true); 
238 
239             if (attrs.Length != 0) 
240             { 
241                 var usage = (AttributeUsageAttribute)attrs[0]; 
242 
243                 return usage.Inherited && usage.AllowMultiple;
244             }

The most important part is to continue skipping replication for custom attributes that allow to be defined multiple times.

@jonorossi:
Would a PR in that sense be accepted (accompanied by tests of course) ?

[jonorossi]

Sorry for the delay in replying, but isn't AllowMultiple meant to indicate whether a specific element is allowed to define the attribute multiple times, i.e. AllowMultiple=false doesn't mean you can't define it if the parent already has defined it. If ParamArrayAttribute is handled special in this case by the compiler I'd prefer if we did the same as we already do for this attribute in the DP codebase. It's 3am here so I might be missing something but does that make sense?

[drieseng]

If AllowMultiple is true, then redefining an attribute on a proxy would cause an "inheritable" attribute to be returned twice when invoking Attribute.GetCustomAttribute(...) with inherit set to true - once for the specified type or member and once for its parent. This is surely not something we want.

If however, we do not replicate an attribute if its both Inherited and AllowMultiple then we'd have the best of both worlds.

Special casing ParamArrayAttribute would also solve this very specific issue, but any code that uses Attribute.GetCustomAttribute(...) with inherit set to false would still end up empty-handed when invoked on a proxy type or member for any other attributes that have Inherited=true and AllowMultiple=false.

Here's a test class that shows a failure in the second assert in MethodOnProxyShouldRedefineInheritedAttributesThatDoNotAllowToBeDefinedMultipleTimes() when my fix is not applied:

namespace CastleTests.DynamicProxy.Tests
{
    using System;
    using System.Linq;

    using Castle.DynamicProxy;

    using NUnit.Framework;

    [AttributeUsage(AttributeTargets.All, AllowMultiple = false, Inherited = true)]
    public class InheritableNotAllowMultipleMyAttribute : Attribute
    {
    }

    [AttributeUsage(AttributeTargets.All, AllowMultiple = true, Inherited = true)]
    public class InheritableAllowMultipleMyAttribute : Attribute
    {
    }

    [TestFixture]
    public class ClassWithMethodWithParameterAttributes
    {
        private Base proxy; 

        [SetUp]
        public void SetUp()
        {
            var generator = new ProxyGenerator();
            proxy = generator.CreateClassProxy<Base>();
        }

        public abstract class Base
        {
            public abstract void Method([InheritableAllowMultipleMyAttribute] object a, [InheritableNotAllowMultipleMyAttribute] params int[] b);
        }

        [Test]
        public void MethodOnProxyShouldRedefineInheritedAttributesThatDoNotAllowToBeDefinedMultipleTimes()
        {

            var baseType = typeof(Base);
            var proxyType = proxy.GetType();

            var baseMethodParamB = baseType.GetMethod("Method").GetParameters().Single(p => p.Name == "b");
            var proxyMethodParamB = proxyType.GetMethod("Method").GetParameters().Single(p => p.Name == "b");

            Assert.IsNotNull(Attribute.GetCustomAttribute(baseMethodParamB, typeof(InheritableNotAllowMultipleMyAttribute), false));
            Assert.IsNotNull(Attribute.GetCustomAttribute(proxyMethodParamB, typeof(InheritableNotAllowMultipleMyAttribute), false));

            Assert.IsNotNull(Attribute.GetCustomAttribute(baseMethodParamB, typeof(InheritableNotAllowMultipleMyAttribute), true));
            Assert.IsNotNull(Attribute.GetCustomAttribute(proxyMethodParamB, typeof(InheritableNotAllowMultipleMyAttribute), true));
        }

        /// <summary>
        /// We cannot redefine attributes that are both inheritable and allow to be defined multiple times
        /// as that would could these attributes to be returned twice when invoking the <c>Attribute.GetCustomAttribute(...)</c>
        /// overloads with <c>inherit</c> set to <c>true</c>.
        /// </summary>
        [Test]
        public void MethodOnProxyShouldNotRedefineInheritedAttributesThatAllowToBeDefinedMultipleTimes()
        {
            var baseType = typeof(Base);
            var proxyType = proxy.GetType();

            var baseMethodParamA = baseType.GetMethod("Method").GetParameters().Single(p => p.Name == "a");
            var proxyMethodParamA = proxyType.GetMethod("Method").GetParameters().Single(p => p.Name == "a");

            Assert.IsNotNull(Attribute.GetCustomAttribute(baseMethodParamA, typeof(InheritableAllowMultipleMyAttribute), false));
            Assert.IsNull(Attribute.GetCustomAttribute(proxyMethodParamA, typeof(InheritableAllowMultipleMyAttribute), false));

            Assert.IsNotNull(Attribute.GetCustomAttribute(baseMethodParamA, typeof(InheritableAllowMultipleMyAttribute), true));
            Assert.IsNotNull(Attribute.GetCustomAttribute(proxyMethodParamA, typeof(InheritableAllowMultipleMyAttribute), true));
        }
    }
}

Note that there's also a bug which causes nested public attributes to never be replicated, but I'll submit a separate issue or PR for that.

[jonorossi]

@drieseng sorry for the delay again, I kept leaving it because I think I'm still missing something. I've put together an example for what I was asking in my last comment about AllowMultiple being unrelated to inheritance but about whether you can have more than one attribute defined on that program element.

Why in this use of attributes would you want the TheoryAttribute to be replicated but the InlineDataAttributes not to be?

[AttributeUsage(AttributeTargets.Method, Inherited = true, AllowMultiple = false)]
public class TheoryAttribute { }

[AttributeUsage(AttributeTargets.Method, Inherited = true, AllowMultiple = true)]
public class InlineDataAttribute { }

public class ClassToProxy
{
    [Theory]
    [InlineData(1)]
    [InlineData(2)]
    [InlineData(3)]
    public void MyMethod(int value)
    {
    }
}

[jonorossi]

Unassigning from the v4.0 milestone.

[jonorossi]

@drieseng have you had a chance to read my last comment with some further questions?

[stakx]

@drieseng, @jonorossi, this issue appears to have turned towards the general question of how attribute replication should be done in relation to an attribute's AllowMultiple and/or Inherited usage parameters. That is certainly a good discussion to have, but please do not forget about the original issue, which was specifically about ParamArrayAttribute. I am pleading for this due to one simple fact:

ParamArrayAttribute is a special case and should be treated as such.

This was already mentioned above:

[It] does look like ParamArrayAttribute has some special treatment by the compiler, being automatically inserted into derived types even if it's not explicitly specified.

The fact that DynamicProxy's doesn't do the same thing leads to unexpected results. The solution, in my opinion, is quite simple: As a special case, ParamArrayAttribute should always be replicated.

I don't see any danger in doing this. ParamArrayAttribute has AllowMultiple=false and Inherited=true, which has the effect that the attribute in the base type gets shadowed in the subtype; Attribute.GetCustomAttributes(target, typeof(ParamArrayAttribute), inherit: true) will not report two instances of the attribute, but only one: that in the subtype.

I would be happy to help with a PR to fix this.

What about the general case?

From my point of view, the following would appear to make sense:

• If Inherited == false, replicate the attribute. If you don't, then the proxy has essentially "lost" it.

• If Inherited == true && AllowMultiple == false, replicate the attribute. The replicated attribute will shadow the original attribute in the base type, meaning it gets preserved but it can't cause duplicate attributes being reported via reflection. (ParamArrayAttribute would fall in this category.)

• If Inherited == true && AllowMultiple == true, the proper course of action depends on what DynamicProxy proxies are supposed to achieve:

  • Is a proxy supposed to pretend that it is the proxied type? In that case, you have a problem, because the proxy needs to have the same definition like the proxied type, so it must include (i.e. replicate) the attribute. Attribute.GetCustomAttribute called with inherit: false should still return the attribute. Unfortunately for the proxy to be able to be a stand-in for the proxied type, it needs to be a subtype, so it will inherit the attributes from the base type. Only now, Attribute.GetCustomAttributes called with inherit: true will report one attribute instance too many and the illusion that the proxy is of the proxied type cannot be upheld perfectly.
  • On the other hand, if a proxy doesn't pretend to be the proxied type, but simply aims to be a "polymorphically compatible" subtype of it, then do not replicate the attribute. After all, if (for example) I subclassed a type manually, the compiler wouldn't replicate attributes from the base class for me, just because they are marked Inherited or AllowMultiple. Those flags simply have a different meaning, they don't trigger replication in the general case.

[jonorossi]

@stakx I agree. See my comment from Feb 26 2016:

If ParamArrayAttribute is handled special in this case by the compiler I'd prefer if we did the same as we already do for this attribute in the DP codebase.

It became a discussion because I didn't understand how the proposed change inspecting AllowMultiple made sense.

[jonorossi]

Thanks everyone for reporting, discussing and fixing this defect. #270 has been merged and will be released in v4.1 later today.