Added support for Generic Methods (used by .NET)

[raffaeler]

I did not modify the tools to support generic methods. The visitors should add the following case:

[&](GenericMethodIndex var) { }

The MethodDef class now has a new method called GenericParam.

[devhawk]

This change is causing any code in the tree that visits a TypeSig to break. Example: https://github.com/Microsoft/xlang/blob/master/src/tool/idl/main.cpp#L213.

I'm interested in what @kennykerr and @Scottj1s think about this. On the one hand, making meta_reader more broadly usable seems like a good thing. On the other, any WinRT TypeSig parsing code will need to add a handler for GenericMethodIndex even though WinRT projections will never encounter generic methods.

Personally, I come down on the side of "make meta_reader more broadly usable", but that means this PR will need to fixup any existing TypeSig visitor to add a GenericMethodIndex visit branch - even if that branch simply will throw_invalid since generic methods are not valid in WinRT.

[kennykerr]

I don't mind the extra flexibility but yes, please confirm that you haven't broken anything. In particular, build and run src\tool\cppwinrt to make sure it continues to work (since it's the most exhaustive user of the metadata reader). You can also add a generic catch-all visit handler that does nothing if needed. Naturally we need a clean build before we can merge.

[Scottj1s]

good catch @devhawk. I agree with you. let's make the metadata library broadly usable and add the new visit case where needed to fix up existing parsers.

[devhawk]

Since MVars are not valid WinRT constructs, I think we should throw in the GenericMethodIndex visit method.

[devhawk]

Let's change this to GenericMethodTypeIndex

[kennykerr]

The philosophy is to use the name exactly as it appears in the ECMA-335 spec, or as close to it as is practical, to make it easier to cross-reference.

[DefaultRyan]

ECMA-335 isn't very rigorous in deciding what to name this construct. I think the closest thing we have is for ELEMENT_TYPE_VAR and ELEMENT_TYPE_MVAR, which are described as "Generic parameter in a generic type definition" and "Generic parameter in a generic method definition".

Maybe GenericTypeParameter and GenericMethodParameter would be the clearest naming?

[devhawk]

I still think GenericMethodTypeIndex is the right choice. Arguably GenericMethodTypeParameterIndex would be better, but I think that's getting too long. Plus, leaving "parameter" off helps distinguish from the normal method parameters

Others brought up good points. Didn't realize this was breaking existing visitors.

[raffaeler]

This change is causing any code in the tree that visits a TypeSig to break. Example: https://github.com/Microsoft/xlang/blob/master/src/tool/idl/main.cpp#L213.

Did you already see it is breaking the tools?
I did a quick test before leaving (I am on a plane for SEA right now) and it didn't break reading the standard generic types.
Which winmd is better for testing on your experience?

[devhawk]

To clarify, the problem isn't the code you added @raffaeler, it's that existing code in the repo that visits TypeSigs now needs to be updated to handle GenericMethodIndex. You can see the broken PR build here: https://dev.azure.com/msft-xlang/70756bba-8415-4c27-8916-4cdb4503623d/_build/results?buildId=930.

Typically, we recommend using the contract metadata files, the ones that live under 'C:\Program Files (x86)\Windows Kits\10\References'

[kennykerr]

If it's just quick smoke test you can also use the metadata here: C:\Windows\System32\WinMetadata

For the cppwinrt you can use "-in local" argument to use WinMetadata.

[raffaeler]

We had a face to face conversation with @kennykerr about this PR two weeks ago.
Please let me know if I can still do something to make this PR be approved.

[kennykerr]

Sorry for the delay. I'll take a look at this soon. Just been a really busy few weeks. It was great to see you at the MVP summit!

[raffaeler]

No worries @kennykerr, take your time!
It was a great pleasure to meet you and your team. Thank you again!

[DefaultRyan]

I've got the build breaks fixed now. All that remains is to agree on the name. I've seen GenericMethodTypeIndex proposed. Do we have consensus?

[kennykerr]

Sounds good, thanks Ryan!