fix(matching): attribute coverage to generic and overloaded methods by pavelsem · Pull Request #4 · 7Factor/crap4dotnet

pavelsem · 2026-09-07T16:37:21Z

Generic methods and some overloaded methods receive coverage 0.0, even when the Cobertura file reports coverage for them.

Roslyn     MyApp.Helper.TryConvertToNumeric<>(string, out T)
Cobertura  MyApp.Helper.TryConvertToNumeric(string, ref T)

Cause

The Roslyn key contains three details that the Cobertura key does not contain:

Method-level generic arity. Roslyn writes Foo<>. Coverlet writes Foo.
Parameter modifiers. Roslyn writes out T or in T. The CLR signature has one by-ref marker, normalized to ref T.
Nullable reference annotations. Roslyn writes string?. The CLR signature has string.

Each detail prevents the exact-key match. The name-only fallback cannot recover these methods: its key keeps <>, and it rejects a name with more than one coverage entry (an overload set).

Change

NormalizeSignatureForMatching folds out and in to ref. It keeps ?, so Foo(int) and Foo(int?) keep distinct keys.
A new relaxed pass runs between the exact pass and the name-only pass. The relaxed key removes method-level generic arity and ? annotations. The pass matches only when exactly one coverage entry and exactly one source method have the relaxed key. Otherwise the method continues to the name-only pass.

Matching order: exact key, relaxed key, name-only key.

Known limits (not changed here)

Coverlet writes one entry for Foo(int) and Foo<T>(int) in the same class. The entry contains the lines of both methods. Foo(int) receives the combined coverage, and Foo<T>(int) receives 0.0. Evidence: a local coverlet run produced one <method name="Foo" signature="(System.Int32)"> entry with lines from both methods.
The name-only pass counts coverage entries, but not source methods. With source Foo(int) and Foo(string) and one coverage entry Foo(System.Int32), both methods receive that entry's coverage. This behavior existed before this PR.

Tests

New: NullableValueTypeOverloads_KeepTheirOwnCoverage, RelaxedMatch_SharedBySeveralSourceMethods_Refuses, and the cases in GenericAndOverloadMatchingTests.
FallbackMatch_OverloadedMethods_NoFallback is renamed to OverloadedMethods_ResolvedByTheExactSignaturePass and now expects 0.7, because out int and System.Int32& produce the same key. ArityRelaxedMatch_AmbiguousCandidates_Refuses covers the ambiguous case.
Core: 225 pass.
Cli.Tests: FullPipeline_MinCrapFilter fails. It also fails on a clean checkout of b173d10, so the failure is not related to matching.

Production check

Measured on 52656cd against one project of a production solution (127 methods). Not re-measured on 6952d40.

7 methods received their reported coverage. No method lost coverage.
crappyMethodCount 7 → 3. totalCrap 1411 → 711.
TryConvertToNumeric<T>: coverage 0.00 → 1.00, CRAP 552 → 23. Cobertura reports branch-rate="1" for this method.
Before the change, all 51 CRAPpy generic methods in the solution had coverage 0.000.

Related to #7.

🤖 Generated with Claude Code

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 <method name> 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<int> 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<T>/Find<T,U> 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<T> 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.

goneflyin · 2026-09-24T21:14:57Z

Review findings are now in the line-specific requested-changes review.

CI diagnosis: the latest build passed restore, build, tests, and pack. It failed at dotnet tool install -g Crap4DotNet --add-source ./nupkg because nuget.config maps all packages to nuget.org; the .NET CLI does not permit --add-source with package source mapping. Self-analysis was skipped.

goneflyin

Requesting changes because signature normalization merges distinct nullable value-type overloads, and the arity-relaxed pass can assign one coverage entry to two source methods. See the inline comments for the cases and requested tests.

goneflyin · 2026-09-24T21:18:54Z

+            // key above rather than stripped there, so a genuine Find<T>/Find<T,U> 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)


With source Foo(int) and Foo<T>(int) and one arityless Foo(int) coverage entry, the nongeneric method matches exactly. This condition then matches the generic method to the same entry. Please reject a relaxed match when multiple source methods share its key, and test this case.

Fixed in 6952d40. The relaxed pass now also requires exactly one source method with the relaxed key. Test: RelaxedMatch_SharedBySeveralSourceMethods_Refuses.

The name-only pass has the same gap: it counts coverage entries, but not source methods. This behavior existed before this PR, so I suggest a separate issue for it.

goneflyin

The added code comments need an editing pass. If available, please use Scott's $precise-technical-writing skill (the code-comment guidance) or a similar standard: state the behavior or invariant briefly, keep problem history in the PR description, and remove claims the code does not enforce. Examples are attached to the relevant lines.

goneflyin · 2026-09-24T21:33:45Z

                continue;
            }

+            // Pass 1b: Retry with the method's generic arity dropped. Roslyn always knows a


This comment retells the PR rationale and says the relaxed match is unique, but the current check counts only coverage entries. After fixing that ambiguity, one or two lines stating when the fallback is safe would suffice. Please remove the emphasis on EVERY.

Fixed in 6952d40. The comment now states the match condition in two lines. EVERY is removed.

goneflyin · 2026-09-24T21:33:45Z

+    /// Reduce a normalized signature to the information both sides can actually carry.
+    /// </summary>
+    /// <remarks>
+    /// Two kinds of detail exist on the Roslyn side and nowhere in a CLR signature, and each


This remarks block repeats the defect history and describes int? becoming int as safe, although those are distinct value-type overloads (see the earlier review). After correcting that behavior, please reduce the documentation to the matching contract: which distinctions the key preserves and which it discards.

Fixed in 6952d40. The remarks now state the contract: out and in fold to ref, and ? is kept.

goneflyin · 2026-09-24T21:33:45Z

+namespace Crap4DotNet.Core.Tests.Matching;
+
+/// <summary>
+/// Coverage is silently discarded for two shapes that are ordinary in real code:


This class header repeats the PR description. The claim that every case is taken verbatim from production is also hard to verify from the constructed MyApp fixtures. Please keep only context the test names and inputs cannot show; a short sentence describing the test scope is enough.

Fixed in 6952d40. The header is one sentence, and the verbatim claim is removed.

goneflyin · 2026-09-24T21:33:45Z

+    [Fact]
+    public void ArityRelaxedMatch_AmbiguousCandidates_Refuses()
+    {
+        // The arity-relaxed pass exists because a Cobertura method name usually carries no


These comment blocks explain the test history and matcher implementation at length. The fixture and assertion already show most of that. Please keep only the nonobvious setup, such as: Neither coverage entry matches Find<T> exactly; both share its arity-stripped key. The earlier overload test comment could be trimmed the same way.

Fixed in 6952d40. The comments now contain only the nonobvious setup. The fixture comment uses your suggested wording, and the earlier overload test comment is one line.

goneflyin · 2026-09-24T22:30:48Z

Hi @pavelsem -- sorry for the slow reply, we have a Pavel here at @7Factor and I presumed you were him erroneously.

Thanks for the PR -- first one on this!

Mostly good, a couple things in the code that are identified. As for the comments -- AI does a lousy job at knowing when to say more, and when to say less. Those are examples of it -- happens all the time. My approach is to use this skill I created, but it's just my take and probably not much better than telling it to use ASD STE-100.

…ared relaxed keys - The exact key keeps `?`, so Foo(int) and Foo(int?) no longer share a key. - The relaxed pass also drops `?`. It matches only when exactly one coverage entry and exactly one source method have the relaxed key. - Shorten code comments to the matching contract. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pavelsem · 2026-09-25T08:06:31Z

Hi @goneflyin - Thanks for your suggestions and thorough review. The changes are implemented, tests added and passing. Can you please have a look again?

pavelsem · 2026-09-25T08:09:25Z

Follow-up: #12 tracks the two limits listed in the PR description. They are the name-only pass gap and the Coverlet entry that combines Foo(int) and Foo<T>(int). This PR does not change them.

goneflyin requested changes Sep 24, 2026

View reviewed changes

goneflyin reviewed Sep 24, 2026

View reviewed changes

goneflyin mentioned this pull request Sep 24, 2026

CI: packed tool installation fails with NuGet package source mapping #10

Closed

pavelsem mentioned this pull request Sep 25, 2026

Coverage is assigned to the wrong method when source methods share a matching key #12

Open

pavelsem requested a review from goneflyin September 25, 2026 08:12

goneflyin mentioned this pull request Sep 28, 2026

Agent reports stale PR review after sandboxed GitHub access fails despite available scoped escalation openai/codex#49083

Open

Conversation

pavelsem commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cause

Change

Known limits (not changed here)

Tests

Production check

Uh oh!

goneflyin commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goneflyin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goneflyin Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goneflyin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goneflyin commented Sep 24, 2026

Uh oh!

pavelsem commented Sep 25, 2026

Uh oh!

pavelsem commented Sep 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pavelsem commented Sep 7, 2026 •

edited

Loading

goneflyin commented Sep 24, 2026 •

edited

Loading

goneflyin left a comment •

edited

Loading

goneflyin Sep 24, 2026 •

edited

Loading