Don't report MSTEST0065 on collection types that declare their own equality#9978
Conversation
…uality Suppress MSTEST0065 when the compared type implements IEquatable<itself> or overrides object.Equals. In that case the author has deliberately opted into a custom, non-reference equality that Assert.AreEqual honors via EqualityComparer<T>.Default, so suggesting a sequence/structural comparison second-guesses a deliberate decision. Fixes #9971 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
There was a problem hiding this comment.
Pull request overview
Suppresses MSTEST0065 for collection types with custom equality semantics, addressing #9971.
Changes:
- Detects self-typed
IEquatable<T>implementations andobject.Equalsoverrides. - Adds the
IEquatable<T>well-known type name. - Adds analyzer tests for equality scenarios.
Show a summary per file
| File | Description |
|---|---|
AvoidAssertAreEqualOnCollectionsAnalyzer.cs |
Adds custom-equality detection. |
WellKnownTypeNames.cs |
Registers IEquatable<T>. |
AvoidAssertAreEqualOnCollectionsAnalyzerTests.cs |
Tests diagnostic suppression and reporting. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Medium
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
This PR correctly suppresses MSTEST0045 for collection types that declare their own equality semantics (via IEquatable<self> or object.Equals override), fixing issue #9971. The implementation is clean and well-tested.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ Clean |
| 2 | Threading & Concurrency | N/A |
| 3 | Security & IPC Contract Safety | N/A |
| 4 | Public API & Binary Compatibility | ✅ Clean — no public API change |
| 5 | Performance & Allocations | |
| 6 | Cross-TFM Compatibility | ✅ Handled — equatableSymbol is nullable for old TFMs |
| 7 | Error Handling | N/A |
| 8 | Diagnostics & Observability | N/A |
| 9 | Naming & Coding Style | ✅ Clean |
| 10 | Documentation & Comments | ✅ Good inline comments |
| 11 | Test Quality | ✅ Four test cases covering key scenarios |
| 12 | Localization | N/A |
| 13 | Configuration & Options | N/A |
| 14 | Resource Management | N/A |
| 15 | Backward Compatibility | ✅ Fewer false positives only |
| 16 | Code Organization | ✅ Clean |
| 17 | Extensibility | N/A |
| 18 | Dependency Management | N/A |
| 19 | Build Infrastructure | N/A |
| 20 | CLI Options | N/A |
| 21 | Agentic Workflows | N/A |
| 22 | PowerShell Hygiene | N/A |
Overall: Looks good. One minor performance note below.
…onstraints, override root - Base the IEquatable/Equals opt-out on the selected generic type argument T (the equality contract EqualityComparer<T>.Default actually uses), so widening to a base type (AreEqual<object>) no longer loses the diagnostic. - Skip the opt-out when an explicit comparer argument is supplied. - Honor IEquatable<T> (and self-equating class) type-parameter constraints. - Follow the override chain to its root and require object.Equals, so a derived override of a base 'new virtual bool Equals(object)' is not mistaken for one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
This comment has been minimized.
This comment has been minimized.
…quals walk - Type-parameter constraints: keep T as the equality target. A where T : ISelf constraint where ISelf implements IEquatable<ISelf> no longer suppresses, since a concrete T need not implement IEquatable<T>. Only IEquatable<T> (self) constraints or class constraints overriding object.Equals suppress. - OverridesObjectEquals now stops before System.ValueType/System.Enum, so a struct collection with no equality of its own is still reported (ValueType.Equals is not the author's intentional equality). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
…dd coverage - Document that transitive type-parameter constraints are intentionally not followed. - Clarify the equatableSymbol==null comment (only IEquatable<self> becomes a no-op). - Add tests: explicit-interface IEquatable implementation (suppresses) and AreNotEqual widened-to-object (reports). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
This comment has been minimized.
This comment has been minimized.
…traints, comment fixes - HasCustomComparerArgument: a null-literal or explicit EqualityComparer<T>.Default comparer now keeps the equality opt-out (both dispatch to the default comparer per Assert.AreEqual.cs), only a genuinely custom comparer re-enables the diagnostic. - Type-parameter constraints are now traversed transitively (where T : U where U : ...), keeping T as the IEquatable equality target while honoring inherited object.Equals. - Reworded the struct fixture comment (no backing field) to describe ValueType's reflection-based field-wise equality accurately. - Added tests: custom comparer (reports), null/default comparer (suppress), transitive class constraint (suppress), IEquatable<Base>-only used as Derived (reports, since IEquatable<T> is invariant). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
… as the default HasCustomComparerArgument now uses IOperation.ConstantValue instead of matching only a null literal, so default, default(IEqualityComparer<T>), and const null fields are all recognized as the default comparer and keep the equality opt-out. Added a default(...) comparer test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
This comment has been minimized.
This comment has been minimized.
Peel only built-in conversions (not user-defined) when classifying the comparer argument, matching GetCollectionArgumentType and the runtime value. A user-defined conversion executes and can change the value, so it must stay opaque. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
This comment has been minimized.
This comment has been minimized.
…ected T IEqualityComparer<in T> is contravariant, so EqualityComparer<Base>.Default compiles as the comparer for a Derived argument but dispatches to Base's equality. Require the property's type argument to equal the selected T so such a base comparer is treated as custom and MSTEST0065 still fires. Added a variance regression test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d66f93-e4cf-45d3-bdde-07ee9f67833d
🧪 Test quality grade — PR #9978
This advisory comment was generated automatically. Grades are heuristic Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
/backport to rel/4.3 |
|
Started backporting to rel/4.3: https://github.com/microsoft/testfx/actions/runs/29490461211 |
…uality by @Evangelink in #9978 (backport to rel/4.3) (#10003) Co-authored-by: Amaury Levé <amauryleve@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes #9971
Problem
MSTEST0065 (
AvoidAssertAreEqualOnCollections) fires purely because the compared type implementsIEnumerable<T>. It doesn't check whether the type also defines its own equality. For a type that is both a collection and meaningfullyIEquatable<T>(or overridesobject.Equals),Assert.AreEqualis the correct call — but the rule flagged it anyway, producing false positives that forced people to disable the whole rule.Approach
The analyzer fundamentally can't read intent:
Assert.AreEqual(a, b)on a collection that is alsoIEquatable<T>is genuinely ambiguous (type equality vs. element/content equality). So the question isn't "how do we detect intent" but "which way do we err when we can't tell?".For a default-on Warning, false positives are more expensive than false negatives (a noisy warning gets suppressed wholesale). When a type implements
IEquatable<self>or overridesobject.Equals, the author deliberately opted into a custom, non-reference equality thatAssert.AreEqualhonors viaEqualityComparer<T>.Default— the strongest static signal we're not in accidental-reference-equality territory. So we suppress there.Changes
AvoidAssertAreEqualOnCollectionsAnalyzerno longer reports when the compared type declares its own equality. Precise semantics (mirroring(comparer ?? EqualityComparer<T>.Default).Equals(...)):T, not the argument's runtime collection type — widening (Assert.AreEqual<object>(c, c)) still reports.null/default/default(IEqualityComparer<T>)comparer or an explicitEqualityComparer<T>.Defaultargument is equivalent to the parameterless overload (Assertfalls back to the default comparer —Assert.AreEqual.cs:146), so those keep the opt-out.IEquatable<T>detection requires the exact self type argument.IEquatable<T>is invariant, so a collection implementing onlyIEquatable<Base>(used as a derived type) still uses reference equality viaEqualityComparer<Derived>.Defaultand is correctly reported; explicit-interface implementations count.object.Equalsdetection follows the override chain to its root and requires it to beSystem.Object.Equals(a derived override of a basenew virtual bool Equals(object)does not count), and stops beforeSystem.ValueType/System.Enumso struct collections with no equality of their own still report.where T : IEquatable<T>(self) or a class constraint overridingobject.Equalssuppress; a barewhere T : IEnumerable<...>, orwhere T : ISelfwhereISelf : IEquatable<ISelf>(a concreteTneed not implementIEquatable<T>), still report. Transitive type-parameter constraints (where T : U where U : ...) are followed, keepingTas the equality target.Known, accepted slippage
Types like
ImmutableArray<T>implementIEquatable<T>but compare by the underlying array reference, so they're no longer flagged even though element-wise is usually what you'd want. There's no static way to tell "IEquatable meaning content" from "IEquatable meaning identity", and chasing it just reintroduces intent-guessing.Verification
AvoidAssertAreEqualOnCollectionsAnalyzerTests: 49/49 pass (covers widened-to-object, custom/null/default/default(...)/explicit-default comparer,IEquatable<self>incl. explicit-interface andIEquatable<Base>-only,object.Equalsoverride,new virtual Equalsbase, struct with/without own equality, and the various direct and transitive type-parameter constraint forms — for bothAreEqualandAreNotEqual).