Flag Assert.AreEqual(x, x) / AreSame(x, x) / AreNotEqual(x, x) / AreNotSame(x, x) (#9087)#9088
Flag Assert.AreEqual(x, x) / AreSame(x, x) / AreNotEqual(x, x) / AreNotSame(x, x) (#9087)#9088Evangelink wants to merge 4 commits into
Assert.AreEqual(x, x) / AreSame(x, x) / AreNotEqual(x, x) / AreNotSame(x, x) (#9087)#9088Conversation
…cal operands
Extends MSTEST0032 (always-true assertions) and MSTEST0025 (always-false
assertions) to detect when the same local, parameter, field, or property
reference is passed for both the expected/notExpected and actual arguments.
Previously, both analyzers only reported when the two operands were
compile-time constants, missing the common bug pattern:
Assert.AreEqual(x, x); // always passes -- bug
Assert.AreSame(x, x); // always passes -- bug
Assert.AreNotEqual(x, x); // always fails -- bug
Assert.AreNotSame(x, x); // always fails -- bug
A new IOperation.IsEquivalentReferenceTo extension walks down conversions
and parentheses then compares two operations structurally by symbol
equality across local/parameter/field/property/this references (including
matching receiver chains). Method invocations and indexer accesses are
intentionally excluded so that GetX() == GetX() is not flagged.
Fixes #9087
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the MSTest analyzers MSTEST0032 (always-true asserts) and MSTEST0025 (always-false asserts) to flag assertions where the expected/notExpected and actual operands are the same reference expression (e.g., Assert.AreEqual(x, x)), even when operands are not compile-time constants.
Changes:
- Added
IOperation.IsEquivalentReferenceTo(...)to detect equivalent side-effect-free reference expressions (locals/params/fields/properties/this), while intentionally excluding invocations/indexers. - Updated MSTEST0032 and MSTEST0025 analyzers to use the new reference-equivalence logic for
AreEqual/AreSameandAreNotEqual/AreNotSamerespectively. - Added new unit tests covering identical locals/params/fields/properties (and selected no-diagnostic cases) for both analyzers.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs | Adds coverage for MSTEST0032 on identical operands for AreEqual/AreSame. |
| test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs | Adds coverage for MSTEST0025 on identical operands for AreNotEqual/AreNotSame and expected code-fix output. |
| src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs | Introduces the IsEquivalentReferenceTo helper used to detect identical reference expressions. |
| src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs | Extends MSTEST0032 to flag identical-operand AreEqual/AreSame. |
| src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs | Extends MSTEST0025 to flag identical-operand AreNotEqual/AreNotSame. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 4
| private static bool HasIdenticalExpectedAndActual(IInvocationOperation operation, string expectedOrNotExpectedParameterName) | ||
| => GetArgumentWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument | ||
| && GetArgumentWithName(operation, ActualParameterName) is { } actualArgument | ||
| && expectedArgument.IsEquivalentReferenceTo(actualArgument); |
There was a problem hiding this comment.
Good catch -- fixed in e6f5d0d. IsEquivalentReferenceTo's inner Unwrap now stops at user-defined conversions (conversion.Conversion.IsUserDefined), and both analyzers now pass the raw arg.Value to it via a new GetRawArgumentValueWithName helper instead of going through GetArgumentWithName (which strips all conversions). Added a regression test using a Wrapper with a side-effecting explicit operator int to lock the behavior in.
There was a problem hiding this comment.
Fixed in e6f5d0d by @Evangelink. HasIdenticalExpectedAndActual now uses a new GetRawArgumentValueWithName helper that returns the raw argument value (no WalkDownConversion), so user-defined conversions reach IsEquivalentReferenceTo and prevent the false positive on Assert.AreEqual((MyType)x, (MyType)x). Regression tests added. Review reply handled.
| private static bool HasIdenticalExpectedAndActual(IInvocationOperation operation, string expectedOrNotExpectedParameterName) | ||
| => GetArgumentWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument | ||
| && GetArgumentWithName(operation, ActualParameterName) is { } actualArgument | ||
| && expectedArgument.IsEquivalentReferenceTo(actualArgument); |
There was a problem hiding this comment.
Fixed in e6f5d0d -- same fix as the analyzer-side comment: HasIdenticalExpectedAndActual now uses GetRawArgumentValueWithName (no WalkDownConversion) and IsEquivalentReferenceTo only unwraps built-in conversions. Regression test added covering AreNotEqual((int)wrapper, (int)wrapper) with a user-defined explicit operator int.
There was a problem hiding this comment.
Same fix as #3404407789 -- PreferAssertFailOverAlwaysFalseConditionsAnalyzer.HasIdenticalExpectedAndActual was updated identically in e6f5d0d with a parallel GetRawArgumentValueWithName helper and an Assert.AreNotEqual regression test. Review reply handled.
| /// <remarks> | ||
| /// This intentionally excludes method invocations and indexer accesses, since those may | ||
| /// have side effects or return different values on repeated evaluation. Parenthesized | ||
| /// expressions and conversions are skipped transparently. |
There was a problem hiding this comment.
Fixed in e6f5d0d. The <remarks> block now reads: Parenthesized expressions and *built-in* conversions are skipped transparently, but user-defined conversions are treated as opaque because their operators may execute arbitrary code with side effects.
There was a problem hiding this comment.
Updated in e6f5d0d -- the remarks now state that only built-in conversions are skipped transparently and that user-defined conversions are treated as opaque because their operators may execute arbitrary code with side effects. Review reply handled.
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | 🟠 1 MAJOR |
| 13 | Test Completeness & Coverage | 🟡 1 MODERATE |
| 17 | Documentation Accuracy | 🔵 1 NIT |
| 18 | Analyzer & Code Fix Quality | 🟠 1 MAJOR |
✅ 18/22 dimensions clean.
- Correctness / Analyzer Quality —
IPropertyReferenceOperationarm doesn't exclude custom indexers (Property.IsIndexer == true);Assert.AreEqual(list[0], list[1])is falsely flagged as MSTEST0032 "always true" whenlistis the same variable but indices differ. See inline comment. - Test Completeness — No no-diagnostic test covering a custom-indexer case (e.g.
Assert.AreEqual(list[0], list[1])), which would have caught the above bug. - Documentation Accuracy — The
<remarks>onIsEquivalentReferenceTocorrectly states "This intentionally excludes ... indexer accesses" but the code does not enforce this forIPropertyReferenceOperationwithIsIndexer == true.
Detailed findings
Dimensions 1 + 18 — MAJOR: Indexer false-positive (inline comment)
See the inline comment on IOperationExtensions.cs lines 92–94 for the full scenario, proof-of-concept, and recommended fix (!pra.Property.IsIndexer guard).
The core issue: in Roslyn, C#-defined indexers (List<T>, Dictionary<K,V>, ImmutableArray<T>, and any public T this[...] accessor) produce IPropertyReferenceOperation nodes. The switch arm compares only the property symbol and receiver — it never compares the index arguments. Two calls with the same receiver but different index values (e.g., list[0] vs. list[1]) pass the equivalence check and trigger a spurious diagnostic.
Note: plain array access (arr[i]) creates IArrayElementReferenceOperation and is already excluded by the _ => false default. Only custom indexers are affected.
Recommended fix:
(IPropertyReferenceOperation pra, IPropertyReferenceOperation prb) =>
!pra.Property.IsIndexer &&
SymbolEqualityComparer.Default.Equals(pra.Property, prb.Property) &&
AreInstancesEquivalent(pra.Instance, prb.Instance),Dimension 13 — MODERATE: Missing no-diagnostic test for indexers
Add at minimum one no-diagnostic test in both ReviewAlwaysTrueAssertConditionAnalyzerTests and PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests verifying that Assert.AreEqual(list[0], list[1]) (same receiver, different indices) produces no diagnostic. This test would also serve as a regression guard for the fix above.
Dimension 17 — NIT: Doc comment claims indexers are excluded; code doesn't enforce it
The <remarks> on IsEquivalentReferenceTo already says:
This intentionally excludes method invocations and indexer accesses, since those may have side effects or return different values on repeated evaluation.
Once the !pra.Property.IsIndexer guard is added, the comment will become accurate. No separate documentation change is needed — the fix and the comment will be in sync.
Everything else
The overall design is solid. The IsEquivalentReferenceTo extension is well-structured: recursive instance checking, transparent parenthesis/conversion unwrapping, intentional exclusion of method calls, and reuse of existing SymbolEqualityComparer.Default. The 21 new tests cover locals, parameters, fields, named arguments, parenthesized expressions, chained properties, different-operand no-diagnostic cases, and method-call no-diagnostic cases. The code-fix path (rewriting to Assert.Fail() including message forwarding) is correctly exercised. No threading, security, public-API-surface, or IPC concerns.
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review (on PR ready) workflow. · 526 AIC · ⌖ 12.7 AIC · ◷
| SymbolEqualityComparer.Default.Equals(pra.Property, prb.Property) && | ||
| AreInstancesEquivalent(pra.Instance, prb.Instance), | ||
|
|
There was a problem hiding this comment.
[MAJOR] Algorithmic Correctness / Analyzer Quality — false positive on custom indexers
The IPropertyReferenceOperation arm does not exclude custom indexers (Property.IsIndexer == true). In Roslyn, list[0] on any type that has a C# indexer (List<T>, Dictionary<K,V>, ImmutableArray<T>, any user-defined this[...] accessor) produces an IPropertyReferenceOperation whose Property.IsIndexer == true. The current code checks only that the property symbol and the receiver instance are equivalent — it does not compare the index arguments.
Concrete false-positive scenario:
var list = new List<string> { "a", "b" };
// list[0] == "a", list[1] == "b" — clearly not always equal, but
Assert.AreEqual(list[0], list[1]); // ← MSTEST0032 fires incorrectly
Assert.AreSame(list[0], list[1]); // ← same false-positive pathBoth operands produce IPropertyReferenceOperation with the same Item property symbol and the same list local as instance. The switch arm returns true, causing a spurious diagnostic.
Note: plain array access (arr[i]) produces IArrayElementReferenceOperation, not IPropertyReferenceOperation, so that case is already excluded by the _ => false default. Only C#-defined indexers hit this arm.
The <remarks> doc comment already states the correct intent — "This intentionally excludes method invocations and indexer accesses" — but the code doesn't enforce it for IPropertyReferenceOperation.
Recommended fix — add !pra.Property.IsIndexer as the first guard:
(IPropertyReferenceOperation pra, IPropertyReferenceOperation prb) =>
!pra.Property.IsIndexer &&
SymbolEqualityComparer.Default.Equals(pra.Property, prb.Property) &&
AreInstancesEquivalent(pra.Instance, prb.Instance),A matching no-diagnostic test is also needed, e.g.:
var list = new List<object>();
Assert.AreEqual(list[0], list[1]); // different indices → no diagnostic
Assert.AreEqual(list[i], list[i]); // same-variable index, still no diagnostic (side-effect risk)There was a problem hiding this comment.
Good catch -- fixed in b220434. Added !pra.Property.IsIndexer as the first guard on the IPropertyReferenceOperation arm so list[0] / dict[k] / any user-defined indexer access falls through to _ => false. The xmldoc remark already documented this intent (This intentionally excludes method invocations and indexer accesses); the code now enforces it. Added WhenAssertAreEqualIsPassedIndexerAccess_NoDiagnostic exercising list[0] vs list[1], list[0] vs list[0], and the AreSame variant. Review reply handled.
Per PR review feedback (#9088), user-defined conversions can execute arbitrary code with side effects, so IsEquivalentReferenceTo must not treat them as transparent. Changes: - IsEquivalentReferenceTo: the inner Unwrap now skips IConversionOperation only when conversion.Conversion.IsUserDefined is false (matching the semantics of WalkDownBuiltInConversion). - HasIdenticalExpectedAndActual in both MSTEST0032 and MSTEST0025: switch from GetArgumentWithName (which calls WalkDownConversion and would strip user-defined conversions) to a new GetRawArgumentValueWithName helper that returns the raw argument value, letting IsEquivalentReferenceTo control the unwrapping. - Updated XML remarks on IsEquivalentReferenceTo to clarify that only built-in conversions are skipped. - Added regression tests: Assert.AreEqual((int)wrapper, (int)wrapper) with a user-defined explicit operator is NOT flagged; the same pattern for Assert.AreNotEqual is also not flagged. Built-in (int -> long) conversions are still flagged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…enceTo
The `IPropertyReferenceOperation` arm in `IsEquivalentReferenceTo` didn't
exclude C#-defined indexers (`Property.IsIndexer == true`). For code like
`Assert.AreEqual(list[0], list[1])` both operands are `IPropertyReferenceOperation`
on the same `Item` property symbol and the same receiver, so the equivalence
check returned true and produced a spurious MSTEST0032 / MSTEST0025 diagnostic.
The fix matches the intent already stated in the xmldoc remarks
("This intentionally excludes method invocations and indexer accesses"): add
`!pra.Property.IsIndexer` as the first guard so any indexer access falls
through to the `_ => false` default. Plain array access
(`IArrayElementReferenceOperation`) was already excluded by that default.
Added `WhenAssertAreEqualIsPassedIndexerAccess_NoDiagnostic` covering
`list[0] vs list[1]`, `list[0] vs list[0]` (same index), and the
`AreSame` variant -- none should be flagged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| public void TestMethod() | ||
| { | ||
| var x = new object(); | ||
| [|Assert.AreNotSame(x, x)|]; |
There was a problem hiding this comment.
Added in 7ff443b -- WhenAssertAreNotSameIsPassedSameLocalWithMessage_Diagnostic covers Assert.AreNotSame(x, x, "references must differ") and asserts the message is forwarded to Assert.Fail("references must differ"). Review reply handled.
| public void TestMethod() | ||
| { | ||
| var x = new object(); | ||
| Assert.Fail(); |
There was a problem hiding this comment.
Covered by the new WhenAssertAreNotSameIsPassedSameLocalWithMessage_Diagnostic test in 7ff443b -- the expected fixed code asserts the message is preserved verbatim in the Assert.Fail(...) rewrite. Review reply handled.
| [TestMethod] | ||
| public async Task WhenAssertAreNotEqualIsPassedSameLocal_Diagnostic() | ||
| { | ||
| string code = """ |
There was a problem hiding this comment.
Added in 7ff443b -- WhenAssertAreNotEqualIsPassedSameProperty_Diagnostic exercises both a same-property operand (h.Value vs h.Value) and a same chained-property operand (h.Inner.Value vs h.Inner.Value). This guards against regressions in IsEquivalentReferenceTo's IPropertyReferenceOperation arm and its recursive AreInstancesEquivalent receiver-walk. Review reply handled.
…erage - `WhenAssertAreNotSameIsPassedSameLocalWithMessage_Diagnostic`: mirrors the existing `AreNotEqual(..., "should differ")` test so the code fix's message-forwarding path (via `AdditionalLocations`) is exercised for the `AreNotSame` variant too. - `WhenAssertAreNotEqualIsPassedSameProperty_Diagnostic`: covers both a same-property operand (`h.Value` vs `h.Value`) and a same chained-property operand (`h.Inner.Value` vs `h.Inner.Value`) so a regression in `IsEquivalentReferenceTo`'s `IPropertyReferenceOperation` arm or its recursive `AreInstancesEquivalent` receiver-walk is caught here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #9087.
Extends two existing analyzers so they no longer require both operands to be compile-time constants:
Assert.AreEqual(x, x)Assert.AreSame(x, x)Assert.AreNotEqual(x, x)Assert.AreNotSame(x, x)How it works
Added a new
IsEquivalentReferenceToextension method onIOperationthat:truewhen both operations reference the same:ILocalReferenceOperation(local variable)IParameterReferenceOperation(parameter)IFieldReferenceOperation(field, with matching receiver chain)IPropertyReferenceOperation(property, with matching receiver chain)IInstanceReferenceOperation(this/base)Method invocations, indexer accesses, and any non-reference expression are intentionally excluded so that
Assert.AreEqual(GetX(), GetX())(which may have side effects) is not flagged. Existing diagnostic messages (""always true"" / ""always false"") already fit this case; no new diagnostic IDs are introduced.Tests
ReviewAlwaysTrueAssertConditionAnalyzerTests(MSTEST0032) covering same local / parameter / field / property / chained property accesses for bothAreEqualandAreSame, plus no-diagnostic cases for different operands and method invocations.PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests(MSTEST0025) covering the same matrix forAreNotEqual/AreNotSame, including the existing code-fix path that rewrites them toAssert.Fail().1244 / 1244analyzer unit tests pass.