Fix and simplify async coverage (#549)

Fix and simplify async coverage

Author: MarcoRossignoli

src/coverlet.core/Coverage.cs

@@ -283,54 +283,11 @@ private void CalculateCoverage()
                         }
                     }
                 }
-
-                // for MoveNext() compiler autogenerated method we need to patch false positive (IAsyncStateMachine for instance) 
-                // we'll remove all MoveNext() not covered branch
-                foreach (var document in result.Documents)
-                {
-                    List<KeyValuePair<BranchKey, Branch>> branchesToRemove = new List<KeyValuePair<BranchKey, Branch>>();
-                    foreach (var branch in document.Value.Branches)
-                    {
-                        //if one branch is covered we search the other one only if it's not covered
-                        if (IsAsyncStateMachineMethod(branch.Value.Method) && branch.Value.Hits > 0)
-                        {
-                            foreach (var moveNextBranch in document.Value.Branches)
-                            {
-                                if (moveNextBranch.Value.Method == branch.Value.Method && moveNextBranch.Value != branch.Value && moveNextBranch.Value.Hits == 0)
-                                {
-                                    branchesToRemove.Add(moveNextBranch);
-                                }
-                            }
-                        }
-                    }
-                    foreach (var branchToRemove in branchesToRemove)
-                    {
-                        document.Value.Branches.Remove(branchToRemove.Key);
-                    }
-                }
-
                 _instrumentationHelper.DeleteHitsFile(result.HitsFilePath);
                 _logger.LogVerbose($"Hit file '{result.HitsFilePath}' deleted");
             }
         }
 
-        private bool IsAsyncStateMachineMethod(string method)
-        {
-            if (!method.EndsWith("::MoveNext()"))
-            {
-                return false;
-            }
-
-            foreach (var instrumentationResult in _results)
-            {
-                if (instrumentationResult.AsyncMachineStateMethod.Contains(method))
-                {
-                    return true;
-                }
-            }
-            return false;
-        }
-
         private string GetSourceLinkUrl(Dictionary<string, string> sourceLinkDocuments, string document)
         {
             if (sourceLinkDocuments.TryGetValue(document, out string url))

src/coverlet.core/CoverageResult.cs

@@ -108,57 +108,6 @@ internal void Merge(Modules modules)
                     }
                 }
             }
-
-            // for MoveNext() compiler autogenerated method we need to patch false positive (IAsyncStateMachine for instance) 
-            // we'll remove all MoveNext() not covered branch
-            List<BranchInfo> branchesToRemove = new List<BranchInfo>();
-            foreach (var module in this.Modules)
-            {
-                foreach (var document in module.Value)
-                {
-                    foreach (var @class in document.Value)
-                    {
-                        foreach (var method in @class.Value)
-                        {
-                            foreach (var branch in method.Value.Branches)
-                            {
-                                //if one branch is covered we search the other one only if it's not covered
-                                if (IsAsyncStateMachineMethod(method.Key) && branch.Hits > 0)
-                                {
-                                    foreach (var moveNextBranch in method.Value.Branches)
-                                    {
-                                        if (moveNextBranch != branch && moveNextBranch.Hits == 0)
-                                        {
-                                            branchesToRemove.Add(moveNextBranch);
-                                        }
-                                    }
-                                }
-                            }
-                            foreach (var branchToRemove in branchesToRemove)
-                            {
-                                method.Value.Branches.Remove(branchToRemove);
-                            }
-                        }
-                    }
-                }
-            }
-        }
-
-        private bool IsAsyncStateMachineMethod(string method)
-        {
-            if (!method.EndsWith("::MoveNext()"))
-            {
-                return false;
-            }
-
-            foreach (var instrumentedResult in InstrumentedResults)
-            {
-                if (instrumentedResult.AsyncMachineStateMethod.Contains(method))
-                {
-                    return true;
-                }
-            }
-            return false;
         }
 
         public ThresholdTypeFlags GetThresholdTypesBelowThreshold(CoverageSummary summary, double threshold, ThresholdTypeFlags thresholdTypes, ThresholdStatistic thresholdStat)

src/coverlet.core/Instrumentation/Instrumenter.cs

@@ -36,7 +36,6 @@ internal class Instrumenter
         private TypeDefinition _customTrackerTypeDef;
         private MethodReference _customTrackerRegisterUnloadEventsMethod;
         private MethodReference _customTrackerRecordHitMethod;
-        private List<string> _asyncMachineStateMethod;
         private List<string> _excludedSourceFiles;
 
         public Instrumenter(
@@ -123,8 +122,6 @@ public InstrumenterResult Instrument()
 
             InstrumentModule();
 
-            _result.AsyncMachineStateMethod = _asyncMachineStateMethod == null ? Array.Empty<string>() : _asyncMachineStateMethod.ToArray();
-
             if (_excludedSourceFiles != null)
             {
                 foreach (string sourceFile in _excludedSourceFiles)
@@ -398,18 +395,18 @@ private void InstrumentIL(MethodDefinition method)
                     index += 2;
                 }
 
-                foreach (var _branchTarget in targetedBranchPoints)
+                foreach (var branchTarget in targetedBranchPoints)
                 {
                     /*
                         * Skip branches with no sequence point reference for now.
                         * In this case for an anonymous class the compiler will dynamically create an Equals 'utility' method.
                         * The CecilSymbolHelper will create branch points with a start line of -1 and no document, which
                         * I am currently not sure how to handle.
                         */
-                    if (_branchTarget.StartLine == -1 || _branchTarget.Document == null)
+                    if (branchTarget.StartLine == -1 || branchTarget.Document == null)
                         continue;
 
-                    var target = AddInstrumentationCode(method, processor, instruction, _branchTarget);
+                    var target = AddInstrumentationCode(method, processor, instruction, branchTarget);
                     foreach (var _instruction in processor.Body.Instructions)
                         ReplaceInstructionTarget(_instruction, instruction, target);
 
@@ -469,43 +466,13 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor
                         Ordinal = branchPoint.Ordinal
                     }
                 );
-
-                if (IsAsyncStateMachineBranch(method.DeclaringType, method))
-                {
-                    if (_asyncMachineStateMethod == null)
-                    {
-                        _asyncMachineStateMethod = new List<string>();
-                    }
-
-                    if (!_asyncMachineStateMethod.Contains(method.FullName))
-                    {
-                        _asyncMachineStateMethod.Add(method.FullName);
-                    }
-                }
             }
 
             _result.HitCandidates.Add(new HitCandidate(true, document.Index, branchPoint.StartLine, (int)branchPoint.Ordinal));
 
             return AddInstrumentationInstructions(method, processor, instruction, _result.HitCandidates.Count - 1);
         }
 
-        private bool IsAsyncStateMachineBranch(TypeDefinition typeDef, MethodDefinition method)
-        {
-            if (!method.FullName.EndsWith("::MoveNext()"))
-            {
-                return false;
-            }
-
-            foreach (InterfaceImplementation implementedInterface in typeDef.Interfaces)
-            {
-                if (implementedInterface.InterfaceType.FullName == "System.Runtime.CompilerServices.IAsyncStateMachine")
-                {
-                    return true;
-                }
-            }
-            return false;
-        }
-
         private Instruction AddInstrumentationInstructions(MethodDefinition method, ILProcessor processor, Instruction instruction, int hitEntryIndex)
         {
             if (_customTrackerRecordHitMethod == null)

src/coverlet.core/Instrumentation/InstrumenterResult.cs

@@ -100,8 +100,6 @@ public InstrumenterResult()
         [DataMember]
         public string Module;
         [DataMember]
-        public string[] AsyncMachineStateMethod;
-        [DataMember]
         public string HitsFilePath;
         [DataMember]
         public string ModulePath;

src/coverlet.core/Symbols/BranchPoint.cs

@@ -1,11 +1,13 @@
 using System;
+using System.Diagnostics;
 using System.Text.RegularExpressions;
 
 namespace Coverlet.Core.Symbols
 {
     /// <summary>
     /// a branch point
     /// </summary>
+    [DebuggerDisplay("StartLine = {StartLine}")]
     public class BranchPoint
     {
         /// <summary>

src/coverlet.core/Symbols/CecilSymbolHelper.cs

@@ -5,6 +5,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Runtime.CompilerServices;
 using System.Text.RegularExpressions;
 
 using Coverlet.Core.Extensions;
@@ -18,7 +19,47 @@ namespace Coverlet.Core.Symbols
     public static class CecilSymbolHelper
     {
         private const int StepOverLineCode = 0xFEEFEE;
-        private static readonly Regex IsMovenext = new Regex(@"\<[^\s>]+\>\w__\w(\w)?::MoveNext\(\)$", RegexOptions.Compiled | RegexOptions.ExplicitCapture);
+
+        private static bool IsMoveNextInsideAsyncStateMachine(MethodDefinition methodDefinition)
+        {
+            if (!methodDefinition.FullName.EndsWith("::MoveNext()"))
+            {
+                return false;
+            }
+
+            if (methodDefinition.DeclaringType.CustomAttributes.Count(ca => ca.AttributeType.FullName == typeof(CompilerGeneratedAttribute).FullName) > 0)
+            {
+                foreach (InterfaceImplementation implementedInterface in methodDefinition.DeclaringType.Interfaces)
+                {
+                    if (implementedInterface.InterfaceType.FullName == "System.Runtime.CompilerServices.IAsyncStateMachine")
+                    {
+                        return true;
+                    }
+                }
+            }
+
+            return false;
+        }
+
+        private static bool IsMoveNextInsideEnumerator(MethodDefinition methodDefinition)
+        {
+            if (!methodDefinition.FullName.EndsWith("::MoveNext()"))
+            {
+                return false;
+            }
+            if (methodDefinition.DeclaringType.CustomAttributes.Count(ca => ca.AttributeType.FullName == typeof(CompilerGeneratedAttribute).FullName) > 0)
+            {
+                foreach (InterfaceImplementation implementedInterface in methodDefinition.DeclaringType.Interfaces)
+                {
+                    if (implementedInterface.InterfaceType.FullName == "System.Collections.IEnumerator")
+                    {
+                        return true;
+                    }
+                }
+            }
+
+            return false;
+        }
 
         public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinition)
         {
@@ -30,9 +71,10 @@ public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinitio
             var instructions = methodDefinition.Body.Instructions;
 
             // if method is a generated MoveNext skip first branch (could be a switch or a branch)
-            var skipFirstBranch = IsMovenext.IsMatch(methodDefinition.FullName);
+            bool isAsyncStateMachineMoveNext = IsMoveNextInsideAsyncStateMachine(methodDefinition);
+            bool skipFirstBranch = isAsyncStateMachineMoveNext || IsMoveNextInsideEnumerator(methodDefinition);
 
-            foreach (var instruction in instructions.Where(instruction => instruction.OpCode.FlowControl == FlowControl.Cond_Branch))
+            foreach (Instruction instruction in instructions.Where(instruction => instruction.OpCode.FlowControl == FlowControl.Cond_Branch))
             {
                 try
                 {
@@ -42,6 +84,15 @@ public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinitio
                         continue;
                     }
 
+                    // Skip get_IsCompleted to avoid unuseful branch due to async/await state machine
+                    if (isAsyncStateMachineMoveNext && instruction.Previous.Operand is MethodReference operand &&
+                            operand.Name == "get_IsCompleted" &&
+                            operand.DeclaringType.FullName.StartsWith("System.Runtime.CompilerServices.TaskAwaiter") &&
+                            operand.DeclaringType.Scope.Name == "System.Runtime")
+                    {
+                        continue;
+                    }
+
                     if (BranchIsInGeneratedExceptionFilter(instruction, methodDefinition))
                         continue;
 

test/coverlet.core.tests/CoverageTests.cs

@@ -138,5 +138,59 @@ public void SelectionStatements_Switch()
                 File.Delete(path);
             }
         }
+
+        [Fact]
+        public void AsyncAwait()
+        {
+            string path = Path.GetTempFileName();
+            try
+            {
+                RemoteExecutor.Invoke(async pathSerialize =>
+                {
+                    CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<AsyncAwait>(instance =>
+                    {
+                        instance.SyncExecution();
+
+                        int res = ((Task<int>)instance.AsyncExecution(true)).ConfigureAwait(false).GetAwaiter().GetResult();
+                        res = ((Task<int>)instance.AsyncExecution(1)).ConfigureAwait(false).GetAwaiter().GetResult();
+                        res = ((Task<int>)instance.AsyncExecution(2)).ConfigureAwait(false).GetAwaiter().GetResult();
+                        res = ((Task<int>)instance.AsyncExecution(3)).ConfigureAwait(false).GetAwaiter().GetResult();
+                        res = ((Task<int>)instance.ContinuationCalled()).ConfigureAwait(false).GetAwaiter().GetResult();
+
+                        return Task.CompletedTask;
+                    }, pathSerialize);
+                    return 0;
+                }, path).Dispose();
+
+                CoverageResult result = TestInstrumentationHelper.GetCoverageResult(path);
+                result.Document("Instrumentation.AsyncAwait.cs")
+                      .AssertLinesCovered(BuildConfiguration.Debug,
+                                            // AsyncExecution(bool)
+                                            (10, 1), (11, 1), (12, 1), (14, 1), (16, 1), (17, 0), (18, 0), (19, 0), (21, 1), (22, 1),
+                                            // Async
+                                            (25, 9), (26, 9), (27, 9), (28, 9),
+                                            // SyncExecution
+                                            (31, 1), (32, 1), (33, 1),
+                                            // Sync
+                                            (36, 1), (37, 1), (38, 1),
+                                            // AsyncExecution(int)
+                                            (41, 3), (42, 3), (43, 3), (46, 1), (47, 1), (48, 1), (51, 1),
+                                            (52, 1), (53, 1), (56, 1), (57, 1), (58, 1), (59, 1),
+                                            (62, 0), (63, 0), (64, 0), (65, 0), (68, 0), (70, 3), (71, 3),
+                                            // ContinuationNotCalled
+                                            (74, 0), (75, 0), (76, 0), (77, 0), (78, 0),
+                                            // ContinuationCalled -> line 83 should be 1 hit some issue with Continuation state machine
+                                            (81, 1), (82, 1), (83, 2), (84, 1), (85, 1)
+                                         )
+                      .AssertBranchesCovered(BuildConfiguration.Debug, (16, 0, 0), (16, 1, 1), (43, 0, 3), (43, 1, 1), (43, 2, 1), (43, 3, 1), (43, 4, 0))
+                      // Real branch should be 2, we should try to remove compiler generated branch in method ContinuationNotCalled/ContinuationCalled
+                      // for Continuation state machine
+                      .ExpectedTotalNumberOfBranches(BuildConfiguration.Debug, 4);
+            }
+            finally
+            {
+                File.Delete(path);
+            }
+        }
     }
 }

test/coverlet.core.tests/Instrumentation/InstrumenterResultTests.cs

@@ -37,7 +37,6 @@ public void CoveragePrepareResult_SerializationRoundTrip()
             ir.Module = "Module";
             ir.ModulePath = "ModulePath";
             ir.SourceLink = "SourceLink";
-            ir.AsyncMachineStateMethod = new string[] { "A", "B" };
 
             ir.HitCandidates.Add(new HitCandidate(true, 1, 2, 3));
             ir.HitCandidates.Add(new HitCandidate(false, 4, 5, 6));
@@ -110,11 +109,6 @@ public void CoveragePrepareResult_SerializationRoundTrip()
                 Assert.Equal(cpr.Results[i].ModulePath, roundTrip.Results[i].ModulePath);
                 Assert.Equal(cpr.Results[i].SourceLink, roundTrip.Results[i].SourceLink);
 
-                for (int k = 0; k < cpr.Results[i].AsyncMachineStateMethod.Length; k++)
-                {
-                    Assert.Equal(cpr.Results[i].AsyncMachineStateMethod[k], roundTrip.Results[i].AsyncMachineStateMethod[k]);
-                }
-
                 for (int k = 0; k < cpr.Results[i].HitCandidates.Count; k++)
                 {
                     Assert.Equal(cpr.Results[i].HitCandidates[k].start, roundTrip.Results[i].HitCandidates[k].start);