Runtime: 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:

``` C#
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.

[nguerrera]

Also, curious about @tmat's thoughts...

[weltkante]

Yes, having them exposed on PropertyAccessors/EventAccessor struct is nicer, I wasn't doing it because it is a struct and adding a field (i.e. making the struct larger) could have negative impact on callers and I don't know how to evaluate that for its impact.

If that isn't a concern I'm fine with moving them over.

(Also note the existing comments on those structs which indicated that there exist callers which care about their performance; possibly the Roslyn compiler.)

[weltkante]

Is there anything I should do to move this forward? I'm not in a particular hurry, but I'm not sure either what the next steps would be. I assume we are waiting for feedback?

[karelz]

@nguerrera @tmat it looks like this slipped through in your area. Can you please check it out?

[tmat]

@weltkante I think we agreed above on adding public ImmutableArray<MethodDefinitionHandle> Others { get; } to the PropertyAccessors/EventAccessors structs. The next step would be to create a PR that implements it and adds tests.

[karelz]

Yep @nguerrera was just waiting on your opinion: https://github.com/dotnet/corefx/issues/4100#issuecomment-292672561

If you (the owners) are ok with the API shape, please update the top post with final API shape and mark it for API review (api-ready-for-review). Once it gets approved, it can be implemented.

[tmat]

@karelz My opinion was expressed by 👍 on Nick's proposal.

[karelz]

Well, not sure when you added it, but as you see it was not clear ;-)

Also you still didn't mark the bug for next stage (API review) - which should have happen once area experts/owners make the decision ...

[tmat]

Updated the proposal and marked for API review.

[weltkante]

Thanks, I've updated the prototype implementation for the new API and also have added a test.

[karelz]

We're blocked on API review -- see API review process. It will happen in Tue meeting, either next week or some week after (depends on how many API proposals we will have to review).

[terrajobst]

Looks good. Just one question, it seems the IL spec also allows .custom. How is that represented?

EDIT: Nevermind, .custom is for custom attributes.

[karelz]

Pending the question, are you @weltkante interested in grabbing the issue?

[weltkante]

Taking @terrajobst edit into account, I'd assume they are already exposed via GetCustomAttributes, so there is probably nothing more to do in this regard.

@karelz if you mean this issue, yes. I've already addressed the comments on my local forks PR, so I assume (according to your linked document) I now make a new PR against corefx. I'll look into that tomorrow.

[karelz]

Great, thanks! Assigned to you. Also sent you collaborator invite - pro tip: Disable monitoring entire repo, it's a lot ;).