From ead9a3b83b1c769043d069b2d16fe0e58085aeb3 Mon Sep 17 00:00:00 2001 From: Brad Wilson Date: Mon, 25 Dec 2023 13:39:22 -0800 Subject: [PATCH] Add xUnit2027 --- .../X2000/SetEqualityAnalyzerTests.cs | 190 +++++++++++++----- src/xunit.analyzers/Utility/Descriptors.cs | 9 +- .../Utility/TypeSymbolFactory.cs | 3 + .../X2000/SetEqualityAnalyzer.cs | 91 ++++++--- 4 files changed, 209 insertions(+), 84 deletions(-) diff --git a/src/xunit.analyzers.tests/Analyzers/X2000/SetEqualityAnalyzerTests.cs b/src/xunit.analyzers.tests/Analyzers/X2000/SetEqualityAnalyzerTests.cs index 5122cdab..401095de 100644 --- a/src/xunit.analyzers.tests/Analyzers/X2000/SetEqualityAnalyzerTests.cs +++ b/src/xunit.analyzers.tests/Analyzers/X2000/SetEqualityAnalyzerTests.cs @@ -4,62 +4,38 @@ public class SetEqualityAnalyzerTests { - const string customSet = @" + const string customSetAndComparer = @" using System.Collections; using System.Collections.Generic; public class MySet : ISet { - public int Count => throw new System.NotImplementedException(); - public bool IsReadOnly => throw new System.NotImplementedException(); - - public bool Add(int item) => throw new System.NotImplementedException(); - public void Clear() => throw new System.NotImplementedException(); - public bool Contains(int item) => throw new System.NotImplementedException(); - public void CopyTo(int[] array, int arrayIndex) => throw new System.NotImplementedException(); - public void ExceptWith(IEnumerable other) => throw new System.NotImplementedException(); - public IEnumerator GetEnumerator() => throw new System.NotImplementedException(); - public void IntersectWith(IEnumerable other) => throw new System.NotImplementedException(); - public bool IsProperSubsetOf(IEnumerable other) => throw new System.NotImplementedException(); - public bool IsProperSupersetOf(IEnumerable other) => throw new System.NotImplementedException(); - public bool IsSubsetOf(IEnumerable other) => throw new System.NotImplementedException(); - public bool IsSupersetOf(IEnumerable other) => throw new System.NotImplementedException(); - public bool Overlaps(IEnumerable other) => throw new System.NotImplementedException(); - public bool Remove(int item) => throw new System.NotImplementedException(); - public bool SetEquals(IEnumerable other) => throw new System.NotImplementedException(); - public void SymmetricExceptWith(IEnumerable other) => throw new System.NotImplementedException(); - public void UnionWith(IEnumerable other) => throw new System.NotImplementedException(); - void ICollection.Add(int item) => throw new System.NotImplementedException(); - IEnumerator IEnumerable.GetEnumerator() => throw new System.NotImplementedException(); -}"; - - [Theory] - [InlineData("Equal", "List", "List")] - [InlineData("Equal", "HashSet", "List")] - [InlineData("Equal", "List", "HashSet")] - [InlineData("NotEqual", "List", "List")] - [InlineData("NotEqual", "HashSet", "List")] - [InlineData("NotEqual", "List", "HashSet")] - public async void ForSetWithNonSet_DoesNotTrigger( - string method, - string collection1Type, - string collection2Type) - { - var code = @$" -using Xunit; -using System.Collections.Generic; - -public class TestClass {{ - [Fact] - public void TestMethod() {{ - var collection1 = new {collection1Type}(); - var collection2 = new {collection2Type}(); - - Assert.{method}(collection1, collection2, (int e1, int e2) => true); - }} -}}"; + public int Count => throw new System.NotImplementedException(); + public bool IsReadOnly => throw new System.NotImplementedException(); + + public bool Add(int item) => throw new System.NotImplementedException(); + public void Clear() => throw new System.NotImplementedException(); + public bool Contains(int item) => throw new System.NotImplementedException(); + public void CopyTo(int[] array, int arrayIndex) => throw new System.NotImplementedException(); + public void ExceptWith(IEnumerable other) => throw new System.NotImplementedException(); + public IEnumerator GetEnumerator() => throw new System.NotImplementedException(); + public void IntersectWith(IEnumerable other) => throw new System.NotImplementedException(); + public bool IsProperSubsetOf(IEnumerable other) => throw new System.NotImplementedException(); + public bool IsProperSupersetOf(IEnumerable other) => throw new System.NotImplementedException(); + public bool IsSubsetOf(IEnumerable other) => throw new System.NotImplementedException(); + public bool IsSupersetOf(IEnumerable other) => throw new System.NotImplementedException(); + public bool Overlaps(IEnumerable other) => throw new System.NotImplementedException(); + public bool Remove(int item) => throw new System.NotImplementedException(); + public bool SetEquals(IEnumerable other) => throw new System.NotImplementedException(); + public void SymmetricExceptWith(IEnumerable other) => throw new System.NotImplementedException(); + public void UnionWith(IEnumerable other) => throw new System.NotImplementedException(); + void ICollection.Add(int item) => throw new System.NotImplementedException(); + IEnumerator IEnumerable.GetEnumerator() => throw new System.NotImplementedException(); +} - await Verify.VerifyAnalyzer(code); - } +public class MyComparer : IEqualityComparer { + public bool Equals(int x, int y) => throw new System.NotImplementedException(); + public int GetHashCode(int obj) => throw new System.NotImplementedException(); +}"; public class X2026_SetsMustBeComparedWithEqualityComparer { @@ -92,7 +68,7 @@ public void TestMethod() {{ }} }}"; - await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSet }); + await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSetAndComparer }); } [Theory] @@ -130,7 +106,7 @@ public void TestMethod() {{ }} }}"; - await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSet }); + await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSetAndComparer }); } [Theory] @@ -161,7 +137,7 @@ public void TestMethod() {{ .WithSpan(12, 9, 12, 68 + method.Length) .WithArguments(method); - await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSet }, expected); + await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSetAndComparer }, expected); } #if ROSLYN_4_2_OR_GREATER // No C# 10 in Roslyn 3.11, so no local functions @@ -217,10 +193,114 @@ bool LocalFunc(int obj1, int obj2) .WithSpan(26, 9, 26, 44 + method.Length + comparerFuncSyntax.Length) .WithArguments(method); - await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, new[] { code, customSet }, expected); + await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, new[] { code, customSetAndComparer }, expected); } #endif } + + public class X2027_SetsShouldNotBeComparedToLinearContainers + { + public static MatrixTheoryData MethodAndLinearContainers => + new( + new[] { "Equal", "NotEqual" }, + new[] { "new List()", "new SortedSet()", "new HashSet().OrderBy(x => x)", "new MySet().OrderBy(x => x)" } + ); + + [Theory] + [MemberData(nameof(MethodAndLinearContainers))] + public async void LinearContainers_DoesNotTrigger( + string method, + string collection) + { + var code = @$" +using Xunit; +using System.Collections.Generic; +using System.Linq; + +public class TestClass {{ + [Fact] + public void TestMethod() {{ + var collection1 = new List(); + var collection2 = {collection}; + + Assert.{method}(collection1, collection2); + Assert.{method}(collection1, collection2, (int e1, int e2) => true); + Assert.{method}(collection1, collection2, new MyComparer()); + }} +}}"; + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSetAndComparer }); + } + + public static MatrixTheoryData MethodAndTypeAndInitializer => + new( + new[] { "Equal", "NotEqual" }, + new[] { + ("HashSet", "new HashSet()"), + ("ImmutableHashSet", "new HashSet().ToImmutableHashSet()"), + ("MySet", "new MySet()") + } + ); + + [Theory] + [MemberData(nameof(MethodAndTypeAndInitializer), DisableDiscoveryEnumeration = true)] + public async void SetWithLinearContainer_Triggers( + string method, + (string type, string initializer) collection) + { + var code = @$" +using Xunit; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; + +public class TestClass {{ + [Fact] + public void TestMethod() {{ + var collection1 = new List(); + var collection2 = {collection.initializer}; + + Assert.{method}(collection1, collection2); + Assert.{method}(collection1, collection2, (int e1, int e2) => true); + Assert.{method}(collection1, collection2, new MyComparer()); + + Assert.{method}(collection2, collection1); + Assert.{method}(collection2, collection1, (int e1, int e2) => true); + Assert.{method}(collection2, collection1, new MyComparer()); + }} +}}"; + var expected = new[] + { + Verify + .Diagnostic("xUnit2027") + .WithSpan(13, 9, 13, 42 + method.Length) + .WithArguments("List", collection.type), + Verify + .Diagnostic("xUnit2027") + .WithSpan(14, 9, 14, 68 + method.Length) + .WithArguments("List", collection.type), + Verify + .Diagnostic("xUnit2027") + .WithSpan(15, 9, 15, 60 + method.Length) + .WithArguments("List", collection.type), + + Verify + .Diagnostic("xUnit2027") + .WithSpan(17, 9, 17, 42 + method.Length) + .WithArguments(collection.type, "List"), + Verify + .Diagnostic("xUnit2027") + .WithSpan(18, 9, 18, 68 + method.Length) + .WithArguments(collection.type, "List"), + Verify + .Diagnostic("xUnit2027") + .WithSpan(19, 9, 19, 60 + method.Length) + .WithArguments(collection.type, "List"), + }; + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp7, new[] { code, customSetAndComparer }, expected); + } + } } diff --git a/src/xunit.analyzers/Utility/Descriptors.cs b/src/xunit.analyzers/Utility/Descriptors.cs index a6109015..975701aa 100644 --- a/src/xunit.analyzers/Utility/Descriptors.cs +++ b/src/xunit.analyzers/Utility/Descriptors.cs @@ -672,7 +672,14 @@ static DiagnosticDescriptor Rule( "Comparison of two sets may produce inconsistent results if GetHashCode() is not overriden. Consider using Assert.{0}(IEnumerable, IEnumerable, IEqualityComparer) instead." ); - // Placeholder for rule X2027 + public static DiagnosticDescriptor X2027_SetsShouldNotBeComparedToLinearContainers { get; } = + Rule( + "xUnit2027", + "Comparison of sets to linear containers have undefined results", + Assertions, + Warning, + "Comparing an instance of {0} with an instance of {1} has undefined results, because the order of items in the set is not predictable. Create a stable order for the set (i.e., by using OrderBy from Linq)." + ); // Placeholder for rule X2028 diff --git a/src/xunit.analyzers/Utility/TypeSymbolFactory.cs b/src/xunit.analyzers/Utility/TypeSymbolFactory.cs index cca61dae..6ae2dece 100644 --- a/src/xunit.analyzers/Utility/TypeSymbolFactory.cs +++ b/src/xunit.analyzers/Utility/TypeSymbolFactory.cs @@ -184,6 +184,9 @@ public static IArrayTypeSymbol ObjectArray(Compilation compilation) => public static INamedTypeSymbol? OptionalAttribute(Compilation compilation) => Guard.ArgumentNotNull(compilation).GetTypeByMetadataName("System.Runtime.InteropServices.OptionalAttribute"); + public static INamedTypeSymbol? SortedSetOfT(Compilation compilation) => + Guard.ArgumentNotNull(compilation).GetTypeByMetadataName("System.Collections.Generic.SortedSet`1"); + public static INamedTypeSymbol? Task(Compilation compilation) => Guard.ArgumentNotNull(compilation).GetTypeByMetadataName("System.Threading.Tasks.Task"); diff --git a/src/xunit.analyzers/X2000/SetEqualityAnalyzer.cs b/src/xunit.analyzers/X2000/SetEqualityAnalyzer.cs index 2e2b34a8..3416538a 100644 --- a/src/xunit.analyzers/X2000/SetEqualityAnalyzer.cs +++ b/src/xunit.analyzers/X2000/SetEqualityAnalyzer.cs @@ -20,7 +20,13 @@ public class SetEqualityAnalyzer : AssertUsageAnalyzerBase }; public SetEqualityAnalyzer() - : base(Descriptors.X2026_SetsMustBeComparedWithEqualityComparer, targetMethods) + : base( + new[] { + Descriptors.X2026_SetsMustBeComparedWithEqualityComparer, + Descriptors.X2027_SetsShouldNotBeComparedToLinearContainers, + }, + targetMethods + ) { } protected override void AnalyzeInvocation( @@ -33,10 +39,6 @@ protected override void AnalyzeInvocation( Guard.ArgumentNotNull(invocationOperation); Guard.ArgumentNotNull(method); - var arguments = invocationOperation.Arguments; - if (arguments.Length != 3) - return; - var semanticModel = context.Operation.SemanticModel; if (semanticModel == null) return; @@ -45,6 +47,10 @@ protected override void AnalyzeInvocation( var readOnlySetType = TypeSymbolFactory.IReadOnlySetOfT(context.Compilation)?.ConstructUnboundGenericType(); var setInterfaces = new HashSet(new[] { setType, readOnlySetType }.WhereNotNull(), SymbolEqualityComparer.Default); + var arguments = invocationOperation.Arguments; + if (arguments.Length < 2) + return; + if (semanticModel.GetTypeInfo(arguments[0].Value.Syntax).Type is not INamedTypeSymbol collection0Type) return; var interface0Type = @@ -52,8 +58,6 @@ protected override void AnalyzeInvocation( .AllInterfaces .Where(i => i.IsGenericType) .FirstOrDefault(i => setInterfaces.Contains(i.ConstructUnboundGenericType())); - if (interface0Type is null) - return; if (semanticModel.GetTypeInfo(arguments[1].Value.Syntax).Type is not INamedTypeSymbol collection1Type) return; @@ -62,32 +66,63 @@ protected override void AnalyzeInvocation( .AllInterfaces .Where(i => i.IsGenericType) .FirstOrDefault(i => setInterfaces.Contains(i.ConstructUnboundGenericType())); - if (interface1Type is null) - return; - if (arguments[2].Value is not IDelegateCreationOperation && arguments[2].Value is not ILocalReferenceOperation) + // No sets + if (interface0Type is null && interface1Type is null) return; - if (arguments[2].Value.Type is not INamedTypeSymbol funcTypeSymbol || funcTypeSymbol.DelegateInvokeMethod == null) - return; + // Both sets, make sure they don't use the comparer function override + if (interface0Type is not null && interface1Type is not null) + { + if (arguments.Length != 3) + return; - var funcDelegate = funcTypeSymbol.DelegateInvokeMethod; - var isFuncOverload = - funcDelegate.ReturnType.SpecialType == SpecialType.System_Boolean && - funcDelegate.Parameters.Length == 2 && - funcDelegate.Parameters[0].Type.Equals(interface0Type.TypeArguments[0], SymbolEqualityComparer.Default) && - funcDelegate.Parameters[1].Type.Equals(interface1Type.TypeArguments[0], SymbolEqualityComparer.Default); + if (arguments[2].Value is not IDelegateCreationOperation && arguments[2].Value is not ILocalReferenceOperation) + return; - // Wrong method overload - if (!isFuncOverload) - return; + if (arguments[2].Value.Type is not INamedTypeSymbol funcTypeSymbol || funcTypeSymbol.DelegateInvokeMethod == null) + return; - context.ReportDiagnostic( - Diagnostic.Create( - Descriptors.X2026_SetsMustBeComparedWithEqualityComparer, - invocationOperation.Syntax.GetLocation(), - method.Name - ) - ); + var funcDelegate = funcTypeSymbol.DelegateInvokeMethod; + var isFuncOverload = + funcDelegate.ReturnType.SpecialType == SpecialType.System_Boolean && + funcDelegate.Parameters.Length == 2 && + funcDelegate.Parameters[0].Type.Equals(interface0Type.TypeArguments[0], SymbolEqualityComparer.Default) && + funcDelegate.Parameters[1].Type.Equals(interface1Type.TypeArguments[0], SymbolEqualityComparer.Default); + + // Wrong method overload + if (!isFuncOverload) + return; + + context.ReportDiagnostic( + Diagnostic.Create( + Descriptors.X2026_SetsMustBeComparedWithEqualityComparer, + invocationOperation.Syntax.GetLocation(), + method.Name + ) + ); + } + // One set, one linear container + else + { + // Make a special allowance for SortedSet<>, since we know it's sorted + var sortedSet = TypeSymbolFactory.SortedSetOfT(context.Compilation); + if (sortedSet is not null) + { + if (interface0Type is not null && sortedSet.Construct(interface0Type.TypeArguments[0]).IsAssignableFrom(collection0Type)) + return; + if (interface1Type is not null && sortedSet.Construct(interface1Type.TypeArguments[0]).IsAssignableFrom(collection1Type)) + return; + } + + context.ReportDiagnostic( + Diagnostic.Create( + Descriptors.X2027_SetsShouldNotBeComparedToLinearContainers, + invocationOperation.Syntax.GetLocation(), + collection0Type.ToMinimalDisplayString(semanticModel, 0), + collection1Type.ToMinimalDisplayString(semanticModel, 0) + ) + ); + } } }