From f0195a99e58084bd1809b9b712fd592c797f3778 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 12 Jun 2026 17:17:10 +0200 Subject: [PATCH 1/4] Flag Assert.AreEqual/AreSame (and AreNotEqual/AreNotSame) with identical 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> --- ...rtFailOverAlwaysFalseConditionsAnalyzer.cs | 9 +- ...ReviewAlwaysTrueAssertConditionAnalyzer.cs | 9 +- .../IOperationExtensions.cs | 62 ++++ ...lOverAlwaysFalseConditionsAnalyzerTests.cs | 276 ++++++++++++++++++ ...wAlwaysTrueAssertConditionAnalyzerTests.cs | 268 +++++++++++++++++ 5 files changed, 622 insertions(+), 2 deletions(-) diff --git a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs index 7154c268d6..920d1a4ae4 100644 --- a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs @@ -92,12 +92,19 @@ private static bool IsAlwaysFalse(IInvocationOperation operation) "IsTrue" => GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: false } }, "IsFalse" => GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: true } }, "AreEqual" => GetEqualityStatus(operation, ExpectedParameterName) == EqualityStatus.NotEqual, - "AreNotEqual" => GetEqualityStatus(operation, NotExpectedParameterName) == EqualityStatus.Equal, + "AreNotEqual" => GetEqualityStatus(operation, NotExpectedParameterName) == EqualityStatus.Equal + || HasIdenticalExpectedAndActual(operation, NotExpectedParameterName), + "AreNotSame" => HasIdenticalExpectedAndActual(operation, NotExpectedParameterName), "IsNotNull" => GetValueArgument(operation) is { ConstantValue: { HasValue: true, Value: null } }, "IsNull" => GetValueArgument(operation) is { } valueArgumentOperation && IsNotNullableType(valueArgumentOperation), _ => false, }; + private static bool HasIdenticalExpectedAndActual(IInvocationOperation operation, string expectedOrNotExpectedParameterName) + => GetArgumentWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument + && GetArgumentWithName(operation, ActualParameterName) is { } actualArgument + && expectedArgument.IsEquivalentReferenceTo(actualArgument); + private static bool IsNotNullableType(IOperation valueArgumentOperation) { ITypeSymbol? valueArgType = valueArgumentOperation.GetReferencedMemberOrLocalOrParameter().GetReferencedMemberOrLocalOrParameter(); diff --git a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs index b0c6dc90c2..efd667bd06 100644 --- a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs @@ -81,13 +81,20 @@ private static bool IsAlwaysTrue(IInvocationOperation operation) { "IsTrue" => GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: true } }, "IsFalse" => GetConditionArgument(operation) is { ConstantValue: { HasValue: true, Value: false } }, - "AreEqual" => GetEqualityStatus(operation, ExpectedParameterName) == EqualityStatus.Equal, + "AreEqual" => GetEqualityStatus(operation, ExpectedParameterName) == EqualityStatus.Equal + || HasIdenticalExpectedAndActual(operation, ExpectedParameterName), "AreNotEqual" => GetEqualityStatus(operation, NotExpectedParameterName) == EqualityStatus.NotEqual, + "AreSame" => HasIdenticalExpectedAndActual(operation, ExpectedParameterName), "IsNull" => GetValueArgument(operation) is { ConstantValue: { HasValue: true, Value: null } }, "IsNotNull" => GetValueArgument(operation) is { } valueArgumentOperation && IsNotNullableType(valueArgumentOperation), _ => false, }; + private static bool HasIdenticalExpectedAndActual(IInvocationOperation operation, string expectedOrNotExpectedParameterName) + => GetArgumentWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument + && GetArgumentWithName(operation, ActualParameterName) is { } actualArgument + && expectedArgument.IsEquivalentReferenceTo(actualArgument); + private static bool IsNotNullableType(IOperation valueArgumentOperation) { ITypeSymbol? valueArgType = valueArgumentOperation.GetReferencedMemberOrLocalOrParameter().GetReferencedMemberOrLocalOrParameter(); diff --git a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs index 11812837e4..c04e83542d 100644 --- a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs +++ b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs @@ -56,4 +56,66 @@ public static IOperation WalkDownBuiltInConversion(this IOperation operation) return operation; } + + /// + /// Returns when the two operations are structurally equivalent + /// side-effect-free references to the same local, parameter, field, property, or this. + /// + /// + /// 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. + /// + public static bool IsEquivalentReferenceTo(this IOperation? left, IOperation? right) + { + if (left is null || right is null) + { + return false; + } + + left = Unwrap(left); + right = Unwrap(right); + + return (left, right) switch + { + (ILocalReferenceOperation la, ILocalReferenceOperation lb) => + SymbolEqualityComparer.Default.Equals(la.Local, lb.Local), + + (IParameterReferenceOperation pa, IParameterReferenceOperation pb) => + SymbolEqualityComparer.Default.Equals(pa.Parameter, pb.Parameter), + + (IFieldReferenceOperation fa, IFieldReferenceOperation fb) => + SymbolEqualityComparer.Default.Equals(fa.Field, fb.Field) && + AreInstancesEquivalent(fa.Instance, fb.Instance), + + (IPropertyReferenceOperation pra, IPropertyReferenceOperation prb) => + SymbolEqualityComparer.Default.Equals(pra.Property, prb.Property) && + AreInstancesEquivalent(pra.Instance, prb.Instance), + + (IInstanceReferenceOperation, IInstanceReferenceOperation) => true, + + _ => false, + }; + + static IOperation Unwrap(IOperation operation) + { + while (true) + { + switch (operation) + { + case IParenthesizedOperation parenthesized: + operation = parenthesized.Operand; + break; + case IConversionOperation conversion: + operation = conversion.Operand; + break; + default: + return operation; + } + } + } + + static bool AreInstancesEquivalent(IOperation? a, IOperation? b) + => (a is null && b is null) || IsEquivalentReferenceTo(a, b); + } } diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs index 358f9b147b..37d6b3b158 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs @@ -1560,4 +1560,280 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, fixedCode); } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocal_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreNotEqual(x, x)|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameParameter_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod(int x) + { + [|Assert.AreNotEqual(x, x)|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod(int x) + { + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameField_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + private int _value; + + [TestMethod] + public void TestMethod() + { + [|Assert.AreNotEqual(_value, _value)|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + private int _value; + + [TestMethod] + public void TestMethod() + { + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithMessage_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreNotEqual(x, x, "should differ")|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + Assert.Fail("should differ"); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedDifferentLocals_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + int y = 2; + Assert.AreNotEqual(x, y); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameMethodCall_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + Assert.AreNotEqual(GetValue(), GetValue()); + } + + private int GetValue() => 1; + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreNotSameIsPassedSameLocal_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + [|Assert.AreNotSame(x, x)|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotSameIsPassedSameLocalNamed_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + [|Assert.AreNotSame(actual: x, notExpected: x)|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotSameIsPassedDifferentLocals_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + var y = new object(); + Assert.AreNotSame(x, y); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs index d1792d4573..da8f4e90aa 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs @@ -1167,4 +1167,272 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocal_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreEqual(x, x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameParameter_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod(int x) + { + [|Assert.AreEqual(x, x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameField_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + private int _value; + + [TestMethod] + public void TestMethod() + { + [|Assert.AreEqual(_value, _value)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameProperty_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + private int Value { get; set; } + + [TestMethod] + public void TestMethod() + { + [|Assert.AreEqual(Value, Value)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSamePropertyChain_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + private SomeType _value = new SomeType(); + + [TestMethod] + public void TestMethod() + { + [|Assert.AreEqual(_value.Inner, _value.Inner)|]; + } + + private sealed class SomeType + { + public int Inner { get; set; } + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedDifferentInstances_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var a = new SomeType(); + var b = new SomeType(); + Assert.AreEqual(a.Inner, b.Inner); + } + + private sealed class SomeType + { + public int Inner { get; set; } + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedDifferentLocals_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + int y = 1; + Assert.AreEqual(x, y); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameMethodCall_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + Assert.AreEqual(GetValue(), GetValue()); + } + + private int GetValue() => 1; + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreSameIsPassedSameLocal_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + [|Assert.AreSame(x, x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreSameIsPassedSameLocalWithNamedArgs_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + [|Assert.AreSame(actual: x, expected: x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreSameIsPassedSameLocalParenthesized_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + [|Assert.AreSame((x), x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreSameIsPassedDifferentLocals_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + var y = new object(); + Assert.AreSame(x, y); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } From e6f5d0df2bb92caf7c3d89050eaeca448aac3ea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 12 Jun 2026 17:33:05 +0200 Subject: [PATCH 2/4] Address review: don't unwrap user-defined conversions 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> --- ...rtFailOverAlwaysFalseConditionsAnalyzer.cs | 7 ++- ...ReviewAlwaysTrueAssertConditionAnalyzer.cs | 7 ++- .../IOperationExtensions.cs | 6 ++- ...lOverAlwaysFalseConditionsAnalyzerTests.cs | 27 +++++++++++ ...wAlwaysTrueAssertConditionAnalyzerTests.cs | 48 +++++++++++++++++++ 5 files changed, 89 insertions(+), 6 deletions(-) diff --git a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs index 920d1a4ae4..8d0d2eb1c5 100644 --- a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs @@ -101,10 +101,13 @@ private static bool IsAlwaysFalse(IInvocationOperation operation) }; private static bool HasIdenticalExpectedAndActual(IInvocationOperation operation, string expectedOrNotExpectedParameterName) - => GetArgumentWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument - && GetArgumentWithName(operation, ActualParameterName) is { } actualArgument + => GetRawArgumentValueWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument + && GetRawArgumentValueWithName(operation, ActualParameterName) is { } actualArgument && expectedArgument.IsEquivalentReferenceTo(actualArgument); + private static IOperation? GetRawArgumentValueWithName(IInvocationOperation operation, string name) + => operation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == name)?.Value; + private static bool IsNotNullableType(IOperation valueArgumentOperation) { ITypeSymbol? valueArgType = valueArgumentOperation.GetReferencedMemberOrLocalOrParameter().GetReferencedMemberOrLocalOrParameter(); diff --git a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs index efd667bd06..4e3783cd95 100644 --- a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs @@ -91,10 +91,13 @@ private static bool IsAlwaysTrue(IInvocationOperation operation) }; private static bool HasIdenticalExpectedAndActual(IInvocationOperation operation, string expectedOrNotExpectedParameterName) - => GetArgumentWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument - && GetArgumentWithName(operation, ActualParameterName) is { } actualArgument + => GetRawArgumentValueWithName(operation, expectedOrNotExpectedParameterName) is { } expectedArgument + && GetRawArgumentValueWithName(operation, ActualParameterName) is { } actualArgument && expectedArgument.IsEquivalentReferenceTo(actualArgument); + private static IOperation? GetRawArgumentValueWithName(IInvocationOperation operation, string name) + => operation.Arguments.FirstOrDefault(arg => arg.Parameter?.Name == name)?.Value; + private static bool IsNotNullableType(IOperation valueArgumentOperation) { ITypeSymbol? valueArgType = valueArgumentOperation.GetReferencedMemberOrLocalOrParameter().GetReferencedMemberOrLocalOrParameter(); diff --git a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs index c04e83542d..6fb5b7ac20 100644 --- a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs +++ b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs @@ -64,7 +64,9 @@ public static IOperation WalkDownBuiltInConversion(this IOperation operation) /// /// 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. + /// 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. /// public static bool IsEquivalentReferenceTo(this IOperation? left, IOperation? right) { @@ -106,7 +108,7 @@ static IOperation Unwrap(IOperation operation) case IParenthesizedOperation parenthesized: operation = parenthesized.Operand; break; - case IConversionOperation conversion: + case IConversionOperation conversion when !conversion.Conversion.IsUserDefined: operation = conversion.Operand; break; default: diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs index 37d6b3b158..3865f9a0c7 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs @@ -1836,4 +1836,31 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameLocalWithUserDefinedConversion_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new Wrapper(); + Assert.AreNotEqual((int)x, (int)x); + } + + private sealed class Wrapper + { + private static int _counter; + public static explicit operator int(Wrapper value) => ++_counter; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs index da8f4e90aa..da60c03025 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs @@ -1435,4 +1435,52 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithUserDefinedConversion_NoDiagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new Wrapper(); + Assert.AreEqual((int)x, (int)x); + } + + private sealed class Wrapper + { + private static int _counter; + public static explicit operator int(Wrapper value) => ++_counter; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedSameLocalWithBuiltInConversion_Diagnostic() + { + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + int x = 1; + [|Assert.AreEqual((long)x, (long)x)|]; + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } From b220434d2e63cb758aab07be925a6a9c4dfdabc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 12 Jun 2026 18:28:22 +0200 Subject: [PATCH 3/4] Address self-review on #9088: exclude indexers from IsEquivalentReferenceTo 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> --- .../IOperationExtensions.cs | 1 + ...wAlwaysTrueAssertConditionAnalyzerTests.cs | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs index 6fb5b7ac20..ccaddeed52 100644 --- a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs +++ b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs @@ -91,6 +91,7 @@ public static bool IsEquivalentReferenceTo(this IOperation? left, IOperation? ri AreInstancesEquivalent(fa.Instance, fb.Instance), (IPropertyReferenceOperation pra, IPropertyReferenceOperation prb) => + !pra.Property.IsIndexer && SymbolEqualityComparer.Default.Equals(pra.Property, prb.Property) && AreInstancesEquivalent(pra.Instance, prb.Instance), diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs index da60c03025..fa674b91d7 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs @@ -1483,4 +1483,31 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, code); } + + [TestMethod] + public async Task WhenAssertAreEqualIsPassedIndexerAccess_NoDiagnostic() + { + // list[0] and list[1] are IPropertyReferenceOperation on the same Item indexer + same + // receiver, but the index arguments differ -- and the analyzer doesn't compare those. + // We must NOT flag this as always-equal. + string code = """ + using System.Collections.Generic; + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var list = new List { "a", "b" }; + Assert.AreEqual(list[0], list[1]); + Assert.AreEqual(list[0], list[0]); + Assert.AreSame(list[0], list[1]); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, code); + } } From 7ff443b885ddb24cd9e73e39c178f524c24429f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 12 Jun 2026 18:51:53 +0200 Subject: [PATCH 4/4] Address review on #9088: add AreNotSame-with-message and property coverage - `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> --- ...lOverAlwaysFalseConditionsAnalyzerTests.cs | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs index 3865f9a0c7..ec98e934d3 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs @@ -1815,6 +1815,94 @@ public void TestMethod() await VerifyCS.VerifyCodeFixAsync(code, fixedCode); } + [TestMethod] + public async Task WhenAssertAreNotSameIsPassedSameLocalWithMessage_Diagnostic() + { + // Mirror the AreNotEqual(..., "should differ") test below so the code fix's + // message-forwarding path is exercised for AreNotSame too. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + [|Assert.AreNotSame(x, x, "references must differ")|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var x = new object(); + Assert.Fail("references must differ"); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + + [TestMethod] + public async Task WhenAssertAreNotEqualIsPassedSameProperty_Diagnostic() + { + // Exercises IsEquivalentReferenceTo's IPropertyReferenceOperation arm directly so a + // regression that breaks property equivalence detection is caught here. + string code = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + public class Holder + { + public int Value { get; set; } + public Holder Inner { get; set; } = new Holder(); + } + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var h = new Holder(); + [|Assert.AreNotEqual(h.Value, h.Value)|]; + [|Assert.AreNotEqual(h.Inner.Value, h.Inner.Value)|]; + } + } + """; + string fixedCode = """ + using Microsoft.VisualStudio.TestTools.UnitTesting; + + public class Holder + { + public int Value { get; set; } + public Holder Inner { get; set; } = new Holder(); + } + + [TestClass] + public class MyTestClass + { + [TestMethod] + public void TestMethod() + { + var h = new Holder(); + Assert.Fail(); + Assert.Fail(); + } + } + """; + + await VerifyCS.VerifyCodeFixAsync(code, fixedCode); + } + [TestMethod] public async Task WhenAssertAreNotSameIsPassedDifferentLocals_NoDiagnostic() {