Add public API to retrieve metadata token from ISymbol

[tmat]

Scenario

Implement improvements in Go To Definition when navigating to a metadata symbol (see #24349).

When the user invokes GTD on a metadata symbol we plan to identify the source file where the symbol is defined based on information stored in the PDB (combination of MethodDebugInformation, CustomDebugInformation and Document PDB tables).
Using debugger provided service we can first retrieve the PDB, find the source info in the PDB tables, then use Source Link (assuming it's present in the PDB), download the source file(s) that define the symbol from the source server, open these files in the editor and finally navigate to the requested symbol.

Problem

Currently ISymbol that represents a metadata symbol does not expose its metadata token. It is possible to find the token within the metadata using MetadataReader APIs but it would involve reimplementing the metadata signature decoder logic that's already implemented in the compiler. Instead we propose to expose the token from ISymbol directly.

Proposal

interface ISymbol
{
   ...
   /// <summary>
   /// If this symbol represents a metadata entity returns its metadata token, otherwise returns 0.
   /// </summary>
   int MetadataToken { get; }
   ...
}

The API would return 0 in all cases when the symbol is not backed by a metadata entity (internally implemented as a PE symbol).

[jaredpar]

Is zero every a valid value for a metadata token?

[CyrusNajmabadi]

Would it be better to add this to Location? Location​ already has IsInSource and IsInMetadata. And if it's from source, exposes the SyntaxTree/Span. For metadata it exposes the MetadataModule so it feels like a natural fit that it could also expose the MetadataToken.

Then, you'd just get this from ISymbol.Locations in a consistent fashion. Thoughts?

[tmat]

We could. However we currently reuse a single ImmutableArray<Location> for all symbols from given PE module. We would need to allocate a new location (and an array) for each metadata symbol.

[tmat]

Is zero every a valid value for a metadata token?

It's not.

[333fred]

@AlekseyTs as API owner.

[tmat]

@AlekseyTs Does the proposed design look good, or should we access the token thru Location API with the downside that we would allocate a lot of Location objects?

[AlekseyTs]

@tmat, it looks like internally we don't have a token as an int, but rather have an instance of opaque System.Reflection.Metadata.MethodDefinitionHandle. What would be the way for us to get the underlying value? Shold we instead expose an instance of System.Reflection.Metadata.Handle?

I agree that adding the information to Location might introduce undesired allocations.

[siegfriedpammer]

What would be the way for us to get the underlying value? Shold we instead expose an instance of System.Reflection.Metadata.Handle?

Just my 50c. As a user of both Roslyn and SRM, I would be perfectly fine with having access to the strongly typed Handle. The literal value can be easily obtained by calling System.Reflection.Metadata.Ecma335.MetadataTokens.GetToken() and other extensions in that class. If you are interested in accessing raw metadata you probably have knowledge of the internal structure of the metadata tables and the SRM API anyway, so this should be no problem.

I am really looking forward to this!

[tmat]

What would be the way for us to get the underlying value? Shold we instead expose an instance of System.Reflection.Metadata.Handle

I wouldn't expose dependency on SRM in public API. The underlying token can be retrieved via MetadataTokens.GetToken(Handle), as @siegfriedpammer pointed out.

Returning int would also be consistent with Reflection API (MemberInfo.MetadataToken) which also returns int.

[333fred]

I wouldn't expose dependency on SRM in public API.

We already have such dependencies in our public API, such as around calling conventions and one older API that I can't remember off the top of my head. So I don't think there's any issue in another similar API. That said, if we think an int is better for other reasons, that's fine.

[tmat]

@333fred Oh, interesting I did not know that. If that is so then returning EntityHandle would be fine.

[AlekseyTs]

I think we expose System.Reflection types from public APIs, but I am not sure if we actually expose any System.Reflection.Metadata types from public APIs today. Keeping that sounds compelling to me.

[333fred]

but I am not sure if we actually expose any System.Reflection.Metadata types from public APIs today

https://sourceroslyn.io/#Microsoft.CodeAnalysis/Symbols/IMethodSymbol.cs,220 is from SRM.

[AlekseyTs]

You are correct there is a precedent. However, that is an enum, effectively an int. Doesn't really say how we read metadata.

[333fred]

API Review

API Approved

• Adding to Location, while somewhat tempting, is concerning from a perf perspective. Metadata symbols can share Location arrays for all symbols from the same metadata symbol, this would prevent us from doing that and potentially incur a large number of allocations.
• While we do have precedent for exposing System.Runtime.Metadata types from our APIs (IModuleSymbol.GetMetadata().GetMetadataReader()), an int is a good abstraction here that is aligned with ECMA 335 and can be used by both System.Reflection and System.Reflection.Emit. We'd also want to hide the handle property on derived types to expose ParameterHandle/PropertyHandle/etc strongly-typed properties, which can be painful.