Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support resolving metadata tokens in MetadataLoadContext #87657

Open
reflectronic opened this issue Jun 15, 2023 · 9 comments
Open

Support resolving metadata tokens in MetadataLoadContext #87657

reflectronic opened this issue Jun 15, 2023 · 9 comments
Labels
area-System.Reflection feature-request help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@reflectronic
Copy link
Contributor

When using MetadataLoadContext, Module.{ResolveType,ResolveMember,ResolveMethod,ResolveField,ResolveSignature,ResolveString} all throw NotSupportedException.

This is inconvenient for advanced reflection scenarios. For example, if you are scanning IL code (perhaps retrieved using MethodBody.GetILAsByteArray), these methods are the only way to resolve MemberRefs and TypeRefs used in IL.

I do not think there is a technical reason that prevents these methods from working. If everything looks OK with this idea then I am happy to implement these.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 15, 2023
@ghost
Copy link

ghost commented Jun 15, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

When using MetadataLoadContext, Module.{ResolveType,ResolveMember,ResolveMethod,ResolveField,ResolveSignature,ResolveString} all throw NotSupportedException.

This is inconvenient for advanced reflection scenarios. For example, if you are scanning IL code (perhaps retrieved using MethodBody.GetILAsByteArray), these methods are the only way to resolve MemberRefs and TypeRefs used in IL.

I do not think there is a technical reason that prevents these methods from working. If everything looks OK with this idea then I am happy to implement these.

Author: reflectronic
Assignees: -
Labels:

area-System.Reflection

Milestone: -

@steveharter
Copy link
Member

I do not think there is a technical reason that prevents these methods from working.

Thoughts @buyaa-n @jkotas @AaronRobinsonMSFT?

MLC does do internal resolution of some of those here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Ecma/EcmaResolver.cs

@steveharter steveharter added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 17, 2023
@AaronRobinsonMSFT
Copy link
Member

I do not think there is a technical reason that prevents these methods from working.

Thoughts @buyaa-n @jkotas @AaronRobinsonMSFT?

I don't see anything wrong with this. There are going to be tricky parts, for example if EnC is involved. However, for assemblies defined with RefEmit or for statically loaded assemblies this seems like a reasonable feature.

@jkotas
Copy link
Member

jkotas commented Jul 17, 2023

It should be very straightforward to implement these methods. @reflectronic Are you interested in submitting a PR?

@reflectronic
Copy link
Contributor Author

reflectronic commented Jul 17, 2023

EnC/S.R.E/etc. don't matter for MLC, right? It just loads assembly files from disk using S.R.M, there's no runtime magic involved. The APIs already exist and work for RuntimeModule, just not on EcmaModule.

And I can see why Ati chose not to implement them; the logic is complicated and the layering of MLC doesn't lend itself to these APIs. FieldDef/MethodDef are admittedly easy, but MemberRef is harder because the signature matching needs to be implemented over the reflection object model (taking into account modifiers and so on). Internally, there is a deliberate separation between EcmaXyz classes, which get to access the metadata reader directly, and RoXyz classes, which never expose or access raw metadata at all. This abstraction makes it hard to get the right information to the right place.

I already have some code written for this, but I'm not really happy with how it looks, so I'll see if I can refactor it and put up a PR. I don't know if I am overthinking it or if there's something we can change to make it easier.

@AaronRobinsonMSFT
Copy link
Member

It just loads assembly files from disk using S.R.M, there's no runtime magic involved.

Files on disk can have EnC delta tables and since MLC also accepts a byte[] this could be an in-memory version. However, those are likely unnecessary to support. Without those tables/parts, as Jan stated, this should be straight forward.

@jkotas
Copy link
Member

jkotas commented Jul 18, 2023

The existing MetadataLoadContext code deals with the token resolution internally. The fix for this issue should just wire-up the existing internal token resolution to the public APIs. I would not worry too much about following all the existing abstractions. It is ok for this fix to cut corners around the abstractions.

The fix for this issue should not be improving the internal token resolution to handle EnC data formats, etc. Improvements like this should be tracked by new issue and fixed by separate PRs.

@reflectronic
Copy link
Contributor Author

Right, I agree that for TypeRef it is very easy because MLC has the code to resolve them. To my knowledge, it does not have the code to generally resolve MemberRefs, because that never comes up in reflection. This is the harder part.

There is this bit in custom attribute parsing:

case HandleKind.MemberReference:
{
TypeContext typeContext = default;
MemberReference mr = ((MemberReferenceHandle)ctorHandle).GetMemberReference(Reader);
MethodSignature<RoType> sig = mr.DecodeMethodSignature(_module, typeContext);
Type[] parameterTypes = sig.ParameterTypes.ToArray();
Type declaringType = mr.Parent.ResolveTypeDefRefOrSpec(_module, typeContext);
const BindingFlags bf = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.ExactBinding;
ConstructorInfo? ci = declaringType.GetConstructor(bf, null, parameterTypes, null);
if (ci == null)
throw new MissingMethodException(SR.Format(SR.MissingCustomAttributeConstructor, declaringType));
return ci;
}

But this approach is not suitable for general signature matching. For example, it does not work for methods overloaded by return type (it's not relevant here since constructors always return void), and it doesn't handle custom modifiers (I think this is an existing bug?). Please prove me wrong if you know of suitable code elsewhere in MLC!

I agree that handling EnC tables is a separate feature. MLC ignores them entirely right now, it would be weird to handle them just for this.

@jkotas
Copy link
Member

jkotas commented Jul 18, 2023

You are right - MemberRefs would need work.

@steveharter steveharter added feature-request help wanted [up-for-grabs] Good issue for external contributors and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Jul 19, 2023
@steveharter steveharter added this to the Future milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection feature-request help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

4 participants