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

Fix CompareReferenceOnly #524

Closed

Conversation

CreateAndInject
Copy link
Contributor

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.

@CreateAndInject CreateAndInject force-pushed the FixCompareReferenceOnly branch from 4d0125a to 74554c9 Compare August 29, 2023 09:59
@ElektroKill
Copy link
Contributor

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
Copy link
Contributor

wtfsck commented Aug 30, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

CreateAndInject commented Aug 31, 2023

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
Copy link
Contributor

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
Copy link
Contributor

wtfsck commented Aug 31, 2023

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

@ElektroKill
Copy link
Contributor

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

bool CompareReferenceOnlyForMemberDefsInSameModule => (options & SigComparerOptions.DisableCompareReferenceOnlyForMemberDefsInSameModule) == 0;

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
Copy link
Contributor Author

CreateAndInject commented Aug 31, 2023

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

bool CompareReferenceOnlyForMemberDefsInSameModule => (options & SigComparerOptions.DisableCompareReferenceOnlyForMemberDefsInSameModule) == 0;

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
Copy link
Contributor

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
Copy link
Contributor Author

CreateAndInject commented Aug 31, 2023

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
Copy link
Contributor

ElektroKill commented Aug 31, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@wtfsck wtfsck closed this in #525 Sep 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants