Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,16 @@ public override void Initialize(AnalysisContext context)
return;
}

context.RegisterOperationAction(context => AnalyzeInvocation(context, assertSymbol, genericEnumerableSymbol), OperationKind.Invocation);
// May be null on very old target frameworks; when it is, only the IEquatable<self> check becomes a
// no-op — the object.Equals override detection still runs.
INamedTypeSymbol? equatableSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemIEquatable1);
INamedTypeSymbol? equalityComparerSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericEqualityComparer1);

context.RegisterOperationAction(context => AnalyzeInvocation(context, assertSymbol, genericEnumerableSymbol, equatableSymbol, equalityComparerSymbol), OperationKind.Invocation);
});
}

private static void AnalyzeInvocation(OperationAnalysisContext context, INamedTypeSymbol assertSymbol, INamedTypeSymbol genericEnumerableSymbol)
private static void AnalyzeInvocation(OperationAnalysisContext context, INamedTypeSymbol assertSymbol, INamedTypeSymbol genericEnumerableSymbol, INamedTypeSymbol? equatableSymbol, INamedTypeSymbol? equalityComparerSymbol)
{
var invocation = (IInvocationOperation)context.Operation;
IMethodSymbol targetMethod = invocation.TargetMethod;
Expand Down Expand Up @@ -89,6 +94,23 @@ targetMethod.Name is not ("AreEqual" or "AreNotEqual") ||
// patterns where the caller widened to a non-collection type (e.g. `Assert.AreEqual<object>(arr1, arr2)`
// or `Assert.AreEqual((object)arr1, (object)arr2)`) which would otherwise silently use reference equality.
ITypeSymbol comparedType = targetMethod.TypeArguments[0];

// Opt out when the compared type declares its own equality (implements IEquatable&lt;itself&gt; or overrides
// object.Equals). The author has then deliberately chosen a custom, non-reference equality that Assert.AreEqual
// honors via EqualityComparer&lt;T&gt;.Default, so suggesting a sequence/structural comparison would second-guess
// a deliberate decision (see issue #9971). Two things are important here:
// * The check must be based on the *selected generic type argument* T, not the argument's runtime collection
// type. Widening to a base type (e.g. Assert.AreEqual&lt;object&gt;(collection, collection)) discards the
// collection's IEquatable&lt;self&gt; and falls back to reference equality — exactly the footgun the rule targets.
// * It must not apply when the caller supplies a *custom* comparer, because then EqualityComparer&lt;T&gt;.Default
// (and therefore the type's own equality) is not used at all. A `null` comparer or an explicit
// `EqualityComparer&lt;T&gt;.Default` argument is equivalent to the parameterless overload (Assert falls back to
// the default comparer — see Assert.AreEqual.cs), so those keep the opt-out.
if (!HasCustomComparerArgument(invocation, equalityComparerSymbol) && DeclaresOwnEquality(comparedType, equatableSymbol))
{
return;
}

ITypeSymbol? reportedType = ShouldReport(comparedType, genericEnumerableSymbol)
? comparedType
: GetCollectionArgumentType(invocation, firstParameterName, genericEnumerableSymbol)
Expand All @@ -104,6 +126,35 @@ targetMethod.Name is not ("AreEqual" or "AreNotEqual") ||
context.ReportDiagnostic(invocation.CreateDiagnostic(Rule, methodName, comparedTypeDisplay));
}

// Returns true only when a `comparer` argument is supplied that is not provably the default comparer.
// `null` and `EqualityComparer<T>.Default` both dispatch to EqualityComparer<T>.Default at runtime, so they must
// keep the equality opt-out; any other comparer bypasses the type's own equality and should re-enable the diagnostic.
private static bool HasCustomComparerArgument(IInvocationOperation invocation, INamedTypeSymbol? equalityComparerSymbol)
{
foreach (IArgumentOperation argument in invocation.Arguments)
{
if (argument.Parameter?.Name != "comparer")
{
continue;
}

IOperation value = argument.Value.WalkDownConversion();
Comment thread
Evangelink marked this conversation as resolved.
Outdated

// Any constant-null comparer (null literal, `default`, `default(IEqualityComparer<T>)`, a const null field, …)
// falls back to EqualityComparer<T>.Default in Assert, and a reference to `EqualityComparer<T>.Default` is that
// same default comparer — neither bypasses the type's own equality.
bool isDefaultComparer =
value.ConstantValue is { HasValue: true, Value: null }
|| (value is IPropertyReferenceOperation { Property: { Name: "Default", IsStatic: true } property }
&& equalityComparerSymbol is not null
&& SymbolEqualityComparer.Default.Equals(property.ContainingType.OriginalDefinition, equalityComparerSymbol));
Comment thread
Evangelink marked this conversation as resolved.
Outdated

return !isDefaultComparer;
}
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

return false;
}

private static ITypeSymbol? GetCollectionArgumentType(IInvocationOperation invocation, string parameterName, INamedTypeSymbol genericEnumerableSymbol)
{
IArgumentOperation? argument = invocation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == parameterName);
Expand All @@ -122,6 +173,115 @@ private static bool ShouldReport(ITypeSymbol comparedType, INamedTypeSymbol gene
=> comparedType.SpecialType != SpecialType.System_String
&& ImplementsGenericEnumerable(comparedType, genericEnumerableSymbol);

private static bool DeclaresOwnEquality(ITypeSymbol type, INamedTypeSymbol? equatableSymbol)
=> type switch
{
INamedTypeSymbol namedType => ImplementsSelfEquatable(namedType, namedType, equatableSymbol) || OverridesObjectEquals(namedType),

// The equality target is the type parameter T itself (the type whose EqualityComparer<T>.Default is used).
ITypeParameterSymbol typeParameter => TypeParameterDeclaresOwnEquality(typeParameter, typeParameter, equatableSymbol),

_ => false,
};

// EqualityComparer&lt;T&gt;.Default honors a `where T : IEquatable<T>` constraint, and a class constraint that overrides
// object.Equals is inherited by every T. A bare `where T : IEnumerable<...>` constraint has neither, so it stays the
// reference-equality footgun we still want to flag.
//
// `equalityTarget` stays the original type parameter T while we traverse constraints, including transitive
// type-parameter constraints (`where T : U where U : ...`). The IEquatable check must be against T: for
// `where T : ISelf` with `ISelf : IEquatable<ISelf>`, a concrete T need not implement IEquatable&lt;T&gt;, so it would
// still use reference equality. An object.Equals override on any reachable class constraint is inherited by T and so
// is honored regardless of the equality target.
Comment thread
Evangelink marked this conversation as resolved.
private static bool TypeParameterDeclaresOwnEquality(ITypeParameterSymbol typeParameter, ITypeSymbol equalityTarget, INamedTypeSymbol? equatableSymbol)
{
foreach (ITypeSymbol constraintType in typeParameter.ConstraintTypes)
{
switch (constraintType)
{
case INamedTypeSymbol namedConstraint when ImplementsSelfEquatable(namedConstraint, equalityTarget, equatableSymbol) || OverridesObjectEquals(namedConstraint):
return true;

case ITypeParameterSymbol nestedTypeParameter when TypeParameterDeclaresOwnEquality(nestedTypeParameter, equalityTarget, equatableSymbol):
return true;
}
}

return false;
}

// Returns true when `candidate` is or implements IEquatable&lt;equalityType&gt;, i.e. the equality contract that
// EqualityComparer&lt;equalityType&gt;.Default would dispatch to. `equalityType` is the type whose default comparer
// is used (the compared type or the type parameter); `candidate` is the type (or constraint) we inspect.
private static bool ImplementsSelfEquatable(INamedTypeSymbol candidate, ITypeSymbol equalityType, INamedTypeSymbol? equatableSymbol)
{
if (equatableSymbol is null)
{
return false;
}

// The candidate can be the IEquatable&lt;T&gt; interface itself (e.g. a `where T : IEquatable<T>` constraint)
// or a type that lists it among its implemented interfaces (e.g. `class C : IEquatable<C>`).
if (IsEquatableOf(candidate, equalityType, equatableSymbol))
{
return true;
}

foreach (INamedTypeSymbol implemented in candidate.AllInterfaces)
{
if (IsEquatableOf(implemented, equalityType, equatableSymbol))
{
return true;
}
}
Comment thread
Evangelink marked this conversation as resolved.

return false;
}

private static bool IsEquatableOf(INamedTypeSymbol type, ITypeSymbol equalityType, INamedTypeSymbol equatableSymbol)
=> SymbolEqualityComparer.Default.Equals(type.OriginalDefinition, equatableSymbol)
&& type.TypeArguments.Length == 1
&& SymbolEqualityComparer.Default.Equals(type.TypeArguments[0], equalityType);
Comment thread
Evangelink marked this conversation as resolved.

private static bool OverridesObjectEquals(INamedTypeSymbol type)
{
// Stop before object AND ValueType: System.ValueType overrides object.Equals, so walking into it would treat
// every struct that implements IEnumerable<T> as declaring its own equality even when it has no Equals of its
// own (its backing array/list field would then be compared by reference). An explicit struct override is found
// on the concrete type before we reach ValueType.
for (INamedTypeSymbol? current = type;
current is not null && current.SpecialType is not (SpecialType.System_Object or SpecialType.System_ValueType or SpecialType.System_Enum);
current = current.BaseType)
{
foreach (ISymbol member in current.GetMembers(nameof(Equals)))
{
if (member is IMethodSymbol { IsOverride: true, Parameters.Length: 1, ReturnType.SpecialType: SpecialType.System_Boolean } method &&
method.Parameters[0].Type.SpecialType == SpecialType.System_Object &&
OverrideRootIsObjectEquals(method))
{
return true;
}
}
Comment thread
Evangelink marked this conversation as resolved.
}

return false;
}

// `IsOverride` only proves the method overrides *some* virtual slot. A base class can declare
// `new virtual bool Equals(object)`, and a derived override of that member is not an override of
// object.Equals — EqualityComparer&lt;T&gt;.Default still dispatches the unchanged object.Equals slot
// (reference equality). Follow the override chain to its root and require it to be object.Equals.
private static bool OverrideRootIsObjectEquals(IMethodSymbol method)
{
IMethodSymbol root = method;
while (root.OverriddenMethod is { } overridden)
{
root = overridden;
}

return root.ContainingType?.SpecialType == SpecialType.System_Object;
}

private static bool HasNullLiteralArgument(IInvocationOperation invocation, string parameterName)
{
IArgumentOperation? argument = invocation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == parameterName);
Expand Down
2 changes: 2 additions & 0 deletions src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,14 @@ internal static class WellKnownTypeNames
public const string MicrosoftVisualStudioTestToolsUnitTestingWorkItemAttribute = "Microsoft.VisualStudio.TestTools.UnitTesting.WorkItemAttribute";

public const string System = "System";
public const string SystemCollectionsGenericEqualityComparer1 = "System.Collections.Generic.EqualityComparer`1";
public const string SystemCollectionsGenericIEnumerable1 = "System.Collections.Generic.IEnumerable`1";
public const string SystemCollectionsIDictionary = "System.Collections.IDictionary";
public const string SystemDescriptionAttribute = "System.ComponentModel.DescriptionAttribute";
public const string SystemFunc1 = "System.Func`1";
public const string SystemIAsyncDisposable = "System.IAsyncDisposable";
public const string SystemIDisposable = "System.IDisposable";
public const string SystemIEquatable1 = "System.IEquatable`1";
public const string SystemLinqEnumerable = "System.Linq.Enumerable";
public const string SystemLinqExpressionsExpression1 = "System.Linq.Expressions.Expression`1";
public const string SystemOperatingSystem = "System.OperatingSystem";
Expand Down
Loading
Loading