Fix S4457 FP: Argument check after async code (#6624)

SonarSource · Jan 26, 2023 · 1e71ac1 · 1e71ac1
1 parent 986b17a
commit 1e71ac1
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 17 deletions.
diff --git a/...ules/ParameterValidationInMethodWalker.cs → ...pers/ParameterValidationInMethodWalker.cs b/...ules/ParameterValidationInMethodWalker.cs → ...pers/ParameterValidationInMethodWalker.cs
@@ -18,7 +18,7 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-namespace SonarAnalyzer.Rules.CSharp
+namespace SonarAnalyzer.Helpers
 {
     internal class ParameterValidationInMethodWalker : SafeCSharpSyntaxWalker
     {
@@ -33,7 +33,7 @@ internal class ParameterValidationInMethodWalker : SafeCSharpSyntaxWalker
         private readonly SemanticModel semanticModel;
         private readonly List<Location> argumentExceptionLocations = new();
 
-        private bool keepWalking = true;
+        protected bool keepWalking = true;
 
         public IEnumerable<Location> ArgumentExceptionLocations => argumentExceptionLocations;
 
@@ -49,9 +49,6 @@ public override void Visit(SyntaxNode node)
             }
         }
 
-        public override void VisitAwaitExpression(AwaitExpressionSyntax node) =>
-            keepWalking = false;
-
         public override void VisitThrowStatement(ThrowStatementSyntax node)
         {
             // When throw is like `throw new XXX` where XXX derives from ArgumentException, save location
@@ -61,16 +58,22 @@ public override void VisitThrowStatement(ThrowStatementSyntax node)
             {
                 argumentExceptionLocations.Add(node.Expression.GetLocation());
             }
+
             // there is no need to visit children
         }
 
         public override void VisitInvocationExpression(InvocationExpressionSyntax node)
         {
             if (node.IsMemberAccessOnKnownType("ThrowIfNull", KnownType.System_ArgumentNullException, semanticModel))
             {
+                // "ThrowIfNull" returns void so it cannot be an argument. We can stop.
                 argumentExceptionLocations.Add(node.GetLocation());
             }
-            // "ThrowIfNull" returns void so it cannot be an argument. We can stop.
+            else
+            {
+                // Need to check the children of this node because of the pattern (await SomeTask()).Invocation()
+                base.VisitInvocationExpression(node);
+            }
         }
     }
 }
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ParameterValidationInAsyncShouldBeWrapped.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ParameterValidationInAsyncShouldBeWrapped.cs
@@ -41,13 +41,24 @@ public sealed class ParameterValidationInAsyncShouldBeWrapped : SonarDiagnosticA
                         return;
                     }
 
-                    var walker = new ParameterValidationInMethodWalker(c.SemanticModel);
+                    var walker = new ParameterValidationInAsyncWalker(c.SemanticModel);
                     walker.SafeVisit(method);
                     if (walker.ArgumentExceptionLocations.Any())
                     {
                         c.ReportIssue(Rule.CreateDiagnostic(c.Compilation, method.Identifier.GetLocation(), walker.ArgumentExceptionLocations));
                     }
                 },
                 SyntaxKind.MethodDeclaration);
+
+        private sealed class ParameterValidationInAsyncWalker : ParameterValidationInMethodWalker
+        {
+            public ParameterValidationInAsyncWalker(SemanticModel semanticModel)
+                : base(semanticModel)
+            {
+            }
+
+            public override void VisitAwaitExpression(AwaitExpressionSyntax node) =>
+                keepWalking = false;
+        }
     }
 }
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ParameterValidationInYieldShouldBeWrapped.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ParameterValidationInYieldShouldBeWrapped.cs
@@ -23,16 +23,14 @@ namespace SonarAnalyzer.Rules.CSharp
     [DiagnosticAnalyzer(LanguageNames.CSharp)]
     public sealed class ParameterValidationInYieldShouldBeWrapped : SonarDiagnosticAnalyzer
     {
-        internal const string DiagnosticId = "S4456";
+        private const string DiagnosticId = "S4456";
         private const string MessageFormat = "Split this method into two, one handling parameters check and the other " +
            "handling the iterator.";
 
-        private static readonly DiagnosticDescriptor rule =
-            DescriptorFactory.Create(DiagnosticId, MessageFormat);
-        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(rule);
+        private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
 
-        protected override void Initialize(SonarAnalysisContext context)
-        {
+        protected override void Initialize(SonarAnalysisContext context) =>
             context.RegisterNodeAction(
                 c =>
                 {
@@ -44,13 +42,12 @@ protected override void Initialize(SonarAnalysisContext context)
                     if (walker.HasYieldStatement &&
                         walker.ArgumentExceptionLocations.Any())
                     {
-                        c.ReportIssue(rule.CreateDiagnostic(c.Compilation, methodDeclaration.Identifier.GetLocation(), additionalLocations: walker.ArgumentExceptionLocations));
+                        c.ReportIssue(Rule.CreateDiagnostic(c.Compilation, methodDeclaration.Identifier.GetLocation(), additionalLocations: walker.ArgumentExceptionLocations));
                     }
                 },
                 SyntaxKind.MethodDeclaration);
-        }
 
-        private class ParameterValidationInYieldWalker : ParameterValidationInMethodWalker
+        private sealed class ParameterValidationInYieldWalker : ParameterValidationInMethodWalker
         {
             public bool HasYieldStatement { get; private set; }
 
@@ -62,7 +59,6 @@ public ParameterValidationInYieldWalker(SemanticModel semanticModel)
             public override void VisitYieldStatement(YieldStatementSyntax node)
             {
                 HasYieldStatement = true;
-
                 base.VisitYieldStatement(node);
             }
         }

diff --git a/...yzers/tests/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInAsyncShouldBeWrapped.cs b/...yzers/tests/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInAsyncShouldBeWrapped.cs
@@ -1,5 +1,6 @@
 using System;
 using System.IO;
+using System.Linq;
 using System.Threading.Tasks;
 
 namespace Tests.Diagnostics
@@ -55,6 +56,20 @@ private static ArgumentNullException GetArgumentExpression(string name)
 
     public class ValidCases
     {
+        // https://github.com/SonarSource/sonar-dotnet/issues/6449
+        public class Repro_6449
+        {
+            public Task<int[]> CheckAsync() => Task.FromResult(new int[] { 1 });
+            public Task<int> Check2Async() => Task.FromResult(1);
+
+            public async Task HasS4457Async(int request) // Compliant 
+            {
+                var identifierType = (await CheckAsync()).FirstOrDefault(x => x == request);
+                if (identifierType == 0)
+                    throw new ArgumentException("message"); 
+            }
+        }
+
         public static Task FooAsync(string something)
         {
             if (something == null) { throw new ArgumentNullException(nameof(something)); }

diff --git a/...ts/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInYieldShouldBeWrapped.CSharp10.cs b/...ts/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInYieldShouldBeWrapped.CSharp10.cs
@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Threading.Tasks;
 
 namespace Tests.Diagnostics
 {
@@ -27,6 +28,32 @@ public static class InvalidCases
 
             yield break;
         }
+
+        // For details, check https://github.com/SonarSource/sonar-dotnet/pull/6624.
+        public static async IAsyncEnumerable<int> AsyncThenYield(object arg) // Noncompliant
+        {
+            if (arg is null)
+            {
+                throw new ArgumentException(nameof(arg));
+//                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Secondary
+            }
+
+            var res = await Task.Run(() => 42);
+            yield return res;
+        }
+
+        // For details, check https://github.com/SonarSource/sonar-dotnet/pull/6624.
+        public static async IAsyncEnumerable<int> NestedAsyncThenYield(object arg) // Noncompliant
+        {
+            if (arg is null)
+            {
+                throw new ArgumentException(nameof(arg));
+//                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Secondary
+            }
+
+            var res = (await Task.Run(() => 42)).GetHashCode();
+            yield return res;
+        }
     }
 
     public static class ValidCases

diff --git a/...yzers/tests/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInYieldShouldBeWrapped.cs b/...yzers/tests/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInYieldShouldBeWrapped.cs
@@ -54,6 +54,20 @@ private static ArgumentNullException GetArgumentExpression(string name)
         {
             return new ArgumentNullException(name);
         }
+
+        // Documenting that this rule fires even if T:ArgumentException has no argument
+        public static IEnumerable<int> ThrowWithoutArgument(int a) // Noncompliant
+        {
+            throw new ArgumentNullException(); // Secondary
+            yield return 42;
+        }
+
+        // Documenting that this rule fires even if T:ArgumentException has an ad-hoc argument
+        public static IEnumerable<int> ThrowWithAdHocArgument(int a) // Noncompliant
+        {
+            throw new ArgumentNullException("i am not a parameter name"); // Secondary
+            yield return 42;
+        }
     }
 
     public static class ValidCases