From 8bf481c2637f5928bd58f82df0f6ee62ce41b682 Mon Sep 17 00:00:00 2001 From: Gregory Paidis <115458417+gregory-paidis-sonarsource@users.noreply.github.com> Date: Thu, 21 Mar 2024 15:58:44 +0100 Subject: [PATCH] Fix S3878 FP: When input array is collection expression with spread operator (#8970) --- .../ShimLayer/SyntaxKindEx.cs | 1 + .../Rules/ArrayPassedAsParams.cs | 26 +++++++++++-------- .../Rules/ArrayPassedAsParamsBase.cs | 4 +-- .../Rules/ArrayPassedAsParams.cs | 2 +- .../TestCases/ArrayPassedAsParams.CSharp12.cs | 19 +++++++++----- 5 files changed, 31 insertions(+), 21 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CFG/ShimLayer/SyntaxKindEx.cs b/analyzers/src/SonarAnalyzer.CFG/ShimLayer/SyntaxKindEx.cs index 82e383da029..968006fa27e 100644 --- a/analyzers/src/SonarAnalyzer.CFG/ShimLayer/SyntaxKindEx.cs +++ b/analyzers/src/SonarAnalyzer.CFG/ShimLayer/SyntaxKindEx.cs @@ -93,5 +93,6 @@ public static class SyntaxKindEx public const SyntaxKind InterpolatedMultiLineRawStringStartToken = (SyntaxKind)9073; public const SyntaxKind InterpolatedRawStringEndToken = (SyntaxKind)9074; public const SyntaxKind ScopedType = (SyntaxKind)9075; + public const SyntaxKind SpreadElement = (SyntaxKind)9078; } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ArrayPassedAsParams.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ArrayPassedAsParams.cs index 819860d4e9f..c37342e0a08 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/ArrayPassedAsParams.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/ArrayPassedAsParams.cs @@ -32,10 +32,17 @@ public sealed class ArrayPassedAsParams : ArrayPassedAsParamsBase - GetLastArgumentIfArrayCreation(GetArgumentListFromExpression(expression)); + protected override ArgumentSyntax LastArgumentIfArrayCreation(SyntaxNode expression) => + LastArgumentIfArrayCreation(ArgumentList(expression)); - private static BaseArgumentListSyntax GetArgumentListFromExpression(SyntaxNode expression) => + private static ArgumentSyntax LastArgumentIfArrayCreation(BaseArgumentListSyntax argumentList) => + argumentList is { Arguments: { Count: > 0 } arguments } + && arguments.Last() is var lastArgument + && IsArrayCreation(lastArgument.Expression) + ? lastArgument + : null; + + private static BaseArgumentListSyntax ArgumentList(SyntaxNode expression) => expression switch { ObjectCreationExpressionSyntax { } creation => creation.ArgumentList, @@ -45,19 +52,16 @@ expression switch _ => null }; - private static ArgumentSyntax GetLastArgumentIfArrayCreation(BaseArgumentListSyntax argumentList) => - argumentList is { Arguments: { Count: > 0 } arguments } - && arguments.Last() is var lastArgument - && IsArrayCreation(lastArgument.Expression) - ? lastArgument - : null; - private static bool IsArrayCreation(ExpressionSyntax expression) => expression switch { ArrayCreationExpressionSyntax { Initializer: not null } => true, ImplicitArrayCreationExpressionSyntax => true, - _ when CollectionExpressionSyntaxWrapper.IsInstance(expression) => true, + _ when CollectionExpressionSyntaxWrapper.IsInstance(expression) => !ContainsSpread((CollectionExpressionSyntaxWrapper)expression), _ => false }; + + // [x, y, ..z] is not possible to be passed as params + private static bool ContainsSpread(CollectionExpressionSyntaxWrapper expression) => + expression.Elements.Any(x => x.SyntaxNode.IsKind(SyntaxKindEx.SpreadElement)); } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/ArrayPassedAsParamsBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/ArrayPassedAsParamsBase.cs index a0439c630c2..27ea96ff92a 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/ArrayPassedAsParamsBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/ArrayPassedAsParamsBase.cs @@ -30,7 +30,7 @@ public abstract class ArrayPassedAsParamsBase : Sona private readonly DiagnosticDescriptor rule; protected abstract TSyntaxKind[] ExpressionKinds { get; } - protected abstract TArgumentNode GetLastArgumentIfArrayCreation(SyntaxNode expression); + protected abstract TArgumentNode LastArgumentIfArrayCreation(SyntaxNode expression); protected ArrayPassedAsParamsBase() : base(DiagnosticId) => rule = Language.CreateDescriptor(DiagnosticId, MessageFormat); @@ -38,7 +38,7 @@ public abstract class ArrayPassedAsParamsBase : Sona protected sealed override void Initialize(SonarAnalysisContext context) => context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c => { - if (GetLastArgumentIfArrayCreation(c.Node) is { } lastArgument + if (LastArgumentIfArrayCreation(c.Node) is { } lastArgument && IsParamParameter(c.SemanticModel, c.Node, lastArgument)) { c.ReportIssue(Diagnostic.Create(rule, lastArgument.GetLocation())); diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/ArrayPassedAsParams.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/ArrayPassedAsParams.cs index a0f9c908fc7..d0d699d4e45 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/ArrayPassedAsParams.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/ArrayPassedAsParams.cs @@ -31,7 +31,7 @@ public sealed class ArrayPassedAsParams : ArrayPassedAsParamsBase + protected override ArgumentSyntax LastArgumentIfArrayCreation(SyntaxNode expression) => GetLastArgumentIfArrayCreation(GetArgumentListFromExpression(expression)); private static ArgumentListSyntax GetArgumentListFromExpression(SyntaxNode expression) => diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/ArrayPassedAsParams.CSharp12.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/ArrayPassedAsParams.CSharp12.cs index 57544925525..ace7c3caa5a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/ArrayPassedAsParams.CSharp12.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/ArrayPassedAsParams.CSharp12.cs @@ -3,18 +3,23 @@ const int a = 1; int[] array = [2, 3]; -_ = new MyClass(1, [1, 2, 3]); // Noncompliant -_ = new MyClass(1, []); // Noncompliant +var class1 = new MyClass(1, [1, 2, 3]); // Noncompliant +_ = new MyClass(1, []); // Noncompliant // repro for https://github.com/SonarSource/sonar-dotnet/issues/8510 -_ = new MyClass(1, [a, .. array]); // Noncompliant FP +_ = new MyClass(1, [a, .. array]); // Compliant -_ = new MyClass2([1], [1, 2, 3]); // Noncompliant +_ = new MyClass2([1], [1, 2, 3]); // Noncompliant _ = new MyClass2([1, 2, 3], 1); -_ = new MyClass3([1, 2, 3], [4, 5, 6]); // Noncompliant FP -_ = new MyClass3([[1, 2, 3], [4, 5, 6]]); // Noncompliant -_ = new MyClass3([[1, 2, 3], [4, 5, 6]]); // Noncompliant + +_ = new MyClass3([1, 2, 3], [4, 5, 6]); // Noncompliant FP + +_ = new MyClass4(class1, new(1, [1, .. array])); // Compliant +_ = new MyClass4([class1, new(1, [1, .. array])]); // Noncompliant, outer collection raises, despite the nested spread operator +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + class MyClass(int a, params int[] args); class MyClass2(int[] a, params int[] args); class MyClass3(params int[][] args); +class MyClass4(params MyClass[] args);