Address code review comments

SonarSource · Feb 18, 2022 · fa5de87 · fa5de87
1 parent 7b46a18
commit fa5de87
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 39 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/LocksReleasedAllPaths.cs b/analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/LocksReleasedAllPaths.cs
@@ -19,9 +19,8 @@
  */
 
 using Microsoft.CodeAnalysis;
-using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
-using SonarAnalyzer.Extensions;
+using SonarAnalyzer.Common;
 using SonarAnalyzer.Helpers;
 using StyleCop.Analyzers.Lightup;
 
@@ -33,26 +32,20 @@ public class LocksReleasedAllPaths : LocksReleasedAllPathsBase
 
         protected override DiagnosticDescriptor Rule => S2222;
 
-        protected override void Visit(SyntaxNode node, LockAcquireReleaseCollector collector)
-        {
-            var walker = new LockAcquireReleaseWalker(collector);
-            walker.SafeVisit(NodeContext.Node);
-        }
+        protected override ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector) => new LockAcquireReleaseWalker(collector);
 
-        private sealed class LockAcquireReleaseWalker : CSharpSyntaxWalker
+        private sealed class LockAcquireReleaseWalker : SafeCSharpSyntaxWalker
         {
             private readonly LockAcquireReleaseCollector collector;
 
-            public LockAcquireReleaseWalker(LockAcquireReleaseCollector collector)
-            {
+            public LockAcquireReleaseWalker(LockAcquireReleaseCollector collector) =>
                 this.collector = collector;
-            }
 
             public override void Visit(SyntaxNode node)
             {
                 if (collector.LockAcquiredAndReleased
-                    // Lambda expressions and local functions are analyzed separately.
-                    || node is LambdaExpressionSyntax or ParenthesizedLambdaExpressionSyntax
+                    // Lambda expressions, anonymous methods and local functions are analyzed separately.
+                    || node is AnonymousFunctionExpressionSyntax
                     || LocalFunctionStatementSyntaxWrapper.IsInstance(node))
                 {
                     return;

diff --git a/...src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RuleChecks/LocksReleasedAllPathsBase.cs b/...src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RuleChecks/LocksReleasedAllPathsBase.cs
@@ -21,6 +21,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using Microsoft.CodeAnalysis;
+using SonarAnalyzer.Common;
 using SonarAnalyzer.Helpers;
 using SonarAnalyzer.SymbolicExecution.Constraints;
 using StyleCop.Analyzers.Lightup;
@@ -46,7 +47,7 @@ public abstract class LocksReleasedAllPathsBase : SymbolicRuleCheck
             "TryEnterWriteLock"
         };
 
-        protected abstract void Visit(SyntaxNode node, LockAcquireReleaseCollector collector);
+        protected abstract ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector);
 
         public override ProgramState PostProcess(SymbolicContext context)
         {
@@ -118,7 +119,11 @@ public override void ExecutionCompleted()
         public override bool ShouldExecute()
         {
             var collector = new LockAcquireReleaseCollector();
-            Visit(NodeContext.Node, collector);
+            var walker = GetSyntaxWalker(collector);
+            foreach (var child in NodeContext.Node.ChildNodes())
+            {
+                walker.SafeVisit(child);
+            }
             return collector.LockAcquiredAndReleased;
         }
 
@@ -196,18 +201,9 @@ protected sealed class LockAcquireReleaseCollector
 
             public void RegisterIdentifier(string name)
             {
-                lockAcquired |= IsLockType(name) || IsLockMethod(name);
-                lockReleased |= IsReleaseMethod(name);
+                lockAcquired = lockAcquired || LockTypes.Contains(name) || LockMethods.Contains(name);
+                lockReleased = lockReleased || ReleaseMethods.Contains(name);
             }
-
-            private static bool IsLockType(string name) =>
-                LockTypes.Contains(name);
-
-            private static bool IsLockMethod(string name) =>
-                LockMethods.Contains(name);
-
-            private static bool IsReleaseMethod(string name) =>
-                ReleaseMethods.Contains(name);
         }
     }
 }
diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/SymbolicExecution/Roslyn/LocksReleasedAllPaths.cs b/analyzers/src/SonarAnalyzer.VisualBasic/SymbolicExecution/Roslyn/LocksReleasedAllPaths.cs
@@ -19,8 +19,8 @@
  */
 
 using Microsoft.CodeAnalysis;
-using Microsoft.CodeAnalysis.VisualBasic;
 using Microsoft.CodeAnalysis.VisualBasic.Syntax;
+using SonarAnalyzer.Common;
 using SonarAnalyzer.Helpers;
 
 namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.VisualBasic
@@ -31,20 +31,14 @@ public class LocksReleasedAllPaths : LocksReleasedAllPathsBase
 
         protected override DiagnosticDescriptor Rule => S2222;
 
-        protected override void Visit(SyntaxNode node, LockAcquireReleaseCollector collector)
-        {
-            var walker = new LockAcquireReleaseWalker(collector);
-            walker.SafeVisit(NodeContext.Node);
-        }
+        protected override ISafeSyntaxWalker GetSyntaxWalker(LockAcquireReleaseCollector collector) => new LockAcquireReleaseWalker(collector);
 
-        private sealed class LockAcquireReleaseWalker : VisualBasicSyntaxWalker
+        private sealed class LockAcquireReleaseWalker : SafeVisualBasicSyntaxWalker
         {
             private readonly LockAcquireReleaseCollector collector;
 
-            public LockAcquireReleaseWalker(LockAcquireReleaseCollector collector)
-            {
+            public LockAcquireReleaseWalker(LockAcquireReleaseCollector collector) =>
                 this.collector = collector;
-            }
 
             public override void Visit(SyntaxNode node)
             {

diff --git a/...r.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.Conditions.cs b/...r.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.Conditions.cs
@@ -10,6 +10,8 @@ class Program
 
         public object PublicObject = new object();
 
+        delegate void VoidDelegate();
+
         private bool condition;
 
         public void Method1()
@@ -329,9 +331,16 @@ public int MyProperty
 
         public void Lambda(bool condition)
         {
-            Action action = () =>
+            Action parenthesizedLambda = () =>
             {
-                Monitor.Enter(obj); // // FN, lambdas are not yet supported
+                Monitor.Enter(obj); // Noncompliant
+                if (condition)
+                    Monitor.Exit(obj);
+            };
+
+            Action<int> simpleLambda = x =>
+            {
+                Monitor.Enter(obj); // Noncompliant
                 if (condition)
                     Monitor.Exit(obj);
             };
@@ -350,6 +359,13 @@ static void StaticLocalFunction()
                 if (1 == 2)
                     Monitor.Exit(l);
             }
+
+            VoidDelegate anonymousMethod = delegate
+            {
+                Monitor.Enter(other); // Noncompliant
+                if (condition)
+                    Monitor.Exit(other);
+            };
         }
     }
 }
diff --git a/...r.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.Conditions.vb b/...r.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.Conditions.vb
@@ -202,7 +202,7 @@ Namespace Monitor_Conditions
 
         Public Sub Lambda()
             Dim l = Function(value)
-                Monitor.Enter(Obj) ' FN, lambdas are not supported yet
+                Monitor.Enter(Obj) ' Noncompliant
                 If value = 42 Then Monitor.Exit(Obj)
                 Return value
             End Function