S3900: Change parameter dereference check to top-down (#7004)

SonarSource · Mar 31, 2023 · 60dc3c5 · 60dc3c5
1 parent 12c8b40
commit 60dc3c5
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 116 deletions.
diff --git a/analyzers/its/expected/Nancy/Nancy--net452-S3900.json b/analyzers/its/expected/Nancy/Nancy--net452-S3900.json
@@ -2828,19 +2828,6 @@
 "uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
 "region":  {
 "startLine":  21,
-"startColumn":  89,
-"endLine":  21,
-"endColumn":  99
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 'constraint' before using it.",
-"location":  {
-"uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
-"region":  {
-"startLine":  21,
 "startColumn":  113,
 "endLine":  21,
 "endColumn":  123
@@ -2854,19 +2841,6 @@
 "uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
 "region":  {
 "startLine":  35,
-"startColumn":  30,
-"endLine":  35,
-"endColumn":  40
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 'constraint' before using it.",
-"location":  {
-"uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
-"region":  {
-"startLine":  35,
 "startColumn":  51,
 "endLine":  35,
 "endColumn":  61

diff --git a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S3900.json b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S3900.json
@@ -2828,19 +2828,6 @@
 "uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
 "region":  {
 "startLine":  21,
-"startColumn":  89,
-"endLine":  21,
-"endColumn":  99
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 'constraint' before using it.",
-"location":  {
-"uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
-"region":  {
-"startLine":  21,
 "startColumn":  113,
 "endLine":  21,
 "endColumn":  123
@@ -2854,19 +2841,6 @@
 "uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
 "region":  {
 "startLine":  35,
-"startColumn":  30,
-"endLine":  35,
-"endColumn":  40
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 'constraint' before using it.",
-"location":  {
-"uri":  "sources\Nancy\src\Nancy\Routing\Constraints\ParameterizedRouteSegmentConstraintBase.cs",
-"region":  {
-"startLine":  35,
 "startColumn":  51,
 "endLine":  35,
 "endColumn":  61

diff --git a/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--net452-S3900.json b/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--net452-S3900.json
@@ -32,19 +32,6 @@
 "location":  {
 "uri":  "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs",
 "region":  {
-"startLine":  31,
-"startColumn":  16,
-"endLine":  31,
-"endColumn":  31
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 'templateContent' before using it.",
-"location":  {
-"uri":  "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs",
-"region":  {
 "startLine":  32,
 "startColumn":  20,
 "endLine":  32,

diff --git a/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--netstandard2.0-S3900.json b/analyzers/its/expected/Nancy/Nancy.ViewEngines.Markdown--netstandard2.0-S3900.json
@@ -32,19 +32,6 @@
 "location":  {
 "uri":  "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs",
 "region":  {
-"startLine":  31,
-"startColumn":  16,
-"endLine":  31,
-"endColumn":  31
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 'templateContent' before using it.",
-"location":  {
-"uri":  "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs",
-"region":  {
 "startLine":  32,
 "startColumn":  20,
 "endLine":  32,

diff --git a/analyzers/its/expected/akka.net/Akka.Tests.Shared.Internals--netstandard2.0-S3900.json b/analyzers/its/expected/akka.net/Akka.Tests.Shared.Internals--netstandard2.0-S3900.json
@@ -137,19 +137,6 @@
 "uri":  "sources\akka.net\src\core\Akka.Tests.Shared.Internals\AkkaSpecExtensions.cs",
 "region":  {
 "startLine":  188,
-"startColumn":  26,
-"endLine":  188,
-"endColumn":  27
-}
-}
-},
-{
-"id":  "S3900",
-"message":  "Refactor this method to add validation of parameter 's' before using it.",
-"location":  {
-"uri":  "sources\akka.net\src\core\Akka.Tests.Shared.Internals\AkkaSpecExtensions.cs",
-"region":  {
-"startLine":  188,
 "startColumn":  50,
 "endLine":  188,
 "endColumn":  51

diff --git a/...arAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs b/...arAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
@@ -30,17 +30,6 @@ public class PublicMethodArgumentsShouldBeCheckedForNull : SymbolicRuleCheck
     private const string DiagnosticId = "S3900";
     private const string MessageFormat = "{0}";
 
-    private static readonly OperationKind[] DereferenceOperations = new[]
-    {
-        OperationKindEx.Invocation,
-        OperationKindEx.FieldReference,
-        OperationKindEx.PropertyReference,
-        OperationKindEx.EventReference,
-        OperationKindEx.Await,
-        OperationKindEx.ArrayElementReference,
-        OperationKindEx.MethodReference
-    };
-
     internal static readonly DiagnosticDescriptor S3900 = DescriptorFactory.Create(DiagnosticId, MessageFormat);
 
     protected override DiagnosticDescriptor Rule => S3900;
@@ -82,30 +71,40 @@ static bool MethodDereferencesArguments(BaseMethodDeclarationSyntax method)
 
     protected override ProgramState PreProcessSimple(SymbolicContext context)
     {
-        var operation = context.Operation.Instance;
-        if (operation.Kind == OperationKindEx.ParameterReference
-            && operation.ToParameterReference().Parameter is var parameter
-            && !parameter.Type.IsValueType
-            && IsParameterDereferenced(context.Operation)
-            && NullableStateIsNotKnownForParameter(parameter)
-            && !parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute))
+        if (NullDereferenceCandidate(context.Operation.Instance) is { } candidate
+            && candidate.Kind == OperationKindEx.ParameterReference
+            && candidate.ToParameterReference() is var parameterReference
+            && !parameterReference.Parameter.Type.IsValueType
+            && !HasObjectConstraint(parameterReference.Parameter)
+            && !parameterReference.Parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute))
         {
             var message = SemanticModel.GetDeclaredSymbol(Node).IsConstructor()
                 ? "Refactor this constructor to avoid using members of parameter '{0}' because it could be null."
                 : "Refactor this method to add validation of parameter '{0}' before using it.";
-            ReportIssue(operation, string.Format(message, operation.Syntax), context);
+            ReportIssue(parameterReference.WrappedOperation, string.Format(message, parameterReference.WrappedOperation.Syntax), context);
         }
 
         return context.State;
 
-        bool NullableStateIsNotKnownForParameter(IParameterSymbol symbol) =>
-            context.State[symbol] is null || !context.State[symbol].HasConstraint<ObjectConstraint>();
+        bool HasObjectConstraint(IParameterSymbol symbol) =>
+            context.State[symbol]?.HasConstraint<ObjectConstraint>() is true;
     }
 
-    private static bool IsParameterDereferenced(IOperationWrapperSonar operation) =>
-        operation.Parent is { } parent
-        && (parent.IsAnyKind(DereferenceOperations)
-            || (parent.Kind == OperationKindEx.Conversion && IsParameterDereferenced(parent.ToSonar())));
+    private static IOperation NullDereferenceCandidate(IOperation operation)
+    {
+        var candidate = operation.Kind switch
+        {
+            OperationKindEx.Invocation => operation.ToInvocation().Instance,
+            OperationKindEx.FieldReference => operation.ToFieldReference().Instance,
+            OperationKindEx.PropertyReference => operation.ToPropertyReference().Instance,
+            OperationKindEx.EventReference => operation.ToEventReference().Instance,
+            OperationKindEx.Await => operation.ToAwait().Operation,
+            OperationKindEx.ArrayElementReference => operation.ToArrayElementReference().ArrayReference,
+            OperationKindEx.MethodReference => operation.ToMethodReference().Instance,
+            _ => null,
+        };
+        return candidate?.UnwrapConversion();
+    }
 
     private sealed class ArgumentDereferenceWalker : SafeCSharpSyntaxWalker
     {

diff --git a/...nitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs b/...nitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs
@@ -722,3 +722,26 @@ private class CustomClass
         public event EventHandler CustomEvent;
     }
 }
+
+public class DereferencedMultipleTimesOnTheSameExecutionPath
+{
+    public void Used(string s)
+    {
+        s.ToLower();            // Noncompliant
+        s.ToLower();            // Compliant - s was dereferenced in the previous line, so it's not null here
+    }
+
+    public void UsedOnMultipleLevels(string s)
+    {
+        s.Replace(              // Compliant - SubString was already called on s when we call Replace, so s is not null
+            "a",
+            s.Substring(1));    // Noncompliant
+    }
+
+    public void UsedTwiceOnSameLevel(string s)
+    {
+        _ = s.Substring(        // Compliant
+            s.IndexOf("a"),     // Noncompliant
+            s.IndexOf("b"));    // Compliant - IndexOf("a") was called before this method call, so s is not null here
+    }
+}