diff --git a/src/coverlet.core/Coverage.cs b/src/coverlet.core/Coverage.cs index 8ce16f064..17ab58106 100644 --- a/src/coverlet.core/Coverage.cs +++ b/src/coverlet.core/Coverage.cs @@ -279,53 +279,10 @@ 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> branchesToRemove = new List>(); - 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); } } - 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 sourceLinkDocuments, string document) { if (sourceLinkDocuments.TryGetValue(document, out string url)) diff --git a/src/coverlet.core/CoverageResult.cs b/src/coverlet.core/CoverageResult.cs index 264eafe8f..00f9cfabe 100644 --- a/src/coverlet.core/CoverageResult.cs +++ b/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 branchesToRemove = new List(); - 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) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index e849e34ce..9f5e0f2f5 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -36,7 +36,6 @@ internal class Instrumenter private TypeDefinition _customTrackerTypeDef; private MethodReference _customTrackerRegisterUnloadEventsMethod; private MethodReference _customTrackerRecordHitMethod; - private List _asyncMachineStateMethod; private List _excludedSourceFiles; public Instrumenter( @@ -121,8 +120,6 @@ public InstrumenterResult Instrument() InstrumentModule(); - _result.AsyncMachineStateMethod = _asyncMachineStateMethod == null ? Array.Empty() : _asyncMachineStateMethod.ToArray(); - if (_excludedSourceFiles != null) { foreach (string sourceFile in _excludedSourceFiles) @@ -396,7 +393,7 @@ 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. @@ -404,10 +401,10 @@ private void InstrumentIL(MethodDefinition 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); @@ -467,19 +464,6 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor Ordinal = branchPoint.Ordinal } ); - - if (IsAsyncStateMachineBranch(method.DeclaringType, method)) - { - if (_asyncMachineStateMethod == null) - { - _asyncMachineStateMethod = new List(); - } - - if (!_asyncMachineStateMethod.Contains(method.FullName)) - { - _asyncMachineStateMethod.Add(method.FullName); - } - } } _result.HitCandidates.Add(new HitCandidate(true, document.Index, branchPoint.StartLine, (int)branchPoint.Ordinal)); @@ -487,23 +471,6 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor 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) diff --git a/src/coverlet.core/Instrumentation/InstrumenterResult.cs b/src/coverlet.core/Instrumentation/InstrumenterResult.cs index 7f6a16ddf..6c7823e32 100644 --- a/src/coverlet.core/Instrumentation/InstrumenterResult.cs +++ b/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; diff --git a/src/coverlet.core/Symbols/BranchPoint.cs b/src/coverlet.core/Symbols/BranchPoint.cs index 427aad61b..943b620bb 100644 --- a/src/coverlet.core/Symbols/BranchPoint.cs +++ b/src/coverlet.core/Symbols/BranchPoint.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics; using System.Text.RegularExpressions; namespace Coverlet.Core.Symbols @@ -6,6 +7,7 @@ namespace Coverlet.Core.Symbols /// /// a branch point /// + [DebuggerDisplay("StartLine = {StartLine}")] public class BranchPoint { /// diff --git a/src/coverlet.core/Symbols/CecilSymbolHelper.cs b/src/coverlet.core/Symbols/CecilSymbolHelper.cs index 92b1887bb..82d58c79e 100644 --- a/src/coverlet.core/Symbols/CecilSymbolHelper.cs +++ b/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 GetBranchPoints(MethodDefinition methodDefinition) { @@ -30,9 +71,10 @@ public static List 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 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; diff --git a/test/coverlet.core.tests/CoverageTests.cs b/test/coverlet.core.tests/CoverageTests.cs index 6521f9476..39ce57193 100644 --- a/test/coverlet.core.tests/CoverageTests.cs +++ b/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(instance => + { + instance.SyncExecution(); + + int res = ((Task)instance.AsyncExecution(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + res = ((Task)instance.AsyncExecution(1)).ConfigureAwait(false).GetAwaiter().GetResult(); + res = ((Task)instance.AsyncExecution(2)).ConfigureAwait(false).GetAwaiter().GetResult(); + res = ((Task)instance.AsyncExecution(3)).ConfigureAwait(false).GetAwaiter().GetResult(); + res = ((Task)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); + } + } } } \ No newline at end of file diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterResultTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterResultTests.cs index 6e56e2a91..98001779b 100644 --- a/test/coverlet.core.tests/Instrumentation/InstrumenterResultTests.cs +++ b/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); diff --git a/test/coverlet.core.tests/InstrumenterHelper.cs b/test/coverlet.core.tests/InstrumenterHelper.cs index 471c6a893..7452e80c2 100644 --- a/test/coverlet.core.tests/InstrumenterHelper.cs +++ b/test/coverlet.core.tests/InstrumenterHelper.cs @@ -54,6 +54,30 @@ public static Document AssertBranchesCovered(this Document document, params (int return AssertBranchesCovered(document, BuildConfiguration.Debug | BuildConfiguration.Release, lines); } + public static Document ExpectedTotalNumberOfBranches(this Document document, BuildConfiguration configuration, int totalExpectedBranch) + { + if (document is null) + { + throw new ArgumentNullException(nameof(document)); + } + + BuildConfiguration buildConfiguration = GetAssemblyBuildConfiguration(); + + if ((buildConfiguration & configuration) != buildConfiguration) + { + return document; + } + + int totalBranch = document.Branches.GroupBy(g => g.Key.Line).Count(); + + if (totalBranch != totalExpectedBranch) + { + throw new XunitException($"Expected total branch is '{totalExpectedBranch}', actual '{totalBranch}'"); + } + + return document; + } + public static Document AssertBranchesCovered(this Document document, BuildConfiguration configuration, params (int line, int ordinal, int hits)[] lines) { if (document is null) @@ -219,7 +243,7 @@ async public static Task Run(Func callM Coverage coverage = new Coverage(newPath, new string[] { - $"[{Path.GetFileNameWithoutExtension(fileName)}*]*" + $"[{Path.GetFileNameWithoutExtension(fileName)}*]{typeof(T).FullName}" }, Array.Empty(), new string[] diff --git a/test/coverlet.core.tests/Samples/Instrumentation.AsyncAwait.cs b/test/coverlet.core.tests/Samples/Instrumentation.AsyncAwait.cs new file mode 100644 index 000000000..f843209dc --- /dev/null +++ b/test/coverlet.core.tests/Samples/Instrumentation.AsyncAwait.cs @@ -0,0 +1,87 @@ +// Remember to use full name because adding new using directives change line numbers + +using System.Threading.Tasks; + +namespace Coverlet.Core.Samples.Tests +{ + public class AsyncAwait + { + async public Task AsyncExecution(bool skipLast) + { + int res = 0; + res += await Async(); + + res += await Async(); + + if (!skipLast) + { + res += await Async(); + } + + return res; + } + + async public Task Async() + { + await Task.Delay(1000); + return 42; + } + + async public Task SyncExecution() + { + await Sync(); + } + + public Task Sync() + { + return Task.CompletedTask; + } + + async public Task AsyncExecution(int val) + { + int res = 0; + switch (val) + { + case 1: + { + res += await Async(); + break; + } + case 2: + { + res += await Async() + await Async(); + break; + } + case 3: + { + res += await Async() + await Async() + + await Async(); + break; + } + case 4: + { + res += await Async() + await Async() + + await Async() + await Async(); + break; + } + default: + break; + } + return res; + } + + async public Task ContinuationNotCalled() + { + int res = 0; + res += await Async().ContinueWith(x => x.Result); + return res; + } + + async public Task ContinuationCalled() + { + int res = 0; + res += await Async().ContinueWith(x => x.Result); + return res; + } + } +} diff --git a/test/coverlet.core.tests/Samples/Samples.cs b/test/coverlet.core.tests/Samples/Samples.cs index 79fc35ad7..d2c713a45 100644 --- a/test/coverlet.core.tests/Samples/Samples.cs +++ b/test/coverlet.core.tests/Samples/Samples.cs @@ -172,6 +172,14 @@ public IEnumerable Fetch() } } + public class AsyncAwaitStateMachine + { + async public Task AsyncAwait() + { + await Task.CompletedTask; + } + } + [ExcludeFromCoverage] public class ClassExcludedByCoverletCodeCoverageAttr { @@ -230,7 +238,8 @@ public class ClassWithPropertyExcludedByObsoleteAttr public class ClassWithSetterOnlyPropertyExcludedByObsoleteAttr { [Obsolete] - public string Property { + public string Property + { set => _ = string.Empty; } } diff --git a/test/coverlet.core.tests/Symbols/CecilSymbolHelperTests.cs b/test/coverlet.core.tests/Symbols/CecilSymbolHelperTests.cs index 26c6add59..e62f012fb 100644 --- a/test/coverlet.core.tests/Symbols/CecilSymbolHelperTests.cs +++ b/test/coverlet.core.tests/Symbols/CecilSymbolHelperTests.cs @@ -262,6 +262,22 @@ public void GetBranchPoints_IgnoresSwitchIn_GeneratedMoveNext() } + [Fact] + public void GetBranchPoints_IgnoresBranchesIn_AsyncAwaitStateMachine() + { + // arrange + var nestedName = typeof(AsyncAwaitStateMachine).GetNestedTypes(BindingFlags.NonPublic).First().Name; + var type = _module.Types.FirstOrDefault(x => x.FullName == typeof(AsyncAwaitStateMachine).FullName); + var nestedType = type.NestedTypes.FirstOrDefault(x => x.FullName.EndsWith(nestedName)); + var method = nestedType.Methods.First(x => x.FullName.EndsWith("::MoveNext()")); + + // act + var points = CecilSymbolHelper.GetBranchPoints(method); + + // assert + Assert.Empty(points); + } + [Fact] public void GetBranchPoints_ExceptionFilter() {