diff --git a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs index 7154c268d6..8d0d2eb1c5 100644 --- a/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/PreferAssertFailOverAlwaysFalseConditionsAnalyzer.cs @@ -92,12 +92,22 @@ 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) + => 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 b0c6dc90c2..4e3783cd95 100644 --- a/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs +++ b/src/Analyzers/MSTest.Analyzers/ReviewAlwaysTrueAssertConditionAnalyzer.cs @@ -81,13 +81,23 @@ 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) + => 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 11812837e4..ccaddeed52 100644 --- a/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs +++ b/src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs @@ -56,4 +56,69 @@ 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 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) + { + 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) => + !pra.Property.IsIndexer && + 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 when !conversion.Conversion.IsUserDefined: + 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..ec98e934d3 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/PreferAssertFailOverAlwaysFalseConditionsAnalyzerTests.cs @@ -1560,4 +1560,395 @@ 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 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() + { + 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); + } + + [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 d1792d4573..fa674b91d7 100644 --- a/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs +++ b/test/UnitTests/MSTest.Analyzers.UnitTests/ReviewAlwaysTrueAssertConditionAnalyzerTests.cs @@ -1167,4 +1167,347 @@ 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); + } + + [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); + } + + [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); + } }