#53285 switch to use MemberInfo.MetadataToken

[FatTigerWang]

Switch to use MemberInfo.MetadataToken

With #53283, we won't build a netstandard1.x configuration of this library anymore and the code path can be optimized.

Fix #53285

[krwq]

Is it possible that left is MethodRef and right is MethodDef? (not sure if those are always resolved) that could potentially produce false where it shouldn't.

Also if the token is equal should it just return true? Could arguments differ for same token?

[FatTigerWang]

As far as I know, MetadataToken is unique in the same Module, so when MetadataToken is equal, it can return true. In addition, according to documentation, I am not sure whether it is necessary to judge the unexpected situation. Do you have any suggestions? I will change the code later.

[jkotas]

The modules can have same name, but different identity. This condition would not work correctly in that case.

Should the whole MethodInfoEqualityComparer type be deleted and the dictionary switched to use the default comparer? It is not clear what this implementation is trying to achieve that the default EqualityComparer would not.

[FatTigerWang]

The default comparator will compare the reference of the object. It cannot be judged correctly when used here, so need to use MethodInfoEqualityComparer

[jkotas]

The default EqualityComparer will call Equals method to compare the objects:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs

Line 135 in 57bfe47

if (y != null) return x.Equals(y);

MethodInfo.Equals should do exactly what the existing code is trying to achieve.

[FatTigerWang]

I did not find information about MethodInfo.Equals override from Object.Equals

[FatTigerWang]

Any feedback?

[jkotas]

It would be useful to understand why a simple MethodInfo.Equals does not work. If it does not work, add comment explaining it. Hopefully, it will also help us figure out how the code can be simplified.

[FatTigerWang]

It would be useful to understand why a simple MethodInfo.Equals does not work. If it does not work, add comment explaining it. Hopefully, it will also help us figure out how the code can be simplified.

You are right, MethodInfo.Equals can be judged correctly, so the default EqualityComparer should be used.

[steveharter]

You are right, MethodInfo.Equals can be judged correctly, so the default EqualityComparer should be used.

Are there automated tests that verify the original issue and\or that the Equals overload works as expected?

[FatTigerWang]

You are right, MethodInfo.Equals can be judged correctly, so the default EqualityComparer should be used.

Are there automated tests that verify the original issue and\or that the Equals overload works as expected?

I will check automated tests and add new tests if necessary.

[FatTigerWang]

I have reviewed the existing automated test cases, and I don’t think there is a need to add new test cases for this change.
If MethodInfo.Equals does not work, then the Proxy_Declares_Interface_Events and Invoke_Indexer_Setter_And_Getter_Invokes_Correct_Methods test cases will fail.

[FatTigerWang]

I read the code again, MethodInfo.Equals was supported in .NET Standard 1.0, here is just to judge the equality of the methods of a single interface, why was the special EqualityComparer implemented at that time?

[jkotas]

The special equality comparer was added in dotnet/corefx#9546

dotnet/corefx#9546 (comment) has more discussion about. I do not think that the arguments in that discussion are valid. In particular, I do not agree that "The default Equality test for MethodInfo is reference equality, not "semantic equality" is true.

[jkotas]

@FatTigerWang Thank you!