Update SignatureComparer.TypeDefOrRef.cs

[Tabho]

Comparing by namespace is not enough, need to compare by declaring type too, because namespace can be null which way it leads to situation where two nested different types with same name can be compared as equal

[Washi1337]

Thanks for spotting the issue, however I don't think this is the right solution that tackles the problem at its core.

First, in the case of a type being nested, the Scope of the type is the declaring type. The Scope equality check that happens after the IsTypeOf call therefore already tests for declaring type. Furthermore, the test you are providing is very shallow. It does not test for the declaring type's namespace, nor it is recursively iterating over all declaring types, should there be nested types within nested types and so forth.

I believe the problem exists because the tests after the Resolve() calls don't test for the declaring type. Equals on the declaring types of the resolved definitions should probably be used instead,

Also, this would probably need a unit test before it should be merged.

[Tabho]

I have made another update hopefully it is better solution now. It is working for me well now.

There was another problem where System.Type is not equal to another System.Type because somewhere else in the code Culture is initialised with null and Utf8string.Empty != null

In order to prevent nested types being compared as equal i have added additional check for last pass to check if modules are the same before trying to resolve() and compare again

[Washi1337]

I have made another update hopefully it is better solution now. It is working for me well now.

It might be, but we should add a representative unit test so that this doesn't happen again in the future after someone makes another change.

There was another problem where System.Type is not equal to another System.Type because somewhere else in the code Culture is initialised with null and Utf8string.Empty != null

This is a separate issue and will need another unit test that represents this exact case.

In order to prevent nested types being compared as equal i have added additional check for last pass to check if modules are the same before trying to resolve() and compare again

I am not sure why we would want to prevent nested types being compared? We would probably want to recursively test whether the declaring types are matching, to support arbitrary amounts of nesting.

[Washi1337]

I think what is missing is a call Equals(definition1.DeclaringType, definition2.DeclaringType) after the Equals(definition1.Module!.Assembly, definition2.Module!.Assembly), since this will guarantee the check is done recursively for all possible levels of nesting. Testing this could then easily be done by inserting your example in your original issue into a unit test.

What do you think?

[Tabho]

I have added test code which will fail with current comparer code.

[Washi1337]

I believe your solution still does not tackle the problem at its core. Without testing for the declaring type itself, we still have a nasty edge case with nested types that are forwarded / exported. In this case, it is possible that x.Name == y.Name and x.Module != y.Module (e.g. if x is a type reference, y is the original definition), yet their declaring type is different. If we don't test for declaring type, the comparer would deem these two type instances equal when in reality they are not.

See below for test-case:

ActualLibrary.il

.assembly ActualLibrary {}

.class public abstract sealed MyClass
    extends [mscorlib] System.Object 
{
    .class nested public abstract sealed MyNestedClass
        extends [mscorlib] System.Object 
    {
        .method public static hidebysig void NestedStaticMethod()
        {
            ldstr "MyClass+MyNestedClass::NestedStaticMethod"
            call void [mscorlib] System.Console::WriteLine(string)
            ret 
        }
    }
}

.class public abstract sealed MyClass2
    extends [mscorlib] System.Object 
{
    .class nested public abstract sealed MyNestedClass
        extends [mscorlib] System.Object 
    {
        .method public static hidebysig void NestedStaticMethod()
        {
            ldstr "MyClass2+MyNestedClass::NestedStaticMethod"
            call void [mscorlib] System.Console::WriteLine(string)
            ret 
        }
    }
}

ForwarderLibrary.il

.assembly ForwarderLibrary {}

.assembly extern ActualLibrary
{
}

.class extern forwarder MyClass
{
	.assembly extern ActualLibrary
}

.class extern forwarder MyNestedClass
{
	.class extern MyClass
}

.class extern forwarder MyClass2
{
	.assembly extern ActualLibrary
}

.class extern forwarder MyNestedClass
{
	.class extern MyClass2
}

Main.il

.assembly Main {}

.assembly extern ForwarderLibrary
{
}

.method public static void Main()
{
    .entrypoint
    call void [ForwarderLibrary] MyClass/MyNestedClass::NestedStaticMethod()
    call void [ForwarderLibrary] MyClass2/MyNestedClass::NestedStaticMethod()
    ret
}

Compile using:

ilasm ActualLibrary.il /dll
ilasm ForwarderLibrary.il /dll
ilasm Main.il

repro.zip

Then test using code:

[Fact]
public void MatchForwardedNestedTypes()
{
    var module = ModuleDefinition.FromFile("/tmp/test/Main.exe");

    var referencedTypes = module.ManagedEntrypointMethod!.CilMethodBody!.Instructions
        .Where(i => i.OpCode.Code == CilCode.Call)
        .Select(i=> ((IMethodDefOrRef) i.Operand!).DeclaringType)
        .ToArray();

    var type1 = referencedTypes[0]!;
    var type2 = referencedTypes[1]!;

    var resolvedType1 = type1.Resolve();
    var resolvedType2 = type2.Resolve();

    Assert.Equal(type1, resolvedType1, _comparer);
    Assert.Equal(type2, resolvedType2, _comparer);

    Assert.NotEqual(type1, type2, _comparer);
    Assert.NotEqual(type1, resolvedType2, _comparer); // Fails
    Assert.NotEqual(type2, resolvedType1, _comparer); // Fails
}

[Tabho]

Please check the update

[Washi1337]

Yes this seems to solve the problem, the edge case passes now just fine. I will merge this and include it in hotfix 4.11.1 that I have planned to push in the next day or two.

Thanks again for spotting the bug!