Rule S4457: Parameter validation in 'async'/'await' methods should be… (

#1235)

...wrapped

sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S4457.json

@@ -0,0 +1,28 @@
+{
+"issues":  [
+{
+"id":  "S4457",
+"message":  "Split this method into two, one handling parameters check and the other handling the iterator.",
+"location":  [
+{
+"uri":  "sources\Nancy\src\Nancy\NancyEngine.cs",
+"region":  {
+"startLine":  102,
+"startColumn":  41,
+"endLine":  102,
+"endColumn":  54
+}
+},
+{
+"uri":  "sources\Nancy\src\Nancy\NancyEngine.cs",
+"region":  {
+"startLine":  110,
+"startColumn":  31,
+"endLine":  110,
+"endColumn":  52
+}
+}
+]
+}
+]
+}

sonaranalyzer-dotnet/rspec/cs/S4457_c#.html

@@ -0,0 +1,40 @@
+<p>Because of the way <code>async/await</code> methods are rewritten by the compiler, any exceptions thrown during the parameters check will happen
+only when the task is observed. That could happen far away from the source of the buggy code or never happen for fire-and-forget tasks.</p>
+<p>Therefore it is recommended to split the method into two: an outer method handling the parameter checks (without being <code>async/await</code>)
+and an inner method to handle the iterator block with the <code>async/await</code> pattern.</p>
+<p>This rule raises an issue when an <code>async</code> method throws any exception derived from <code>ArgumentException</code> and contains
+<code>await</code> keyword.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+public static async Task SkipLinesAsync(this TextReader reader, int linesToSkip) // Noncompliant
+{
+    if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
+    if (linesToSkip &lt; 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }
+
+    for (var i = 0; i &lt; linesToSkip; ++i)
+    {
+        var line = await reader.ReadLineAsync().ConfigureAwait(false);
+        if (line == null) { break; }
+    }
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+public static Task SkipLinesAsync(this TextReader reader, int linesToSkip)
+{
+    if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
+    if (linesToSkip &lt; 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }
+
+    return reader.SkipLinesInternalAsync(linesToSkip);
+}
+
+private static async Task SkipLinesInternalAsync(this TextReader reader, int linesToSkip)
+{
+    for (var i = 0; i &lt; linesToSkip; ++i)
+    {
+        var line = await reader.ReadLineAsync().ConfigureAwait(false);
+        if (line == null) { break; }
+    }
+}
+</pre>
+

sonaranalyzer-dotnet/rspec/cs/S4457_c#.json

@@ -0,0 +1,15 @@
+{
+  "title": "Parameter validation in \"async\"\/\"await\" methods should be wrapped",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "15"
+  },
+  "tags": [
+    "async-await"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-4457",
+  "sqKey": "S4457"
+}

sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json

@@ -198,6 +198,7 @@
     "S4220",
     "S4260",
     "S4277",
-    "S4428"
+    "S4428",
+    "S4457"
   ]
 }

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx

@@ -8760,6 +8760,33 @@
   <data name="S4428_Type" xml:space="preserve">
     <value>BUG</value>
   </data>
+  <data name="S4457_Category" xml:space="preserve">
+    <value>Major Code Smell</value>
+  </data>
+  <data name="S4457_Description" xml:space="preserve">
+    <value>Because of the way async/await methods are rewritten by the compiler, any exceptions thrown during the parameters check will happen only when the task is observed. That could happen far away from the source of the buggy code or never happen for fire-and-forget tasks.</value>
+  </data>
+  <data name="S4457_IsActivatedByDefault" xml:space="preserve">
+    <value>True</value>
+  </data>
+  <data name="S4457_Remediation" xml:space="preserve">
+    <value>Constant/Issue</value>
+  </data>
+  <data name="S4457_RemediationCost" xml:space="preserve">
+    <value>15</value>
+  </data>
+  <data name="S4457_Severity" xml:space="preserve">
+    <value>Major</value>
+  </data>
+  <data name="S4457_Tags" xml:space="preserve">
+    <value>async-await</value>
+  </data>
+  <data name="S4457_Title" xml:space="preserve">
+    <value>Parameter validation in "async"/"await" methods should be wrapped</value>
+  </data>
+  <data name="S4457_Type" xml:space="preserve">
+    <value>CODE_SMELL</value>
+  </data>
   <data name="S818_Category" xml:space="preserve">
     <value>Minor Code Smell</value>
   </data>

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/ParameterValidationInAsyncShouldBeWrapped.cs

@@ -0,0 +1,75 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2018 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+using System.Collections.Immutable;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+using SonarAnalyzer.Common;
+using SonarAnalyzer.Helpers;
+
+namespace SonarAnalyzer.Rules.CSharp
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    [Rule(DiagnosticId)]
+    public sealed class ParameterValidationInAsyncShouldBeWrapped : SonarDiagnosticAnalyzer
+    {
+        internal const string DiagnosticId = "S4457";
+        private const string MessageFormat = "Split this method into two, one handling parameters check and the other " +
+            "handling the iterator.";
+
+        private static readonly DiagnosticDescriptor rule =
+            DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager);
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(rule);
+
+        protected override void Initialize(SonarAnalysisContext context)
+        {
+            context.RegisterSyntaxNodeActionInNonGenerated(
+                c =>
+                {
+                    var awaitExpression = (AwaitExpressionSyntax)c.Node;
+
+                    var methodDeclaration = awaitExpression.FirstAncestorOrSelf<MethodDeclarationSyntax>();
+                    if (methodDeclaration == null)
+                    {
+                        return;
+                    }
+
+                    var argumentExceptionLocations = methodDeclaration.DescendantNodes()
+                        .OfType<ObjectCreationExpressionSyntax>()
+                        .Select(objectCreationSyntax => objectCreationSyntax.Type)
+                        .OfType<IdentifierNameSyntax>()
+                        .Select(identifierSyntax => c.SemanticModel.GetSymbolInfo(identifierSyntax).Symbol.ToSymbolWithSyntax(identifierSyntax))
+                        .Where(tuple => (tuple.Symbol.OriginalDefinition as ITypeSymbol).DerivesFrom(KnownType.System_ArgumentException))
+                        .Select(tuple => tuple.Syntax.Identifier.GetLocation())
+                        .ToList();
+
+                    if (argumentExceptionLocations.Count > 0)
+                    {
+                        c.ReportDiagnosticWhenActive(Diagnostic.Create(rule, methodDeclaration.Identifier.GetLocation(),
+                            additionalLocations: argumentExceptionLocations));
+                    }
+                },
+                SyntaxKind.AwaitExpression);
+        }
+    }
+}

sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4457.html

@@ -0,0 +1,40 @@
+<p>Because of the way <code>async/await</code> methods are rewritten by the compiler, any exceptions thrown during the parameters check will happen
+only when the task is observed. That could happen far away from the source of the buggy code or never happen for fire-and-forget tasks.</p>
+<p>Therefore it is recommended to split the method into two: an outer method handling the parameter checks (without being <code>async/await</code>)
+and an inner method to handle the iterator block with the <code>async/await</code> pattern.</p>
+<p>This rule raises an issue when an <code>async</code> method throws any exception derived from <code>ArgumentException</code> and contains
+<code>await</code> keyword.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+public static async Task SkipLinesAsync(this TextReader reader, int linesToSkip) // Noncompliant
+{
+    if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
+    if (linesToSkip &lt; 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }
+
+    for (var i = 0; i &lt; linesToSkip; ++i)
+    {
+        var line = await reader.ReadLineAsync().ConfigureAwait(false);
+        if (line == null) { break; }
+    }
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+public static Task SkipLinesAsync(this TextReader reader, int linesToSkip)
+{
+    if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
+    if (linesToSkip &lt; 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }
+
+    return reader.SkipLinesInternalAsync(linesToSkip);
+}
+
+private static async Task SkipLinesInternalAsync(this TextReader reader, int linesToSkip)
+{
+    for (var i = 0; i &lt; linesToSkip; ++i)
+    {
+        var line = await reader.ReadLineAsync().ConfigureAwait(false);
+        if (line == null) { break; }
+    }
+}
+</pre>
+

sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/CsRuleTypeMapping.cs

@@ -4454,7 +4454,7 @@ static internal class CsRuleTypeMapping
             //["4454"],
             //["4455"],
             //["4456"],
-            //["4457"],
+            ["4457"] = "CODE_SMELL",
             //["4458"],
             //["4459"],
             //["4460"],

sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/Rules/ParameterValidationInAsyncShouldBeWrappedTest.cs

@@ -0,0 +1,38 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2018 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+extern alias csharp;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using csharp::SonarAnalyzer.Rules.CSharp;
+
+namespace SonarAnalyzer.UnitTest.Rules
+{
+    [TestClass]
+    public class ParameterValidationInAsyncShouldBeWrappedTest
+    {
+        [TestMethod]
+        [TestCategory("Rule")]
+        public void ParameterValidationInAsyncShouldBeWrapped()
+        {
+            Verifier.VerifyAnalyzer(@"TestCases\ParameterValidationInAsyncShouldBeWrapped.cs",
+                new ParameterValidationInAsyncShouldBeWrapped());
+        }
+    }
+}

sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ParameterValidationInAsyncShouldBeWrapped.cs

@@ -0,0 +1,94 @@
+using System;
+using System.IO;
+using System.Threading.Tasks;
+
+namespace Tests.Diagnostics
+{
+    public class InvalidCases
+    {
+        public static async Task SkipLinesAsync(this TextReader reader, int linesToSkip) // Noncompliant {{Split this method into two, one handling parameters check and the other handling the iterator.}}
+//                               ^^^^^^^^^^^^^^
+        {
+            if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
+//                                          ^^^^^^^^^^^^^^^^^^^^^ Secondary
+            if (linesToSkip < 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }
+//                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Secondary
+
+            for (var i = 0; i < linesToSkip; ++i)
+            {
+                var line = await reader.ReadLineAsync().ConfigureAwait(false);
+                if (line == null) { break; }
+            }
+        }
+
+        public async Task DoSomethingAsync(string value) // Noncompliant - this is an edge case that might be worth handling later on
+        {
+            await Task.Delay(0);
+
+            if (value == null)
+            {
+                throw new ArgumentNullException(nameof(value)); // Secondary
+            }
+        }
+
+        public async void OnSomeEvent(object sender, EventArgs args) // Noncompliant - it might looks weird to throw from some event method but that's valid syntax
+        {
+            if (sender == null)
+            {
+                throw new ArgumentNullException(nameof(sender)); // Secondary
+            }
+
+            await Task.Yield();
+        }
+
+        public static Task SkipLinesAsync(this TextReader reader, int linesToSkip) // Noncompliant - Not sure what should be the expected result (i.e. do local functions suffer from this behavior?)
+        {
+            if (reader == null) { throw new ArgumentNullException(nameof(reader)); } // Secondary
+            if (linesToSkip < 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); } // Secondary
+
+            return reader.SkipLinesInternalAsync(linesToSkip);
+
+            async Task SkipLinesInternalAsync(this TextReader reader, int linesToSkip)
+            {
+                for (var i = 0; i < linesToSkip; ++i)
+                {
+                    var line = await reader.ReadLineAsync().ConfigureAwait(false);
+                    if (line == null) { break; }
+                }
+            }
+        }
+    }
+
+    public class ValidCases
+    {
+        public static Task SkipLinesAsync(this TextReader reader, int linesToSkip)
+        {
+            if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
+            if (linesToSkip < 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }
+
+            return reader.SkipLinesInternalAsync(linesToSkip);
+        }
+
+        private static async Task SkipLinesInternalAsync(this TextReader reader, int linesToSkip)
+        {
+            for (var i = 0; i < linesToSkip; ++i)
+            {
+                var line = await reader.ReadLineAsync().ConfigureAwait(false);
+                if (line == null) { break; }
+            }
+        }
+
+        public async Task DoAsync() // Compliant - no args check
+        {
+            await Task.Delay(0);
+        }
+
+        public async Task FooAsync(int age) // Compliant - the exception doesn't derive from ArgumentException
+        {
+            if (age == 0)
+            {
+                throw new Exception("Wrong age");
+            }
+        }
+    }
+}