Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implement HasSameMetadataDefinitionAs() on CoreCLR #11774

Merged
merged 1 commit into from May 22, 2017
Merged

Implement HasSameMetadataDefinitionAs() on CoreCLR #11774

merged 1 commit into from May 22, 2017

Conversation

ghost
Copy link

@ghost ghost commented 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.

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.
@ghost ghost requested review from MichalStrehovsky and jkotas May 22, 2017 02:24
@jkotas jkotas merged commit c557fcb into dotnet:master May 22, 2017
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request 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 pushed a commit to dotnet/corert that referenced this pull request 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 pushed a commit to dotnet/corert that referenced this pull request 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.
@ghost ghost deleted the sameas branch May 22, 2017 20:17
@karelz karelz modified the milestone: 2.1.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants