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

Portable mechanism needed to determine if two MemberInfos are backed by the same metadata definition #16288

Closed
nguerrera opened this issue Feb 4, 2016 · 9 comments · Fixed by dotnet/corefx#20112
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection
Milestone

Comments

@nguerrera
Copy link
Contributor

Currently, there is a hole in reflection whereby there's no straight-forward way to determine if:

1. MemberInfos differ only by ReflectedType (i.e. they are the same member, but one of them was obtained from a derived type.)

public class C { public int F; }
public class D : C {}

static class Program {
    static void Main(string[] args) {
        var f1 = typeof(C).GetField("F");
        var f2 = typeof(D).GetField("F");
        System.Console.WriteLine(f1.Equals(f2)); // False
    }
}

2. One MemberInfo is a generic instantiation of another.

static class Program {
    static void Main(string[] args) {
        var m1 = typeof(List<>).GetMethod("Add");
        var m2 = typeof(List<int>).GetMethod("Add");

        // how to determine that m2 is instantiated from m1?
    }
}

On platforms that support the MetadataToken, this can be worked around by comparing the tokens, but without them, one has to resort to error-prone comparisons of the declaring type, name, and signature.

There should be a first-class portable way to address both.

Proposal for New API: HasSameMetadataDefinitionAs

Add a new API that determines if a member is backed/instantiated from the same metadata definition. In ECMA-335 terms, this is a member/type from the same module that has the same metadata def token. Platforms like .NET Native that do not keep around metadata tokens should still preserve enough information to answer this query portably.

public class MemberInfo {
    public virtual bool HasSameMetadataDefinitionAs(MemberInfo other);
    ...
}

A mostly correct implementation is:

return Module.Equals(other.Module) && MetadataToken ==  other.MetadataToken;

But that ignores edge cases such as TypeInfo instances for array, byref, pointer, and type parameters types, which we'd need to spec and test.

@nguerrera
Copy link
Contributor Author

cc @weshaggard

@weshaggard weshaggard assigned ghost Feb 4, 2016
@MichalStrehovsky
Copy link
Member

Microsoft.CSharp uses the MetadataToken workaround on platforms where it's available and does something random otherwise. This leads to different behaviors on AOT platforms where tokens are not available.

@jkotas
Copy link
Member

jkotas commented Feb 25, 2017

Needs to be primarily fixed for ProjectN/UWP.

@karelz karelz unassigned ghost Feb 28, 2017
@karelz
Copy link
Member

karelz commented Feb 28, 2017

@atsushikan @DnlHarvey does the API proposal above look reasonable? If yes, let's mark it so.

@ghost
Copy link

ghost commented Mar 1, 2017

@nguerrera: I think HasSameMetadataDefinitionAs() is a good way to go but should be defined on the specific MemberInfos (Type, MethodInfo, etc.) with the properly constrained parameter type.

@JonHanna
Copy link
Contributor

JonHanna commented Mar 1, 2017

If I have e.g. two MethodInfo references and want to know if they have the same metadata definition, then whether this method is defined in terms of MethodInfo or MemberInfo won't make any difference to me, so as an API consumer I don't care.

If I have two MemberInfo references and want to know if they have the same metadata definition, then if it's defined in terms of MemberInfo I can use it, but otherwise I have to branch on a chain of is tests, so as an API consumer I'd prefer the former.

What advantage the more specific definition?

@nguerrera
Copy link
Contributor Author

I agree with @JonHanna. Why can't this be polymorphic?

@ghost
Copy link

ghost commented Mar 1, 2017

Defining the api polymorphically means every caller pays the penalty of comparing the "MemberTypes" and casting to the specific type. Parameter types should also be as specific as possible for better compile time bug detection.

How frequent is the use case where the caller doesn't already know the specific type? There is no case where MemberInfos of different flavors will be considered "equivalent" by this api.

@karelz
Copy link
Member

karelz commented Mar 7, 2017

API review:
@weshaggard mentioned this would have been very useful in the past. Partners are likely asking for it.

We like the original proposal (as virtual) - approved. If there are reasons to do something else, please speak up = reply with clear proposal change and defend it.

BTW: We discussed as well having overloads with specific types - mainly for performance. We do not think it is a must have right now. We can consider it later.

Marking as up for grabs
Complexity: Medium-Hard (requires deep reflection knowledge - all cases need to be covered)

@ghost ghost self-assigned this May 22, 2017
jkotas referenced this issue in dotnet/coreclr May 22, 2017
This api was approved here:

  https://github.com/dotnet/corefx/issues/5884

and is a necessary step to fixing the System.Dynamic.Runtime.Tests
failure:

  https://github.com/dotnet/corefx/issues/19895

which is caused by Microsoft.CSharp trying to do the impossible
and emulate this api without GetMetadataToken() support.

This approach opts for the most straightforward and efficient
implementation without any special-casing for weird situations
(this is also what Microsoft.CSharp implements today as
well as what someone else trying to trampoline members
across generic instantaitions is like to do.)

This results in the following behavior for these
corner cases. With the possible exception of #3,
I think they are tolerable enough to accept and codify:


1. "other" implemented by an entirely different Reflection
   provider than "this".

   Behavior:
     returns false without invoking any methods on the
     "other" Member.

   To change it to throw an ArgumentException would
   mean extra cast checks against the 6 possible
   Runtime types (or having said RuntimeTypes implement
   a sentinel interface.)

   Given that HasSameMetadataDefinitionAs() is a
   "looser cousin of Equals()" and "Equals()"
   doesn't throw for objects from a different universe,
   this seems reasonable.


2. Arrays, ByRefs, Pointers and Types from GetTypeFromCLSID()

   Behavior:
     Arrays, ByRefs, Pointers all return token 0x0600000
     and so they'll return "true" wrt to each other (provided
     both types are implemented by the same provider.)

     CLSID types all return the typedef of __ComObject
     so they'll return "true" wrt to each other.

     The constructor exposed by CLSID types all return
     the typedef of some constructor on __ComObject so
     they'll return "true" wrt to each other.

   I do not think these are interesting cases that merit
   special handling. These types will never appear
   in an enumeration of the members of a type. (The
   fact that Reflection surfaces them in objects
   that are assignable to MemberInfo is a structural
   flaw in Reflection's object model.)

3. Synthesized constructors and methods on array types.

   Behavior:
     These methods all return 0x06000000 as a token
     so the constructors will all compare true wrt
     each other, and likewise with the methods.

   This is a bit crummy though it's not clear
   what the "right" policy should look like.
   I could be persuaded to throw NotSupported
   for these, to leave the possibility open
   for a better story later. On the other hand,
   I wouldn't demand it either.
dotnet-bot referenced this issue in dotnet/corert May 22, 2017
)

This api was approved here:

  https://github.com/dotnet/corefx/issues/5884

and is a necessary step to fixing the System.Dynamic.Runtime.Tests
failure:

  https://github.com/dotnet/corefx/issues/19895

which is caused by Microsoft.CSharp trying to do the impossible
and emulate this api without GetMetadataToken() support.

This approach opts for the most straightforward and efficient
implementation without any special-casing for weird situations
(this is also what Microsoft.CSharp implements today as
well as what someone else trying to trampoline members
across generic instantaitions is like to do.)

This results in the following behavior for these
corner cases. With the possible exception of #3,
I think they are tolerable enough to accept and codify:

1. "other" implemented by an entirely different Reflection
   provider than "this".

   Behavior:
     returns false without invoking any methods on the
     "other" Member.

   To change it to throw an ArgumentException would
   mean extra cast checks against the 6 possible
   Runtime types (or having said RuntimeTypes implement
   a sentinel interface.)

   Given that HasSameMetadataDefinitionAs() is a
   "looser cousin of Equals()" and "Equals()"
   doesn't throw for objects from a different universe,
   this seems reasonable.

2. Arrays, ByRefs, Pointers and Types from GetTypeFromCLSID()

   Behavior:
     Arrays, ByRefs, Pointers all return token 0x0600000
     and so they'll return "true" wrt to each other (provided
     both types are implemented by the same provider.)

     CLSID types all return the typedef of __ComObject
     so they'll return "true" wrt to each other.

     The constructor exposed by CLSID types all return
     the typedef of some constructor on __ComObject so
     they'll return "true" wrt to each other.

   I do not think these are interesting cases that merit
   special handling. These types will never appear
   in an enumeration of the members of a type. (The
   fact that Reflection surfaces them in objects
   that are assignable to MemberInfo is a structural
   flaw in Reflection's object model.)

3. Synthesized constructors and methods on array types.

   Behavior:
     These methods all return 0x06000000 as a token
     so the constructors will all compare true wrt
     each other, and likewise with the methods.

   This is a bit crummy though it's not clear
   what the "right" policy should look like.
   I could be persuaded to throw NotSupported
   for these, to leave the possibility open
   for a better story later. On the other hand,
   I wouldn't demand it either.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
ghost referenced this issue in dotnet/corert May 22, 2017
)

This api was approved here:

  https://github.com/dotnet/corefx/issues/5884

and is a necessary step to fixing the System.Dynamic.Runtime.Tests
failure:

  https://github.com/dotnet/corefx/issues/19895

which is caused by Microsoft.CSharp trying to do the impossible
and emulate this api without GetMetadataToken() support.

This approach opts for the most straightforward and efficient
implementation without any special-casing for weird situations
(this is also what Microsoft.CSharp implements today as
well as what someone else trying to trampoline members
across generic instantaitions is like to do.)

This results in the following behavior for these
corner cases. With the possible exception of #3,
I think they are tolerable enough to accept and codify:

1. "other" implemented by an entirely different Reflection
   provider than "this".

   Behavior:
     returns false without invoking any methods on the
     "other" Member.

   To change it to throw an ArgumentException would
   mean extra cast checks against the 6 possible
   Runtime types (or having said RuntimeTypes implement
   a sentinel interface.)

   Given that HasSameMetadataDefinitionAs() is a
   "looser cousin of Equals()" and "Equals()"
   doesn't throw for objects from a different universe,
   this seems reasonable.

2. Arrays, ByRefs, Pointers and Types from GetTypeFromCLSID()

   Behavior:
     Arrays, ByRefs, Pointers all return token 0x0600000
     and so they'll return "true" wrt to each other (provided
     both types are implemented by the same provider.)

     CLSID types all return the typedef of __ComObject
     so they'll return "true" wrt to each other.

     The constructor exposed by CLSID types all return
     the typedef of some constructor on __ComObject so
     they'll return "true" wrt to each other.

   I do not think these are interesting cases that merit
   special handling. These types will never appear
   in an enumeration of the members of a type. (The
   fact that Reflection surfaces them in objects
   that are assignable to MemberInfo is a structural
   flaw in Reflection's object model.)

3. Synthesized constructors and methods on array types.

   Behavior:
     These methods all return 0x06000000 as a token
     so the constructors will all compare true wrt
     each other, and likewise with the methods.

   This is a bit crummy though it's not clear
   what the "right" policy should look like.
   I could be persuaded to throw NotSupported
   for these, to leave the possibility open
   for a better story later. On the other hand,
   I wouldn't demand it either.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
ghost referenced this issue in dotnet/corert May 22, 2017
This api was approved here:

  https://github.com/dotnet/corefx/issues/5884

and is a necessary step to fixing the System.Dynamic.Runtime.Tests
failure:

  https://github.com/dotnet/corefx/issues/19895

which is caused by Microsoft.CSharp trying to do the impossible
and emulate this api without GetMetadataToken() support.

This emulates the behavior of the CoreCLR version
(which simply compares MetadataTokens and Modules.)

The CoreCLR version of this is currently a PR

  dotnet/coreclr#11774


-- Copied from CoreCLR commit text---

This results in the following behavior for these
corner cases. With the possible exception of #3,
I think they are tolerable enough to accept and codify:


1. "other" implemented by an entirely different Reflection
   provider than "this".

   Behavior:
     returns false without invoking any methods on the
     "other" Member.

   To change it to throw an ArgumentException would
   mean extra cast checks against the 6 possible
   Runtime types (or having said RuntimeTypes implement
   a sentinel interface.)

   Given that HasSameMetadataDefinitionAs() is a
   "looser cousin of Equals()" and "Equals()"
   doesn't throw for objects from a different universe,
   this seems reasonable.


2. Arrays, ByRefs, Pointers and Types from GetTypeFromCLSID()

   Behavior:
     Arrays, ByRefs, Pointers all return token 0x0600000
     and so they'll return "true" wrt to each other (provided
     both types are implemented by the same provider.)

     CLSID types all return the typedef of __ComObject
     so they'll return "true" wrt to each other.

     The constructor exposed by CLSID types all return
     the typedef of some constructor on __ComObject so
     they'll return "true" wrt to each other.

   I do not think these are interesting cases that merit
   special handling. These types will never appear
   in an enumeration of the members of a type. (The
   fact that Reflection surfaces them in objects
   that are assignable to MemberInfo is a structural
   flaw in Reflection's object model.)

3. Synthesized constructors and methods on array types.

   Behavior:
     These methods all return 0x06000000 as a token
     so the constructors will all compare true wrt
     each other, and likewise with the methods.

   This is a bit crummy though it's not clear
   what the "right" policy should look like.
   I could be persuaded to throw NotSupported
   for these, to leave the possibility open
   for a better story later. On the other hand,
   I wouldn't demand it either.
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 3, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants