Fix S2259 FP: Do not track symbol via TryCast (#6992)

SonarSource · Mar 29, 2023 · bbed3eb · bbed3eb
1 parent d36199d
commit bbed3eb
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 78 deletions.
diff --git a/analyzers/its/expected/akka.net/Akka.Streams--netstandard2.0-S2259.json b/analyzers/its/expected/akka.net/Akka.Streams--netstandard2.0-S2259.json
@@ -2,45 +2,6 @@
 "issues":  [
 {
 "id":  "S2259",
-"message":  "'graph' is null on at least one execution path.",
-"location":  {
-"uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\Flow.cs",
-"region":  {
-"startLine":  477,
-"startColumn":  76,
-"endLine":  477,
-"endColumn":  81
-}
-}
-},
-{
-"id":  "S2259",
-"message":  "'g' is null on at least one execution path.",
-"location":  {
-"uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\RunnableGraph.cs",
-"region":  {
-"startLine":  163,
-"startColumn":  68,
-"endLine":  163,
-"endColumn":  69
-}
-}
-},
-{
-"id":  "S2259",
-"message":  "'source' is null on at least one execution path.",
-"location":  {
-"uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\Source.cs",
-"region":  {
-"startLine":  518,
-"startColumn":  65,
-"endLine":  518,
-"endColumn":  71
-}
-}
-},
-{
-"id":  "S2259",
 "message":  "'module' is null on at least one execution path.",
 "location":  {
 "uri":  "sources\akka.net\src\core\Akka.Streams\Implementation\Fusing\StreamOfStreams.cs",
@@ -51,32 +12,6 @@
 "endColumn":  39
 }
 }
-},
-{
-"id":  "S2259",
-"message":  "'inlet' is null on at least one execution path.",
-"location":  {
-"uri":  "sources\akka.net\src\core\Akka.Streams\Shape.cs",
-"region":  {
-"startLine":  57,
-"startColumn":  92,
-"endLine":  57,
-"endColumn":  97
-}
-}
-},
-{
-"id":  "S2259",
-"message":  "'outlet' is null on at least one execution path.",
-"location":  {
-"uri":  "sources\akka.net\src\core\Akka.Streams\Shape.cs",
-"region":  {
-"startLine":  122,
-"startColumn":  98,
-"endLine":  122,
-"endColumn":  104
-}
-}
 }
 ]
 }
diff --git a/analyzers/its/expected/akka.net/Akka.Streams--netstandard2.0-S3900.json b/analyzers/its/expected/akka.net/Akka.Streams--netstandard2.0-S3900.json
@@ -379,6 +379,19 @@
 },
 {
 "id":  "S3900",
+"message":  "Refactor this method to add validation of parameter 'graph' before using it.",
+"location":  {
+"uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\Flow.cs",
+"region":  {
+"startLine":  477,
+"startColumn":  76,
+"endLine":  477,
+"endColumn":  81
+}
+}
+},
+{
+"id":  "S3900",
 "message":  "Refactor this method to add validation of parameter 'flow' before using it.",
 "location":  {
 "uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\FlowWithContextOperations.cs",
@@ -1107,6 +1120,19 @@
 },
 {
 "id":  "S3900",
+"message":  "Refactor this method to add validation of parameter 'g' before using it.",
+"location":  {
+"uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\RunnableGraph.cs",
+"region":  {
+"startLine":  163,
+"startColumn":  68,
+"endLine":  163,
+"endColumn":  69
+}
+}
+},
+{
+"id":  "S3900",
 "message":  "Refactor this method to add validation of parameter 'graph' before using it.",
 "location":  {
 "uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\Sink.cs",
@@ -1159,6 +1185,19 @@
 },
 {
 "id":  "S3900",
+"message":  "Refactor this method to add validation of parameter 'source' before using it.",
+"location":  {
+"uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\Source.cs",
+"region":  {
+"startLine":  518,
+"startColumn":  65,
+"endLine":  518,
+"endColumn":  71
+}
+}
+},
+{
+"id":  "S3900",
 "message":  "Refactor this method to add validation of parameter 'props' before using it.",
 "location":  {
 "uri":  "sources\akka.net\src\core\Akka.Streams\Dsl\Source.cs",
@@ -2173,6 +2212,32 @@
 },
 {
 "id":  "S3900",
+"message":  "Refactor this method to add validation of parameter 'inlet' before using it.",
+"location":  {
+"uri":  "sources\akka.net\src\core\Akka.Streams\Shape.cs",
+"region":  {
+"startLine":  57,
+"startColumn":  92,
+"endLine":  57,
+"endColumn":  97
+}
+}
+},
+{
+"id":  "S3900",
+"message":  "Refactor this method to add validation of parameter 'outlet' before using it.",
+"location":  {
+"uri":  "sources\akka.net\src\core\Akka.Streams\Shape.cs",
+"region":  {
+"startLine":  122,
+"startColumn":  98,
+"endLine":  122,
+"endColumn":  104
+}
+}
+},
+{
+"id":  "S3900",
 "message":  "Refactor this method to add validation of parameter 'shape' before using it.",
 "location":  {
 "uri":  "sources\akka.net\src\core\Akka.Streams\Shape.cs",

diff --git a/...zers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs b/...zers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/Extensions/IOperationExtensions.cs
@@ -22,17 +22,22 @@ namespace SonarAnalyzer.SymbolicExecution.Roslyn
 {
     internal static class IOperationExtensions
     {
-        internal static ISymbol TrackedSymbol(this IOperation operation) =>
-            operation?.Kind switch
+        internal static ISymbol TrackedSymbol(this IOperation operation)
+        {
+            return operation?.Kind switch
             {
-                OperationKindEx.Conversion => TrackedSymbol(operation.ToConversion().Operand),
-                OperationKindEx.FieldReference when IFieldReferenceOperationWrapper.FromOperation(operation) is var fieldReference && IsStaticOrThis(fieldReference) => fieldReference.Field,
-                OperationKindEx.LocalReference => ILocalReferenceOperationWrapper.FromOperation(operation).Local,
-                OperationKindEx.ParameterReference => IParameterReferenceOperationWrapper.FromOperation(operation).Parameter,
-                OperationKindEx.Argument => IArgumentOperationWrapper.FromOperation(operation).Value.TrackedSymbol(),
+                OperationKindEx.Conversion when operation.ToConversion() is var conversion && !IsTryDownCast(conversion) => TrackedSymbol(conversion.Operand),
+                OperationKindEx.FieldReference when operation.ToFieldReference() is var fieldReference && IsStaticOrThis(fieldReference) => fieldReference.Field,
+                OperationKindEx.LocalReference => operation.ToLocalReference().Local,
+                OperationKindEx.ParameterReference => operation.ToParameterReference().Parameter,
+                OperationKindEx.Argument => operation.ToArgument().Value.TrackedSymbol(),
                 _ => null
             };
 
+            static bool IsTryDownCast(IConversionOperationWrapper conversion) =>
+                conversion.IsTryCast && !conversion.Operand.Type.DerivesOrImplements(conversion.Type);
+        }
+
         internal static IInvocationOperationWrapper? AsInvocation(this IOperation operation) =>
             operation.As(OperationKindEx.Invocation, IInvocationOperationWrapper.FromOperation);
 
@@ -66,6 +71,9 @@ internal static IInvocationOperationWrapper ToInvocation(this IOperation operati
         internal static IFieldReferenceOperationWrapper ToFieldReference(this IOperation operation) =>
             IFieldReferenceOperationWrapper.FromOperation(operation);
 
+        internal static ILocalReferenceOperationWrapper ToLocalReference(this IOperation operation) =>
+            ILocalReferenceOperationWrapper.FromOperation(operation);
+
         internal static IPropertyReferenceOperationWrapper ToPropertyReference(this IOperation operation) =>
             IPropertyReferenceOperationWrapper.FromOperation(operation);
 

diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/IOperationExtensionsTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/IOperationExtensionsTest.cs
@@ -31,7 +31,7 @@ public class IOperationExtensionsTest
         public void TrackedSymbol_LocalReference_IsVariableSymbol()
         {
             var localReference = ((ISimpleAssignmentOperation)TestHelper.CompileCfgBodyCS("var a = true;").Blocks[1].Operations[0]).Target;
-            var symbol = ILocalReferenceOperationWrapper.FromOperation(localReference).Local;
+            var symbol = localReference.ToLocalReference().Local;
             localReference.TrackedSymbol().Should().Be(symbol);
         }
 

diff --git a/...sts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.IsNull.cs b/...sts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.IsNull.cs
@@ -205,5 +205,27 @@ public void IsNull_ConditionalAccess_OnUnknown()
                 "WasNotNull",
                 "End");
         }
+
+        [TestMethod]
+        public void IsNull_TryCast_DownCast()
+        {
+            var validator = SETestContext.CreateCS("""
+                (arg as Exception)?.ToString();
+                Tag("Arg", arg);
+                """, ", object arg").Validator;
+            validator.ValidateTag("Arg", x => x.Should().HaveNoConstraints());
+        }
+
+        [TestMethod]
+        public void IsNull_TryCast_UpCast()
+        {
+            var validator = SETestContext.CreateCS("""
+                (arg as Exception)?.ToString();
+                Tag("Arg", arg);
+                """, ", ArgumentException arg").Validator;
+            validator.TagValues("Arg").Should().SatisfyRespectively(
+                x => x.Should().HaveOnlyConstraint(ObjectConstraint.Null),
+                x => x.Should().HaveOnlyConstraint(ObjectConstraint.NotNull));
+        }
     }
 }
diff --git a/...narAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/NullPointerDereference.CSharp8.cs b/...narAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/NullPointerDereference.CSharp8.cs
@@ -24,19 +24,19 @@ public void NullCoalescenceOperator_Null_Noncompliant2()
             name = name ?? name.ToString(); // Noncompliant
         }
 
-        public void NullCoalesce_Conversion_DownCast(AggregateException arg)
+        public void NullCoalesce_Conversion_UpCast(AggregateException arg)
         {
-            var value = arg as Exception ?? new Exception(arg.Message);     // Noncompliant, arg must be null on the right side
+            var value = arg as Exception ?? new Exception(arg.Message);     // Noncompliant, arg must be null on the right side because the conversion is always possible
         }
 
-        public void NullCoalesce_Conversion_UpCast(Exception arg)
+        public void NullCoalesce_Conversion_DownCast(Exception arg)
         {
-            var value = arg as AggregateException ?? new AggregateException(arg.Message);     // Noncompliant, arg can be null or another Exception type on the right side
+            var value = arg as AggregateException ?? new AggregateException(arg.Message);     // Compliant, caused a lot of FPs. While arg could be in theory be null, we don't infer that.
         }
 
         public void NullCoalesce_WithAs(IProperty arg)
         {
-            ISomethingElse value = arg as ISomethingElse ?? new SomethingElse(arg.Property);    // Noncompliant FP
+            ISomethingElse value = arg as ISomethingElse ?? new SomethingElse(arg.Property);    // Compliant, caused a lot of FPs. While arg could be in theory be null, we don't infer that.
         }
 
         public void NullCoalescenceAssignment_Null()