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
Method data-source rows such as (Fixture, CancellationToken, int[]) incorrectly receive TUnit0001 when the test accepts (Fixture, CancellationToken, T[]). Traverse tuple and array shapes to accept compatible open elements, checking array rank and SZ-array shape along the way.
Roslyn still decides concrete conversions. Tuple fields inside value-type arrays retain identity requirements, and uncertain nested generic bindings remain accepted without a bound-inference engine. Generic tuples without arrays keep their existing behavior; generic constraint work remains separate in #7520.
Validation: 212 analyzer tests passed on net10.0 with SDK 10.0.403: 16 focused tuple/array cases (including imported T[*] metadata), 123 MethodDataSourceAnalyzerTests, 67 ReferenceTupleMethodDataSourceTests, 3 EnumerableMethodDataTupleTypeTests, 2 TupleParameterMismatchTests, and 1 GenericMethodSourceDataTests. Existing NU1902 restore warnings were recorded on #7412.
Review was skipped as selected files did not have any reviewable changes.
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: b9b3f5d6-6dfb-40fa-b3e9-be05c94f70bb
Commits
Reviewing files that changed from the base of the PR and between 53caaf4 and 7d5938d.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
The analyzer now checks eligible tuple and array shapes when it evaluates generic method data-source arguments. New tests cover non-SZ arrays and tuple-array combinations, including cases that expect a wrong-argument-type diagnostic.
HasImplicitConversionOrGenericParameter routes eligible pairs to shape matching. The matcher checks array rank and SZ-array status, array element identity, tuple arity, and tuple elements. It accepts uncertain open generic shapes without validating nested inference, constraints, or shared bounds.
Accept compatible generic array elements inside tuple data-source parameters — This PR adds the requested tuple and open-array shape traversal and covers compatible and incompatible cases.
Analyzer and runtime conversion parity for tuple elements — The PR uses Roslyn conversions for concrete nested tuple-array cases, but does not add a shared analyzer/runtime parity matrix.
Structural fix for generic tuple-array binding — The PR conservatively accepts uncertain shared generic bounds and does not add bounded inference.
Possibly related PRs
thomhurst/TUnit#7517 — Both PRs change open-array and tuple shape matching in CompilationExtensions.cs to address compatible generic array elements in tuple data rows.
Merge Risk: Minimal · previous review 53caa
No concrete issue currently blocks merging; complete the normal CI checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 25.00% 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 "Fix generic array binding inside tuple data-source parameters" matches the main change. The raw summary and objectives show the PR routes tuple and array shape matching through HasImplicitC…
Linked Issues check
Directly linked issue #7539 is the only active target. #7520 is referenced only as separate scope, so its constraint work is not required here. Coding requirements and evidence at head 53caaf4:
1. A…
Out of Scope Changes check
The whole-PR diff against merge base 10a0af7 changes two files. src/TUnit.Analyzers/Extensions/CompilationExtensions.cs is the file that #7539 assigns to this work. `tests/TUnit.Analyzers.Tests/Gen…
Full details: Title check
Explanation
The title "Fix generic array binding inside tuple data-source parameters" matches the main change. The raw summary and objectives show the PR routes tuple and array shape matching through HasImplicitConversionOrGenericParameter, so that compatible rows such as (Fixture, CancellationToken, int[]) against (Fixture, CancellationToken, T[]) no longer raise TUnit0001. The title is concise, specific, and free of noise.
Full details: Linked Issues check
Explanation
Directly linked issue #7539 is the only active target. #7520 is referenced only as separate scope, so its constraint work is not required here. Coding requirements and evidence at head 53caaf4:
Accept (Fixture, CancellationToken, int[]) against (Fixture, CancellationToken, T[]): HasImplicitConversionOrGenericParameter routes tuples that contain an array and a type parameter to MatchesOpenArrayShape. The tuple walk accepts the open element T[] through the array branch. The int element is accepted through the open type-parameter branch. Covered by TupleArrayElements case int[] vs T[] (no diagnostic) and the tuple-level cases.
Nested tuples, jagged arrays, rank and SZ-array mismatches: covered by (int, (int, int[][])) vs (T, (T, T[][])), int[][] vs T[][], int[,] vs T[,], int[] vs T[,] (diagnostic), and RankOneNonSzArrayDoesNotBindToOpenVector, which uses imported T[*] metadata. The code checks Rank and IsSZArray before recursing.
Roslyn conversions for decidable concrete cases: concrete elements use HasImplicitConversionOrGenericParameter or ClassifyConversion. Value-type array elements require IsIdentity. Covered by (int, string)[] vs (T, object)[] (diagnostic), (int, string[]) vs (T, int[]) (diagnostic), and int[] vs string[] (diagnostic).
Conservative acceptance of uncertain open generic compatibility: nested generic named types fall through to return true. Covered by List<int>[] vs IEnumerable<T>[], IConsumer<string, object>[] vs IConsumer<T, string>[], and (int, string[]) vs (T, T[]).
Existing valid variance and identity behavior: (int, string[]) vs (T, object[]) is accepted through reference-array covariance. dynamic/object identity is covered in both directions.
Generic tuples without arrays are not routed (ContainsArray is false), so their existing behavior is unchanged.
Limitation: I did not run the test suite. Test expectations were checked by tracing them against the added logic. The branch issue-7489-tuple-array was not inspected. Its reuse is a process instruction, and the traversal it describes is present at head.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against merge base 10a0af7 changes two files. src/TUnit.Analyzers/Extensions/CompilationExtensions.cs is the file that #7539 assigns to this work. tests/TUnit.Analyzers.Tests/GenericTupleArrayMethodDataSourceTests.cs contains the focused tuple-array analyzer tests that #7539 requests. The new helpers (ContainsArray, MatchesOpenArrayShape) serve the tuple/array traversal. The added comments refer to #7520 and document the deliberate exclusion of constraint and shared-bound inference. No unrelated production code, configuration, or documentation changes appear in the diff.
Full details: Docstring Coverage
Explanation
Docstring coverage is 25.00% 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 4 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
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 nested arrays, shapes can show. Rank and bounds are tested with care, A wrong-type warning marks mismatch there. Then carrots hop into the burrow.
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: fda05f40-51ef-4099-8c7f-52a47fb71397
Commits
Reviewing files that changed from the base of the PR and between 10a0af7 and 53caaf4.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.
Uncertain nested generic bindings stay permissive, so the change cannot add new false-positive TUnit0001 diagnostics. Roslyn still makes the final call.
Non-blocking notes
requiresIdentity is set once an array element is reached and is then passed down unchanged. That is right for value-type elements, but it also applies to reference-type elements such as string[][] → T[][]. Array covariance would allow those. The fallthrough return true for anything containing a type parameter hides this today. If someone later tightens that fallthrough, they could produce false positives. Setting requiresIdentity only when the element type is a value type would be more precise. A comment or a test row on this would also help.
ContainsArray and ContainsTypeParameter are called on every comparison before the shape matching starts. They walk the type each time. This is probably fine for analyzer paths, but a single combined walk would be cheaper.
The special case in HasImplicitConversionOrGenericParameter routes only types that contain both an array and a type parameter. That is the right narrow scope, but a short comment pointing at Analyzer misses generic constraint violations inside tuple and array parameters #7520 for the constraint and inference follow-up would keep the intent clear.
Generic method data-source rows match tuple and array shapes.
The earlier scalar-to-tuple finding is fixed: tuple targets now reject non-tuple sources after checking conversions. That thread has no supplied comment number.
thomhurst explicitly leaves uncertain nested generic bindings accepted and defers inference and constraint work to #7520.
Codex tuple shape mismatch: fixed and thread resolved; 21 focused tests and 123 MethodDataSourceAnalyzerTests pass on net10.0 using SDK 10.0.403.
Claude note 1: added a string[][] to T[][] regression row to preserve the intended reference-array behavior. The conservative inference fallback remains intentional.
Claude note 2: retaining the separate shape walks for this correctness fix. No measured analyzer performance regression warrants restructuring the traversal here.
Existing build warnings remain tracked in #7410 (release tracking/nullable warnings) and #7412 (restore advisories). Keeping this draft for fresh reviews of the new head.
The change is correct for the scenarios it targets. The new analyzer path in CompilationExtensions.cs is narrowly gated: it applies only when the parameter type contains both an array and a type parameter. The 20-case matrix, including the imported T[*] metadata case, covers the array-rank/SZ-array checks and the identity requirement for value-type array elements. I found nothing blocking.
Non-blocking observations:
ContainsArray is narrower than the traversal in MatchesOpenArrayShape. It only looks through tuples and arrays. A parameter such as List<(T, int[])> or Func<T[]> skips the new path and uses the old behavior. That matches the stated scope (generic constraint work stays in Analyzer misses generic constraint violations inside tuple and array parameters #7520). A short comment on ContainsArray saying nested generics are deliberately excluded would stop someone from 'fixing' it later.
The fallback return true is permissive on purpose. For (T, object[]) against (int, string[]), the matrix expects no diagnostic because Roslyn does the concrete conversion check. If the same type parameter is bound to two incompatible elements, for example (T, T[]) with (int, string[]), no diagnostic is reported. The PR states this limitation. It would be worth adding one pinning test, so that when Analyzer misses generic constraint violations inside tuple and array parameters #7520 adds bound inference the behavior change shows up as a test diff.
requiresIdentity is a boolean flowing through recursion. It's fine at this size. If more shape rules arrive, a small enum such as Variance { Covariant, Invariant } would read better than a bare true argument.
The metadata-built T[*] test is clever but fragile. It hand-assembles a PE image with magic bytes. The comment helps. Extracting the blob into a named constant or a small helper would make the signature easier to audit.
Overall this is a well-scoped, well-tested fix. Approving from my side.
Codex identity-conversion finding: fixed using Roslyn ClassifyConversion(...).IsIdentity, with dynamic/object regression coverage in both directions and nested tuple arrays. Numeric and reference conversion rejection remains covered.
Claude observation 1: added a comment documenting the deliberate exclusion of nested generic arguments from ContainsArray.
Claude observation 3: retain the boolean at this size, as the review recommends; reconsider an enum if additional shape rules are introduced.
Claude observation 4: retain the inline metadata signature and its adjacent byte-by-byte description. It has one use, so extracting it would not improve the current test's auditability.
Validation: the three new dynamic/object cases failed before the fix. After the fix, all 26 focused tuple-array tests and 123 MethodDataSourceAnalyzerTests pass using SDK 10.0.403 on net10.0. Existing restore/release-tracking/nullable warnings remain tracked in #7412 and #7410. Keeping the PR draft for fresh review of this head.
I read the diff and did not build it or run the tests. I found no blocking issues.
What it does: it stops TUnit0001 from firing on rows like (Fixture, CancellationToken, int[]) when the test takes (Fixture, CancellationToken, T[]). It walks tuple and array shapes, comparing rank, SZ-array-ness and tuple arity. It requires identity for elements inside arrays, because arrays of value types are invariant. For anything it can't resolve it falls back to accepting, so it won't add false positives.
Strengths
The scope is narrow. The new path only runs when the parameter type contains both an array and a type parameter, so existing generic-tuple behaviour is unchanged.
requiresIdentity is carried down through nested tuples, which keeps (int, dynamic)[] against (T, object)[] correct.
The tests are thorough. They include a metadata-imported T[*] case that C# can't express, which confirms the IsSZArray check.
requiresIdentity is also applied to reference-type elements inside an array. That is stricter than C# array covariance, so string[][] against T[][] is tested as no diagnostic. That holds only because the open branch accepts. Worth keeping in mind when Analyzer misses generic constraint violations inside tuple and array parameters #7520 adds bound inference.
The fallback return true for nested generics is permissive by design, and the tests record it ("Shared-bound inference remains permissive"). That is honest.
Promoted head 53caaf4 after positive Claude and Codex verdicts and verification that every review thread is resolved. The existing scope comment and covariance regression cover the latest non-blocking observations; retaining the current helper structure. Awaiting the ready-for-review Claude run and CI before auto-merge can be armed.
I read the diff but didn't build it or run the tests. My first attempt to run the review skill failed, so this was done by hand.
The fix looks sound and is well scoped. It traverses tuple and array shapes and leaves concrete conversions to Roslyn's ClassifyConversion. It checks rank and IsSZArray, so T[*] metadata arrays are told apart from vectors. It requires identity for elements inside arrays, which matches C# invariance for value-type elements. Nested generic bindings stay permissive, with #7520 as the follow-up. The test matrix is thorough, including the metadata-built T[*] case.
Non-blocking suggestions:
ContainsArray only recurses through tuples, while MatchesOpenArrayShape falls through to return true for other open types. A short comment at that fall-through would explain why List<T[]> stays on the old path.
requiresIdentity is passed down through tuple recursion. That is correct, but a one-line doc comment on the parameter would make the invariance reasoning easier to follow.
The earlier blocker has cleared: #7549 merged as 4891fce, and .NET run 41766395044 completed successfully. Rebased this PR onto that fresh main and pushed head 7d5938d. Range-diff confirms all three analyzer commits retain identical patches, and git diff --check passed.
Validation after rebase: full TUnit.Analyzers.Tests suite on SDK 10.0.403/net10.0 passed (1,064 passed, 1 skipped, 0 failed). Existing warnings remain tracked in #7410 and #7412. Fresh CI and current-head reviews are required before merging; no workflow rerun, on-hold label, or external decision is needed. This PR still delivers #7539 and is not superseded.
The fix is sound and narrowly scoped. It does what the description says: it walks tuple and array shapes, checks rank and SZ-array-ness, and leaves generic constraints and inference to #7520. I read the diff only and did not build or run the tests.
What I checked
The identity requirement for elements of value-type arrays is applied correctly. Tuple fields inside (…)[] are invariant, and the (int,int)[] → (T,long)[] case reports a diagnostic as it should.
The imported T[*] metadata test covers the rank-1 non-vector edge case that C# source can't express. It is a good test, if a bit heavy.
The change adds no new reflection or allocations beyond the Any call in ContainsArray, which only runs for tuple parameter types that contain a type parameter.
Non-blocking suggestions
ContainsArray and ContainsTypeParameter gate entry. Arrays nested inside non-tuple generics, such as List<T[]>, still use the old permissive path. The comment mentions this, so a one-line link to Analyzer misses generic constraint violations inside tuple and array parameters #7520 next to the gate would make the limit obvious to future readers.
MatchesOpenArrayShape ends with return true for unknown nested generics. That is intentional, but a short comment saying that it fails open (so it can only miss diagnostics, never add false ones) would help.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Failure
Method data-source rows such as
(Fixture, CancellationToken, int[])incorrectly receive TUnit0001 when the test accepts(Fixture, CancellationToken, T[]). Traverse tuple and array shapes to accept compatible open elements, checking array rank and SZ-array shape along the way.Roslyn still decides concrete conversions. Tuple fields inside value-type arrays retain identity requirements, and uncertain nested generic bindings remain accepted without a bound-inference engine. Generic tuples without arrays keep their existing behavior; generic constraint work remains separate in #7520.
Validation: 212 analyzer tests passed on net10.0 with SDK 10.0.403: 16 focused tuple/array cases (including imported T[*] metadata), 123 MethodDataSourceAnalyzerTests, 67 ReferenceTupleMethodDataSourceTests, 3 EnumerableMethodDataTupleTypeTests, 2 TupleParameterMismatchTests, and 1 GenericMethodSourceDataTests. Existing NU1902 restore warnings were recorded on #7412.
Closes #7539
Summary by CodeRabbit