From 52656cdc28c60b58f0afd712bac2fdf7f3620382 Mon Sep 17 00:00:00 2001 From: pavelsem Date: Fri, 4 Sep 2026 18:13:36 +0200 Subject: [PATCH] fix(matching): attribute coverage to generic and overloaded methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A generic method and an overload set both scored 0.0 coverage however well tested, because the two canonical keys could not be equal: Roslyn MyApp.Helper.TryConvertToNumeric<>(string, out T) Cobertura MyApp.Helper.TryConvertToNumeric(string, ref T) Three independent mismatches, each of which alone is enough to lose the match: 1. Method-level generic arity. Roslyn knows the method is generic and writes Foo<>; a Cobertura usually carries no arity, so neither the exact pass nor the name-only fallback can match. The fallback strips the signature but keeps the arity marker. 2. Parameter modifiers. Roslyn writes `out T`/`in T`; the CLR records one by-ref marker, normalized here to `ref T`. 3. Nullable-reference annotations. Roslyn writes `string?`; a CLR signature cannot express it. 2 and 3 only break the exact-signature pass — but that is the only pass able to resolve an overload set, since the name-only fallback rightly refuses multiple candidates. So overloads went unmatched too. The fix, in two parts: * NormalizeSignatureForMatching reduces a signature to what BOTH sides can express: out/in/ref fold to ref, and `?` is dropped on both sides (so Roslyn's `int?` and Cobertura's Nullable still agree). The canonical key is a matching key, not a display name. * A new arity-relaxed pass sits between the exact and name-only passes. Arity is deliberately KEPT in the exact key so a real Find/Find pair stays distinguishable; the new pass only relaxes it, and only when exactly one candidate remains. Ambiguity declines rather than guessing — inventing coverage is worse than reporting none. Verified against a production solution's coverlet output (127 methods, one project): 7 methods gained their real coverage, 0 lost any, crappyMethodCount 7 -> 3, totalCrap 1411 -> 711. TryConvertToNumeric goes 0.00 -> 1.00 and CRAP 552 -> 23 against a Cobertura that always said branch-rate="1" for it. Across that whole solution 51 of 51 crappy generics had read exactly 0.000 — not one generic anywhere carried coverage. Tests: 223 pass in Core. Four existing assertions changed, each a deliberate contract change with the reason recorded at the test: the two generic-key tests now pass unchanged, Cobertura_NullableType loses its `?`, and the overload test that asserted 0.0 was never ambiguous — it is split into the case that now resolves and a genuinely ambiguous one that must still refuse. Not fixed here: Cli.Tests FullPipeline_MinCrapFilter fails on a clean checkout of b173d10 as well, unrelated to matching. --- .../Matching/CoberturaMethodParser.cs | 3 +- .../Matching/MethodCoverageMatcher.cs | 36 +++++- .../Matching/MethodIdentityNormalizer.cs | 76 ++++++++++++ .../Matching/RoslynMethodParser.cs | 2 +- .../GenericAndOverloadMatchingTests.cs | 111 ++++++++++++++++++ .../Matching/MethodCoverageMatcherTests.cs | 38 +++++- .../Matching/MethodIdentityNormalizerTests.cs | 12 +- 7 files changed, 269 insertions(+), 9 deletions(-) create mode 100644 tests/Crap4DotNet.Core.Tests/Matching/GenericAndOverloadMatchingTests.cs diff --git a/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs b/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs index eb4df5b..74bf492 100644 --- a/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs +++ b/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs @@ -67,7 +67,8 @@ public static string ToCanonicalKey(CoberturaMethodCoverage coverage) var className = NormalizeClassName(coverage.ClassName); var methodName = NormalizeMethodName(coverage.MethodName, className); var signature = NormalizeSignature(coverage.Signature); - return $"{className}.{methodName}{signature}"; + return $"{className}.{methodName}" + + MethodKeyHelper.NormalizeSignatureForMatching(signature); } private static string NormalizeClassName(string className) diff --git a/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs b/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs index 0238395..82bc7ed 100644 --- a/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs +++ b/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs @@ -16,6 +16,7 @@ public static MatchResult Match( { // Build coverage lookups by normalized key var fullKeyLookup = new Dictionary>(StringComparer.Ordinal); + var arityKeyLookup = new Dictionary>(StringComparer.Ordinal); var nameKeyLookup = new Dictionary>(StringComparer.Ordinal); foreach (var entry in coverageEntries) @@ -23,11 +24,14 @@ public static MatchResult Match( var fullKey = CoberturaMethodParser.ToCanonicalKey(entry); AddToLookup(fullKeyLookup, fullKey, entry); + AddToLookup(arityKeyLookup, StripArity(fullKey), entry); + var nameKey = MethodKeyHelper.GetNameOnlyKey(fullKey); AddToLookup(nameKeyLookup, nameKey, entry); } var matchedFullKeys = new HashSet(StringComparer.Ordinal); + var matchedArityKeys = new HashSet(StringComparer.Ordinal); var matchedNameKeys = new HashSet(StringComparer.Ordinal); var methods = new List(); var unmatchedNames = new List(); @@ -49,6 +53,24 @@ public static MatchResult Match( continue; } + // Pass 1b: Retry with the method's generic arity dropped. Roslyn always knows a + // method is generic and writes Foo<>; a Cobertura usually carries no + // method-level arity at all, so those two keys can never be equal and EVERY generic + // method would otherwise score 0.0 however well tested. Arity is kept in the exact + // key above rather than stripped there, so a genuine Find/Find pair stays + // distinguishable; this pass only relaxes it, and only when the result is unique. + var arityKey = StripArity(fullKey); + if (arityKeyLookup.TryGetValue(arityKey, out var arityMatches) && arityMatches.Count == 1) + { + matchedArityKeys.Add(arityKey); + methods.Add(new MatchedMethod + { + Complexity = complexity, + Coverage = arityMatches[0].Coverage + }); + continue; + } + // Pass 2: Fallback to name-only key (without signature) var nameKey = MethodKeyHelper.GetNameOnlyKey(fullKey); if (nameKeyLookup.TryGetValue(nameKey, out var nameMatches) && nameMatches.Count == 1) @@ -80,7 +102,10 @@ public static MatchResult Match( if (matchedFullKeys.Contains(kvp.Key)) continue; - // Check if matched by name-only fallback + // Check if matched by the arity-relaxed or name-only fallback + if (matchedArityKeys.Contains(StripArity(kvp.Key))) + continue; + var nameKey = MethodKeyHelper.GetNameOnlyKey(kvp.Key); if (matchedNameKeys.Contains(nameKey)) continue; @@ -138,6 +163,15 @@ public static MatchResult Match( }; } + /// Canonical key with the method's own generic arity removed. + private static string StripArity(string canonicalKey) + { + var sigStart = MethodKeyHelper.FindSignatureStart(canonicalKey); + return sigStart < 0 + ? MethodKeyHelper.StripMethodGenericArity(canonicalKey) + : MethodKeyHelper.StripMethodGenericArity(canonicalKey[..sigStart]) + canonicalKey[sigStart..]; + } + private static void AddToLookup( Dictionary> lookup, string key, diff --git a/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs b/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs index 8123776..f2a276d 100644 --- a/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs +++ b/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs @@ -74,6 +74,82 @@ public static List SplitTypeList(string typeList) return result; } + /// + /// Strip the generic arity marker from the method-name segment only. + /// + /// + /// Roslyn knows a method is generic and writes Foo<>; a Cobertura + /// <method name> carries no method-level arity at all, so the two keys can + /// never be equal while the marker survives. Class-level arity is left alone: Cobertura + /// does encode that, as a backtick (Cache`1), and dropping it would make + /// Cache<T>.Get and a non-generic Cache.Get collide. + /// Only the final segment is touched, and only when its angle brackets are balanced. + /// + public static string StripMethodGenericArity(string namePart) + { + var depth = 0; + var lastDot = -1; + for (var i = 0; i < namePart.Length; i++) + { + switch (namePart[i]) + { + case '<': depth++; break; + case '>': depth--; break; + case '.' when depth == 0: lastDot = i; break; + } + } + + var segStart = lastDot + 1; + var segment = namePart[segStart..]; + var open = segment.IndexOf('<', StringComparison.Ordinal); + if (open < 0 || !segment.EndsWith('>')) + return namePart; + + return namePart[..segStart] + segment[..open]; + } + + /// + /// Reduce a normalized signature to the information both sides can actually carry. + /// + /// + /// Two kinds of detail exist on the Roslyn side and nowhere in a CLR signature, and each + /// silently blocks the exact-signature pass — which is the only pass that can resolve an + /// overload set, because the name-only fallback deliberately refuses an ambiguous one: + /// + /// parameter modifiers: C# distinguishes out/in/ref, the CLR + /// records one by-ref marker, so all three fold to ref; + /// nullable-reference annotations: string? and string are the same CLR + /// type. ? is dropped on both sides rather than one, so int? (Roslyn) and + /// Nullable<int> (Cobertura, which normalizes to int?) still agree. + /// + /// The cost is that an overload set differing only by nullability or by + /// out vs ref becomes ambiguous; such a set cannot be declared in C# anyway + /// for the modifier case, and the name-only pass still refuses rather than guessing. + /// + public static string NormalizeSignatureForMatching(string signature) + { + if (string.IsNullOrEmpty(signature) || signature == "()") + return "()"; + if (!signature.StartsWith('(') || !signature.EndsWith(')')) + return signature; + + var inner = signature[1..^1]; + if (string.IsNullOrWhiteSpace(inner)) + return "()"; + + var reduced = SplitTypeList(inner).Select(t => + { + var x = t.Trim(); + if (x.StartsWith("out ", StringComparison.Ordinal)) + x = "ref " + x[4..]; + else if (x.StartsWith("in ", StringComparison.Ordinal)) + x = "ref " + x[3..]; + return x.Replace("?", "", StringComparison.Ordinal); + }); + + return "(" + string.Join(", ", reduced) + ")"; + } + /// /// Convert CLR backtick generic arity notation to angle bracket notation. /// Cache`1 → Cache<>, Dictionary`2 → Dictionary<,> diff --git a/src/Crap4DotNet.Core/Matching/RoslynMethodParser.cs b/src/Crap4DotNet.Core/Matching/RoslynMethodParser.cs index a83d93e..731a75a 100644 --- a/src/Crap4DotNet.Core/Matching/RoslynMethodParser.cs +++ b/src/Crap4DotNet.Core/Matching/RoslynMethodParser.cs @@ -51,6 +51,6 @@ private static string NormalizeGenericTypeParams(string fullName) return "<" + new string(',', paramCount - 1) + ">"; }); - return normalized + sigPart; + return normalized + MethodKeyHelper.NormalizeSignatureForMatching(sigPart); } } diff --git a/tests/Crap4DotNet.Core.Tests/Matching/GenericAndOverloadMatchingTests.cs b/tests/Crap4DotNet.Core.Tests/Matching/GenericAndOverloadMatchingTests.cs new file mode 100644 index 0000000..80d8efc --- /dev/null +++ b/tests/Crap4DotNet.Core.Tests/Matching/GenericAndOverloadMatchingTests.cs @@ -0,0 +1,111 @@ +using Crap4DotNet.Core.Complexity; +using Crap4DotNet.Core.Coverage; +using Crap4DotNet.Core.Matching; +using Crap4DotNet.Core.Models; +using FluentAssertions; +using Xunit; + +namespace Crap4DotNet.Core.Tests.Matching; + +/// +/// Coverage is silently discarded for two shapes that are ordinary in real code: +/// generic methods and overload sets. Every case below is taken verbatim from a +/// coverlet run over a production solution, where the Cobertura reported +/// branch-rate="1" for the method while the report scored it 0.0. +/// +/// Three independent mismatches cause it, and each needs its own case because +/// fixing one still leaves the others failing: +/// * the Roslyn key carries method-level generic arity (Foo<>) that a +/// Cobertura method name never has; +/// * Roslyn writes C# parameter modifiers (out T) where the CLR signature +/// writes by-ref (T&); +/// * Roslyn carries nullable-reference annotations (string?) that the CLR +/// signature cannot express at all. +/// +public sealed class GenericAndOverloadMatchingTests +{ + private static MethodComplexityResult Source(string methodName, string signature, string className = "Helper") => + new() + { + Identity = new MethodIdentity + { + Namespace = "MyApp", + ClassName = className, + MethodName = methodName, + Signature = signature, + FullName = $"MyApp.{className}.{methodName}{signature}", + FilePath = "Test.cs", + LineNumber = 1 + }, + Complexity = 10 + }; + + private static CoberturaMethodCoverage Cover(string methodName, string signature, double coverage, + string className = "MyApp.Helper") => + new() { ClassName = className, MethodName = methodName, Signature = signature, Coverage = coverage }; + + [Fact] + public void GenericMethod_WithByRefOutParameter_TakesItsCoverage() + { + var result = MethodCoverageMatcher.Match( + [Source("TryConvertToNumeric", "(string, out T)")], + [Cover("TryConvertToNumeric", "(System.String,T&)", 1.0)]); + + result.Methods.Should().HaveCount(1); + result.Methods[0].Coverage.Should().Be(1.0); + } + + [Fact] + public void GenericMethod_NoParameters_TakesItsCoverage() + { + var result = MethodCoverageMatcher.Match( + [Source("GetObjectTypeName", "()")], + [Cover("GetObjectTypeName", "()", 0.75)]); + + result.Methods[0].Coverage.Should().Be(0.75); + } + + [Fact] + public void NullableReferenceAnnotation_DoesNotBlockTheMatch() + { + var result = MethodCoverageMatcher.Match( + [Source("BuildLink", "(string, string, int, string?)")], + [Cover("BuildLink", "(System.String,System.String,System.Int32,System.String)", 0.5)]); + + result.Methods[0].Coverage.Should().Be(0.5); + } + + [Fact] + public void OverloadSet_PairsEachOverloadWithItsOwnCoverage() + { + // The name-only fallback deliberately refuses an ambiguous set, so an overload + // set can only be resolved by the exact-signature pass. That is what makes + // signature normalization load-bearing rather than cosmetic. + var result = MethodCoverageMatcher.Match( + [ + Source("BuildLink", "(string, string, int, string?)"), + Source("BuildLink", "(EcsDtoBase, string, string?)") + ], + [ + Cover("BuildLink", "(System.String,System.String,System.Int32,System.String)", 0.25), + Cover("BuildLink", "(MyApp.Dto.EcsDtoBase,System.String,System.String)", 0.75) + ]); + + result.Methods.Should().HaveCount(2); + result.Methods[0].Coverage.Should().Be(0.25); + result.Methods[1].Coverage.Should().Be(0.75); + } + + [Fact] + public void GenuinelyUncoveredMethod_StillReportsZero() + { + // The repair must not invent coverage: a source method with no coverage entry + // at all still defaults to 0.0 and is still reported as unmatched. + var result = MethodCoverageMatcher.Match( + [Source("Untested", "(int)")], + [Cover("SomethingElse", "()", 1.0)]); + + result.Methods[0].Coverage.Should().Be(0.0); + result.Warnings.Should().Contain(w => w.Code == "UNMATCHED_METHODS"); + } +} diff --git a/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs b/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs index 1479db4..2aaac2e 100644 --- a/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs +++ b/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs @@ -345,9 +345,13 @@ public void FallbackMatch_SignatureMismatch_SingleCandidate() } [Fact] - public void FallbackMatch_OverloadedMethods_NoFallback() + public void OverloadedMethods_ResolvedByTheExactSignaturePass() { - // Two overloads with incompatible signatures → fallback finds multiple candidates → no match + // This case used to assert 0.0. It was never ambiguous: `out int` and `System.Int32&` + // are the same parameter, so the exact-signature pass can pick the right overload once + // C# modifiers and CLR by-ref are folded together. Reaching the name-only fallback at + // all was the bug -- the fallback is right to refuse two candidates, but it should + // never have been asked. var complexity = new[] { MakeComplexity("Process", signature: "(out int)") @@ -360,9 +364,37 @@ public void FallbackMatch_OverloadedMethods_NoFallback() var result = MethodCoverageMatcher.Match(complexity, coverage); - // Fallback finds 2 candidates for "Process" → ambiguous → defaults to 0.0 + result.Methods.Should().ContainSingle() + .Which.Coverage.Should().Be(0.7); + } + + [Fact] + public void ArityRelaxedMatch_AmbiguousCandidates_Refuses() + { + // The arity-relaxed pass exists because a Cobertura method name usually carries no + // method-level arity. When it DOES (coverlet emits Find`1 in some runs) a merged + // coverage set can hold both spellings of the same name, and both reduce to the same + // arity-stripped key. Two candidates means nothing identifies which belongs to the + // source method, so the pass declines rather than guessing -- inventing coverage is + // the one outcome worse than reporting none. + var complexity = new[] + { + MakeComplexity("Find", signature: "(int)") + }; + var coverage = new[] + { + // Deliberately neither is Find`1: an exact hit on Find<>(int) would be resolved by + // the pass above and never reach the relaxed one. These two differ in arity, so + // both reduce to Find(int) while matching the source key exactly zero times. + MakeCoverage("Find`2", signature: "(System.Int32)", coverage: 0.9), + MakeCoverage("Find", signature: "(System.Int32)", coverage: 0.1) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + result.Methods.Should().ContainSingle() .Which.Coverage.Should().Be(0.0); + result.Warnings.Should().Contain(w => w.Code == "UNMATCHED_METHODS"); } // === Preserves order === diff --git a/tests/Crap4DotNet.Core.Tests/Matching/MethodIdentityNormalizerTests.cs b/tests/Crap4DotNet.Core.Tests/Matching/MethodIdentityNormalizerTests.cs index 9e0a166..cbceee6 100644 --- a/tests/Crap4DotNet.Core.Tests/Matching/MethodIdentityNormalizerTests.cs +++ b/tests/Crap4DotNet.Core.Tests/Matching/MethodIdentityNormalizerTests.cs @@ -269,12 +269,18 @@ public void Cobertura_GenericParameterType() } [Fact] - public void Cobertura_NullableType() - { + public void Cobertura_NullableType_DropsTheAnnotationBecauseTheOtherSideCannotCarryIt() + { + // The canonical key is a MATCHING key, not a display name: it may only carry detail + // both sides can express. Nullable-REFERENCE annotations exist on the Roslyn side and + // nowhere in a CLR signature, so `?` is dropped -- and dropped on both sides, which is + // why Nullable normalizing to `int?` must lose its `?` here too. Otherwise + // Roslyn's `int?` and this key would disagree and every such overload would go + // unmatched. Formatting for humans is the reporter's job, not this key's. var cov = MakeCobertura( signature: "(System.Nullable`1)"); CoberturaMethodParser.ToCanonicalKey(cov) - .Should().Be("MyApp.Service.DoWork(int?)"); + .Should().Be("MyApp.Service.DoWork(int)"); } [Fact]