Missing accessors on PropertyDefinition

[weltkante]

The implementation of System.Reflection.Metadata PropertyDefinition and EventDefinition swallows some accessors and doesn't make them available publically. This has been marked as TODO in the source code. These 'other' accessors are not often encountered, but handling them is required for certain special case tooling.

usage of 'other' property accessors

• The .NET Framework exposes 'other' accessors through PropertyInfo.GetAccessors
• .NET assemblies generated by the 'tlbimp' tool can generate this metadata when a COM property has both a PUT and a PUTREF accessor.
• They are valid metadata, even though rarely used, but if you build tools for processing or inspecting .NET assemblies (like tlbexp or ildasm do) you want to handle them.

The last point is a real-world use case I have. The tlbexp functionality built into the .NET Framework requires loading an assembly into the AppDomain to generate a TLB. Loading the assembly can fail for various reasons, so generating a TLB by only inspecting metadata is desireable. The omission of 'other' accessors is a problem here.

Proposed API

namespace System.Reflection.Metadata
{
public struct PropertyAccessors 
{
    // Existing
    public MethodDefinitionHandle Getter { get; }
    public MethodDefinitionHandle Setter { get; } 

    // New
    public ImmutableArray<MethodDefinitionHandle> Others { get; }
}

public struct EventAccessors
{  
    // Existing
    public MethodDefinitionHandle Adder { get; }
    public MethodDefinitionHandle Remover { get; }
    public MethodDefinitionHandle Raiser { get; }

    // New
    public ImmutableArray<MethodDefinitionHandle> Others { get; }
}
}

Details

In the large majority of cases there will be no 'other' accessors in the metadata. It is trivially possible to implement this case without any additional allocations compared to the current implementation.

For the purpose of the discussion I have provided an initial implementation of the suggested change.

Updates

• Expose other accessors directly on the PropertyAccessors and EventAccessors structs instead of returning them as an out-param

[joshfree]

@weshaggard @pallavit

[karelz]

We need formal API proposal.

[weltkante]

Oh, ok, I initially submitted this as a bug, but if its not clear how to add the missing API I'll look into making a proposal and maybe even submit a possible implementation, didn't look that hard back when I noticed the bug.

[karelz]

FYI: Here's process for new APIs, incl. preferred format for proposals. Add it to this issue if/when you have it.

[karelz]

If you want to contribute to CoreFX, please check dotnet/corefx#17619 for guidance what to focus on. Thanks!

[weltkante]

@karelz I've updated the initial post and changed the bug report into a proposal request, hope I didn't miss anything.

[nguerrera]

Thanks @weltkante. I think it would be nicer without the extra overloads and out params if instead there were Others collections hanging off of ProperyAccessors and EventAccessors. Here's my counter proposal:

public struct PropertyAccessors 
{
    // Existing
    public MethodDefinitionHandle Getter { get; }
    public MethodDefinitionHandle Setter { get; } 

    // New
    public ImmutableArray<MethodDefinitionHandle> Others { get; }
}

public struct EventAccessors
{  
    // Existing
    public MethodDefinitionHandle Adder { get; }
    public MethodDefinitionHandle Remover { get;  }
    public MethodDefinitionHandle Raiser { get; } }

    // New
    public ImmutableArray<MethodDefinitionHandle> Others { get; }
}

Others would just be ImmutableArray.Empty in the common case. I believe that other methods are rare enough that is fine to allocate unconditionally if there are any. That would be the simplest implementation-wise. Alternatively, the getter could do its work on demand, or it could be implemented like the other strongly typed collections that are allocation-free and just act as cursors over the backing metadata.

[karelz]

@nguerrera if you think it is ready for API review, please update top post and mark it so. If you want to double check with @weltkante first, fine with me :)

[nguerrera]

Since @weltkante is graciously driving this, I'll wait for his input on my suggestion.