From a075e6748118f18f9de4e67ce5bcb05d5e7263f4 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 15:42:48 +0100 Subject: [PATCH 01/19] Create ValuesUsageAnalyzer.cs and add basic test. Implementation does not yet do anything but basic structure is in place. --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 23 ++++++ .../nunit.analyzers.tests.csproj | 3 +- .../Constants/AnalyzerIdentifiers.cs | 1 + .../Constants/NUnitFrameworkConstants.cs | 2 + .../ValuesUsage/ValuesUsageAnalyzer.cs | 74 +++++++++++++++++++ .../ValuesUsageAnalyzerConstants.cs | 17 +++++ src/nunit.analyzers/nunit.analyzers.csproj | 4 +- 7 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs create mode 100644 src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs create mode 100644 src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs new file mode 100644 index 00000000..c22944aa --- /dev/null +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -0,0 +1,23 @@ +using Gu.Roslyn.Asserts; +using NUnit.Analyzers.ValuesUsage; +using NUnit.Framework; + +namespace NUnit.Analyzers.Tests.ValuesUsage +{ + public class ValuesUsageAnalyzerTests + { + private readonly ValuesUsageAnalyzer analyzer = new ValuesUsageAnalyzer(); + + [Test] + public void Blah() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class Foo + { + [Test] + public void ATest([Values(true, false)] bool blah) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + } +} diff --git a/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj b/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj index 8e51f4ee..650dd3fd 100644 --- a/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj +++ b/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj @@ -18,8 +18,7 @@ - + diff --git a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs index b30c7c46..9d884ee5 100644 --- a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs +++ b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs @@ -86,6 +86,7 @@ internal static class AnalyzerIdentifiers internal const string UseAssertMultiple = "NUnit2045"; internal const string UsePropertyConstraint = "NUnit2046"; internal const string WithinIncompatibleTypes = "NUnit2047"; + internal const string ValuesParameterTypeMismatchUsage = "NUnit2048"; #endregion Assertion diff --git a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs index f7ed38bb..6fab5f2e 100644 --- a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs +++ b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs @@ -105,6 +105,7 @@ public static class NUnitFrameworkConstants public const string FullNameOfTypeParallelizableAttribute = "NUnit.Framework.ParallelizableAttribute"; public const string FullNameOfTypeITestBuilder = "NUnit.Framework.Interfaces.ITestBuilder"; public const string FullNameOfTypeISimpleTestBuilder = "NUnit.Framework.Interfaces.ISimpleTestBuilder"; + public const string FullNameOfTypeValuesAttribute = "NUnit.Framework.ValuesAttribute"; public const string FullNameOfTypeValueSourceAttribute = "NUnit.Framework.ValueSourceAttribute"; public const string FullNameOfTypeIParameterDataSource = "NUnit.Framework.Interfaces.IParameterDataSource"; public const string FullNameOfTypeTestCaseData = "NUnit.Framework.TestCaseData"; @@ -138,6 +139,7 @@ public static class NUnitFrameworkConstants public const string NameOfTestCaseSourceAttribute = "TestCaseSourceAttribute"; public const string NameOfTestAttribute = "TestAttribute"; public const string NameOfParallelizableAttribute = "ParallelizableAttribute"; + public const string NameOfValuesAttribute = "ValuesAttribute"; public const string NameOfValueSourceAttribute = "ValueSourceAttribute"; public const string NameOfExpectedResult = "ExpectedResult"; diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs new file mode 100644 index 00000000..993ce7a0 --- /dev/null +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs @@ -0,0 +1,74 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Analyzers.Constants; + +namespace NUnit.Analyzers.ValuesUsage +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class ValuesUsageAnalyzer : DiagnosticAnalyzer + { + private static readonly DiagnosticDescriptor parameterTypeMismatch = DiagnosticDescriptorCreator.Create(id: AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, + title: ValuesUsageAnalyzerConstants.ParameterTypeMismatchTitle, + messageFormat: ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, + category: Categories.Structure, + defaultSeverity: DiagnosticSeverity.Error, + description: ValuesUsageAnalyzerConstants.ParameterTypeMismatchDescription); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(parameterTypeMismatch); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(AnalyzeCompilationStart); + } + + private static void AnalyzeCompilationStart(CompilationStartAnalysisContext context) + { + var testCaseType = context.Compilation.GetTypeByMetadataName(NUnitFrameworkConstants.FullNameOfTypeValuesAttribute); + if (testCaseType is null) + { + return; + } + + context.RegisterSyntaxNodeAction(symbolContext => AnalyzeAttribute(symbolContext, testCaseType), SyntaxKind.Attribute); + } + + private static void AnalyzeAttribute(SyntaxNodeAnalysisContext context, INamedTypeSymbol valuesType) + { + var blah = (AttributeSyntax)context.Node; + var attributeSymbol = context.SemanticModel.GetSymbolInfo(blah).Symbol; + if (valuesType.ContainingAssembly.Identity != attributeSymbol?.ContainingAssembly.Identity || + attributeSymbol.ContainingType.Name != NUnitFrameworkConstants.NameOfValuesAttribute) + { + return; + } + + // TODO: Extract type of argument which this attribute is on. + + context.CancellationToken.ThrowIfCancellationRequested(); + + if (blah.ArgumentList is null) + { + return; + } + + var arguments = blah.ArgumentList.Arguments; + if (arguments.Count == 0) + { + return; + } + + foreach (var argument in arguments) + { + var expression = argument.Expression; + + // TODO: Extract type of expression. + // TODO: Compare type of expression with type of argument. + } + } + } +} diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs new file mode 100644 index 00000000..c063d490 --- /dev/null +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs @@ -0,0 +1,17 @@ +namespace NUnit.Analyzers.ValuesUsage +{ + internal static class ValuesUsageAnalyzerConstants + { + internal const string NotEnoughArgumentsTitle = "The TestCaseAttribute provided too few arguments"; + internal const string NotEnoughArgumentsMessage = "The TestCaseAttribute provided too few arguments. Expected '{0}', but got '{1}'."; + internal const string NotEnoughArgumentsDescription = "The number of arguments provided by a TestCaseAttribute must match the number of parameters of the method."; + + internal const string TooManyArgumentsTitle = "The TestCaseAttribute provided too many arguments"; + internal const string TooManyArgumentsMessage = "The TestCaseAttribute provided too many arguments. Expected '{0}', but got '{1}'."; + internal const string TooManyArgumentsDescription = "The number of arguments provided by a TestCaseAttribute must match the number of parameters of the method."; + + internal const string ParameterTypeMismatchTitle = "The individual arguments provided by a TestCaseAttribute must match the type of the corresponding parameter of the method"; + internal const string ParameterTypeMismatchMessage = "The value of the argument at position '{0}' of type {1} cannot be assigned to the parameter '{2}' of type {3}"; + internal const string ParameterTypeMismatchDescription = "The individual arguments provided by a TestCaseAttribute must match the type of the corresponding parameter of the method."; + } +} diff --git a/src/nunit.analyzers/nunit.analyzers.csproj b/src/nunit.analyzers/nunit.analyzers.csproj index 0452b360..ad673d5d 100644 --- a/src/nunit.analyzers/nunit.analyzers.csproj +++ b/src/nunit.analyzers/nunit.analyzers.csproj @@ -8,11 +8,11 @@ true - + - + From 817941a362b6dee5c6fe6c74c812c455cbcf37e6 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 18:17:58 +0100 Subject: [PATCH 02/19] Fix up ValuesUsageAnalyzerConstants.cs (remove unused options, correct messages). --- .../ValuesUsage/ValuesUsageAnalyzerConstants.cs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs index c063d490..b55cdd60 100644 --- a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzerConstants.cs @@ -2,16 +2,8 @@ namespace NUnit.Analyzers.ValuesUsage { internal static class ValuesUsageAnalyzerConstants { - internal const string NotEnoughArgumentsTitle = "The TestCaseAttribute provided too few arguments"; - internal const string NotEnoughArgumentsMessage = "The TestCaseAttribute provided too few arguments. Expected '{0}', but got '{1}'."; - internal const string NotEnoughArgumentsDescription = "The number of arguments provided by a TestCaseAttribute must match the number of parameters of the method."; - - internal const string TooManyArgumentsTitle = "The TestCaseAttribute provided too many arguments"; - internal const string TooManyArgumentsMessage = "The TestCaseAttribute provided too many arguments. Expected '{0}', but got '{1}'."; - internal const string TooManyArgumentsDescription = "The number of arguments provided by a TestCaseAttribute must match the number of parameters of the method."; - - internal const string ParameterTypeMismatchTitle = "The individual arguments provided by a TestCaseAttribute must match the type of the corresponding parameter of the method"; + internal const string ParameterTypeMismatchTitle = "The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method"; internal const string ParameterTypeMismatchMessage = "The value of the argument at position '{0}' of type {1} cannot be assigned to the parameter '{2}' of type {3}"; - internal const string ParameterTypeMismatchDescription = "The individual arguments provided by a TestCaseAttribute must match the type of the corresponding parameter of the method."; + internal const string ParameterTypeMismatchDescription = "The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method."; } } From 5e4e5a508b6cc6279d396b33ac6168f88f1c53eb Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 18:18:45 +0100 Subject: [PATCH 03/19] Copy patterns from TestCaseUsageAnalyzer instead of ValueSourceUsageAnalyzer. Add functioning test cases. --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 22 ++++++- .../ValuesUsage/ValuesUsageAnalyzer.cs | 58 ++++++++++++++++++- 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs index c22944aa..fb4189b2 100644 --- a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -1,4 +1,6 @@ -using Gu.Roslyn.Asserts; +using System.Globalization; +using Gu.Roslyn.Asserts; +using NUnit.Analyzers.Constants; using NUnit.Analyzers.ValuesUsage; using NUnit.Framework; @@ -9,7 +11,7 @@ public class ValuesUsageAnalyzerTests private readonly ValuesUsageAnalyzer analyzer = new ValuesUsageAnalyzer(); [Test] - public void Blah() + public void AnalyzeWhenArgumentTypeIsCorrect() { var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" public sealed class Foo @@ -19,5 +21,21 @@ public void ATest([Values(true, false)] bool blah) { } }"); RoslynAssert.Valid(this.analyzer, testCode); } + + [Test] + public void AnalyzeWhenArgumentTypeIsIncorrect() + { + var expectedDiagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, + string.Format(CultureInfo.InvariantCulture, + ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, + 1, "object", "blah", "bool")); + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class Foo + { + [Test] + public void ATest([Values(true, null)] bool blah) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); + } } } diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs index 993ce7a0..cfcb6f95 100644 --- a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs @@ -1,9 +1,11 @@ using System.Collections.Immutable; +using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using NUnit.Analyzers.Constants; +using NUnit.Analyzers.Extensions; namespace NUnit.Analyzers.ValuesUsage { @@ -28,18 +30,65 @@ public override void Initialize(AnalysisContext context) private static void AnalyzeCompilationStart(CompilationStartAnalysisContext context) { - var testCaseType = context.Compilation.GetTypeByMetadataName(NUnitFrameworkConstants.FullNameOfTypeValuesAttribute); - if (testCaseType is null) + var valuesType = context.Compilation.GetTypeByMetadataName(NUnitFrameworkConstants.FullNameOfTypeValuesAttribute); + if (valuesType is null) { return; } - context.RegisterSyntaxNodeAction(symbolContext => AnalyzeAttribute(symbolContext, testCaseType), SyntaxKind.Attribute); + // TODO: Which approach is correct? + // context.RegisterSyntaxNodeAction(symbolContext => AnalyzeAttribute(symbolContext, valuesType), SyntaxKind.Attribute); + context.RegisterSymbolAction(symbolContext => AnalyzeParameter(symbolContext, valuesType), SymbolKind.Parameter); + } + + private static void AnalyzeParameter(SymbolAnalysisContext symbolContext, INamedTypeSymbol valuesType) + { + var parameterSymbol = (IParameterSymbol)symbolContext.Symbol; + var attributes = parameterSymbol.GetAttributes(); + if (attributes.Length == 0) + { + return; + } + + foreach (var attribute in attributes.Where(x => x.ApplicationSyntaxReference is not null + && SymbolEqualityComparer.Default.Equals(x.AttributeClass, valuesType))) + { + symbolContext.CancellationToken.ThrowIfCancellationRequested(); + for (var index = 0; index < attribute.ConstructorArguments.Length; index++) + { + var constructorArgument = attribute.ConstructorArguments[index]; + var argumentTypeMatchesParameterType = constructorArgument.CanAssignTo(parameterSymbol.Type, + symbolContext.Compilation, + allowImplicitConversion: true, + allowEnumToUnderlyingTypeConversion: true); + if (argumentTypeMatchesParameterType) + { + continue; + } + + var attributeArgumentSyntax = attribute.GetConstructorArgumentSyntax(index, + symbolContext.CancellationToken); + if (attributeArgumentSyntax is null) + { + continue; + } + + var diagnostic = Diagnostic.Create(parameterTypeMismatch, + attributeArgumentSyntax.GetLocation(), + index, + constructorArgument.Type?.ToDisplayString() ?? "", + parameterSymbol.Name, + parameterSymbol.Type); + symbolContext.ReportDiagnostic(diagnostic); + } + } } private static void AnalyzeAttribute(SyntaxNodeAnalysisContext context, INamedTypeSymbol valuesType) { var blah = (AttributeSyntax)context.Node; + + // var foo = context. var attributeSymbol = context.SemanticModel.GetSymbolInfo(blah).Symbol; if (valuesType.ContainingAssembly.Identity != attributeSymbol?.ContainingAssembly.Identity || attributeSymbol.ContainingType.Name != NUnitFrameworkConstants.NameOfValuesAttribute) @@ -51,6 +100,9 @@ private static void AnalyzeAttribute(SyntaxNodeAnalysisContext context, INamedTy context.CancellationToken.ThrowIfCancellationRequested(); + // var fooBar = valuesType.Co + // TODO: remove !s. + var parameterType = ((ParameterSyntax)blah.Parent!.Parent!).Type!; if (blah.ArgumentList is null) { return; From 47e0e9bedc72ccb35bdccc394e4b6d5dff1ab34e Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 18:53:58 +0100 Subject: [PATCH 04/19] Add tests, mimicking tests from TestCaseUsageAnalyzerTests. --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 315 +++++++++++++++++- 1 file changed, 314 insertions(+), 1 deletion(-) diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs index fb4189b2..2c681b5d 100644 --- a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -1,5 +1,9 @@ -using System.Globalization; +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; using Gu.Roslyn.Asserts; +using Microsoft.CodeAnalysis; using NUnit.Analyzers.Constants; using NUnit.Analyzers.ValuesUsage; using NUnit.Framework; @@ -10,6 +14,61 @@ public class ValuesUsageAnalyzerTests { private readonly ValuesUsageAnalyzer analyzer = new ValuesUsageAnalyzer(); + private static IEnumerable SpecialConversions + { + get + { + yield return new TestCaseData("2019-10-10", typeof(DateTime)); + yield return new TestCaseData("23:59:59", typeof(TimeSpan)); + yield return new TestCaseData("2019-10-10", typeof(DateTimeOffset)); + yield return new TestCaseData("2019-10-14T19:15:25+00:00", typeof(DateTimeOffset)); + yield return new TestCaseData("https://github.com/nunit/", typeof(Uri)); + } + } + + [Test] + public void VerifySupportedDiagnostics() + { + var diagnostics = this.analyzer.SupportedDiagnostics; + + Assert.That(diagnostics, Has.Length.EqualTo(1)); + var diagnostic = diagnostics[0]; + Assert.Multiple(() => + { + Assert.That(diagnostic.Id, Is.EqualTo(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage)); + Assert.That(diagnostic.Title.ToString(CultureInfo.InvariantCulture), Is.Not.Empty); + Assert.That(diagnostic.Category, Is.EqualTo(Categories.Structure)); + Assert.That(diagnostic.DefaultSeverity, Is.EqualTo(DiagnosticSeverity.Error)); + }); + } + + [Test] + public void AnalyzeWhenAttributeIsNotInNUnit() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenAttributeIsNotInNUnit + { + [Test] + public void ATest([Values] bool value) { } + + private sealed class ValuesAttribute : Attribute + { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenAttributeHasNoArguments() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenAttributeIsNotInNUnit + { + [Test] + public void ATest([Values] bool value) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + [Test] public void AnalyzeWhenArgumentTypeIsCorrect() { @@ -22,6 +81,219 @@ public void ATest([Values(true, false)] bool blah) { } RoslynAssert.Valid(this.analyzer, testCode); } + [TestCaseSource(nameof(SpecialConversions))] + public void AnalyzeWhenArgumentIsSpecialConversion(string value, Type targetType) + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@" + public sealed class AnalyzeWhenArgumentIsSpecialConversion + {{ + [Test] + public void Test([Values(""{value}"")] {targetType.Name} a) {{ }} + }}"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenArgumentIsACast() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentIsACast + { + [Test] + public void Test([Values((byte)2)] byte a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenArgumentIsAPrefixedValue() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentIsAPrefixedValue + { + [Test] + public void Test([Values(-2)] int a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsEnum() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentIsEnum + { + public enum TestEnum { A,B,C } + + [Test] + public void Test([Values(TestEnum.A, TestEnum.B)] TestEnum e) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsIntConvertedToEnum() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentIsIntConvertedToEnum + { + public enum TestEnum { A,B,C } + + [Test] + public void Test([Values(0, 1)] TestEnum e) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsEnumConvertedToInt() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentIsEnumConvertedToInt + { + public enum TestEnum { A,B,C } + + [Test] + public void Test([Values(TestEnum.A, TestEnum.B)] int e) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeParameterIsArray() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentIsArray + { + [Test] + public void Test([Values(new byte[] { }, new byte[] { 1 }, new byte[] { 1, 2, 3 })] byte[] buffer) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsStringConvertedToEnum() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentIsStringConvertedToEnum + { + public enum TestEnum { A,B,C } + + [Test] + public void Test([Values(""A"", ""B"")] TestEnum e) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, + ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), + testCode); + } + + [Test] + public void AnalyzeArgumentIsEnumConvertedToString() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentIsEnumConvertedToString + { + public enum TestEnum { A,B,C } + + [Test] + public void Test([Values(TestEnum.A, TestEnum.B)] string e) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, + ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), + testCode); + } + + [Test] + public void AnalyzeArgumentHasImplicitConversion() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentHasImplicitConversion + { + [Test] + public void Test([Values(uint.MaxValue)] long e) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsStringConvertedToTypeWithCustomConverter() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + using System.ComponentModel; + + public sealed class AnalyzeArgumentIsStringConvertedToTypeWithCustomConverter + { + [Test] + public void Test([Values(""A"")] CustomType p) { } + } + + [TypeConverter(typeof(CustomTypeConverter))] + struct CustomType { } + class CustomTypeConverter : TypeConverter { }"); + + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsNullConvertedToTypeWithCustomConverter() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + using System.ComponentModel; + + public sealed class AnalyzeArgumentIsNullConvertedToTypeWithCustomConverter + { + [Test] + public void Test([Values(null)] CustomType p) { } + } + + [TypeConverter(typeof(CustomTypeConverter))] + struct CustomType { } + class CustomTypeConverter : TypeConverter { }"); + + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsStringConvertedToTypeWithCustomConverterOnBaseType() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + using System.ComponentModel; + + public sealed class AnalyzeArgumentIsStringConvertedToTypeWithCustomConverterOnBaseType + { + [Test] + public void Test([Values(""A"")] CustomType p) { } + } + + class CustomType : BaseType { } + [TypeConverter(typeof(BaseTypeConverter))] + class BaseType { } + class BaseTypeConverter : TypeConverter { }"); + + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeArgumentIsIntConvertedToTypeWithCustomConverter() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + using System.ComponentModel; + + public sealed class AnalyzeArgumentIsIntConvertedToTypeWithCustomConverter + { + [Test] + public void Test([Values(2)] CustomType p) { } + } + + [TypeConverter(typeof(CustomTypeConverter))] + struct CustomType { } + class CustomTypeConverter : TypeConverter { }"); + + RoslynAssert.Diagnostics(this.analyzer, + ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), + testCode); + } + [Test] public void AnalyzeWhenArgumentTypeIsIncorrect() { @@ -37,5 +309,46 @@ public void ATest([Values(true, null)] bool blah) { } }"); RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); } + + [Test] + public void AnalyzeArgumentHasTypeNotAllowedInAttributes() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeArgumentHasTypeNotAllowedInAttributes + { + [Test] + public void Test([Values(1.0m)] decimal d) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode, Settings.Default.WithAllowedCompilerDiagnostics(AllowedCompilerDiagnostics.WarningsAndErrors)); + } + + [Test] + public void AnalyzeWhenArgumentPassesNullToValueType() + { + var expectedDiagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, + string.Format(CultureInfo.InvariantCulture, + ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, + 0, "", "a", "char")); + + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentPassesNullToValueType + { + [Test] + public void Test([Values(null)] char a) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); + } + + [Test] + public void AnalyzeWhenArgumentPassesNullToNullableType() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + [Test] + public void Test([Values(null)] int? a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } } } From 218d8040dec89d9adaee8066c195893d38f89cf5 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 19:06:30 +0100 Subject: [PATCH 05/19] Fix up all tests. AnalyzeWhenArgumentPassesNullToValueType reports object?[] for the wrong type for [Values(null)] - can we do better? --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 21 ++++++++++--------- .../ValuesUsage/ValuesUsageAnalyzer.cs | 8 +++++++ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs index 2c681b5d..25e1bac8 100644 --- a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -228,8 +228,8 @@ public void Test([Values(""A"")] CustomType p) { } } [TypeConverter(typeof(CustomTypeConverter))] - struct CustomType { } - class CustomTypeConverter : TypeConverter { }"); + public struct CustomType { } + public class CustomTypeConverter : TypeConverter { }"); RoslynAssert.Valid(this.analyzer, testCode); } @@ -247,8 +247,8 @@ public void Test([Values(null)] CustomType p) { } } [TypeConverter(typeof(CustomTypeConverter))] - struct CustomType { } - class CustomTypeConverter : TypeConverter { }"); + public struct CustomType { } + public class CustomTypeConverter : TypeConverter { }"); RoslynAssert.Valid(this.analyzer, testCode); } @@ -265,10 +265,10 @@ public sealed class AnalyzeArgumentIsStringConvertedToTypeWithCustomConverterOnB public void Test([Values(""A"")] CustomType p) { } } - class CustomType : BaseType { } + public class CustomType : BaseType { } [TypeConverter(typeof(BaseTypeConverter))] - class BaseType { } - class BaseTypeConverter : TypeConverter { }"); + public class BaseType { } + public class BaseTypeConverter : TypeConverter { }"); RoslynAssert.Valid(this.analyzer, testCode); } @@ -286,8 +286,8 @@ public void Test([Values(2)] CustomType p) { } } [TypeConverter(typeof(CustomTypeConverter))] - struct CustomType { } - class CustomTypeConverter : TypeConverter { }"); + public struct CustomType { } + public class CustomTypeConverter : TypeConverter { }"); RoslynAssert.Diagnostics(this.analyzer, ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), @@ -325,10 +325,11 @@ public void Test([Values(1.0m)] decimal d) { } [Test] public void AnalyzeWhenArgumentPassesNullToValueType() { + // TODO: Can we get this to report instead of object?[] var expectedDiagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, string.Format(CultureInfo.InvariantCulture, ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, - 0, "", "a", "char")); + 0, "object?[]", "a", "char")); var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" public sealed class AnalyzeWhenArgumentPassesNullToValueType diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs index cfcb6f95..d48f13b5 100644 --- a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs @@ -57,6 +57,14 @@ private static void AnalyzeParameter(SymbolAnalysisContext symbolContext, INamed for (var index = 0; index < attribute.ConstructorArguments.Length; index++) { var constructorArgument = attribute.ConstructorArguments[index]; + + // If the compiler detects an illegal constant, we shouldn't check it. + // Unfortunately the constant 'null' is marked as Error with a null type. + if (constructorArgument.Kind == TypedConstantKind.Error && constructorArgument.Type is not null) + { + continue; + } + var argumentTypeMatchesParameterType = constructorArgument.CanAssignTo(parameterSymbol.Type, symbolContext.Compilation, allowImplicitConversion: true, From ee744d67068547ff74d118bac365900a220e0584 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 19:08:05 +0100 Subject: [PATCH 06/19] Remove unused code. --- .../ValuesUsage/ValuesUsageAnalyzer.cs | 43 ------------------- 1 file changed, 43 deletions(-) diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs index d48f13b5..74274cce 100644 --- a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs @@ -1,8 +1,6 @@ using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using NUnit.Analyzers.Constants; using NUnit.Analyzers.Extensions; @@ -36,8 +34,6 @@ private static void AnalyzeCompilationStart(CompilationStartAnalysisContext cont return; } - // TODO: Which approach is correct? - // context.RegisterSyntaxNodeAction(symbolContext => AnalyzeAttribute(symbolContext, valuesType), SyntaxKind.Attribute); context.RegisterSymbolAction(symbolContext => AnalyzeParameter(symbolContext, valuesType), SymbolKind.Parameter); } @@ -91,44 +87,5 @@ private static void AnalyzeParameter(SymbolAnalysisContext symbolContext, INamed } } } - - private static void AnalyzeAttribute(SyntaxNodeAnalysisContext context, INamedTypeSymbol valuesType) - { - var blah = (AttributeSyntax)context.Node; - - // var foo = context. - var attributeSymbol = context.SemanticModel.GetSymbolInfo(blah).Symbol; - if (valuesType.ContainingAssembly.Identity != attributeSymbol?.ContainingAssembly.Identity || - attributeSymbol.ContainingType.Name != NUnitFrameworkConstants.NameOfValuesAttribute) - { - return; - } - - // TODO: Extract type of argument which this attribute is on. - - context.CancellationToken.ThrowIfCancellationRequested(); - - // var fooBar = valuesType.Co - // TODO: remove !s. - var parameterType = ((ParameterSyntax)blah.Parent!.Parent!).Type!; - if (blah.ArgumentList is null) - { - return; - } - - var arguments = blah.ArgumentList.Arguments; - if (arguments.Count == 0) - { - return; - } - - foreach (var argument in arguments) - { - var expression = argument.Expression; - - // TODO: Extract type of expression. - // TODO: Compare type of expression with type of argument. - } - } } } From daa68a36bdba0ee2c44026b2f03629bfed6d17c2 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Sat, 12 Aug 2023 19:11:02 +0100 Subject: [PATCH 07/19] Tidy names of classes. --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs index 25e1bac8..c784b41a 100644 --- a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -61,7 +61,7 @@ private sealed class ValuesAttribute : Attribute public void AnalyzeWhenAttributeHasNoArguments() { var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" - public sealed class AnalyzeWhenAttributeIsNotInNUnit + public sealed class AnalyzeWhenAttributeHasNoArguments { [Test] public void ATest([Values] bool value) { } @@ -73,7 +73,7 @@ public void ATest([Values] bool value) { } public void AnalyzeWhenArgumentTypeIsCorrect() { var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" - public sealed class Foo + public sealed class AnalyzeWhenArgumentTypeIsCorrect { [Test] public void ATest([Values(true, false)] bool blah) { } From 4a253f9a916ab37bf16d10e053650c9705f71d4a Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Mon, 14 Aug 2023 11:55:59 +0100 Subject: [PATCH 08/19] Fix up failing tests in NUnitFrameworkConstantsTests.cs. --- .../Constants/NUnitFrameworkConstantsTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs index 7d375796..45dcef86 100644 --- a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs +++ b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs @@ -118,6 +118,7 @@ public sealed class NUnitFrameworkConstantsTests (nameof(NUnitFrameworkConstants.NameOfTestAttribute), nameof(TestAttribute)), (nameof(NUnitFrameworkConstants.NameOfParallelizableAttribute), nameof(ParallelizableAttribute)), (nameof(NUnitFrameworkConstants.NameOfValueSourceAttribute), nameof(ValueSourceAttribute)), + (nameof(NUnitFrameworkConstants.NameOfValuesAttribute), nameof(ValuesAttribute)), (nameof(NUnitFrameworkConstants.NameOfExpectedResult), nameof(TestAttribute.ExpectedResult)), @@ -137,6 +138,7 @@ public sealed class NUnitFrameworkConstantsTests (nameof(NUnitFrameworkConstants.FullNameOfTypeTestAttribute), typeof(TestAttribute)), (nameof(NUnitFrameworkConstants.FullNameOfTypeParallelizableAttribute), typeof(ParallelizableAttribute)), (nameof(NUnitFrameworkConstants.FullNameOfTypeValueSourceAttribute), typeof(ValueSourceAttribute)), + (nameof(NUnitFrameworkConstants.FullNameOfTypeValuesAttribute), typeof(ValuesAttribute)), (nameof(NUnitFrameworkConstants.FullNameOfTypeITestBuilder), typeof(ITestBuilder)), (nameof(NUnitFrameworkConstants.FullNameOfTypeISimpleTestBuilder), typeof(ISimpleTestBuilder)), (nameof(NUnitFrameworkConstants.FullNameOfTypeIParameterDataSource), typeof(IParameterDataSource)), From 01057af242425ce8942257274ed4c07e18e0c8e0 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Mon, 14 Aug 2023 12:14:22 +0100 Subject: [PATCH 09/19] Add documentation for NUnit2048. --- documentation/NUnit2048.md | 101 +++++++++++++++++++++++++++++++++++++ src/nunit.analyzers.sln | 1 + 2 files changed, 102 insertions(+) create mode 100644 documentation/NUnit2048.md diff --git a/documentation/NUnit2048.md b/documentation/NUnit2048.md new file mode 100644 index 00000000..24fb494c --- /dev/null +++ b/documentation/NUnit2048.md @@ -0,0 +1,101 @@ +# NUnit2048 + +## The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method + +| Topic | Value +| :-- | :-- +| Id | NUnit2048 +| Severity | Error +| Enabled | True +| Category | Structure +| Code | [ValuesUsageAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs) + +## Description + +The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method. + +## Motivation + +To prevent tests that will fail at runtime due to improper construction. + +## How to fix violations + +### Example Violation + +```csharp +[Test] +public void SampleTest([Values(0.0, 1.0)] int numberValue) +{ + Assert.That(numberValue, Is.AnyOf(0, 1)); +} +``` + +### Problem + +In the test above, `numberValue` is declared as an integer. However, `[Values(0.0, 1.0)]` provides values as doubles. This will lead to a runtime failure. + +### Fix + +Ensure that the type of the objects used by the `ValuesAttribute` matches that of the parameter. + +So, this fix would be acceptable: + +```csharp +// Both use type int. +[Test] +public void SampleTest([Values(0, 1)] int numberValue) +{ + Assert.That(numberValue, Is.AnyOf(0, 1)); +} +``` + +And this would also work: +```csharp +// Both use type double +[Test] +public void SampleTest([Values(0.0, 1.0)] double numberValue) +{ + Assert.That(numberValue, Is.AnyOf(0, 1)); +} +``` + +ADD HOW TO FIX VIOLATIONS HERE + + +## Configure severity + +### Via ruleset file + +Configure the severity per project, for more info see [MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022). + +### Via .editorconfig file + +```ini +# NUnit2048: The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +dotnet_diagnostic.NUnit2048.severity = chosenSeverity +``` + +where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`. + +### Via #pragma directive + +```csharp +#pragma warning disable NUnit2048 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +Code violating the rule here +#pragma warning restore NUnit2048 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +``` + +Or put this at the top of the file to disable all instances. + +```csharp +#pragma warning disable NUnit2048 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +``` + +### Via attribute `[SuppressMessage]` + +```csharp +[System.Diagnostics.CodeAnalysis.SuppressMessage("Structure", + "NUnit2048:The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method", + Justification = "Reason...")] +``` + diff --git a/src/nunit.analyzers.sln b/src/nunit.analyzers.sln index 6df28636..8e498bae 100644 --- a/src/nunit.analyzers.sln +++ b/src/nunit.analyzers.sln @@ -89,6 +89,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat ..\documentation\NUnit2045.md = ..\documentation\NUnit2045.md ..\documentation\NUnit2046.md = ..\documentation\NUnit2046.md ..\documentation\NUnit2047.md = ..\documentation\NUnit2047.md + ..\documentation\NUnit2048.md = ..\documentation\NUnit2048.md ..\documentation\NUnit3001.md = ..\documentation\NUnit3001.md ..\documentation\NUnit3002.md = ..\documentation\NUnit3002.md ..\documentation\NUnit3003.md = ..\documentation\NUnit3003.md From fdfe3dab5cdd5dfe199f968b2808f05943e8069c Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Mon, 14 Aug 2023 12:44:04 +0100 Subject: [PATCH 10/19] Rename NUnit2048 to NUnit1031, since it reports a structural issue, not improving an assertion. --- documentation/{NUnit2048.md => NUnit1031.md} | 16 ++++++++-------- src/nunit.analyzers.sln | 2 +- .../Constants/AnalyzerIdentifiers.cs | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) rename documentation/{NUnit2048.md => NUnit1031.md} (86%) diff --git a/documentation/NUnit2048.md b/documentation/NUnit1031.md similarity index 86% rename from documentation/NUnit2048.md rename to documentation/NUnit1031.md index 24fb494c..7db8443a 100644 --- a/documentation/NUnit2048.md +++ b/documentation/NUnit1031.md @@ -1,10 +1,10 @@ -# NUnit2048 +# NUnit1031 ## The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method | Topic | Value | :-- | :-- -| Id | NUnit2048 +| Id | NUnit1031 | Severity | Error | Enabled | True | Category | Structure @@ -71,8 +71,8 @@ Configure the severity per project, for more info see [MSDN](https://learn.micro ### Via .editorconfig file ```ini -# NUnit2048: The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method -dotnet_diagnostic.NUnit2048.severity = chosenSeverity +# NUnit1031: The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +dotnet_diagnostic.NUnit1031.severity = chosenSeverity ``` where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`. @@ -80,22 +80,22 @@ where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, ### Via #pragma directive ```csharp -#pragma warning disable NUnit2048 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +#pragma warning disable NUnit1031 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method Code violating the rule here -#pragma warning restore NUnit2048 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +#pragma warning restore NUnit1031 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method ``` Or put this at the top of the file to disable all instances. ```csharp -#pragma warning disable NUnit2048 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method +#pragma warning disable NUnit1031 // The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method ``` ### Via attribute `[SuppressMessage]` ```csharp [System.Diagnostics.CodeAnalysis.SuppressMessage("Structure", - "NUnit2048:The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method", + "NUnit1031:The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method", Justification = "Reason...")] ``` diff --git a/src/nunit.analyzers.sln b/src/nunit.analyzers.sln index 8e498bae..7b0f7d35 100644 --- a/src/nunit.analyzers.sln +++ b/src/nunit.analyzers.sln @@ -42,6 +42,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat ..\documentation\NUnit1028.md = ..\documentation\NUnit1028.md ..\documentation\NUnit1029.md = ..\documentation\NUnit1029.md ..\documentation\NUnit1030.md = ..\documentation\NUnit1030.md + ..\documentation\NUnit1030.md = ..\documentation\NUnit1031.md ..\documentation\NUnit2001.md = ..\documentation\NUnit2001.md ..\documentation\NUnit2002.md = ..\documentation\NUnit2002.md ..\documentation\NUnit2003.md = ..\documentation\NUnit2003.md @@ -89,7 +90,6 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat ..\documentation\NUnit2045.md = ..\documentation\NUnit2045.md ..\documentation\NUnit2046.md = ..\documentation\NUnit2046.md ..\documentation\NUnit2047.md = ..\documentation\NUnit2047.md - ..\documentation\NUnit2048.md = ..\documentation\NUnit2048.md ..\documentation\NUnit3001.md = ..\documentation\NUnit3001.md ..\documentation\NUnit3002.md = ..\documentation\NUnit3002.md ..\documentation\NUnit3003.md = ..\documentation\NUnit3003.md diff --git a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs index 9d884ee5..d4daff02 100644 --- a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs +++ b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs @@ -34,6 +34,7 @@ internal static class AnalyzerIdentifiers internal const string NonTestMethodIsPublic = "NUnit1028"; internal const string TestCaseSourceMismatchInNumberOfTestMethodParameters = "NUnit1029"; internal const string TestCaseSourceMismatchWithTestMethodParameterType = "NUnit1030"; + internal const string ValuesParameterTypeMismatchUsage = "NUnit1031"; #endregion Structure @@ -86,7 +87,6 @@ internal static class AnalyzerIdentifiers internal const string UseAssertMultiple = "NUnit2045"; internal const string UsePropertyConstraint = "NUnit2046"; internal const string WithinIncompatibleTypes = "NUnit2047"; - internal const string ValuesParameterTypeMismatchUsage = "NUnit2048"; #endregion Assertion From c656890e61ee4e0891a464220c11b7e996a6de0f Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Mon, 14 Aug 2023 12:44:32 +0100 Subject: [PATCH 11/19] Add NUnit1031 to index.md. --- documentation/index.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/documentation/index.md b/documentation/index.md index 54b29028..8911db38 100644 --- a/documentation/index.md +++ b/documentation/index.md @@ -17,7 +17,7 @@ The severity of a diagnostic can the one of the following (in increasing severit Rules which enforce structural requirements on the test code. -| Id | Title | :mag: | :memo: | :bulb: | +| Id | Title | :mag: | :memo: | :bulb: | | :-- | :-- | :--: | :--: | :--: | | [NUnit1001](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1001.md) | The individual arguments provided by a TestCaseAttribute must match the type of the corresponding parameter of the method | :white_check_mark: | :exclamation: | :x: | | [NUnit1002](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1002.md) | The TestCaseSource should use nameof operator to specify target | :white_check_mark: | :warning: | :white_check_mark: | @@ -49,6 +49,7 @@ Rules which enforce structural requirements on the test code. | [NUnit1028](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1028.md) | The non-test method is public | :white_check_mark: | :information_source: | :white_check_mark: | | [NUnit1029](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1029.md) | The number of parameters provided by the TestCaseSource does not match the number of parameters in the Test method | :white_check_mark: | :exclamation: | :x: | | [NUnit1030](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1030.md) | The type of parameter provided by the TestCaseSource does not match the type of the parameter in the Test method | :white_check_mark: | :exclamation: | :x: | +| [NUnit1031](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1031.md) | The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method | :white_check_mark: | :exclamation: | :x: | ### Assertion Rules (NUnit2001 - ) From 772cf08e7336dc7771906038c8bd425ae35928ff Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Mon, 14 Aug 2023 13:02:58 +0100 Subject: [PATCH 12/19] Fix erroneous whitespace change in index.md. --- documentation/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/index.md b/documentation/index.md index 8911db38..a0216c20 100644 --- a/documentation/index.md +++ b/documentation/index.md @@ -17,7 +17,7 @@ The severity of a diagnostic can the one of the following (in increasing severit Rules which enforce structural requirements on the test code. -| Id | Title | :mag: | :memo: | :bulb: | +| Id | Title | :mag: | :memo: | :bulb: | | :-- | :-- | :--: | :--: | :--: | | [NUnit1001](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1001.md) | The individual arguments provided by a TestCaseAttribute must match the type of the corresponding parameter of the method | :white_check_mark: | :exclamation: | :x: | | [NUnit1002](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1002.md) | The TestCaseSource should use nameof operator to specify target | :white_check_mark: | :warning: | :white_check_mark: | From 7f92e072afeaf2e8ee6cfd638f7476b466ad3138 Mon Sep 17 00:00:00 2001 From: Andrew McClement Date: Tue, 15 Aug 2023 11:42:15 +0100 Subject: [PATCH 13/19] Tidy NUnit1031.md. --- documentation/NUnit1031.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/documentation/NUnit1031.md b/documentation/NUnit1031.md index 7db8443a..d754de7a 100644 --- a/documentation/NUnit1031.md +++ b/documentation/NUnit1031.md @@ -18,8 +18,6 @@ The individual arguments provided by a ValuesAttribute must match the type of th To prevent tests that will fail at runtime due to improper construction. -## How to fix violations - ### Example Violation ```csharp @@ -34,7 +32,7 @@ public void SampleTest([Values(0.0, 1.0)] int numberValue) In the test above, `numberValue` is declared as an integer. However, `[Values(0.0, 1.0)]` provides values as doubles. This will lead to a runtime failure. -### Fix +## How to fix violations Ensure that the type of the objects used by the `ValuesAttribute` matches that of the parameter. @@ -59,8 +57,6 @@ public void SampleTest([Values(0.0, 1.0)] double numberValue) } ``` -ADD HOW TO FIX VIOLATIONS HERE - ## Configure severity From fc41e9478eec77c1e4d8692e7fed17ed559d8a28 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Sat, 19 Aug 2023 12:11:24 +0800 Subject: [PATCH 14/19] Re-align with 1001 version --- documentation/NUnit1031.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/documentation/NUnit1031.md b/documentation/NUnit1031.md index d754de7a..1797afed 100644 --- a/documentation/NUnit1031.md +++ b/documentation/NUnit1031.md @@ -18,6 +18,8 @@ The individual arguments provided by a ValuesAttribute must match the type of th To prevent tests that will fail at runtime due to improper construction. +## How to fix violations + ### Example Violation ```csharp @@ -32,7 +34,7 @@ public void SampleTest([Values(0.0, 1.0)] int numberValue) In the test above, `numberValue` is declared as an integer. However, `[Values(0.0, 1.0)]` provides values as doubles. This will lead to a runtime failure. -## How to fix violations +### Fix Ensure that the type of the objects used by the `ValuesAttribute` matches that of the parameter. @@ -48,6 +50,7 @@ public void SampleTest([Values(0, 1)] int numberValue) ``` And this would also work: + ```csharp // Both use type double [Test] From dafd8e6ccf38e3836a3f22f234f20eec70219cb6 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Sat, 19 Aug 2023 12:19:46 +0800 Subject: [PATCH 15/19] Fix test to reflect change in CustomTypeConverter assumptions. --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs index c784b41a..81cad693 100644 --- a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -289,9 +289,7 @@ public void Test([Values(2)] CustomType p) { } public struct CustomType { } public class CustomTypeConverter : TypeConverter { }"); - RoslynAssert.Diagnostics(this.analyzer, - ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), - testCode); + RoslynAssert.Valid(this.analyzer, testCode); } [Test] From 66817af720c5fcb117928e5127976e67074353ea Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Sat, 19 Aug 2023 12:20:40 +0800 Subject: [PATCH 16/19] Prevent polluting the nunit test output unnecessary. --- .../DocumentationTests.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/nunit.analyzers.tests/DocumentationTests.cs b/src/nunit.analyzers.tests/DocumentationTests.cs index f947ff76..e4ea0e87 100644 --- a/src/nunit.analyzers.tests/DocumentationTests.cs +++ b/src/nunit.analyzers.tests/DocumentationTests.cs @@ -85,7 +85,7 @@ public void EnsureAllDescriptorsHaveDocumentation(DescriptorInfo descriptorInfo) { var descriptor = descriptorInfo.Descriptor; var id = descriptor.Id; - DumpIfDebug(descriptorInfo.Stub); + DumpIfRequested(descriptorInfo.Stub); File.WriteAllText(descriptorInfo.DocumentationFile.Name + ".generated", descriptorInfo.Stub); Assert.Fail($"Documentation is missing for {id}"); } @@ -98,7 +98,7 @@ public void EnsureAllSuppressorsHaveDocumentation(SuppressorInfo suppressorInfo) { var descriptor = suppressorInfo.Descriptor; var id = descriptor.Id; - DumpIfDebug(suppressorInfo.Stub); + DumpIfRequested(suppressorInfo.Stub); File.WriteAllText(suppressorInfo.DocumentationFile.Name + ".generated", suppressorInfo.Stub); Assert.Fail($"Documentation is missing for {id}"); } @@ -156,8 +156,8 @@ public void Description(DescriptorInfo descriptorInfo) .First(l => !string.IsNullOrWhiteSpace(l)); var actual = Replace(actualRaw, "`", string.Empty); - DumpIfDebug(expected); - DumpIfDebug(actual); + DumpIfRequested(expected); + DumpIfRequested(actual); Assert.That(actual, Is.EqualTo(expected)); } @@ -166,7 +166,7 @@ public void EnsureThatTableIsAsExpected(BaseInfo info) { const string headerRow = "| Topic | Value"; var expected = GetTable(info.Stub, headerRow); - DumpIfDebug(expected); + DumpIfRequested(expected); var actual = GetTable(info.DocumentationFile.AllText, headerRow); CodeAssert.AreEqual(expected, actual); } @@ -175,7 +175,7 @@ public void EnsureThatTableIsAsExpected(BaseInfo info) public void EnsureThatConfigSeverityIsAsExpected(BaseInfo info) { var expected = GetConfigSeverity(info.Stub); - DumpIfDebug(expected); + DumpIfRequested(expected); var actual = GetConfigSeverity(info.DocumentationFile.AllText); CodeAssert.AreEqual(expected, actual); @@ -224,7 +224,7 @@ public void EnsureThatAnalyzerIndexIsAsExpected(string category, int tableNumber } var expected = builder.ToString(); - DumpIfDebug(expected); + DumpIfRequested(expected); var actual = GetTable(File.ReadAllText(Path.Combine(DocumentsDirectory.FullName, "index.md")), headerRow, tableNumber); CodeAssert.AreEqual(expected, actual); } @@ -254,7 +254,7 @@ public void EnsureThatSuppressionIndexIsAsExpected(int tableNumber) } var expected = builder.ToString(); - DumpIfDebug(expected); + DumpIfRequested(expected); var actual = GetTable(File.ReadAllText(Path.Combine(DocumentsDirectory.FullName, "index.md")), headerRow, tableNumber); CodeAssert.AreEqual(expected, actual); } @@ -290,8 +290,8 @@ int TableLength() } } - [Conditional("DEBUG")] - private static void DumpIfDebug(string text) + [Conditional("DUMP_DOCUMENTATION")] + private static void DumpIfRequested(string text) { Console.Write(text); Console.WriteLine(); From d29dd57fd74504c37904d4b163a3799132fcd557 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Sat, 19 Aug 2023 16:59:50 +0800 Subject: [PATCH 17/19] Added support for Nullable to TestCaseUsageAnalyzer Moved AdjustArguments to re-usable extension method. Added GetAdjustedArgumentSyntax to get proper argument from explicit array parameters Added unit tests for explicit array parameters Added unit tests for nullable --- .../TestCaseUsageAnalyzerTests.cs | 72 ++++++++++++++++++- ...ttributeArgumentTypedConstantExtensions.cs | 43 ++++++++++- .../Extensions/AttributeDataExtensions.cs | 28 ++++++++ .../TestCaseUsage/TestCaseUsageAnalyzer.cs | 31 ++------ 4 files changed, 143 insertions(+), 31 deletions(-) diff --git a/src/nunit.analyzers.tests/TestCaseUsage/TestCaseUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/TestCaseUsage/TestCaseUsageAnalyzerTests.cs index 7d5b84fd..8b38990b 100644 --- a/src/nunit.analyzers.tests/TestCaseUsage/TestCaseUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/TestCaseUsage/TestCaseUsageAnalyzerTests.cs @@ -213,6 +213,32 @@ public void Test(byte[] buffer) { } RoslynAssert.Valid(this.analyzer, testCode); } + [Test] + public void AnalyzeParameterIsArrayOfObjectWithGoodArguments() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + class AnalyzeArgumentIsArrayOfObject + { + [TestCase(new object[] { 1, 2.0 })] + public void Test(int i, double d) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeParameterIsArrayOfObjectWithBadArguments() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + class AnalyzeArgumentIsArrayOfObject + { + [TestCase(new object[] { 1, ↓2.0 })] + public void Test(int i, int ii) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, + ExpectedDiagnostic.Create(AnalyzerIdentifiers.TestCaseParameterTypeMismatchUsage), + testCode); + } + [Test] public void AnalyzeArgumentIsStringConvertedToEnum() { @@ -386,7 +412,7 @@ public void Test(char a) { } public void AnalyzeWhenArgumentPassesNullToNullableType() { var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" - public sealed class AnalyzeWhenArgumentPassesNullToNullableType + public sealed class AnalyzeWhenArgumentPassesNullToNullableValueType { [TestCase(null)] public void Test(int? a) { } @@ -394,6 +420,48 @@ public void Test(int? a) { } RoslynAssert.Valid(this.analyzer, testCode); } + [Test] + public void AnalyzeWhenArgumentPassesNullToNullableReferenceType() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + [TestCase(null)] + public void Test(object? a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenArgumentPassesNullToUnspecifiedReferenceType() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + #nullable disable + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + [TestCase(null)] + public void Test(object a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenArgumentPassesNullToNonNullableReferenceType() + { + var expectedDiagnostic = ExpectedDiagnostic.Create( + AnalyzerIdentifiers.TestCaseParameterTypeMismatchUsage, + string.Format(CultureInfo.InvariantCulture, + TestCaseUsageAnalyzerConstants.ParameterTypeMismatchMessage, 0, "", "a", "object")); + + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + [TestCase(↓null)] + public void Test(object a) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); + } + [Test] public void AnalyzeWhenArgumentPassesValueToNullableType() { @@ -534,7 +602,7 @@ public void AnalyzeWhenMethodHasOnlyParamsAndArgumentPassesNullToReferenceType() public sealed class AnalyzeWhenMethodHasOnlyParamsAndArgumentPassesNullToReferenceType { [TestCase(null)] - public void Test(params string[] a) { } + public void Test(params string[]? a) { } }"); RoslynAssert.Valid(this.analyzer, testCode); } diff --git a/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs b/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs index 15e25d87..ad5f30b4 100644 --- a/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs +++ b/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.ComponentModel; using System.Globalization; using System.Linq; @@ -10,6 +11,8 @@ namespace NUnit.Analyzers.Extensions { internal static class AttributeArgumentTypedConstantExtensions { + #region CanAssignTo + private static readonly IReadOnlyList ConvertibleTypes = new List { typeof(short), @@ -71,8 +74,13 @@ internal static bool CanAssignTo(this TypedConstant @this, ITypeSymbol target, C if (argumentValue is null) { - if (target.IsReferenceType || - target.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T) + if ( +#if NETSTANDARD1_6 + target.IsReferenceType +#else + (target.IsReferenceType && target.NullableAnnotation != NullableAnnotation.NotAnnotated) +#endif + || target.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T) { return true; } @@ -222,5 +230,36 @@ private static bool CanBeTranslatedByTypeConverter( return false; } + +#endregion + +#pragma warning disable SA1202 // Elements should be ordered by access. Prefer grouping + internal static ImmutableArray AdjustArguments(this ImmutableArray attributePositionalArguments) +#pragma warning restore SA1202 // Elements should be ordered by access + { + if (attributePositionalArguments.Length == 1 + && attributePositionalArguments[0] is { Kind: TypedConstantKind.Array } arrayArgument + && arrayArgument.Type is IArrayTypeSymbol arrayType + && arrayType.ElementType.SpecialType == SpecialType.System_Object) + { + // params overload, need to check array values instead + + if (arrayArgument.IsNull) + { + // If null is provided - analyze it as single null value, not as null array + // Thebelow creates an invalid TypedConstant (Error, NullType) + // but no public constructor is available to create a proper one. + return ImmutableArray.Create(default(TypedConstant)); + } + else + { + // This is a special case where the argument is explicitly specified as an array. + // TestCase(new object[] { 1.0, 2 }) is then translated into TestCase(1.0, 2) + return arrayArgument.Values; + } + } + + return attributePositionalArguments; + } } } diff --git a/src/nunit.analyzers/Extensions/AttributeDataExtensions.cs b/src/nunit.analyzers/Extensions/AttributeDataExtensions.cs index c4d80cc3..495c2ee0 100644 --- a/src/nunit.analyzers/Extensions/AttributeDataExtensions.cs +++ b/src/nunit.analyzers/Extensions/AttributeDataExtensions.cs @@ -1,3 +1,4 @@ +using System.Collections.Immutable; using System.Linq; using System.Threading; using Microsoft.CodeAnalysis; @@ -53,6 +54,33 @@ public static bool IsSetUpOrTearDownMethodAttribute(this AttributeData @this, Co .ElementAtOrDefault(position); } + public static ExpressionSyntax? GetAdjustedArgumentSyntax(this AttributeData @this, int position, + ImmutableArray attributePositionalArguments, + CancellationToken cancellationToken = default) + { + AttributeArgumentSyntax? attributeArgumentSyntax; + + if (@this.ConstructorArguments == attributePositionalArguments) + { + attributeArgumentSyntax = @this.GetConstructorArgumentSyntax(position, cancellationToken); + } + else + { + // Arguments have been adjusted to expand explicitly passed in array. + // Get the first argument. + attributeArgumentSyntax = @this.GetConstructorArgumentSyntax(0, cancellationToken); + + if (attributeArgumentSyntax?.Expression is ArrayCreationExpressionSyntax arrayCreationExpressionSyntax && + arrayCreationExpressionSyntax.Initializer is InitializerExpressionSyntax initializerExpressionSyntax) + { + // Get the position element of the array initializer + return initializerExpressionSyntax.Expressions.ElementAtOrDefault(position); + } + } + + return attributeArgumentSyntax?.Expression; + } + public static AttributeArgumentSyntax? GetNamedArgumentSyntax(this AttributeData @this, string name, CancellationToken cancellationToken = default) { diff --git a/src/nunit.analyzers/TestCaseUsage/TestCaseUsageAnalyzer.cs b/src/nunit.analyzers/TestCaseUsage/TestCaseUsageAnalyzer.cs index b7109d3c..b9333def 100644 --- a/src/nunit.analyzers/TestCaseUsage/TestCaseUsageAnalyzer.cs +++ b/src/nunit.analyzers/TestCaseUsage/TestCaseUsageAnalyzer.cs @@ -74,7 +74,7 @@ private static void AnalyzeMethod(SymbolAnalysisContext context, INamedTypeSymbo var (methodRequiredParameters, methodOptionalParameters, methodParamsParameters) = methodSymbol.GetParameterCounts(); - var attributePositionalArguments = AdjustArguments(attribute.ConstructorArguments); + var attributePositionalArguments = attribute.ConstructorArguments.AdjustArguments(); // From NUnit.Framework.TestCaseAttribute.GetParametersForTestCase // Special handling when sole method parameter is an object[]. @@ -119,29 +119,6 @@ private static void AnalyzeMethod(SymbolAnalysisContext context, INamedTypeSymbo } } - private static ImmutableArray AdjustArguments(ImmutableArray attributePositionalArguments) - { - if (attributePositionalArguments.Length == 1 - && attributePositionalArguments[0] is { Kind: TypedConstantKind.Array } arrayArgument - && arrayArgument.Type is IArrayTypeSymbol arrayType - && arrayType.ElementType.SpecialType == SpecialType.System_Object) - { - // params overload, need to check array values instead - - if (arrayArgument.IsNull) - { - // If null is provided - analyze it as single null value, not as null array - return ImmutableArray.Create(default(TypedConstant)); - } - else - { - return arrayArgument.Values; - } - } - - return attributePositionalArguments; - } - private static bool IsSoleParameterAnObjectArray(IMethodSymbol methodSymbol) { if (methodSymbol.Parameters.Length != 1) @@ -222,13 +199,13 @@ private static void AnalyzePositionalArgumentsAndParameters( } } - var attributeArgumentSyntax = attribute.GetConstructorArgumentSyntax(i, context.CancellationToken); + var argumentSyntax = attribute.GetAdjustedArgumentSyntax(i, attributePositionalArguments, context.CancellationToken); - if (attributeArgumentSyntax is null) + if (argumentSyntax is null) continue; context.ReportDiagnostic(Diagnostic.Create(parameterTypeMismatch, - attributeArgumentSyntax.GetLocation(), + argumentSyntax.GetLocation(), i, argumentType?.ToDisplayString() ?? "", methodParameterName, From 965e1916fac86d617b3318f9c59178687df37327 Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Sat, 19 Aug 2023 17:01:00 +0800 Subject: [PATCH 18/19] ValueUsageAnalyzer now also calls AdjustArguments Added unit tests for explicit array parameters Added unit tests for nullable --- .../ValuesUsage/ValuesUsageAnalyzerTests.cs | 77 +++++++++++++++++-- .../ValuesUsage/ValuesUsageAnalyzer.cs | 17 ++-- 2 files changed, 80 insertions(+), 14 deletions(-) diff --git a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs index 81cad693..54a38b0a 100644 --- a/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/ValuesUsage/ValuesUsageAnalyzerTests.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.Globalization; -using System.Linq; using Gu.Roslyn.Asserts; using Microsoft.CodeAnalysis; using NUnit.Analyzers.Constants; @@ -171,6 +170,30 @@ public sealed class AnalyzeArgumentIsArray RoslynAssert.Valid(this.analyzer, testCode); } + [Test] + public void AnalyzeParameterIsArrayOfObjectWithGoodArguments() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + class AnalyzeArgumentIsArrayOfObject + { + public void Test([Values(new object[] { 1, 2.0 })] double d) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeParameterIsArrayOfObjectWithBadArguments() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + class AnalyzeArgumentIsArrayOfObject + { + public void Test([Values(new object[] { 1, ↓2.0 })] int i) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, + ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), + testCode); + } + [Test] public void AnalyzeArgumentIsStringConvertedToEnum() { @@ -180,7 +203,7 @@ public sealed class AnalyzeArgumentIsStringConvertedToEnum public enum TestEnum { A,B,C } [Test] - public void Test([Values(""A"", ""B"")] TestEnum e) { } + public void Test([Values(↓""A"", ↓""B"")] TestEnum e) { } }"); RoslynAssert.Diagnostics(this.analyzer, ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), @@ -196,7 +219,7 @@ public sealed class AnalyzeArgumentIsEnumConvertedToString public enum TestEnum { A,B,C } [Test] - public void Test([Values(TestEnum.A, TestEnum.B)] string e) { } + public void Test([Values(↓TestEnum.A, ↓TestEnum.B)] string e) { } }"); RoslynAssert.Diagnostics(this.analyzer, ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage), @@ -303,7 +326,7 @@ public void AnalyzeWhenArgumentTypeIsIncorrect() public sealed class Foo { [Test] - public void ATest([Values(true, null)] bool blah) { } + public void ATest([Values(true, ↓null)] bool blah) { } }"); RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); } @@ -323,23 +346,22 @@ public void Test([Values(1.0m)] decimal d) { } [Test] public void AnalyzeWhenArgumentPassesNullToValueType() { - // TODO: Can we get this to report instead of object?[] var expectedDiagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, string.Format(CultureInfo.InvariantCulture, ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, - 0, "object?[]", "a", "char")); + 0, "", "a", "char")); var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" public sealed class AnalyzeWhenArgumentPassesNullToValueType { [Test] - public void Test([Values(null)] char a) { } + public void Test([Values(↓null)] char a) { } }"); RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); } [Test] - public void AnalyzeWhenArgumentPassesNullToNullableType() + public void AnalyzeWhenArgumentPassesNullToNullableValueType() { var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" public sealed class AnalyzeWhenArgumentPassesNullToNullableType @@ -349,5 +371,44 @@ public void Test([Values(null)] int? a) { } }"); RoslynAssert.Valid(this.analyzer, testCode); } + + [Test] + public void AnalyzeWhenArgumentPassesNullToNullableReferenceType() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + public void Test([Values(null)] object? a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenArgumentPassesNullToUnspecifiedReferenceType() + { + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + #nullable disable + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + public void Test([Values(null)] object a) { } + }"); + RoslynAssert.Valid(this.analyzer, testCode); + } + + [Test] + public void AnalyzeWhenArgumentPassesNullToNonNullableReferenceType() + { + var expectedDiagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, + string.Format(CultureInfo.InvariantCulture, + ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, + 0, "", "a", "object")); + + var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" + public sealed class AnalyzeWhenArgumentPassesNullToNullableType + { + public void Test([Values(↓null)] object a) { } + }"); + RoslynAssert.Diagnostics(this.analyzer, expectedDiagnostic, testCode); + } } } diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs index 74274cce..58aa52f1 100644 --- a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs @@ -50,9 +50,12 @@ private static void AnalyzeParameter(SymbolAnalysisContext symbolContext, INamed && SymbolEqualityComparer.Default.Equals(x.AttributeClass, valuesType))) { symbolContext.CancellationToken.ThrowIfCancellationRequested(); - for (var index = 0; index < attribute.ConstructorArguments.Length; index++) + + var attributePositionalArguments = attribute.ConstructorArguments.AdjustArguments(); + + for (var index = 0; index < attributePositionalArguments.Length; index++) { - var constructorArgument = attribute.ConstructorArguments[index]; + var constructorArgument = attributePositionalArguments[index]; // If the compiler detects an illegal constant, we shouldn't check it. // Unfortunately the constant 'null' is marked as Error with a null type. @@ -70,15 +73,17 @@ private static void AnalyzeParameter(SymbolAnalysisContext symbolContext, INamed continue; } - var attributeArgumentSyntax = attribute.GetConstructorArgumentSyntax(index, - symbolContext.CancellationToken); - if (attributeArgumentSyntax is null) + var argumentSyntax = attribute.GetAdjustedArgumentSyntax(index, + attributePositionalArguments, + symbolContext.CancellationToken); + + if (argumentSyntax is null) { continue; } var diagnostic = Diagnostic.Create(parameterTypeMismatch, - attributeArgumentSyntax.GetLocation(), + argumentSyntax.GetLocation(), index, constructorArgument.Type?.ToDisplayString() ?? "", parameterSymbol.Name, From f3d9c74ae712f3a0e31be6fbc2bfd2240fe2db7a Mon Sep 17 00:00:00 2001 From: Manfred Brands Date: Tue, 29 Aug 2023 11:26:39 +0800 Subject: [PATCH 19/19] Apply suggestions from code review Co-authored-by: Mikkel Nylander Bundgaard --- src/nunit.analyzers.sln | 2 +- .../AttributeArgumentTypedConstantExtensions.cs | 2 +- .../ValuesUsage/ValuesUsageAnalyzer.cs | 13 +++++++------ 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/nunit.analyzers.sln b/src/nunit.analyzers.sln index 7b0f7d35..274995f3 100644 --- a/src/nunit.analyzers.sln +++ b/src/nunit.analyzers.sln @@ -42,7 +42,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat ..\documentation\NUnit1028.md = ..\documentation\NUnit1028.md ..\documentation\NUnit1029.md = ..\documentation\NUnit1029.md ..\documentation\NUnit1030.md = ..\documentation\NUnit1030.md - ..\documentation\NUnit1030.md = ..\documentation\NUnit1031.md + ..\documentation\NUnit1031.md = ..\documentation\NUnit1031.md ..\documentation\NUnit2001.md = ..\documentation\NUnit2001.md ..\documentation\NUnit2002.md = ..\documentation\NUnit2002.md ..\documentation\NUnit2003.md = ..\documentation\NUnit2003.md diff --git a/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs b/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs index ad5f30b4..b6b66e61 100644 --- a/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs +++ b/src/nunit.analyzers/Extensions/AttributeArgumentTypedConstantExtensions.cs @@ -247,7 +247,7 @@ internal static ImmutableArray AdjustArguments(this ImmutableArra if (arrayArgument.IsNull) { // If null is provided - analyze it as single null value, not as null array - // Thebelow creates an invalid TypedConstant (Error, NullType) + // The code below creates an invalid TypedConstant (Error, NullType) // but no public constructor is available to create a proper one. return ImmutableArray.Create(default(TypedConstant)); } diff --git a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs index 58aa52f1..f659ca03 100644 --- a/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs +++ b/src/nunit.analyzers/ValuesUsage/ValuesUsageAnalyzer.cs @@ -10,12 +10,13 @@ namespace NUnit.Analyzers.ValuesUsage [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class ValuesUsageAnalyzer : DiagnosticAnalyzer { - private static readonly DiagnosticDescriptor parameterTypeMismatch = DiagnosticDescriptorCreator.Create(id: AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, - title: ValuesUsageAnalyzerConstants.ParameterTypeMismatchTitle, - messageFormat: ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, - category: Categories.Structure, - defaultSeverity: DiagnosticSeverity.Error, - description: ValuesUsageAnalyzerConstants.ParameterTypeMismatchDescription); + private static readonly DiagnosticDescriptor parameterTypeMismatch = DiagnosticDescriptorCreator.Create( + id: AnalyzerIdentifiers.ValuesParameterTypeMismatchUsage, + title: ValuesUsageAnalyzerConstants.ParameterTypeMismatchTitle, + messageFormat: ValuesUsageAnalyzerConstants.ParameterTypeMismatchMessage, + category: Categories.Structure, + defaultSeverity: DiagnosticSeverity.Error, + description: ValuesUsageAnalyzerConstants.ParameterTypeMismatchDescription); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(parameterTypeMismatch);