Skip to content

Commit

Permalink
Fix compareTypesForEquality (dotnet#97062)
Browse files Browse the repository at this point in the history
  • Loading branch information
jkotas authored and tmds committed Jan 23, 2024
1 parent 75a89d9 commit 6a30885
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 47 deletions.
73 changes: 49 additions & 24 deletions src/coreclr/tools/Common/Compiler/TypeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -182,43 +182,68 @@ public static bool IsArrayTypeWithoutGenericInterfaces(this TypeDesc type)

public static bool? CompareTypesForEquality(TypeDesc type1, TypeDesc type2)
{
bool? result = null;

// If neither type is a canonical subtype, type handle comparison suffices
if (!type1.IsCanonicalSubtype(CanonicalFormKind.Any) && !type2.IsCanonicalSubtype(CanonicalFormKind.Any))
{
result = type1 == type2;
return type1 == type2;
}

// If either or both types are canonical subtypes, we can sometimes prove inequality.
else
if (AreGuaranteedToRepresentDifferentTypes(type1, type2))
{
return false;
}

return null;

static bool AreGuaranteedToRepresentDifferentTypes(TypeDesc type1, TypeDesc type2)
{
// If either is a value type then the types cannot
// be equal unless the type defs are the same.
if (type1.IsValueType || type2.IsValueType)
if (type1.IsCanonicalDefinitionType(CanonicalFormKind.Any) || type2.IsCanonicalDefinitionType(CanonicalFormKind.Any))
{
if (!type1.IsCanonicalDefinitionType(CanonicalFormKind.Universal) && !type2.IsCanonicalDefinitionType(CanonicalFormKind.Universal))
{
if (!type1.HasSameTypeDefinition(type2))
{
result = false;
}
}
// Universal canonical definition can match any type. We can't prove inequality.
if (type1.IsCanonicalDefinitionType(CanonicalFormKind.Universal) || type2.IsCanonicalDefinitionType(CanonicalFormKind.Universal))
return false;

return type1.IsGCPointer != type2.IsGCPointer;
}
// If we have two ref types that are not __Canon, then the
// types cannot be equal unless the type defs are the same.
else

TypeFlags category = type1.Category;
if (category != type2.Category)
return true;

switch (category)
{
if (!type1.IsCanonicalDefinitionType(CanonicalFormKind.Any) && !type2.IsCanonicalDefinitionType(CanonicalFormKind.Any))
{
if (!type1.HasSameTypeDefinition(type2))
case TypeFlags.Array:
if (((ArrayType)type1).Rank != ((ArrayType)type2).Rank)
return true;
return AreGuaranteedToRepresentDifferentTypes(((ArrayType)type1).ElementType, ((ArrayType)type2).ElementType);
case TypeFlags.SzArray:
case TypeFlags.ByRef:
case TypeFlags.Pointer:
return AreGuaranteedToRepresentDifferentTypes(((ParameterizedType)type1).ParameterType, ((ParameterizedType)type2).ParameterType);

default:
if (type1.IsDefType || type2.IsDefType)
{
result = false;
if (!type1.HasSameTypeDefinition(type2))
return true;

Instantiation inst1 = type1.Instantiation;
if (inst1.Length != 0)
{
var inst2 = type2.Instantiation;
Debug.Assert(inst1.Length == inst2.Length);
for (int i = 0; i < inst1.Length; i++)
{
if (AreGuaranteedToRepresentDifferentTypes(inst1[i], inst2[i]))
return true;
}
}
}
}
break;
}
return false;
}

return result;
}

public static TypeDesc MergeTypesToCommonParent(TypeDesc ta, TypeDesc tb)
Expand Down
79 changes: 58 additions & 21 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4448,6 +4448,61 @@ TypeCompareState CEEInfo::compareTypesForCast(
return result;
}

// Returns true if hnd1 and hnd2 are guaranteed to represent different types. Returns false if hnd1 and hnd2 may represent the same type.
static bool AreGuaranteedToRepresentDifferentTypes(TypeHandle hnd1, TypeHandle hnd2)
{
STANDARD_VM_CONTRACT;

CorElementType et1 = hnd1.GetSignatureCorElementType();
CorElementType et2 = hnd2.GetSignatureCorElementType();

TypeHandle canonHnd = TypeHandle(g_pCanonMethodTableClass);
if ((hnd1 == canonHnd) || (hnd2 == canonHnd))
{
return CorTypeInfo::IsObjRef(et1) != CorTypeInfo::IsObjRef(et2);
}

if (et1 != et2)
{
return true;
}

switch (et1)
{
case ELEMENT_TYPE_ARRAY:
if (hnd1.GetRank() != hnd2.GetRank())
return true;
return AreGuaranteedToRepresentDifferentTypes(hnd1.GetTypeParam(), hnd2.GetTypeParam());
case ELEMENT_TYPE_SZARRAY:
case ELEMENT_TYPE_BYREF:
case ELEMENT_TYPE_PTR:
return AreGuaranteedToRepresentDifferentTypes(hnd1.GetTypeParam(), hnd2.GetTypeParam());

default:
if (!hnd1.IsTypeDesc())
{
if (!hnd1.AsMethodTable()->HasSameTypeDefAs(hnd2.AsMethodTable()))
return true;

if (hnd1.AsMethodTable()->HasInstantiation())
{
Instantiation inst1 = hnd1.AsMethodTable()->GetInstantiation();
Instantiation inst2 = hnd2.AsMethodTable()->GetInstantiation();
_ASSERTE(inst1.GetNumArgs() == inst2.GetNumArgs());

for (DWORD i = 0; i < inst1.GetNumArgs(); i++)
{
if (AreGuaranteedToRepresentDifferentTypes(inst1[i], inst2[i]))
return true;
}
}
}
break;
}

return false;
}

/*********************************************************************/
// See if types represented by cls1 and cls2 compare equal, not
// equal, or the comparison needs to be resolved at runtime.
Expand All @@ -4473,30 +4528,12 @@ TypeCompareState CEEInfo::compareTypesForEquality(
{
result = (hnd1 == hnd2 ? TypeCompareState::Must : TypeCompareState::MustNot);
}
// If either or both types are canonical subtypes, we can sometimes prove inequality.
else
{
// If either is a value type then the types cannot
// be equal unless the type defs are the same.
if (hnd1.IsValueType() || hnd2.IsValueType())
// If either or both types are canonical subtypes, we can sometimes prove inequality.
if (AreGuaranteedToRepresentDifferentTypes(hnd1, hnd2))
{
if (!hnd1.GetMethodTable()->HasSameTypeDefAs(hnd2.GetMethodTable()))
{
result = TypeCompareState::MustNot;
}
}
// If we have two ref types that are not __Canon, then the
// types cannot be equal unless the type defs are the same.
else
{
TypeHandle canonHnd = TypeHandle(g_pCanonMethodTableClass);
if ((hnd1 != canonHnd) && (hnd2 != canonHnd))
{
if (!hnd1.GetMethodTable()->HasSameTypeDefAs(hnd2.GetMethodTable()))
{
result = TypeCompareState::MustNot;
}
}
result = TypeCompareState::MustNot;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ public static Span<T> AsSpan<T>(List<T>? list)
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}

// Commented out to workaround https://github.com/dotnet/runtime/issues/96876
// Debug.Assert(typeof(T[]) == list._items.GetType(), "Implementation depends on List<T> always using a T[] and not U[] where U : T.");
Debug.Assert(typeof(T[]) == list._items.GetType(), "Implementation depends on List<T> always using a T[] and not U[] where U : T.");
span = new Span<T>(ref MemoryMarshal.GetArrayDataReference(items), size);
}

Expand Down
26 changes: 26 additions & 0 deletions src/tests/Regressions/coreclr/GitHub_96876/test96876.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System;
using System.Runtime.CompilerServices;
using Xunit;

public class Test96876
{
[Fact]
public static void TestEntryPoint()
{
Assert.True(foo<string>(new string[1]));
Assert.False(foo<object>(new string[1]));

Assert.True(foo2<string>());
Assert.False(foo2<object>());
}

// Validate that the type equality involving shared array types is handled correctly
// in shared generic code.
[MethodImpl(MethodImplOptions.NoInlining)]
static bool foo<T>(string[] list) => typeof(T[]) == list.GetType();

[MethodImpl(MethodImplOptions.NoInlining)]
static bool foo2<T>() => typeof(T[]) == typeof(string[]);
}
11 changes: 11 additions & 0 deletions src/tests/Regressions/coreclr/GitHub_96876/test96876.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<ItemGroup>
<Compile Include="test96876.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
</ItemGroup>
</Project>

0 comments on commit 6a30885

Please sign in to comment.