From 7847dc311006686eb83000cbfbb85bf12fda0df8 Mon Sep 17 00:00:00 2001 From: James Terwilliger Date: Wed, 22 Nov 2023 21:26:52 -0800 Subject: [PATCH 1/3] Candidate fix for https://github.com/xunit/xunit/issues/2826 --- ...mberDataShouldReferenceValidMemberTests.cs | 24 ++++++++++ .../MemberDataShouldReferenceValidMember.cs | 46 ++++++++++++++++--- 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs b/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs index e6da9545..a59effd8 100644 --- a/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs +++ b/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs @@ -837,6 +837,30 @@ public void TestMethod(T? n) {{ }} await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source); } + [Fact] + public async void DoesNotFindWarning_WithArrayArguments() + { + var source = $@" +using System.Collections.Generic; +using Xunit; +public class TestClass +{{ + public static IEnumerable GetSequences(IEnumerable seq) => + new List(); + + [Theory] + [MemberData(nameof(GetSequences), new int[] {{ 1, 2 }})] + [MemberData(nameof(GetSequences), new [] {{ 3, 4, 5}})] + public void Test(IEnumerable seq) + {{ + Assert.NotEmpty(seq); + }} +}} +"; + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source); + } + [Theory] [MemberData(nameof(MemberSyntaxAndArgs))] public async void FindWarning_IfHasValidTheoryDataMemberWithTooManyTypeParameters( diff --git a/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs b/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs index 0f3e198b..5f045e5e 100644 --- a/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs +++ b/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs @@ -1,7 +1,10 @@ +using System; using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using System.Reflection; +using System.Reflection.Metadata; +using System.Runtime.InteropServices.ComTypes; using System.Threading; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -215,26 +218,34 @@ public static (INamedTypeSymbol? TestClass, ITypeSymbol? MemberClass) GetClassTy return (testClassTypeSymbol, declaredMemberTypeSymbol); } - static IList? GetParameterExpressionsFromArrayArgument(List arguments) + static (IList? Expressions, ExpressionSyntax? ArraySyntax) GetParameterExpressionsFromArrayArgument( + List arguments) { if (arguments.Count > 1) - return arguments.Select(a => a.Expression).ToList(); + return (arguments.Select(a => a.Expression).ToList(), null); if (arguments.Count != 1) - return null; + return (null, null); var argumentExpression = arguments.Single().Expression; - var initializer = argumentExpression.Kind() switch + var kind = argumentExpression.Kind(); + var initializer = kind switch { SyntaxKind.ArrayCreationExpression => ((ArrayCreationExpressionSyntax)argumentExpression).Initializer, SyntaxKind.ImplicitArrayCreationExpression => ((ImplicitArrayCreationExpressionSyntax)argumentExpression).Initializer, _ => null, }; + var arraySyntax = kind switch + { + SyntaxKind.ArrayCreationExpression => argumentExpression, + SyntaxKind.ImplicitArrayCreationExpression => argumentExpression, + _ => null, + }; if (initializer is null) - return new List { argumentExpression }; + return (new List { argumentExpression }, null); - return initializer.Expressions.ToList(); + return (initializer.Expressions.ToList(), arraySyntax); } public static ISymbol? FindMethodSymbol( @@ -493,13 +504,34 @@ static void VerifyDataMethodParameterUsage( string memberName, List extraArguments) { - var argumentSyntaxList = GetParameterExpressionsFromArrayArgument(extraArguments); + (var argumentSyntaxList, var arraySyntax) = GetParameterExpressionsFromArrayArgument(extraArguments); if (argumentSyntaxList is null) return; var dataMethodSymbol = (IMethodSymbol)memberSymbol; var dataMethodParameterSymbols = dataMethodSymbol.Parameters; + if (arraySyntax is not null + && dataMethodParameterSymbols.Length > 0 + && !dataMethodParameterSymbols[0].IsParams + && dataMethodParameterSymbols.Skip(1).All(s => s.IsParams || s.IsOptional)) + { + // We may have a situation where an array argument to the attribute is intended as an array and not params + var arrayArgumentTypeSymbol = semanticModel.GetTypeInfo(arraySyntax).Type as IArrayTypeSymbol; + var arrayArgumentElementTypeSymbol = arrayArgumentTypeSymbol?.ElementType; + + var dataMethodFirstParameterSymbolType = dataMethodParameterSymbols[0].Type; + var dataMethodFirstParameterElementSymbolType = dataMethodFirstParameterSymbolType.Kind == SymbolKind.ArrayType + ? ((IArrayTypeSymbol)dataMethodFirstParameterSymbolType).ElementType + : dataMethodFirstParameterSymbolType.GetEnumerableType(); + + if (arrayArgumentElementTypeSymbol is not null + && dataMethodFirstParameterElementSymbolType is not null + && ConversionChecker.IsConvertible( + compilation, arrayArgumentElementTypeSymbol, dataMethodFirstParameterElementSymbolType, xunitContext)) + argumentSyntaxList = new List { arraySyntax }; + } + int valueIdx = 0, paramIdx = 0; for (; valueIdx < argumentSyntaxList.Count && paramIdx < dataMethodParameterSymbols.Length; valueIdx++) { From 595095e88878d6d4dc5072aae6ffbb989a96fc78 Mon Sep 17 00:00:00 2001 From: James Terwilliger Date: Wed, 22 Nov 2023 21:38:35 -0800 Subject: [PATCH 2/3] Adding additional test --- ...mberDataShouldReferenceValidMemberTests.cs | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs b/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs index a59effd8..5fbed95c 100644 --- a/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs +++ b/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs @@ -838,11 +838,12 @@ public void TestMethod(T? n) {{ }} } [Fact] - public async void DoesNotFindWarning_WithArrayArguments() + public async void DoesNotFindWarning_WithIntArrayArguments() { var source = $@" using System.Collections.Generic; using Xunit; + public class TestClass {{ public static IEnumerable GetSequences(IEnumerable seq) => @@ -861,6 +862,30 @@ public void Test(IEnumerable seq) await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source); } + [Fact] + public async void DoesNotFindWarning_WithObjectArrayArguments() + { + var source = $@" +using System.Collections.Generic; +using Xunit; + +public class TestClass +{{ + public static IEnumerable GetSequences(IEnumerable seq) => + new List(); + + [Theory] + [MemberData(nameof(GetSequences), new object[] {{ 1, 2 }})] + public void Test(IEnumerable seq) + {{ + Assert.NotEmpty(seq); + }} +}} +"; + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source); + } + [Theory] [MemberData(nameof(MemberSyntaxAndArgs))] public async void FindWarning_IfHasValidTheoryDataMemberWithTooManyTypeParameters( From fed650d9af2d120fda85649c27c96732c65fce64 Mon Sep 17 00:00:00 2001 From: James Terwilliger Date: Thu, 23 Nov 2023 11:49:26 -0800 Subject: [PATCH 3/3] Try a different approach testing specifically for array element type --- ...mberDataShouldReferenceValidMemberTests.cs | 18 +++++++- .../MemberDataShouldReferenceValidMember.cs | 46 +++++-------------- 2 files changed, 28 insertions(+), 36 deletions(-) diff --git a/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs b/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs index 5fbed95c..ec07d3cb 100644 --- a/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs +++ b/src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs @@ -863,7 +863,7 @@ public void Test(IEnumerable seq) } [Fact] - public async void DoesNotFindWarning_WithObjectArrayArguments() + public async void FindsWarning_WithObjectArrayArguments() { var source = $@" using System.Collections.Generic; @@ -883,7 +883,21 @@ public void Test(IEnumerable seq) }} "; - await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source); + DiagnosticResult[] expected = + { + Verify + .Diagnostic("xUnit1035") + .WithSpan(11, 52, 11, 53) + .WithArguments("seq", "System.Collections.Generic.IEnumerable") + .WithSeverity(DiagnosticSeverity.Error), + Verify + .Diagnostic("xUnit1036") + .WithSpan(11, 55, 11, 56) + .WithArguments("2") + .WithSeverity(DiagnosticSeverity.Error) + }; + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source, expected); } [Theory] diff --git a/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs b/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs index 5f045e5e..0024efca 100644 --- a/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs +++ b/src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs @@ -218,13 +218,13 @@ public static (INamedTypeSymbol? TestClass, ITypeSymbol? MemberClass) GetClassTy return (testClassTypeSymbol, declaredMemberTypeSymbol); } - static (IList? Expressions, ExpressionSyntax? ArraySyntax) GetParameterExpressionsFromArrayArgument( - List arguments) + static IList? GetParameterExpressionsFromArrayArgument( + List arguments, SemanticModel semanticModel) { if (arguments.Count > 1) - return (arguments.Select(a => a.Expression).ToList(), null); + return arguments.Select(a => a.Expression).ToList(); if (arguments.Count != 1) - return (null, null); + return null; var argumentExpression = arguments.Single().Expression; @@ -235,17 +235,16 @@ public static (INamedTypeSymbol? TestClass, ITypeSymbol? MemberClass) GetClassTy SyntaxKind.ImplicitArrayCreationExpression => ((ImplicitArrayCreationExpressionSyntax)argumentExpression).Initializer, _ => null, }; - var arraySyntax = kind switch - { - SyntaxKind.ArrayCreationExpression => argumentExpression, - SyntaxKind.ImplicitArrayCreationExpression => argumentExpression, - _ => null, - }; if (initializer is null) - return (new List { argumentExpression }, null); + return new List { argumentExpression }; - return (initializer.Expressions.ToList(), arraySyntax); + // In the special case where the argument is an object[], treat like params + var type = semanticModel.GetTypeInfo(argumentExpression).Type; + if (type is IArrayTypeSymbol arrayType && arrayType.ElementType.SpecialType == SpecialType.System_Object) + return initializer.Expressions.ToList(); + + return new List { argumentExpression }; } public static ISymbol? FindMethodSymbol( @@ -504,34 +503,13 @@ static void VerifyDataMethodParameterUsage( string memberName, List extraArguments) { - (var argumentSyntaxList, var arraySyntax) = GetParameterExpressionsFromArrayArgument(extraArguments); + var argumentSyntaxList = GetParameterExpressionsFromArrayArgument(extraArguments, semanticModel); if (argumentSyntaxList is null) return; var dataMethodSymbol = (IMethodSymbol)memberSymbol; var dataMethodParameterSymbols = dataMethodSymbol.Parameters; - if (arraySyntax is not null - && dataMethodParameterSymbols.Length > 0 - && !dataMethodParameterSymbols[0].IsParams - && dataMethodParameterSymbols.Skip(1).All(s => s.IsParams || s.IsOptional)) - { - // We may have a situation where an array argument to the attribute is intended as an array and not params - var arrayArgumentTypeSymbol = semanticModel.GetTypeInfo(arraySyntax).Type as IArrayTypeSymbol; - var arrayArgumentElementTypeSymbol = arrayArgumentTypeSymbol?.ElementType; - - var dataMethodFirstParameterSymbolType = dataMethodParameterSymbols[0].Type; - var dataMethodFirstParameterElementSymbolType = dataMethodFirstParameterSymbolType.Kind == SymbolKind.ArrayType - ? ((IArrayTypeSymbol)dataMethodFirstParameterSymbolType).ElementType - : dataMethodFirstParameterSymbolType.GetEnumerableType(); - - if (arrayArgumentElementTypeSymbol is not null - && dataMethodFirstParameterElementSymbolType is not null - && ConversionChecker.IsConvertible( - compilation, arrayArgumentElementTypeSymbol, dataMethodFirstParameterElementSymbolType, xunitContext)) - argumentSyntaxList = new List { arraySyntax }; - } - int valueIdx = 0, paramIdx = 0; for (; valueIdx < argumentSyntaxList.Count && paramIdx < dataMethodParameterSymbols.Length; valueIdx++) {