Fix switch pattern

Documentation/Changelog.md

@@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 -Attribute exclusion does not work if attribute name does not end with "Attribute" [#884](https://github.com/coverlet-coverage/coverlet/pull/884) by https://github.com/bddckr  
 -Fix deterministic build+source link bug [#895](https://github.com/coverlet-coverage/coverlet/pull/895)  
 -Fix anonymous delegate compiler generate bug [#896](https://github.com/coverlet-coverage/coverlet/pull/896)  
--Fix incorrect branch coverage with await ValueTask [#949](https://github.com/coverlet-coverage/coverlet/pull/949) by https://github.com/alexthornton1
+-Fix incorrect branch coverage with await ValueTask [#949](https://github.com/coverlet-coverage/coverlet/pull/949) by https://github.com/alexthornton1  
+-Fix switch pattern coverage [#1006](https://github.com/coverlet-coverage/coverlet/pull/1006)
 
 ### Added
 -Skip autoprops feature [#912](https://github.com/coverlet-coverage/coverlet/pull/912)  

src/coverlet.core/Coverage.cs

@@ -392,7 +392,33 @@ private void CalculateCoverage()
                     }
                 }
 
-                List<(int docIndex, int line)> zeroHitsLines = new List<(int docIndex, int line)>();
+                // Calculate lines to skip for every hits start/end candidate
+                // Nested ranges win on outermost one
+                foreach (HitCandidate hitCandidate in result.HitCandidates)
+                {
+                    if (hitCandidate.isBranch || hitCandidate.end == hitCandidate.start)
+                    {
+                        continue;
+                    }
+
+                    foreach (HitCandidate hitCandidateToCompare in result.HitCandidates)
+                    {
+                        if (hitCandidate != hitCandidateToCompare && !hitCandidateToCompare.isBranch)
+                        {
+                            if (hitCandidateToCompare.start >= hitCandidate.start &&
+                               hitCandidateToCompare.end <= hitCandidate.end)
+                            {
+                                for (int i = hitCandidateToCompare.start;
+                                     i <= (hitCandidateToCompare.end == 0 ? hitCandidateToCompare.start : hitCandidateToCompare.end);
+                                     i++)
+                                {
+                                    (hitCandidate.AccountedByNestedInstrumentation ??= new HashSet<int>()).Add(i);
+                                }
+                            }
+                        }
+                    }
+                }
+
                 var documentsList = result.Documents.Values.ToList();
                 using (var fs = _fileSystem.NewFileStream(result.HitsFilePath, FileMode.Open))
                 using (var br = new BinaryReader(fs))
@@ -416,27 +442,14 @@ private void CalculateCoverage()
                         {
                             for (int j = hitLocation.start; j <= hitLocation.end; j++)
                             {
-                                var line = document.Lines[j];
-                                line.Hits += hits;
-
-                                // We register 0 hit lines for later cleanup false positive of nested lambda closures
-                                if (hits == 0)
+                                if (hitLocation.AccountedByNestedInstrumentation?.Contains(j) == true)
                                 {
-                                    zeroHitsLines.Add((hitLocation.docIndex, line.Number));
+                                    continue;
                                 }
-                            }
-                        }
-                    }
-                }
 
-                // Cleanup nested state machine false positive hits
-                foreach (var (docIndex, line) in zeroHitsLines)
-                {
-                    foreach (var lineToCheck in documentsList[docIndex].Lines)
-                    {
-                        if (lineToCheck.Key == line)
-                        {
-                            lineToCheck.Value.Hits = 0;
+                                var line = document.Lines[j];
+                                line.Hits += hits;
+                            }
                         }
                     }
                 }

src/coverlet.core/Instrumentation/InstrumenterResult.cs

@@ -88,6 +88,7 @@ internal class HitCandidate
         public int start { get; set; }
         [DataMember]
         public int end { get; set; }
+        public HashSet<int> AccountedByNestedInstrumentation { get; set; }
     }
 
     [DataContract]

src/coverlet.core/Symbols/CecilSymbolHelper.cs

@@ -21,6 +21,7 @@ internal class CecilSymbolHelper : ICecilSymbolHelper
         private const int StepOverLineCode = 0xFEEFEE;
         // Create single instance, we cannot collide because we use full method name as key
         private readonly ConcurrentDictionary<string, int[]> _compilerGeneratedBranchesToExclude = new ConcurrentDictionary<string, int[]>();
+        private readonly ConcurrentDictionary<string, List<int>> _sequencePointOffsetToSkip = new ConcurrentDictionary<string, List<int>>();
 
         // In case of nested compiler generated classes, only the root one presents the CompilerGenerated attribute.
         // So let's search up to the outermost declaring type to find the attribute
@@ -395,6 +396,12 @@ private bool SkipGeneratedBranchesForExceptionHandlers(MethodDefinition methodDe
             return _compilerGeneratedBranchesToExclude[methodDefinition.FullName].Contains(instruction.Offset);
         }
 
+        // https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Expression%20Breakpoints.md
+        private bool SkipExpressionBreakpointsBranches(Instruction instruction) => instruction.Previous is not null && instruction.Previous.OpCode == OpCodes.Ldc_I4 &&
+                                                                                    instruction.Previous.Operand is int operandValue && operandValue == 1 &&
+                                                                                    instruction.Next is not null && instruction.Next.OpCode == OpCodes.Nop &&
+                                                                                    instruction.Operand == instruction.Next?.Next;
+
         public IReadOnlyList<BranchPoint> GetBranchPoints(MethodDefinition methodDefinition)
         {
             var list = new List<BranchPoint>();
@@ -441,6 +448,11 @@ public IReadOnlyList<BranchPoint> GetBranchPoints(MethodDefinition methodDefinit
                         }
                     }
 
+                    if (SkipExpressionBreakpointsBranches(instruction))
+                    {
+                        continue;
+                    }
+
                     if (SkipLambdaCachedField(instruction))
                     {
                         continue;
@@ -620,6 +632,10 @@ private static uint BuildPointsForSwitchCases(List<BranchPoint> list, BranchPoin
             return ordinal;
         }
 
+        public bool SkipNotCoverableInstruction(MethodDefinition methodDefinition, Instruction instruction) =>
+            SkipNotCoverableInstructionAfterExceptionRethrowInsiceCatchBlock(methodDefinition, instruction) ||
+            SkipExpressionBreakpointsSequences(methodDefinition, instruction);
+
         /*
            Need to skip instrumentation after exception re-throw inside catch block (only for async state machine MoveNext())
            es:
@@ -660,7 +676,7 @@ await ...
            IL_00eb: br.s IL_00ed
            ...
        */
-        public bool SkipNotCoverableInstruction(MethodDefinition methodDefinition, Instruction instruction)
+        public bool SkipNotCoverableInstructionAfterExceptionRethrowInsiceCatchBlock(MethodDefinition methodDefinition, Instruction instruction)
         {
             if (!IsMoveNextInsideAsyncStateMachine(methodDefinition))
             {
@@ -714,6 +730,44 @@ static Instruction GetPreviousNoNopInstruction(Instruction i)
             }
         }
 
+        private bool SkipExpressionBreakpointsSequences(MethodDefinition methodDefinition, Instruction instruction)
+        {
+            if (_sequencePointOffsetToSkip.ContainsKey(methodDefinition.FullName) && _sequencePointOffsetToSkip[methodDefinition.FullName].Contains(instruction.Offset) && instruction.OpCode == OpCodes.Nop)
+            {
+                return true;
+            }
+            /* 
+               Sequence to skip https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Expression%20Breakpoints.md
+               // if (1 == 0)
+               // sequence point: (line 33, col 9) to (line 40, col 10) in C:\git\coverletfork\test\coverlet.core.tests\Samples\Instrumentation.SelectionStatements.cs
+               IL_0000: ldc.i4.1
+               IL_0001: brtrue.s IL_0004
+               // if (value is int)
+               // sequence point: (line 34, col 9) to (line 40, col 10) in C:\git\coverletfork\test\coverlet.core.tests\Samples\Instrumentation.SelectionStatements.cs
+               IL_0003: nop
+                // sequence point: hidden
+                ...
+             */
+            if (
+                    instruction.OpCode == OpCodes.Ldc_I4 && instruction.Operand is int operandValue && operandValue == 1 &&
+                    instruction.Next?.OpCode == OpCodes.Brtrue &&
+                    instruction.Next?.Next?.OpCode == OpCodes.Nop &&
+                    instruction.Next?.Operand == instruction.Next?.Next?.Next &&
+                    methodDefinition.DebugInformation.GetSequencePoint(instruction.Next?.Next) is not null
+                )
+            {
+                if (!_sequencePointOffsetToSkip.ContainsKey(methodDefinition.FullName))
+                {
+                    _sequencePointOffsetToSkip.TryAdd(methodDefinition.FullName, new List<int>());
+                }
+                _sequencePointOffsetToSkip[methodDefinition.FullName].Add(instruction.Offset);
+                _sequencePointOffsetToSkip[methodDefinition.FullName].Add(instruction.Next.Offset);
+                _sequencePointOffsetToSkip[methodDefinition.FullName].Add(instruction.Next.Next.Offset);
+            }
+
+            return false;
+        }
+
         private static bool SkipBranchGeneratedExceptionFilter(Instruction branchInstruction, MethodDefinition methodDefinition)
         {
             if (!methodDefinition.Body.HasExceptionHandlers)

test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs

@@ -85,5 +85,69 @@ public void SelectionStatements_Switch()
                 File.Delete(path);
             }
         }
+
+        [Fact]
+        public void SelectionStatements_Switch_CSharp8_OneBranch()
+        {
+            string path = Path.GetTempFileName();
+            try
+            {
+                FunctionExecutor.Run(async (string[] pathSerialize) =>
+                {
+                    CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<SelectionStatements>(instance =>
+                    {
+                        instance.SwitchCsharp8(int.MaxValue);
+                        return Task.CompletedTask;
+                    }, persistPrepareResultToFile: pathSerialize[0]);
+                    return 0;
+                }, new string[] { path });
+
+                TestInstrumentationHelper.GetCoverageResult(path)
+                .Document("Instrumentation.SelectionStatements.cs")
+                .AssertLinesCovered(BuildConfiguration.Debug, 33, 34, 35, 36, 40)
+                .AssertLinesNotCovered(BuildConfiguration.Debug, 37, 38, 39)
+                .AssertBranchesCovered(BuildConfiguration.Debug, (34, 0, 1), (34, 1, 0), (34, 2, 0), (34, 3, 0), (34, 4, 0), (34, 5, 0))
+                .ExpectedTotalNumberOfBranches(3);
+            }
+            finally
+            {
+                File.Delete(path);
+            }
+        }
+
+        [Fact]
+        public void SelectionStatements_Switch_CSharp8_AllBranches()
+        {
+            string path = Path.GetTempFileName();
+            try
+            {
+                FunctionExecutor.Run(async (string[] pathSerialize) =>
+                {
+                    CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<SelectionStatements>(instance =>
+                    {
+                        instance.SwitchCsharp8(int.MaxValue);
+                        instance.SwitchCsharp8(uint.MaxValue);
+                        instance.SwitchCsharp8(short.MaxValue);
+                        try
+                        {
+                            instance.SwitchCsharp8("");
+                        }
+                        catch { }
+                        return Task.CompletedTask;
+                    }, persistPrepareResultToFile: pathSerialize[0]);
+                    return 0;
+                }, new string[] { path });
+
+                TestInstrumentationHelper.GetCoverageResult(path)
+                .Document("Instrumentation.SelectionStatements.cs")
+                .AssertLinesCovered(BuildConfiguration.Debug, 33, 34, 35, 36, 37, 38, 39, 40)
+                .AssertBranchesCovered(BuildConfiguration.Debug, (34, 0, 1), (34, 1, 3), (34, 2, 1), (34, 3, 2), (34, 4, 1), (34, 5, 1))
+                .ExpectedTotalNumberOfBranches(3);
+            }
+            finally
+            {
+                File.Delete(path);
+            }
+        }
     }
 }

test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs

@@ -320,6 +320,57 @@ public static Document AssertLinesCovered(this Document document, BuildConfigura
             return document;
         }
 
+        public static Document AssertLinesCovered(this Document document, BuildConfiguration configuration, params int[] lines)
+        {
+            return AssertLinesCoveredInternal(document, configuration, true, lines);
+        }
+
+        public static Document AssertLinesNotCovered(this Document document, BuildConfiguration configuration, params int[] lines)
+        {
+            return AssertLinesCoveredInternal(document, configuration, false, lines);
+        }
+
+        private static Document AssertLinesCoveredInternal(this Document document, BuildConfiguration configuration, bool covered, params int[] lines)
+        {
+            if (document is null)
+            {
+                throw new ArgumentNullException(nameof(document));
+            }
+
+            BuildConfiguration buildConfiguration = GetAssemblyBuildConfiguration();
+
+            if ((buildConfiguration & configuration) != buildConfiguration)
+            {
+                return document;
+            }
+
+            List<int> linesToCover = new List<int>(lines);
+            foreach (KeyValuePair<int, Line> line in document.Lines)
+            {
+                foreach (int lineToCheck in lines)
+                {
+                    if (line.Value.Number == lineToCheck)
+                    {
+                        if (covered && line.Value.Hits > 0)
+                        {
+                            linesToCover.Remove(line.Value.Number);
+                        }
+                        if (!covered && line.Value.Hits == 0)
+                        {
+                            linesToCover.Remove(line.Value.Number);
+                        }
+                    }
+                }
+            }
+
+            if (linesToCover.Count != 0)
+            {
+                throw new XunitException($"Not all requested line found, {linesToCover.Select(l => l.ToString()).Aggregate((a, b) => $"{a}, {b}")}");
+            }
+
+            return document;
+        }
+
         public static Document AssertNonInstrumentedLines(this Document document, BuildConfiguration configuration, int from, int to)
         {
             if (document is null)

test/coverlet.core.tests/Samples/Instrumentation.SelectionStatements.cs

@@ -28,5 +28,15 @@ public int Switch(int caseSwitch)
                     return 0;
             }
         }
+
+        public string SwitchCsharp8(object value) =>
+        value
+        switch
+        {
+            int i => i.ToString(System.Globalization.CultureInfo.InvariantCulture),
+            uint ui => ui.ToString(System.Globalization.CultureInfo.InvariantCulture),
+            short s => s.ToString(System.Globalization.CultureInfo.InvariantCulture),
+            _ => throw new System.NotSupportedException()
+        };
     }
 }