Rule S2234: Parameters should be passed in the correct order (#2262)

SonarSource · Feb 7, 2019 · f310ce8 · f310ce8
1 parent 8d8d7e4
commit f310ce8
Show file tree

Hide file tree

Showing 24 changed files with 631 additions and 258 deletions.
diff --git a/sonaranalyzer-dotnet/rspec/vbnet/S2234_vb.net.html b/sonaranalyzer-dotnet/rspec/vbnet/S2234_vb.net.html
@@ -0,0 +1,32 @@
+<p>When the names of parameters in a procedure call match the names of the procedure arguments, it contributes to a clearer, more readable code.
+However, when the names match but are passed in a different order than the method arguments, it indicates a mistake in the parameter order which will
+likely lead to unexpected results.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+Public Function Divide(ByVal divisor As Integer, ByVal dividend As Integer) As Double
+    Return divisor / dividend
+End Function
+
+Public Sub DoTheThing()
+    Dim divisor = 15
+    Dim dividend = 5
+
+    Dim result = Divide(dividend, divisor)  // Noncompliant; operation succeeds, but result is unexpected
+    //...
+End Sub
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+Public Function Divide(ByVal divisor As Integer, ByVal dividend As Integer) As Double
+    Return divisor / dividend
+End Function
+
+Public Sub DoTheThing()
+    Dim divisor = 15
+    Dim dividend = 5
+
+    Dim result = Divide(divisor, dividend)
+    //...
+End Sub
+</pre>
+
diff --git a/sonaranalyzer-dotnet/rspec/vbnet/S2234_vb.net.json b/sonaranalyzer-dotnet/rspec/vbnet/S2234_vb.net.json
@@ -0,0 +1,16 @@
+{
+  "title": "Parameters should be passed in the correct order",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-2234",
+  "sqKey": "S2234",
+  "scope": "All"
+}
diff --git a/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile.json b/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile.json
@@ -32,6 +32,7 @@
     "S2068",
     "S2077",
     "S2178",
+    "S2234",
     "S2255",
     "S2304",
     "S2340",

diff --git a/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile_no_hotspot.json b/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile_no_hotspot.json
@@ -29,6 +29,7 @@
                      "S1940",
                      "S2068",
                      "S2178",
+                     "S2234",
                      "S2304",
                      "S2340",
                      "S2342",

diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs
@@ -18,87 +18,29 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using System.Collections.Generic;
-using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 
 namespace SonarAnalyzer.Helpers
 {
-    // todo: this should come from the Roslyn API (https://github.com/dotnet/roslyn/issues/9)
-    internal class CSharpMethodParameterLookup
+    internal class CSharpMethodParameterLookup : AbstractMethodParameterLookup<ArgumentSyntax>
     {
-        private readonly ArgumentListSyntax argumentList;
-        public IMethodSymbol MethodSymbol { get; }
-
         public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, SemanticModel semanticModel) :
             this(invocation.ArgumentList, semanticModel)
         {
         }
 
         public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel)
+            : base(argumentList.Arguments, semanticModel.GetSymbolInfo(argumentList.Parent).Symbol as IMethodSymbol)
         {
-            this.argumentList = argumentList;
-            MethodSymbol = semanticModel.GetSymbolInfo(argumentList.Parent).Symbol as IMethodSymbol;
-        }
-
-        public static bool TryGetParameterSymbol(ArgumentSyntax argument, ArgumentListSyntax argumentList,
-            IMethodSymbol method, out IParameterSymbol parameter)
-        {
-            parameter = null;
-            if (!argumentList.Arguments.Contains(argument) ||
-                method == null ||
-                method.IsVararg)
-            {
-                return false;
-            }
-
-            if (argument.NameColon != null)
-            {
-                parameter = method.Parameters
-                    .FirstOrDefault(symbol => symbol.Name == argument.NameColon.Name.Identifier.ValueText);
-                return parameter != null;
-            }
-
-            var argumentIndex = argumentList.Arguments.IndexOf(argument);
-            var parameterIndex = argumentIndex;
-
-            if (parameterIndex >= method.Parameters.Length)
-            {
-                var lastParameter = method.Parameters.Last();
-                parameter = lastParameter.IsParams ? lastParameter : null;
-                return parameter != null;
-            }
-            parameter = method.Parameters[parameterIndex];
-            return true;
         }
 
-        public bool TryGetParameterSymbol(ArgumentSyntax argument, out IParameterSymbol parameter)
+        public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol)
+            : base(argumentList.Arguments, methodSymbol)
         {
-            return TryGetParameterSymbol(argument, this.argumentList, MethodSymbol, out parameter);
         }
 
-        internal IEnumerable<ArgumentParameterMapping> GetAllArgumentParameterMappings()
-        {
-            foreach (var argument in this.argumentList.Arguments)
-            {
-                if (TryGetParameterSymbol(argument, out var parameter))
-                {
-                    yield return new ArgumentParameterMapping(argument, parameter);
-                }
-            }
-        }
-
-        public class ArgumentParameterMapping
-        {
-            public ArgumentSyntax Argument { get; set; }
-            public IParameterSymbol Parameter { get; set; }
-
-            public ArgumentParameterMapping(ArgumentSyntax argument, IParameterSymbol parameter)
-            {
-                Argument = argument;
-                Parameter = parameter;
-            }
-        }
+        protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) =>
+            argument.NameColon?.Name.Identifier;
     }
 }
diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs
@@ -203,9 +203,9 @@ public static bool ContainsMethodInvocation(this BaseMethodDeclarationSyntax met
                 .Any(symbolPredicate);
         }
 
-        public static SyntaxToken GetIdentifierOrDefault(this BaseMethodDeclarationSyntax methodDeclaration)
+        public static SyntaxToken? GetIdentifierOrDefault(this BaseMethodDeclarationSyntax methodDeclaration)
         {
-            switch (methodDeclaration.Kind())
+            switch (methodDeclaration?.Kind())
             {
                 case SyntaxKind.ConstructorDeclaration:
                     return ((ConstructorDeclarationSyntax)methodDeclaration).Identifier;
@@ -217,7 +217,7 @@ public static SyntaxToken GetIdentifierOrDefault(this BaseMethodDeclarationSynta
                     return ((MethodDeclarationSyntax)methodDeclaration).Identifier;
 
                 default:
-                    return default(SyntaxToken);
+                    return null;
             }
         }
 
@@ -247,10 +247,8 @@ public static SyntaxToken GetIdentifierOrDefault(this BaseMethodDeclarationSynta
             }
         }
 
-        public static Location FindIdentifierLocation(this BaseMethodDeclarationSyntax methodDeclaration)
-        {
-            return GetIdentifierOrDefault(methodDeclaration).GetLocation();
-        }
+        public static Location FindIdentifierLocation(this BaseMethodDeclarationSyntax methodDeclaration) =>
+            GetIdentifierOrDefault(methodDeclaration)?.GetLocation();
 
         public static bool IsCatchingAllExceptions(this CatchClauseSyntax catchClause)
         {

diff --git a/...analyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/ArgumentSpecifiedForCallerInfoParameter.cs b/...analyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/ArgumentSpecifiedForCallerInfoParameter.cs
@@ -63,8 +63,8 @@ protected override void Initialize(SonarAnalysisContext context)
                     var argumentMappings = methodParameterLookup.GetAllArgumentParameterMappings();
                     foreach (var argumentMapping in argumentMappings)
                     {
-                        var parameter = argumentMapping.Parameter;
-                        var argument = argumentMapping.Argument;
+                        var parameter = argumentMapping.Symbol;
+                        var argument = argumentMapping.SyntaxNode;
 
                         var callerInfoAttributeDataOnCall = GetCallerInfoAttribute(parameter);
                         if (callerInfoAttributeDataOnCall == null)

diff --git a/...ranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/InvocationResolvesToOverrideWithParams.cs b/...ranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/InvocationResolvesToOverrideWithParams.cs
@@ -110,10 +110,12 @@ private static void CheckCall(SyntaxNode node, ArgumentListSyntax argumentList,
         private static bool IsInvocationWithExplicitArray(ArgumentListSyntax argumentList, IMethodSymbol invokedMethodSymbol,
             SemanticModel semanticModel)
         {
+            var methodParameterLookup = new CSharpMethodParameterLookup(argumentList, invokedMethodSymbol);
+
             var allParameterMatches = new List<IParameterSymbol>();
             foreach (var argument in argumentList.Arguments)
             {
-                if (!CSharpMethodParameterLookup.TryGetParameterSymbol(argument, argumentList, invokedMethodSymbol, out var parameter))
+                if (!methodParameterLookup.TryGetParameterSymbol(argument, out var parameter))
                 {
                     return false;
                 }
@@ -138,12 +140,14 @@ private static bool IsInvocationWithExplicitArray(ArgumentListSyntax argumentLis
         private static bool ArgumentsMatchParameters(ArgumentListSyntax argumentList, List<INamedTypeSymbol> argumentTypes,
             IMethodSymbol possibleOtherMethod, SemanticModel semanticModel)
         {
+            var methodParameterLookup = new CSharpMethodParameterLookup(argumentList, possibleOtherMethod);
+
             var matchedParameters = new List<IParameterSymbol>();
             for (var i = 0; i < argumentList.Arguments.Count; i++)
             {
                 var argument = argumentList.Arguments[i];
                 var argumentType = argumentTypes[i];
-                if (!CSharpMethodParameterLookup.TryGetParameterSymbol(argument, argumentList, possibleOtherMethod, out var parameter))
+                if (!methodParameterLookup.TryGetParameterSymbol(argument, out var parameter))
                 {
                     return false;
                 }

diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/MethodsShouldNotHaveTooManyLines.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/MethodsShouldNotHaveTooManyLines.cs
@@ -59,7 +59,7 @@ protected override IEnumerable<SyntaxToken> GetMethodTokens(BaseMethodDeclaratio
                 ?? baseMethodDeclaration.Body?.Statements.SelectMany(s => s.DescendantTokens())
                 ?? Enumerable.Empty<SyntaxToken>();
 
-        protected override SyntaxToken GetMethodIdentifierToken(BaseMethodDeclarationSyntax baseMethodDeclaration) =>
+        protected override SyntaxToken? GetMethodIdentifierToken(BaseMethodDeclarationSyntax baseMethodDeclaration) =>
             baseMethodDeclaration.GetIdentifierOrDefault();
 
         protected override string GetMethodKindAndName(SyntaxToken identifierToken)