You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Accept generic array elements in tuple method data rows - #7517
A method source returning (Fixture, CancellationToken, int[]) incorrectly raises TUnit0001 for a single (Fixture, CancellationToken, T[]) parameter. Match open arrays recursively inside value tuples so compatible rows keep their tuple container, while retaining checks for rank, shape and concrete element conversions.
The isolated OpenArrayMatcher shares inferred identity/reference bounds across tuple elements, preserves array covariance and declared generic variance, and retries interface alternatives when later arguments conflict. Conservative upper-bound checks reject impossible class, array, interface and unrelated delegate combinations while preserving possible common implementations and compatible delegate variance. Array bounds and supported generic array-interface requirements are checked jointly, including nested arrays, to reject impossible three-way substitutions. Covariant delegate arguments are projected jointly as well, including nested delegates and arrays. A per-row work budget bounds recursive search and bound comparisons; exhaustion conservatively accepts rather than emitting an unproven mismatch. Constraints remain tracked separately in #7520.
Analyzer regressions cover ordinary, jagged and multidimensional arrays, nested invariance, containing types, repeated substitutions, variance bounds, tuple siblings and interface alternatives. Delegate regressions cover unrelated definitions, nested arrays, unsupported bases/interfaces, same-definition variance, invariant/value-type identity, and supported Delegate/MulticastDelegate/ICloneable conversions. Nested delegate bounds require identity-compatible containing constructions, including dynamic/object identity. Regressions cover container differences, deeper nesting, arrays and valid nested variance. Three explicit generic delegate cases compile conversions to both bounds and fail on the previous head.
Validation: full net10.0 analyzer suite: 1112 passed, one skipped; focused tuple-array suite: 136 passed. Five new delegate negatives fail on the previous head; an adversarial interface-search regression exercises conservative budget exhaustion. Other frameworks remain for CI. No generator output, public API, engine metadata or reflection changes. Existing build/restore warnings are tracked in #7410 and #7412. Ambiguous nested-type diagnostic text is tracked in #7534.
Design rationale: the structural-only alternative would fix the headline example, but would also accept concrete incompatible rows that the previous analyzer rejected. Binding repeated parameters by identity would instead reject valid covariance and contravariance cases. The retained bound matcher preserves both sets of user-visible behavior, with Roslyn-proven conversions first and conservative acceptance when proof is uncertain. The 512-step limit is a bounded-work policy, not a completeness guarantee: the measured headline, interface and repeated-binding examples used 3, 7 and 10 steps respectively (see the measurement comment), leaving substantial headroom for ordinary rows. Failed alternatives consume the same budget; exhaustion accepts the whole row regardless of later conflicts. This trades diagnostic precision for bounded analysis cost. The measured allocations are documented in the review conversation; this change makes no performance improvement claim. Generic constraints remain separate in #7520.
Summary by CodeRabbit
Bug Fixes
Improved analyzer handling of generic method data sources that use arrays, tuples, and nested types. Valid arguments involving array covariance, runtime conversions, and delegate variance are now recognized more accurately, reducing incorrect type-mismatch diagnostics.
Added coverage for complex generic argument combinations and cases where compatibility cannot be conclusively determined.
The analyzer now tracks type-parameter bindings through implicit conversion checks. It matches open arrays and tuple elements, including nested types and variance. New analyzer tests cover compatible and incompatible tuple-array inputs and bounded matching behavior.
Conversion checks now carry type-parameter bindings through recursive checks. The matcher compares array element bounds across arrays, named types, interfaces, base classes, and variance. A per-row step budget limits matching work, with conservative acceptance when exhausted.
Valid method-data rows that place a nested generic type in a contravariant position can still trigger a spurious TUnit0001 analyzer error. The case is narrow and has a safe workaround, but it should be fixed or accepted before merge.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: accepting generic array elements within tuple method data rows.
Linked Issues check
[#7489] The objective is to accept a single tuple method data row whose array element is compatible with a generic array element in the test parameter, for example `(Fixture, CancellationToken, int[])…
Out of Scope Changes check
All changed production code is in CompilationExtensions.cs, in the parameter-matching path used by the method data source analyzer. All changed test code is in `GenericTupleArrayMethodDataSourceTest…
Full details: Linked Issues check
Explanation
[#7489] The objective is to accept a single tuple method data row whose array element is compatible with a generic array element in the test parameter, for example (Fixture, CancellationToken, int[]) for (Fixture, CancellationToken, T[]), and to add positive and negative analyzer regressions. In src/TUnit.Analyzers/Extensions/CompilationExtensions.cs, HasImplicitConversionOrGenericParameter now (1) returns true for conversions Roslyn proves, (2) matches an open array parameter by rank, SZ-array shape and element type, and (3) recurses element by element through a value tuple parameter that contains type parameters inside arrays, keeping the tuple container. The new MatchesArrayElement helper enforces concrete element conversions. tests/TUnit.Analyzers.Tests/GenericTupleArrayMethodDataSourceTests.cs covers the issue shape as a positive case (int[] to T[], no diagnostic) and negative cases (int to T[], int[] to T[,], int[] to string[], (int, int)[] to (T, long)[], and nested array and constructed-type mismatches). The regressions cover the requirement. The reported net10.0 analyzer run passed. I did not inspect the TUnit0001 call site or run the tests, so this verdict relies on the code diff and the test expectations.
Full details: Out of Scope Changes check
Explanation
All changed production code is in CompilationExtensions.cs, in the parameter-matching path used by the method data source analyzer. All changed test code is in GenericTupleArrayMethodDataSourceTests.cs. The tuple and array recursion implements #7489. The supporting parts, namely the early Roslyn conversion check, covariance and contravariance handling in type arguments, containing-type matching, and interface and base-class matching for reference arrays, serve the same matching logic. The tests exercise each of these parts. No generator output, public API, engine metadata or reflection changes appear in the diff summary. Contravariant and nested-invariance handling goes beyond the issue's minimal example, but it prevents false acceptance of incompatible elements, which the issue's negative regressions require. No unrelated change was found.
Full details: Docstring Coverage
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each tuple row Through open arrays, bounds can flow When nested types come into view The matcher tests their shape and hue With bounded steps, it reaches the end
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 8ad71b6c-7a2c-47b7-a732-59f9c6e036aa
Commits
Reviewing files that changed from the base of the PR and between a57e050 and 46d82ec.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.
The fix is sound. The helper ContainsTypeParameter already exists, so there are no new dependencies. Open arrays and value tuples are matched recursively, and the rank and IsSZArray checks keep int[] vs T[,] and int vs T[] as diagnostics. The six tests cover the positive and negative cases, and the PR description says the three positive ones fail without the fix.
Minor, non-blocking points:
Both new branches treat a bare T as matching anything. They ignore T's constraints (e.g. T : class with an int[] element). The existing generic path seems to be equally lenient, so this is consistent. Constraint checking could be a follow-up.
Zip(...).All(...) allocates in an analyzer path. A plain for loop over TupleElements would avoid that and is just as readable. This is a low-priority optimisation.
A tuple test with a nested open tuple, for example (Fixture, (int, T[])), would exercise the recursion further. The code appears to handle it.
Addressed both Greptile findings in 2b659cc. Full net10.0 analyzer suite: 985 passed, one skipped. The two regressions fail against the previous PR head (7 passed, 2 failed) and pass with the fixes. Existing build warnings are tracked in #7410.
The fix is sound and well tested for the reported case. Positive and negative cases cover rank, jagged and multidimensional arrays, scalar elements and concrete elements.
Concerns
The array branch is stricter than before for top-level arrays. It now returns directly, without falling through to the existing conversion path. MatchesArrayElement requires an exact type match or an identical generic definition. Array covariance and variance, such as List<int>[] to IList<T>[] or string[] to IEnumerable<T>[], were accepted before this change if the old path allowed them. They are now rejected, which risks new false-positive TUnit0001 reports. Fall back to the old logic when the new check fails (return matched || <existing path>), or add tests that pin the intended behaviour.
The tuple branch recurses element by element. Its Zip and All calls allocate in an analyzer path that runs often. A plain for loop would avoid that. This is minor.
The parameter-side T shortcut ignores constraints. Accepting any argument for a bare T element (where T : class, for example) matches the existing behaviour. It deserves a comment saying so.
Overall this is a reasonable, narrowly scoped fix. I'd address point 1 or add a covariance test before merging.
Fixed both array reference conversion findings in 79332ee; both regressions fail on the previous head. Full net10.0 analyzer suite: 987 passed, one skipped. Other target frameworks remain for CI. Existing build warnings are tracked in #7410 and #7412.
Claude's conversation findings are also dispositioned: the covariance concern is fixed and tested, Zip/All is replaced with a loop, and a comment documents the existing constraint behavior. The suggested extra nested tuple test is optional coverage; the reported covariance cases and the existing recursive tuple-array cases are covered.
The fix is correct for the reported case, and the regressions cover it well: ordinary, jagged and multidimensional arrays, rank mismatch, scalar elements and concrete element conversions. I read the diff but didn't build or run the tests.
Design concerns (non-blocking)
MatchesArrayElement is a fairly large hand-written approximation of C# array covariance and generic inference. It handles type-argument identity, interface and base-type walks and the allowReferenceConversions flag. That is hard to reason about, and the interface/base walk could grow expensive on deep hierarchies. If there is an existing type-inference helper in the analyzers, or a way to substitute the inferred type arguments and then call ClassifyConversion, it would be more robust than a parallel matcher. Otherwise, a short comment stating the intended invariant would help future maintainers (for example: "matches what CastHelper does at runtime").
The new early HasImplicitConversion return at the top applies to every call, not only the open-array case. That seems safe, but it does widen the change. A test pinning an existing negative case would guard against accidental acceptance.
The tuple branch recurses through HasImplicitConversionOrGenericParameter, so tuple elements that are bare type parameters skip constraint checks. The comment acknowledges this. It is an acceptable trade-off, but it is a known gap that could hide a real constraint mismatch until runtime.
Tests
Adding a nested tuple-in-tuple case, or a List<int[]> versus IEnumerable<T[]> case, would exercise the allowReferenceConversions: false path that is currently only lightly covered.
Overall this is a reasonable, well-tested fix. I'd approve it, with the simplification noted above as optional follow-up.
Fixed nested array invariance in 2601483. Both new negative regressions fail against the previous head; full net10.0 analyzer suite passes (991 passed, one skipped). Other frameworks remain for CI; existing build warnings remain tracked in #7410 and #7412.
Claude’s latest non-blocking observations are dispositioned: the helper’s existing comment states the CastHelper invariant; the added tests exercise nested tuples and invariant array recursion, and the existing negative conversion regressions pass. Constraint handling remains the documented existing inference behavior. A broader substitution-based matcher is optional future design work, outside this fix.
The change fixes the reported TUnit0001 false positive for (Fixture, CancellationToken, T[]). The regression matrix is thorough. It covers rank, jagged arrays, concrete element conversions, covariance and nested invariance, and the PR says the positive cases fail without the fix.
Concerns (non-blocking)
Size of the new code.MatchesArrayElement is a hand-written, partial re-implementation of C# variance and conversion rules. It handles reference conversions, the invariance flag, and walks AllInterfaces and BaseType. Each review round has added a branch, which suggests more edge cases remain: generic variance (IEnumerable<out T>), nullable annotations, and AllInterfaces plus BaseType recursion that can revisit the same types. A more robust approach is to substitute the type parameters and then ask Roslyn. Infer the type-parameter bindings from the argument type, build the closed parameter type, and call ClassifyConversion. That reuses the compiler's rules, so there is nothing to keep in sync. If full inference is too heavy, an optimistic check that only verifies the array's shape (rank and IsSZArray) and defers element checks to runtime would be simpler. The cost is that the analyzer would no longer catch the int[] → string[] mismatch.
New early return for all callers. The HasImplicitConversion(argumentType, parameterType) shortcut at the top now runs before the existing logic for every call. It should only widen what is accepted, but it changes a shared helper's behaviour well beyond tuples containing arrays. Please confirm that no existing negative test relied on the later path rejecting something Roslyn considers convertible. The PR reports the full analyzer suite passing on net10.0, which is reassuring.
Constraints are ignored. The code comment says the shape check ignores constraints. A where T : struct parameter with a string[] source would pass the analyzer. This matches the existing bare-T behaviour, so it is acceptable, but it is worth a follow-up issue.
Test naming. The comment inside TupleArrayElements is helpful. Consider splitting positive and negative cases into separate test methods, so failures read clearly without the expectDiagnostic branching.
Overall the change is correct for the cases covered and well tested. Item 1 is the main long-term maintainability risk.
Fixed both remaining findings in a185a2d; containing type arguments remain invariant, and declared interface variance preserves valid reference conversions. Six additional regressions cover nested type identity, covariance, contravariance, reverse-direction rejection and value-type invariance. Both reported cases fail against the previous head. Full net10.0 analyzer suite: 997 passed, one skipped; other frameworks remain for CI.
Claude’s non-blocking observations are dispositioned: existing negative tests pass; the CastHelper invariant is documented; broader substitution-based inference and test naming are optional design follow-ups. The nested constraint gap is now tracked in #7520. Existing build/restore warnings remain tracked in #7410 and #7412.
Review of PR 7517: the fix is correct for the reported case. A (Fixture, CancellationToken, int[]) row now matches a (Fixture, CancellationToken, T[]) parameter and keeps its tuple container. Rank and IsSZArray are checked, and the regressions cover the shapes that matter. I read the analyzer change in CompilationExtensions.cs but did not run the tests; the PR description reports 997 passing on net10.0 only.
Concerns:
MatchesArrayElement is a hand-written copy of conversion rules Roslyn already implements: variance (In/Out), nested-type containment, array covariance, and interface/base-type walking. The several review rounds described in the PR show how easy it is to miss a case. Cases I would check: Out variance when the argument is a value type; type parameters in a nested type's containing type; pointer and function-pointer element types, which fall through to false. Suggested alternative: infer T from the argument with the existing InferTypeArguments machinery, substitute it into the parameter type (the file already has a substitution helper using CreateArrayTypeSymbol), then call ClassifyConversion once on the closed types. Roslyn then owns the variance and covariance rules. The cost is a fallback when inference is inconclusive, for example accepting, as the bare-T branch already does.
The tuple branch partly duplicates the array branch, since both recurse structurally over a type that contains a type parameter. If you keep the hand-rolled approach, fold them into one MatchesOpenType handling array, tuple and named types, so a new shape needs one fix.
allowVariance is a four-way boolean that is hard to read. A short worked example in a comment would help, such as IEnumerable to IEnumerable allowed versus IEnumerable to IEnumerable rejected.
Overall this is a sound, well-tested fix. I would still prefer the substitute-then-classify approach in point 1 over growing the hand-written matcher. Non-blocking.
Fixed the joint array/interface bound finding in fb74a74 and resolved its thread with regression evidence. Full net10.0 analyzer suite: 1103 passed, one skipped; focused suite: 127 passed; four new negative rows fail on the previous head. Other frameworks remain for CI; existing warnings are tracked in #7410 and #7412.
The earlier run 41746555513 completed successfully and no longer blocks this PR. Resolution routes checked: main does not deliver #7489, this PR retains Closes #7489, and no prerequisite, replacement, finished parent or stale acceptance criterion prevents shipping. #7520 remains separate constraint validation. No self-ending wait was labelled on-hold. The PR remains draft for fresh Claude and Codex verdicts on this head.
Claude’s latest non-blocking suggestions are dispositioned: the class comment now explicitly documents how unsubstituted type parameters use Roslyn constraint information, and the trailing blank line is removed. Retaining the isolated matcher preserves concrete mismatch, shared-binding and interface-alternative regressions; first-wins substitution or wildcard matching would change that contract. A step budget, conversion cache, stress benchmark and Bound record remain optional maintenance work; this change adds no interface search branches and makes no performance claim. CodeRabbit’s optional documentation-coverage suggestion is covered by the existing explanatory comments rather than a repository documentation gate. All historical review surfaces were checked; CodeRabbit’s only newer inline reply confirms its fix and resolution.
The fix does what the PR describes. (Fixture, CancellationToken, T[]) now matches a (Fixture, CancellationToken, int[]) row, which closes 7489. I read OpenArrayMatcher.cs in full. I did not run the tests, so I can't confirm the 1103-pass result.
Main concern: the change is far larger than the bug
The original report needed one thing: recurse into value tuples and match T[] against concrete arrays. OpenArrayMatcher instead reimplements a good part of C# conversion and inference semantics in about 330 lines: lower and upper variance bounds, joint array and array-interface bound solving, delegate-variance common-subtype reasoning, and continuation-passing backtracking over interfaces and base types.
Risk: a false negative only means a missed diagnostic, which the runtime would catch anyway. A false positive blocks valid user code. The return-false branches (CanShareReferenceSubtype, CanArraySubtypeImplement, the sealed-class and class/class checks) are where false positives can come from, so each rejection needs to be provably right.
Cost: the matcher is recursive and backtracking. It tries every AllInterfaces entry with a continuation, and each retry reruns the remaining arguments, so deeply nested generic arrays can grow exponentially. AllInterfaces also allocates on each call. The analyzer runs on every keystroke, so I'd like a depth or step cap, or a note on the worst case.
Maintenance: the matcher duplicates compiler inference rules without sharing code with the runtime CastHelper behaviour. If the two disagree, the analyzer and runtime diverge.
Suggested simplification
Make the analyzer lenient by default:
Handle tuple recursion and the T[] vs concrete-array shape check (rank, IsSZArray).
Bind T to a single exact element type, and require identity for later T occurrences.
Accept anything else that is plausibly convertible, and skip the delegate, array-interface and three-way bound solving.
That covers the issue's case and the common ones. Rarer cases would be left to the runtime, as they were before.
If you keep the full matcher: split it by concern (bound bookkeeping, array/interface sharing, delegate sharing) so each part is reviewable alone; add a test for a pathological nested case to show it terminates quickly; and check that ContainsTypeParameter handles every symbol kind the matcher passes it.
Smaller points
The Func<ArrayBindings,bool> lambdas allocate per call in a hot analyzer path. They're acceptable if the depth is capped.
The header comment points at 7520 for constraints. A short note on what still leaks through would help future readers.
Tests look thorough (ordinary, jagged and multidimensional arrays, variance, tuple siblings), and the three previous-head regressions are a good touch. I'd approve on behaviour, and I'd like the complexity and cost question settled before merge.
Fixed the joint covariant-delegate finding in 681ce18; replied with evidence and resolved its thread. Eight added delegate cases cover ordering, nested delegates/arrays and valid common substitutions. Five negatives fail on the previous head. Full net10.0 analyzer suite: 1112 passed, one skipped; focused suite: 136 passed. Other frameworks remain for CI. Existing warnings are tracked in #7410 and #7412.
Claude's reiterated complexity concern is addressed in the same commit: one matcher instance shares a 512-step budget across tuple siblings, recursive visits, bound comparisons and interface retries. Exhaustion conservatively accepts the entire row, including when a later sibling would otherwise reject it. An adversarial regression combines three 40-interface searches with a conflicting final sibling. Retaining bound checks preserves the incompatible-row regressions; first-wins exact substitution would reject valid variance cases. A conversion cache and a Bound record remain optional refactoring rather than prerequisites; no performance improvement is claimed. ContainsTypeParameter covers the type parameters, arrays and named/containing constructions traversed by this matcher; pointer traversal is already supported, while function pointers cannot be generic type arguments. Constraint validation remains #7520.
The earlier run 41746555513 completed successfully. Resolve routes checked: origin/main does not implement #7489; this PR still contains Closes #7489; #7489 has no native parent, children or prerequisite dependencies, and there is no replacement PR delivering this analyzer change. Its acceptance criteria still match the code. Rework budget is not exceeded (17 Claude rounds, no force-pushes). No on-hold label was added for a self-ending wait. The PR remains draft for fresh Claude and Codex verdicts on this head.
The fix addresses #7489. Open arrays are now matched recursively inside value tuples, so (Fixture, CancellationToken, int[]) against (Fixture, CancellationToken, T[]) no longer raises TUnit0001. Rank and SZ-array checks still reject genuine mismatches. I read OpenArrayMatcher.cs in full. I did not build the PR or run the tests, and I only skimmed the CompilationExtensions.cs change and the new tests.
Main concern: the matcher is much bigger than the bug.
The matcher is about 410 lines. It implements bound tracking, variance projection, delegate and array-interface common-subtype reasoning, and a 512-step budget. In effect it is a partial C# type-inference and assignability engine inside an analyzer. A diagnostic that can never be proven wrong is expensive here, because every false TUnit0001 blocks a user's build.
Fragile code with a lot to review. The CanShareReferenceSubtype rules cover sealed classes, delegates and array-interface cases. They are hand-written approximations of the CLR/C# rules. They are hard to verify by inspection and easy to get subtly wrong.
Silent exhaustion. Exhausting the step budget accepts the row, so a false negative is silent and depends on the budget. It is conservative, which is the safe direction, but there is no test showing that a legitimate, realistic row stays under 512 steps.
Allocations in a hot path. The analyzer runs per method-data-source row. ImmutableDictionary and ImmutableArray rebuilds, Func closures and ImmutableArray.CreateBuilder calls allocate on every call.
The closure remaining in MatchArguments is created for each type argument.
argument.AllInterfaces is re-enumerated in the retry loop.
AGENTS.md asks for minimal allocations in hot paths, and none of this is measured.
Suggested alternative.
Narrow the scope to what #7489 needs, and fall back to "accept" for anything uncertain:
Unify the open parameter's shape structurally: a tuple element or array element is a type parameter, array or named type.
Bind each type parameter once, and require identity for repeated occurrences.
Skip the variance, delegate and common-subtype reasoning, and accept anything that isn't clearly wrong.
The false-positive-free guarantee is easier to defend with a small matcher. If the extra precision is wanted, split it into a follow-up with benchmarks.
Smaller points.
MatchesArrayElement takes allowReferenceConversions and variance parameters whose interaction (allowReferenceConversions ? variance : None) is hard to follow. A small enum for the bound kind (Exact, Lower, Upper) would be clearer than VarianceKind reused as a bound direction.
The tuple-array case, the headline scenario, should have one plain test with no delegate or variance setup, so the primary regression is easy to find.
The nested-type containing-construction check (allowReferenceConversions: false on the containing type) looks right but needs a test that shows it.
Overall the behaviour looks correct and conservative. I'd prefer a smaller matcher, or at least evidence that the cost and the budget are acceptable, before merging.
Follow-up on Claude's current-head review (681ce18): the earlier run 41746555513 succeeded. Codex's mutable summary now explicitly clears this head; all inline threads are resolved.
I measured the unchanged matcher in a standalone Release net10.0 harness using SDK 10.0.403 and Roslyn 4.8.0 on macOS arm64. The harness compiles the actual OpenArrayMatcher.cs, uses the same ContainsTypeParameter implementation, builds symbols once, warms 2,000 calls, then measures 20,000 new matcher instances with Stopwatch and GC.GetAllocatedBytesForCurrentThread. Reflection reads the remaining budget after each call and is included in these timings/allocations. Inputs are array element symbols, as passed by CompilationExtensions; this measures the matcher, not full analyzer execution or tuple dispatch.
Match
Steps used / 512
Microseconds / match
Bytes / match
int[] to T[] (headline tuple's array element)
3
0.85
376
List[] to IEnumerable[]
7
9.75
1,768
Tuple<string,string>[] to Tuple<T,T>[]
10
5.55
1,168
Every iteration asserted a successful result without exhaustion. These are focused warm measurements, not a throughput guarantee or a before/after improvement claim. The existing adversarial 40-interface regression covers conservative exhaustion. Ordinary matches above use under 2% of the budget; immutable bookkeeping and continuations have a measurable but small cost on these cases. Retaining the matcher preserves the concrete mismatch/shared-binding/variance regressions; first-wins identity binding would reject valid variance cases. A conversion cache or bound enum remains optional refactoring. The plain tuple-array cases are in TupleArrayElements (including the issue's Fixture/CancellationToken tuple); nested containing-construction cases are also covered there.
Resolution routes rechecked against freshly fetched origin/main: #7489 remains unimplemented there, #7517 is its only open delivering PR and retains Closes #7489; #7489 has no parent, children or blocked_by prerequisite. No replacement, stale scope or finished parent can resolve this instead. #7520 is separate constraint work, not a prerequisite. Eighteen Claude rounds and zero force-pushes are below the rework threshold. No on-hold label is appropriate for review/CI.
Requesting a fresh Claude review of this evidence and the unchanged head before promotion, since the last verdict preferred cost/budget evidence before merge. No production changes or test results are claimed by this unit.
Review: PR 7517, generic array elements in tuple method data rows
I read the diff, but I could not run the build or the tests. The code-review skill errored, so this was a manual read of the main files.
What works
The fix targets the reported bug. (Fixture, CancellationToken, T[]) now keeps its tuple container, and the new tuple-element recursion in CompilationExtensions is a reasonable shape.
Roslyn-provable conversions are tried first. This keeps existing behaviour, including open array covariance.
Budget exhaustion accepts the row instead of reporting a diagnostic. For an analyzer, a missed diagnostic is better than a false TUnit0001.
The test list is thorough, and the PR says the delegate negatives fail on the previous head.
Concerns
Size against the problem. About 400 lines of new OpenArrayMatcher in a hand-written type-inference engine fix one false positive. It models bounds, variance, interface alternatives, delegate projection, nested containing types and a step budget. That is a partial reimplementation of C# generic inference. Each edge case (delegate bounds, unrelated classes, three-way array/interface substitutions) is another rule to maintain and another place to be subtly wrong.
Simpler option. The analyzer could stop at structure. Check tuple arity, array rank and IsSZArray. For an element that contains a type parameter, require only that the concrete element has a compatible shape: same generic definition, or any reference type when the parameter is a bare T. It would leave the cross-element bound checks to the runtime. Analyzer misses generic constraint violations inside tuple and array parameters #7520 already tracks constraints as a separate gap, which supports keeping the analyzer conservative. The bar for this fix should be no false positives, not a complete inference check.
Budget of 512. The budget is a magic number, and it is consumed by the recursive search and by the pairwise bound comparisons. A large tuple row can run out and silently turn off validation for that row. I suggest naming the constant, and adding a test that shows exhaustion on a plausible real shape, not only the adversarial one.
State shared across next callbacks. The matcher uses continuation-passing Func<ArrayBindings, bool> closures, with a mutable _remainingSteps counter inside what looks like a pure backtracking search. Backtracking consumes budget, so the result depends on the order in which alternatives are tried. That is hard to reason about. If the matcher stays, a small comment on this invariant and a test of order-dependence would help.
Allocations. Each analysis allocates an ImmutableDictionary, immutable arrays and closures, but only when the parameter type contains a type parameter. That limits the cost. It is still analyzer hot-path code, so a short note or benchmark would be reassuring.
Cosmetic. The BOM was removed from CompilationExtensions.cs and the file now has using System.Collections.Immutable; first. That is harmless. The long ArrayBindings alias is declared in two files. It could be a shared global using or an internal type, so the definitions do not drift.
Verdict: I would approve the behaviour. My main request is to consider shrinking the matcher (point 2), because I expect the long-term maintenance cost to exceed the value of catching these rare mismatches. If you keep it, please name the budget constant and document the order-dependence (points 3 and 4).
Run 41755672591 completed successfully. Claude's latest verdict approves the behavior and requests a named budget and documented traversal-order dependence if the matcher is retained. Addressed both in d17bbe7: MaxStepsPerRow names the unchanged 512-step limit; the comment explains that failed branches do not refund steps, traversal order can affect exhaustion, and exhaustion accepts the whole row conservatively. No behavior change. Focused net10.0 GenericTupleArrayMethodDataSourceTests: 136 passed; git diff --check passed. Existing budget-exhaustion coverage remains applicable. Other frameworks remain for CI.
Retaining the matcher preserves the reviewed mismatch, shared-binding and valid variance regressions; the alternative structural-only design would discard that coverage. The prior comment supplies ordinary-case budget/allocation measurements. Further splitting, alias cleanup and additional plausible-shape/order tests are optional follow-ups, rather than blockers to this unchanged behavior.
Resolve routes rechecked: freshly fetched origin/main does not contain this tuple-array matcher; #7489 remains open with current acceptance criteria, no sub-issues or prerequisite dependencies. #7517 is its only open delivering PR and retains Closes #7489. No superseding PR or finished parent applies; #7520 is separate constraint work. Nineteen Claude rounds and zero force-pushes do not meet the rework threshold with the latest behavior-approved verdict. No on-hold label was added for CI/review. Keeping draft for fresh reviews of this head.
The fix does what the issue asks. (Fixture, CancellationToken, int[]) against a single (Fixture, CancellationToken, T[]) parameter no longer raises TUnit0001, and rank and IsSZArray mismatches still produce the diagnostic. I read OpenArrayMatcher.cs but did not build the project or run the tests.
Main concern: size and complexity compared with the bug
The bug is a missed recursion into value tuples. The PR fixes it with a new ~400-line OpenArrayMatcher containing a type-inference engine with variance bounds, CPS backtracking through Func<ArrayBindings, bool> continuations, a 512-step work budget, and hand-written reference-subtype reasoning for arrays, interfaces and delegates. That is much more than the issue needs, and it brings three problems:
Wrong direction for an analyzer. A false positive blocks a user's build, while a false negative only means the error shows up at runtime. The matcher hand-reimplements parts of C# type inference and the CLR's common-subtype rules, so any gap in it is a new false positive. The file's own comments admit the conservative fallbacks ("exhaustion accepts", "unknown implementations remain accepted").
Allocation in an analyzer hot path. It allocates an ImmutableDictionary of ImmutableArray per binding, builders per call, and a closure plus Func per type argument, on every data-source row. The repo guidance asks for minimal allocations in discovery paths.
Hard to maintain. The budget makes the verdict depend on traversal order, as the comment says: "Interface traversal order can therefore affect when validation exhausts its budget". A correctness result that depends on interface enumeration order is difficult to debug and review.
Suggested simpler approach
Recurse through the tuple and array structure only: rank and IsSZArray agree, an array is matched with an array, tuple arity matches. This is what the issue needs.
If you want shared-binding checks such as T bound to both int and string, add a small Dictionary<ITypeParameterSymbol, ITypeSymbol> with identity comparison. Leave out variance, bounds and backtracking.
That would be about 40-60 lines, allocation-light, and would not introduce new false positives. It would also drop most of the delegate and array-interface speculation (CanShareReferenceSubtype, CanArraySubtypeImplement), which no user has asked for.
Smaller points
MaxStepsPerRow is a magic number with no measurement behind it. If the budget stays, say why 512 was chosen.
The tests are thorough, but many pin down internal heuristics (three-way array/interface substitutions, nested delegate variance) and would block a later simplification. I'd keep the ones that describe user-visible behavior.
I'd prefer the simplified matcher. If you keep the full one, please add a short design note on why a conservative structural check wasn't enough.
Run 41755672591 succeeded; the current head d17bbe7 also has passing Claude and positive Codex receipts, with all inline threads resolved. Added the design note requested by Claude's latest conversation review to the PR description. It explains why structural-only acceptance loses existing diagnostics, why identity-only bindings reject valid variance, the measured ordinary-case budget use, and the bounded-work policy. This takes the review's explicit alternative of retaining the matcher with a design note; no new head or test result is claimed.
Requesting Claude's follow-up verdict on this rationale before promotion. The earlier cost measurements remain linked in this conversation. Resolve routes checked: #7489 is open with current scope and no children or prerequisite dependencies; #7517 remains its only delivering PR and retains Closes #7489. No finished parent or replacement applies. #7520 is separate constraint work, not a blocker. No human, upstream, credential or lock blocker was found. This is a self-ending review wait, so no on-hold label is appropriate.
Review: PR 7517 (TUnit0001 on tuple-with-open-array rows)
I read the diff and the matcher's opening section (OpenArrayMatcher.cs lines 1-120). I did not read the rest of the matcher, and I did not build or run the tests. The PR reports a full analyzer suite pass: 1112 passed, one skip.
What works: The fix is aimed at the right place. (Fixture, CancellationToken, int[]) now matches a single (Fixture, CancellationToken, T[]) parameter. Putting the logic in its own OpenArrayMatcher keeps CompilationExtensions from growing further. The per-row step budget fails safe: when it runs out, the row is accepted instead of reporting an unproven TUnit0001. The tests are broad, and the PR says the three generic delegate cases fail on the previous head.
Concerns:
Size against the original bug. The bug is a false positive on tuples. The PR adds a type-compatibility engine of roughly 400+ lines. It covers variance bounds, joint array/interface substitution, delegate projection and CanShareReferenceSubtype. This is a second, partial reimplementation of C# conversion and inference rules, and it will be hard to maintain. For an analyzer, a false negative is much cheaper than a false positive. A simpler design would do three things:
Recurse through tuples only.
Bind T from array elements with plain ClassifyConversion.
Accept whenever it can't decide.
I'd ask whether the 'reject impossible combinations' precision (unrelated delegates, three-way interface substitutions) justifies the extra code. If it does, say why in the PR. If it doesn't, drop those parts.
Budget behaviour depends on order. The code comments admit that interface traversal order can change when the budget is exhausted. So a diagnostic could appear or disappear depending on how Roslyn orders interfaces. Also, MaxStepsPerRow = 512 has no justification. A test that deliberately exhausts the budget and asserts no diagnostic would pin this behaviour down.
Mutable state in a type with no thread-safety note.OpenArrayMatcher holds _remainingSteps. Analyzer callbacks can run concurrently. This is fine only if a new instance is created per row, and I did not check that the callers do this. A comment saying so, or a ref budget struct passed in, would make that explicit.
Allocations. In MatchesArrayElement, occurrences.Add, ImmutableArray.CreateBuilder, bindings.SetItem and the Func continuation allocate on every recursive visit. Rows are small and the budget is bounded, so this is acceptable. Still, this runs in an analyzer on every data source, so a quick check that ImmutableDictionary churn doesn't show up in the analyzer benchmarks would be reasonable.
Nested loops in MatchesArrayElement. The invariant check (lines 52-76) and the lower/upper check (lines 79-103) are both quadratic in occurrences, with a ClassifyConversion call in the inner loop. The budget bounds this, but pre-partitioning occurrences by variance would remove the repeated filtering.
Verdict: The fix is correct in direction and fails safe, and I found no blocking bug in what I read. My main request is to cut scope (point 1), or justify it in the PR. Checking points 2 and 3 would also help.
Run 41757609417 completed successfully. Claude’s follow-up explicitly found no blocking bug; Codex’s completed summary clears current head d17bbe7, and all 28 review threads are resolved.
Disposition of the follow-up suggestions: the PR description already explains why retaining concrete mismatch diagnostics and valid variance warrants the matcher, and records measured ordinary matches at 3, 7 and 10 of 512 steps. InterfaceSearchStopsConservativelyWhenWorkBudgetIsExhausted already asserts conservative acceptance on exhaustion. CompilationExtensions creates a fresh matcher per public matching invocation (line 16), so mutable budget state is not shared between concurrent analyzer callbacks. Allocation measurements are recorded above; partitioning bounds and further benchmark work are optional improvements, with no blocking correctness finding.
Resolve routes checked again: main has no OpenArrayMatcher; #7489 remains open with current scope, no native children or prerequisite dependencies, and #7517 is its sole open delivering PR with Closes #7489. No replacement or finished parent applies; #7520 remains independent constraint work. No new code or validation result is claimed. Promoting the unchanged reviewed head; ready-for-review triggers fresh CI and Claude review, which must pass before auto-merge can be armed.
Promoted unchanged head d17bbe7 after both reviewers cleared it. Waiting for ready-for-review Claude run 41758092822 and CI; read its verdict before arming auto-merge. The earlier run 41757609417 passed and is no longer the blocker.
Verdict: the fix addresses #7489 and the tests are thorough. I'd still push back on the amount of machinery behind it. I only read the code and didn't run the tests.
What works
Recursing into value tuples and open arrays keeps (Fixture, CancellationToken, T[]) from raising a false TUnit0001.
Rank and IsSZArray checks and the concrete element conversions still produce diagnostics.
The 512-step per-row budget accepts on exhaustion. So a failure of the matcher can only hide a diagnostic. It can't produce a false positive, and for an analyzer that is the right way to fail.
Concerns
Scope and maintainability.OpenArrayMatcher.cs is about 410 lines. It re-implements a large part of C# conversion and variance logic: joint covariant delegate projection, array/generic-interface joint bounds, nested-container identity and common-subtype reasoning. The reported bug was a single T[] inside a tuple. Every extra rule is a chance for a new false positive, and each is justified only by hand-written cases. A narrower approach would be to bind and infer only identity for open arrays, and to accept (not report) anything involving interface, delegate or variance bounds that needs reasoning. The analyzer would then miss a few rare mismatches. It wouldn't emit wrong ones, and the code would be roughly a third of the size.
Budget semantics. The budget is not refunded on backtracking, so whether a row is validated or silently accepted depends on the order of AllInterfaces. Treat that as a known limitation, and consider a test that pins the exhaustion behaviour (accepts, no diagnostic) so a later refactor can't change it quietly.
Allocations. Each row allocates Func continuations, ImmutableArray builders and ImmutableDictionary.SetItem copies. That is acceptable for an analyzer, but MatchArguments builds a new closure per type argument. Passing the continuation state in a small struct or reusing delegates would reduce this if profiling shows a cost.
Matcher instance lifetime.MatchesArrayElement is internal and depends on a mutable _remainingSteps. If a caller reuses one instance across rows, the budget leaks between them. Please confirm CompilationExtensions creates one instance per row. A comment or constructor-enforced lifetime would make that safe.
Overall this is correct in intent and well tested. I'd prefer the simpler conservative approach, but I wouldn't block on it if the maintainers want the stricter diagnostics.
Run 41758092822 succeeded; it is no longer a blocker. The new Codex finding is valid: CanShareReferenceSubtype falls through for System.Array/IDisposable, treating the restricted Array base as an ordinary extensible class and accepting an impossible substitution.
This PR has 22 automated Claude review rounds and a new blocking finding, exceeding the skill's 20-round rework budget. No further fixes will be pushed here. Split into native children of #7489:
No files are currently conflicting (zero force-pushes); the churn is in OpenArrayMatcher.cs and its bound/variance regressions. Both issues name their owned files and the reusable branch/head. Closing this PR as superseded by these deliverables, keeping branch issue-7489-tuple-array. #7489 stays open until its acceptance criteria are delivered.
Claude's latest nonblocking suggestions are dispositioned: scope reduction is now assigned to #7539/#7540; budget exhaustion already has a conservative-acceptance regression; CompilationExtensions constructs a fresh matcher per invocation; allocation tuning remains conditional on profiling; generic constraints remain open in #7520. No new validation is claimed in this split-only unit. Current CI need not finish for closing this superseded PR. No human, upstream, credential, or lock blocker requires deferral.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
GitHub Actions
Failure
A method source returning
(Fixture, CancellationToken, int[])incorrectly raises TUnit0001 for a single(Fixture, CancellationToken, T[])parameter. Match open arrays recursively inside value tuples so compatible rows keep their tuple container, while retaining checks for rank, shape and concrete element conversions.The isolated
OpenArrayMatchershares inferred identity/reference bounds across tuple elements, preserves array covariance and declared generic variance, and retries interface alternatives when later arguments conflict. Conservative upper-bound checks reject impossible class, array, interface and unrelated delegate combinations while preserving possible common implementations and compatible delegate variance. Array bounds and supported generic array-interface requirements are checked jointly, including nested arrays, to reject impossible three-way substitutions. Covariant delegate arguments are projected jointly as well, including nested delegates and arrays. A per-row work budget bounds recursive search and bound comparisons; exhaustion conservatively accepts rather than emitting an unproven mismatch. Constraints remain tracked separately in #7520.Analyzer regressions cover ordinary, jagged and multidimensional arrays, nested invariance, containing types, repeated substitutions, variance bounds, tuple siblings and interface alternatives. Delegate regressions cover unrelated definitions, nested arrays, unsupported bases/interfaces, same-definition variance, invariant/value-type identity, and supported Delegate/MulticastDelegate/ICloneable conversions. Nested delegate bounds require identity-compatible containing constructions, including dynamic/object identity. Regressions cover container differences, deeper nesting, arrays and valid nested variance. Three explicit generic delegate cases compile conversions to both bounds and fail on the previous head.
Closes #7489
Validation: full net10.0 analyzer suite: 1112 passed, one skipped; focused tuple-array suite: 136 passed. Five new delegate negatives fail on the previous head; an adversarial interface-search regression exercises conservative budget exhaustion. Other frameworks remain for CI. No generator output, public API, engine metadata or reflection changes. Existing build/restore warnings are tracked in #7410 and #7412. Ambiguous nested-type diagnostic text is tracked in #7534.
Design rationale: the structural-only alternative would fix the headline example, but would also accept concrete incompatible rows that the previous analyzer rejected. Binding repeated parameters by identity would instead reject valid covariance and contravariance cases. The retained bound matcher preserves both sets of user-visible behavior, with Roslyn-proven conversions first and conservative acceptance when proof is uncertain. The 512-step limit is a bounded-work policy, not a completeness guarantee: the measured headline, interface and repeated-binding examples used 3, 7 and 10 steps respectively (see the measurement comment), leaving substantial headroom for ordinary rows. Failed alternatives consume the same budget; exhaustion accepts the whole row regardless of later conflicts. This trades diagnostic precision for bounded analysis cost. The measured allocations are documented in the review conversation; this change makes no performance improvement claim. Generic constraints remain separate in #7520.
Summary by CodeRabbit