Make a `MemberComparer` class more reliable

[Taritsyn]

Current implementation of the MemberComparer class uses a try-catch block to suppress exceptions. I think it's worth changing the implementation to a more standard one.

[ClearScriptLib]

Hi Andrey,

Can you give us an idea of the motivation for this PR?

Thanks!

[Taritsyn]

Hello!

I used a similar code in my library (only without the try-catch block). This code worked without errors on almost any version of .NET.

But recently I found out that when running on .NET Core a NullReferenceException exception is thrown. Simple checks for null equality solved this problem. Therefore, I consider using a try-catch block to be redundant.

[ClearScriptLib]

Hi Andrey,

We like the null checks.

However, the .NET documentation specifies that IEqualityComparer<T>.Equals doesn't throw exceptions, whereas Module and MetadataToken do. Therefore, we still favor try-catch for the Module and MetadataToken comparisons.

On the other hand, there are valid arguments against exception "swallowing". A good solution might be to swallow only the exception types specified for those properties, as they're unlikely to be indicative of bugs.

Opinions?

[Taritsyn]

…, whereas Module and MetadataToken do. Therefore, we still favor try-catch for the Module and MetadataToken comparisons.

It changes a lot.

A good solution might be to swallow only the exception types specified for those properties, as they're unlikely to be indicative of bugs.

Such a catch block looks even more redundant:

catch (Exception exception) when (exception is NotImplementedException || exception is InvalidOperationException)
{
    return x == y;
}

It’s probably really worth leaving the old implementation and just adding a check for null to it.

[ClearScriptLib]

Thanks, Andrey!