Fix CompareReferenceOnly

[CreateAndInject]

We can't compare reference only for eg. MethodEqualityComparer.DontCompareDeclaringTypes, otherwise de4dot renaming will be failed for virtual methods. Seems we have to fix dnlib 4.0 nuget immediately, lots of projects don't run when updating to dnlib 4.0.

[ElektroKill]

You can fix this issue in de4dot by adding the newly added flag to the signature comparer wherever necessary. The reason for this issue is that (I assume, I don’t remember de4dot code of the top of my head) de4dot uses the comparer to determine whether a method can be a valid overload by having the comparer compare the signature and name of the method. After this new option was added, the default behavior changed from name/signature comparison to reference comparison which is not what de4dot code expects. This is exactly why I suggested adding a option to disable this new behavior in the original PR to allow code which relied on the old behavior to be updated slightly to still use old behavior. The new version is not and cannot be treated as a drop in replacement and we should not be updating dnlib to fix porting issues.

TLDR: This is a porting issue and not a dnlib issue. It was well known from the moment the initial change to SigComparer that 4.0 is not a drop in replacement.

[wtfsck]

I agree, if it can be fixed by updating the de4dot code to pass in the new flag then that code should be updated. Or keep using v3.x instead.

[CreateAndInject]

I agree, if it can be fixed by updating the de4dot code to pass in the new flag then that code should be updated. Or keep using v3.x instead.

What's the purpose of DontCompareDeclaringTypes? It's used to let members in inheritance tree are equivalence:

interface IComparable {
    int CompareTo(object obj);
}

class Base : IComparable {
    public virtual int CompareTo(object obj) => throw new NotImplementedException();
}

class Derived : Base {
    public override int CompareTo(object obj) => throw new NotImplementedException();
}

I made a bug in previous PR in case that DontCompareDeclaringTypes no longer work.

[ElektroKill]

I agree, if it can be fixed by updating the de4dot code to pass in the new flag then that code should be updated. Or keep using v3.x instead.

What's the purpose of DontCompareDeclaringTypes? It's used to let members in inheritance tree are equivalence:

interface IComparable {
  int CompareTo(object obj);
}

class Base : IComparable {
  public virtual int CompareTo(object obj) => throw new NotImplementedException();
}

class Derived : Base {
  public override int CompareTo(object obj) => throw new NotImplementedException();
}

I made a bug in previous PR in case that DontCompareDeclaringTypes no longer work.

It works as long as you also disable reference comparison using the option added in the previous PR!

[CreateAndInject]

DontCompareDeclaringTypes is a member of dnlib.DotNet.MethodEqualityComparer in dnlib:

/// <summary>
/// Doesn't compare the declaring types
/// </summary>
public static readonly MethodEqualityComparer DontCompareDeclaringTypes = new MethodEqualityComparer(0);

This one will lose efficacy in 4.0, since it uses 0 as SigComparerOptions

[ElektroKill]

Indeed, I believe the proper solution in this case is to change the implementation of reference comparison to an OPT-IN feature rather than an OPT-OUT feature. Such a change would make the existing code function like before and all the properties in the different compares would work as expected. Changing it like this would also make the behavior much clearer rather than having this weird and not really clear reliance on the declaring type flag. This would make the change more of a drop in replacement while allowing users interested in reference comparison to use it. This would be a breaking change however so I'm not entirely sure how we should proceed with fixing these inconsistencies within dnlib 4.0.

[wtfsck]

4.0 was released 6 days ago, so not likely to break anything if we change something quickly and release 4.1.

[ElektroKill]

4.0 was released 6 days ago, so not likely to break anything if we change something quickly and release 4.1.

Yes, I'd agree that we can still squeeze in a breaking change in a 4.1 release due to not many people adopting t the new behavior.

I think the best solution would be to change the DisableCompareReferenceOnlyForMemberDefsInSameModule flag to CompareReferenceOnlyForMemberDefsInSameModule and change https://github.com/0xd4d/dnlib/blob/aa9ac47a38211b5f24d4b92a2044bd6ca334b904/src/DotNet/SigComparer.cs#L544 to something like bool CompareReferenceOnlyForMemberDefsInSameModule => (options & SigComparerOptions.CompareReferenceOnlyForMemberDefsInSameModule) != 0; which effectively makes this new reference comparison behavior opt-in preserving the old pre-4.0 behavior and making all existing predefined compares work as expected.

[CreateAndInject]

4.0 was released 6 days ago, so not likely to break anything if we change something quickly and release 4.1.

Yes, I'd agree that we can still squeeze in a breaking change in a 4.1 release due to not many people adopting t the new behavior.

I think the best solution would be to change the DisableCompareReferenceOnlyForMemberDefsInSameModule flag to CompareReferenceOnlyForMemberDefsInSameModule and change

https://github.com/0xd4d/dnlib/blob/aa9ac47a38211b5f24d4b92a2044bd6ca334b904/src/DotNet/SigComparer.cs#L544

to something like bool CompareReferenceOnlyForMemberDefsInSameModule => (options & SigComparerOptions.CompareReferenceOnlyForMemberDefsInSameModule) != 0; which effectively makes this new reference comparison behavior opt-in preserving the old pre-4.0 behavior and making all existing predefined compares work as expected.

If we add CompareReferenceOnlyForMemberDefsInSameModule instread of DisableCompareReferenceOnlyForMemberDefsInSameModule, it means we can't treat members in inheritance tree as equivalence(We need this behavior even if in obfuscated assembly). Instead, use DisableCompareReferenceOnlyForMemberDefsInSameModule, means CompareReferenceOnly is optional rather than required, so we can decide if use this behavior as needed if DisableCompareReferenceOnlyForMemberDefsInSameModule isn't set. Let's say: CompareReferenceOnly is used for obfuscated assemblies only which members contain same signature, DontCompareDeclaringTypes is a normal behavior, we need it in both normal and obfuscated assemblies. In the previous PR, CompareReferenceOnly bring 1 better but 100 worse, I am a sinner who bring a bug.

[ElektroKill]

If we apply the changes I mentioned, the behavior without the flag will be the same as before your previous PR (same as in dnlib 3.6). The changes will also allow users to use reference comparison whenever they want it to be used since there are legitimate usages for reference comparison. I don't see any issue with my proposed solution.

[CreateAndInject]

Use CompareReferenceOnlyForMemberDefsInSameModule option means it's not enabled by default, I think we shouldn't use it unless we can't resolve the bug I mentioned If this PR #524 can resolve the bug which previous PR produced and nothing worse, people should be more glad to use new SigComparer() than new SigComparer(SigComparerOptions.CompareReferenceOnlyForMemberDefsInSameModule)

[ElektroKill]

Use CompareReferenceOnlyForMemberDefsInSameModule option means it's not enabled by default, I think we shouldn't use it unless we can't resolve the bug I mentioned If this PR #524 can resolve the bug which previous PR produced and nothing worse, people should be more glad to use new SigComparer() than new SigComparer(SigComparerOptions.CompareReferenceOnlyForMemberDefsInSameModule)

The issue with the current implementation of this PR is that it can be confusing and less clear to the user. I think making it opt-in is better for user experience and clarity of the API. It also increases maintainability as we don't have to worry about the relation of DisableCompareReferenceOnlyForMemberDefsInSameModule and the compare declaring type options. My implementation also has the added benefit of backward compatibility with dnlib 3.x which means that if any other breaking issues arise with this new feature, we don't have to push another update.

[CreateAndInject]

How to implement isn't matter, the most important thing is that it's really a bug of dnlib, not something we should fix in de4dot. If we don't think it's a bug, we should delete MethodEqualityComparer.DontCompareDeclaringTypes from dnlib, since it's no longer work.

[ElektroKill]

How to implement isn't matter, the most important thing is that it's really a bug of dnlib, not something we should fix in de4dot. If we don't think it's a bug, we should delete MethodEqualityComparer.DontCompareDeclaringTypes from dnlib, since it's no longer work.

We already agreed that this is an issue/bug created by the previous PR which implemented reference comparison. When I made my initial comment I didn’t consider the existing predefined comparer properties and hence the idea of it just being a porting mistake came up. I know it needs to be fixed very well :p I just disagree to the way you’ve fixed it in the current PR and instead suggest a opt in option for reference comparison and perhaps maybe some new predefined comparer properties for reference equality!

[ElektroKill]

Due to the urgency of this issue, I have opened PR #525 to demonstrate the alternative fix I proposed in the comments here.