UnnecessaryAsync analyzer becomes smarter with dataflow analysis (#9)

Author: semihokur

AsyncFixer.Test/AsyncCallInsideUsingBlockTests.cs

@@ -183,7 +183,7 @@ static async void foo()
         using (var stream = new FileStream(""existing"", FileMode.Open))
         {
             await stream.CopyToAsync(newStream);
-            stream.CopyToAsync(Stream.Null); 
+            newStream.CopyToAsync(Stream.Null); 
         }
     }
 }";

AsyncFixer.Test/UnnecessaryAsyncTests.cs

@@ -575,6 +575,120 @@ class Program
             VerifyCSharpDiagnostic(test);
         }
 
+        [Fact]
+        public void NoWarn_UsingStatement()
+        {
+            var test = @"
+using System;
+using System.Threading.Tasks;
+
+class Program
+{
+    async Task foo()
+    {
+        using var stream = FileStream.Null;
+        var stream2 = FileStream.Null;
+        await Task.Run(() => stream2.CopyToAsync(stream));
+    }
+}";
+
+            VerifyCSharpDiagnostic(test);
+        }
+
+        [Fact]
+        public void NoWarn_UsingStatement2()
+        {
+            var test = @"
+using System;
+using System.Threading.Tasks;
+
+class Program
+{
+    async Task foo()
+    {
+        using var stream = FileStream.Null;
+        await stream.ReadAsync(null);
+    }
+}";
+
+            VerifyCSharpDiagnostic(test);
+        }
+
+        [Fact]
+        public void NoWarn_UsingStatementWithDataflow()
+        {
+            var test = @"
+using System;
+using System.Threading.Tasks;
+
+class Program
+{
+    async Task foo()
+    {
+        using var stream = new MemoryStream();
+        int streamOperation()
+        {
+            return stream.Read(null);
+        }
+
+        await Task.Run(() => streamOperation());
+    }
+}";
+
+            VerifyCSharpDiagnostic(test);
+        }
+
+        [Fact]
+        public void NoWarn_UsingStatementWithDataflow2()
+        {
+            var test = @"
+using System;
+using System.Threading.Tasks;
+
+class Program
+{
+    async Task foo()
+    {
+        using var stream = new MemoryStream();
+        int streamOperation()
+        {
+            return stream.Read(null);
+        }
+
+        var t = Task.Run(() => streamOperation());
+        await t;
+    }
+}";
+
+            VerifyCSharpDiagnostic(test);
+        }
+
+        [Fact(Skip = "TODO: fix the dataflow analysis to analyze all locations of the symbol, not only the definitions")]
+        public void NoWarn_UsingStatementWithDataflowComplicated()
+        {
+            var test = @"
+using System;
+using System.Threading.Tasks;
+
+class Program
+{
+    async Task foo()
+    {
+        using var stream = new MemoryStream();
+        int streamOperation()
+        {
+            return stream.Read(null);
+        }
+
+        Task t = null;
+        t = Task.Run(() => streamOperation());
+        await t;
+    }
+}";
+
+            VerifyCSharpDiagnostic(test);
+        }
+
         protected override CodeFixProvider GetCSharpCodeFixProvider()
         {
             return new UnnecessaryAsyncFixer();

AsyncFixer/UnnecessaryAsync/UnnecessaryAsyncAnalyzer.cs

@@ -6,6 +6,8 @@
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Operations;
+using System.Collections.Generic;
+using System;
 
 namespace AsyncFixer.UnnecessaryAsync
 {
@@ -63,7 +65,7 @@ private void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context)
             // Retrieve all await expressions excluding the ones under lambda functions.
             var awaitExpressions = node.DescendantNodes().OfType<AwaitExpressionSyntax>().Where(a => a.FirstAncestorOrSelfUnderGivenNode<LambdaExpressionSyntax>(node) == null).ToList();
 
-            if (node.Body == null && 
+            if (node.Body == null &&
                 node.ExpressionBody?.Expression.Kind() == SyntaxKind.AwaitExpression)
             {
                 // Expression-bodied members 
@@ -96,6 +98,13 @@ private void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context)
             var controlFlow = context.SemanticModel.AnalyzeControlFlow(node.Body);
             var returnStatements = controlFlow?.ReturnStatements ?? ImmutableArray<SyntaxNode>.Empty;
 
+            // Retrieve the disposable object identifiers from the using statements. 
+            // For instance, for the following statement, we'd like to return 'source'.
+            //      using FileStream source = File.Open("data", FileMode.Open);
+            var disposableObjectNames = node.Body.DescendantNodes().OfType<LocalDeclarationStatementSyntax>()
+                .Where(a => a.UsingKeyword.Kind() != SyntaxKind.None)
+                .SelectMany(a => a.DescendantNodes().OfType<VariableDeclaratorSyntax>().Select(b => b.Identifier.ValueText)).ToList();
+
             var numAwait = 0;
             if (returnStatements.Any())
             {
@@ -107,7 +116,7 @@ private void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context)
                         return;
                     }
 
-                    if (HasUsingOrTryParent(returnStatement, node))
+                    if (!IsSafeToRemoveAsyncAwait(returnStatement))
                     {
                         return;
                     }
@@ -125,13 +134,18 @@ private void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context)
             else
             {
                 // if awaitExpression is the last statement's expression
+                // using (var stream = new MemoryStream())
+                // {
+                //    await Task.FromResult(3); ==> this is NOT the last statement because of the using block.
+                // }
+
                 var lastStatement = node.Body.Statements.LastOrDefault();
                 if ((lastStatement as ExpressionStatementSyntax)?.Expression?.Kind() != SyntaxKind.AwaitExpression)
                 {
                     return;
                 }
 
-                if (HasUsingOrTryParent(lastStatement, node))
+                if (!IsSafeToRemoveAsyncAwait(lastStatement))
                 {
                     return;
                 }
@@ -144,26 +158,83 @@ private void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context)
                 return;
             }
 
-            // Make sure that we do not give a warning about the await statement involving a disposable object.
+            context.ReportDiagnostic(diagnostic);
 
-            // Retrieve the disposable object identifiers from the using statements. 
-            // For instance, for the following statement, we'd like to return 'source'.
-            //      using FileStream source = File.Open("data", FileMode.Open);
-            var disposableObjectNames = node.Body.DescendantNodes().OfType<LocalDeclarationStatementSyntax>()
-                .Where(a => a.UsingKeyword.Kind() != SyntaxKind.None)
-                .SelectMany(a => a.DescendantNodes().OfType<VariableDeclaratorSyntax>().Select(b => b.Identifier.ValueText));
-            if (disposableObjectNames.Any())
+            bool IsSafeToRemoveAsyncAwait(StatementSyntax statement)
             {
-                // There are disposable objects.
-                // Let's check whether at least one await expression uses one of those disposable objects.
-                if (awaitExpressions.SelectMany(a => a.DescendantNodes().OfType<IdentifierNameSyntax>())
-                    .Any(a => disposableObjectNames.Contains(a.Identifier.ValueText)))
+                if (HasUsingOrTryParent(statement, node))
                 {
-                    return;
+                    // If it is under 'using' or 'try' block, it is not safe to remove async/await.
+                    return false;
+                }
+
+                // Make sure that we do not give a warning about the await statement involving an object which is created by an using statement.
+                // We use dataflow analysis to accurately detect a case like below:
+                //  async Task foo()
+                //  {
+                //      using var stream = new MemoryStream();
+                //      int streamOperation()
+                //      {
+                //          return stream.Read(null);
+                //      }
+                //      
+                //      var t = Task.Run(() => streamOperation())
+                //      await t;
+                //  }
+                // In the example above, we need to find out whether 'stream' is accessed in the last statement.
+
+                var names = GetAccessedVariableNamesWithPointsToAnalysis(context.SemanticModel, node, statement).ToList();
+                
+                if (names.Any(a => disposableObjectNames.Contains(a)))
+                {
+                    return false;
                 }
+
+                return true;
             }
+        }
 
-            context.ReportDiagnostic(diagnostic);
+        /// <summary>
+        /// Return the names of all variables that are read-accessed in the given statement.
+        /// </summary>
+        /// <remarks>
+        /// The method iteratively goes through the definitions to find implicitly-accessed variables. 
+        /// </remarks>
+        private IEnumerable<string> GetAccessedVariableNamesWithPointsToAnalysis(SemanticModel semanticModel, SyntaxNode root, SyntaxNode node, int depth = 0)
+        {
+            if (depth == 5 || node == null || root == null)
+            {
+                // Put a limit for the call stack frame
+                yield break;
+            }
+
+            var dataFlowResult = semanticModel.AnalyzeDataFlow(node);
+            if (dataFlowResult?.Succeeded == true)
+            {
+                foreach (ISymbol symbol in dataFlowResult.ReadInside)
+                {
+                    yield return symbol.Name;
+
+                    if (symbol.DeclaringSyntaxReferences == null)
+                    {
+                        continue;
+                    }
+                    
+                    foreach (var syntaxRef in symbol.DeclaringSyntaxReferences)
+                    {
+                        var expressions = root.FindNode(syntaxRef.Span, getInnermostNodeForTie: true).DescendantNodes((n) => !(n is ExpressionSyntax)).OfType<ExpressionSyntax>();
+
+                        foreach (var expr in expressions)
+                        {
+                            var names = GetAccessedVariableNamesWithPointsToAnalysis(semanticModel, root, expr, depth + 1);
+                            foreach (var name in names)
+                            {
+                                yield return name;
+                            }
+                        }
+                    }
+                }
+            }
         }
     }
 }