Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false-positive errors from xunit/xunit#2826 #170

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Fix false-positive errors from xunit/xunit#2826 #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7847dc3
Candidate fix for https://github.com/xunit/xunit/issues/2826
Nov 23, 2023
595095e
Adding additional test
Nov 23, 2023
fed650d
Try a different approach testing specifically for array element type
Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions src/xunit.analyzers.tests/Analyzers/X1000/MemberDataShouldReferenceValidMemberTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,69 @@ public void TestMethod<T>(T? n) {{ }}
await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source);
}

[Fact]
public async void DoesNotFindWarning_WithIntArrayArguments()
{
var source = $@"
using System.Collections.Generic;
using Xunit;

public class TestClass
{{
public static IEnumerable<object[]> GetSequences(IEnumerable<int> seq) =>
new List<object[]>();

[Theory]
[MemberData(nameof(GetSequences), new int[] {{ 1, 2 }})]
[MemberData(nameof(GetSequences), new [] {{ 3, 4, 5}})]
public void Test(IEnumerable<int> seq)
{{
Assert.NotEmpty(seq);
}}
}}
";

await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source);
}

[Fact]
public async void FindsWarning_WithObjectArrayArguments()
{
var source = $@"
using System.Collections.Generic;
using Xunit;

public class TestClass
{{
public static IEnumerable<object[]> GetSequences(IEnumerable<object> seq) =>
new List<object[]>();

[Theory]
[MemberData(nameof(GetSequences), new object[] {{ 1, 2 }})]
JamesTerwilliger marked this conversation as resolved.
Show resolved Hide resolved
public void Test(IEnumerable<int> seq)
{{
Assert.NotEmpty(seq);
}}
}}
";

DiagnosticResult[] expected =
{
Verify
.Diagnostic("xUnit1035")
.WithSpan(11, 52, 11, 53)
.WithArguments("seq", "System.Collections.Generic.IEnumerable<object>")
.WithSeverity(DiagnosticSeverity.Error),
Verify
.Diagnostic("xUnit1036")
.WithSpan(11, 55, 11, 56)
.WithArguments("2")
.WithSeverity(DiagnosticSeverity.Error)
};

await Verify.VerifyAnalyzer(LanguageVersion.CSharp10, source, expected);
}

[Theory]
[MemberData(nameof(MemberSyntaxAndArgs))]
public async void FindWarning_IfHasValidTheoryDataMemberWithTooManyTypeParameters(
Expand Down
18 changes: 14 additions & 4 deletions src/xunit.analyzers/X1000/MemberDataShouldReferenceValidMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -215,7 +218,8 @@ public static (INamedTypeSymbol? TestClass, ITypeSymbol? MemberClass) GetClassTy
return (testClassTypeSymbol, declaredMemberTypeSymbol);
}

static IList<ExpressionSyntax>? GetParameterExpressionsFromArrayArgument(List<AttributeArgumentSyntax> arguments)
static IList<ExpressionSyntax>? GetParameterExpressionsFromArrayArgument(
List<AttributeArgumentSyntax> arguments, SemanticModel semanticModel)
{
if (arguments.Count > 1)
return arguments.Select(a => a.Expression).ToList();
Expand All @@ -224,7 +228,8 @@ public static (INamedTypeSymbol? TestClass, ITypeSymbol? MemberClass) GetClassTy

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,
Expand All @@ -234,7 +239,12 @@ public static (INamedTypeSymbol? TestClass, ITypeSymbol? MemberClass) GetClassTy
if (initializer is null)
return new List<ExpressionSyntax> { argumentExpression };

return initializer.Expressions.ToList();
// 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<ExpressionSyntax> { argumentExpression };
Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JamesTerwilliger could this be shortcutted by inverting the if on 244 and executing the if between line 231 and 232? then in this case, it does not have to determine initializer. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viceroypenguin I had it this way thinking that initializer being not null means that it is an array type, and that the semantic model call is likely more expensive. Said differently, the initializer being null is the short-circuit that I want so that the (I believe) expensive call is only done if need be.

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good reasoning; and I'll trust your experience with this code, not having enough experience with touching the semanticModel in analyzers. LGTM

}

public static ISymbol? FindMethodSymbol(
Expand Down Expand Up @@ -493,7 +503,7 @@ static void VerifyDataMethodParameterUsage(
string memberName,
List<AttributeArgumentSyntax> extraArguments)
{
var argumentSyntaxList = GetParameterExpressionsFromArrayArgument(extraArguments);
var argumentSyntaxList = GetParameterExpressionsFromArrayArgument(extraArguments, semanticModel);
if (argumentSyntaxList is null)
return;

Expand Down