CustomAttributeExtensions.GetCustomAttribute does not return inherited attributes for indexer argument

[drieseng]

System.Reflection.CustomAttributeExtensions.GetCustomAttribute(this ParameterInfo element, Type attributeType, bool inherit) - which is located in the System.Reflection.Extensions assembly - does not returned inherited attributes for indexer arguments.

It does work fine for method arguments though (for example).

To reproduce, compile and run the following code snippet:

using System;
using System.Reflection;

namespace ConsoleApp1
{
    public class Program
    {
        static void Main()
        {
            var indexerParam = typeof(Sub).GetProperty("Item").GetIndexParameters()[0];

            var indexerParamAttr = indexerParam.GetCustomAttribute(typeof(MyAttribute), true);
            Console.WriteLine($"Indexer: {indexerParamAttr != null}");

            var methodParam = typeof(Sub).GetMethod("Run").GetParameters()[0];

            var methodParamAttr = methodParam.GetCustomAttribute(typeof(MyAttribute), true);
            Console.WriteLine($"Method: {methodParamAttr != null}");
        }
    }

    [AttributeUsage(AttributeTargets.All)]
    public class MyAttribute : Attribute
    {

    }

    public abstract class Base
    {
        public abstract string Run([MyAttribute] int[] args);
        public abstract object this[[MyAttribute] int[] args] { get; set; }
    }

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

        public override object this[int[] args]
        {
            get { return null; }
            set { }
        }
    }
}

Expected result:

Indexer: True
Method: True

Actual result:

Indexer: False
Method: True

[ghost]

cc @weshaggard
cc @nguerrera
cc @mellinoe

Guys, what are your thoughts on this?

Issue

ParameterInfo objects returned by callling PropertyInfo.GetIndexParameters() are reported as not seeing inherited custom attributes.

How to fix this
Defining what custom attributes are attached or even what inheritance means is dodgy because at the metadata level, properties do not have parameters. Only their getter/setter methods do. (And in theory, the getter and setter methods could attach different custom attributes to their parameters, and have different inheritance chains.)

PropertyInfo.GetIndexParameters() works by looking for a getter method - if none exists, then a setter method. It captures the parameter list of the chosen accessor method, wraps them in a secondary ParameterInfo object that behaves identically to the method version except that the Member property returns a PropertyInfo rather than a MethodInfo. In other worse, this ParameterInfo is a bit of a lie.

The desktop/NETCore implementation does copy the original parameter's metadata token over. This allows GetCustomAttributes to "work" for directly defined attributes (provided you're not worried about the freak case where the getter and setter define different custom attributes.) Which is in a way unfortunate, because if we hadn't done this, it would make it easier to justify not adding support for inherited custom attributes.

GetCustomAttributes does not find inherited attributes as the code only scans up the chain if the ParameterInfo.Member property returns a MethodInfo. Otherwise, it assumes without checking that the member is a ConstructorInfo, which can never be "inherited." The third possibility - PropertyInfo was probably overlooked.

The most consistent way to "fix" the current behavior would probably be:

• Remember if the ParameterInfo wraps the getter or the setter.
• Treat a query for custom attributes on the ParameterInfo as if it were a query for custom attributes
  on the original getter/setter ParameterInfo.

Bottom Line

I see this a cost-benefit decision. I'm leaning against fixing this but it's worth throwing up for api discussion.

Pros of fixing:

Returning directly defined custom attributes but disregarding the "inherited" flag breaks the principle of least surprise.

Cons of fixing:

This would be a potentially breaking change for the full framework (from 3.5 onward). It would also have to be fixed independently in the .Net Native implementation as we emulated the desktop behavior over there.

This is easy to work around: use the PropertyInfo from the actual getter/setter method.

This doubles down on the questionable decision of exposing a "property parameter" through Reflection in the first place. Such a thing simply doesn't reflect the underlying metadata model.

[ghost]

Another data point: ICustomAttributeProvider.GetCustomAttributes() (implemented by ParameterInfo) disregards the "inherit" argument entirely. MSDN actually documents this (https://msdn.microsoft.com/en-us/library/cwtf69s6(v=vs.110).aspx), along with a recommendation to use Attribute.GetCustomAttributes() as a workaround.

Either way we go, we have a messy api story here.

[nguerrera]

This is easy to work around: use the PropertyInfo from the actual getter/setter method.

I believe this should read "use the ParameterInfo from the actual getter/setter method"

[mellinoe]

Eh, what a mess, indeed. I'm inclined to just say "don't fix it" simply because of the MSDN documentation you linked. On one hand, it would obviously make sense to fix it, and that result in "more correct" behavior, based on a reasonable interpretation of the method/parameter names. On the other hand, this description kind of makes it sound like the whole method is ill-defined in the first place:

PropertyInfo.GetIndexParameters() works by looking for a getter method - if none exists, then a setter method. It captures the parameter list of the chosen accessor method, wraps them in a secondary ParameterInfo object that behaves identically to the method version except that the Member property returns a PropertyInfo rather than a MethodInfo. In other worse, this ParameterInfo is a bit of a lie.

[nguerrera]

In metadata, properties do have a distinct signature blob from their accessors, but that only encodes parameter types. There is indeed no way to have Param records parented by Property records and so only the accessor parameters can have names and custom attributes.

All that to say that I agree that the concept of a ParameterInfo is misplaced on PropertyInfo. Type[] GetIndexParameterTypes() would have been OK. Too bad that we don't have a time machine... ;)

I would also lean against changing it. The "fix" still surprises in some cases:

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

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

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

abstract class A {
  public virtual int this[[X] int x] { get { throw null; } set { } }
}

class B : A {
  public override int this[[Y] int x] { set { } }
}

class C : B {
  public override int this[[Z] int x] { get { throw null; } set { } }
}

• C.get_Item's parameter has (inherited) attributes [X, Z]
• C.set_Item's parameter has [X, Y, Z].
• The "fix" would say that C.Item's parameter X has [X, Z], but one might reasonably expect [X, Y, Z].

I would actualy be happier with an amended "fix" where we aggregate attributes across the getters and setters in the chain. I think that would be more intuitive from the point of view of basing expectations on C# syntax. But I'm not sure that the complexity and risk of a breaking change is worth it...

[ghost]

Thanks for the feedback. Based on the comments and my own investigation, I'm closing this as won't fix.