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
Extract typed-row binding and verify invocation parity - #7507
Extract the analyzer's closed-signature typed-row binding calculation into TypedRowParameterBinding, next to the linked inference helper. Keep the existing conversion, optional omission and declared-position token rules unchanged.
Add compiled parity coverage that checks expected validation types, inference success versus null in both discovery modes, and concrete results from generated argument expressions and the reflection invoker. Cover omitted optional arguments and trailing arrays, packed and nested arrays, element conversions, loose trailing values, injected versus inferred CancellationTokens, and all five supported params collection interfaces. Compare collection support with both invocation implementations, including unsupported concrete collections and spans.
No actionable comments were generated in the recent review.
Walkthrough
The analyzer now uses TypedRowParameterBinding.GetExpectedTypes to calculate effective parameter types for inferred generic typed rows. New tests compare those types with generated and reflection-based invocation across optional parameters, arrays, params collections, tuples, and cancellation tokens.
Adds a helper that excludes declared cancellation-token parameters, drops eligible omitted trailing parameters, and handles trailing params or SZ-array arguments as elements when they do not bind as a whole.
Integrate and verify typed-row binding src/TUnit.Analyzers/TestDataAnalyzer.cs, tests/TUnit.Core.SourceGenerator.Tests/TypedRowParameterBindingTests.cs
CheckDataGenerator uses the helper instead of inline parameter-type construction. Tests compare inferred types and expected row argument types with generated and reflection-based invocation, and check supported params element types.
thomhurst/TUnit#7460 — Introduced the typed-row inference and inline closed-signature validation that this PR centralizes in a helper.
thomhurst/TUnit#7343 — Introduced typed argument-row inference and analyzer validation for optional and params parameters.
Merge Risk: Minimal · up to 3c52f
No merge-blocking issue was identified in the typed-row binding change. It is mergeable after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 12.50% 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 "Extract typed-row binding and verify invocation parity" matches the main change. The PR moves the analyzer's typed-row parameter binding calculation into a new TypedRowParameterBinding help…
Linked Issues check
Issue #7465 requires a pure helper for the closed parameter-type calculation, regression coverage, and preserved diagnostics and execution behavior.
Helper: The new file src/TUnit.Analyzers/Extension…
Out of Scope Changes check
The whole-PR diff against the merge base b5561fb changes three files: TestDataAnalyzer.cs (extraction and call-site replacement), the new TypedRowParameterBinding.cs (extracted helper), and the new T…
Full details: Title check
Explanation
The title "Extract typed-row binding and verify invocation parity" matches the main change. The PR moves the analyzer's typed-row parameter binding calculation into a new TypedRowParameterBinding helper, and adds tests that compare expected validation types with generated and reflection-based invocation results. The title is specific and concise, and it names both the refactor and the added parity coverage.
Full details: Linked Issues check
Explanation
Issue #7465 requires a pure helper for the closed parameter-type calculation, regression coverage, and preserved diagnostics and execution behavior.
Helper: The new file src/TUnit.Analyzers/Extensions/TypedRowParameterBinding.cs adds GetExpectedTypes. Its logic matches the inline code removed from TestDataAnalyzer.CheckDataGenerator. It covers optional and defaulted trailing parameters, trailing params and SZ-array elements, the single-array-as-element rule, and CancellationToken removal by declared signature. It also adds GetParamsElementType for the five supported collection interfaces and SZ arrays. The helper only reads symbols and queries conversions, so it is pure in the sense the issue requires.
Analyzer call site: TestDataAnalyzer.CheckDataGenerator now calls TypedRowParameterBinding.GetExpectedTypes with the same arguments. CheckInferredTypeArgumentConstraints still runs before this call, so constraint diagnostics are unchanged. The diff does not touch invalid-element diagnostics.
Coverage: TypedRowParameterBindingTests covers the requested cases. These are omitted optional and trailing arrays, packed and nested arrays, element conversions, loose trailing values, injected and inferred CancellationTokens, and all five params collection interfaces, gated to NET9_0_OR_GREATER. Generated invocation and the reflection invoker are checked against the expected types. Collection support is compared with both invocation implementations, including unsupported List, HashSet, Span, and ReadOnlySpan inputs.
Limits: I did not run the tests. The reported counts (30, 17, 41 passed) are author-reported. The new tests call private helpers by reflection. This coupling is a maintenance risk, but it does not affect this verdict.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against the merge base b5561fb changes three files: TestDataAnalyzer.cs (extraction and call-site replacement), the new TypedRowParameterBinding.cs (extracted helper), and the new TypedRowParameterBindingTests.cs (parity tests). Each change supports issue #7465. The extraction removes inline logic and adds a call to the helper, with no behavior change. The tests cover the objectives. The PR does not change generator output, public API, reflection discovery, or diagnostic messages. No change lacks a connection to the issue. Related issues #7386, #7371, #7410, and #7412 are context only and do not add requirements.
Full details: Docstring Coverage
Explanation
Docstring coverage is 12.50% 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 8 functions across 3 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 row with care Optional values vanish there Arrays keep their shape or spread Tokens find their proper thread Tests compare each route ahead And nibble on the code instead
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
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: a7c41ba9-7882-4d4d-913b-c890a6bfca35
Commits
Reviewing files that changed from the base of the PR and between b5561fb and 3c52f4e.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.
This is a behaviour-preserving extraction. I compared the diff to the code removed from TestDataAnalyzer.CheckDataGenerator: GetExpectedTypes and GetParamsElementType are moved verbatim, with context.Compilation and typeArguments replaced by parameters. The call site still calls method.Construct(inferred) only after CheckInferredTypeArgumentConstraints, so the ordering is unchanged. I didn't build or run the tests.
The new parity coverage is valuable. It compiles real code, emits it, and compares the analyzer's expected types with what the generated argument expressions and the reflection invoker actually do.
Suggestions, none of them blocking:
Reflection lookups by string name. The tests find internal members with GetType("...") and GetMethod("..."): GetExpectedTypes, GetParamsElementType, GetTrailingElementType, InferTypesFromTypedRow, CreateReflectionTestInvoker, InferMethodTypeArgumentsFromRowType and GetRowArgumentTypes. A rename will fail at runtime with a null-forgiving NullReferenceException instead of a compile error. If the test project can get InternalsVisibleTo for the analyzer, call the analyzer helpers directly (the test project already references TUnit.Analyzers, since it uses TestDataAnalyzer). For the engine, add one small helper that wraps the lookup and throws a message naming the missing member, so a failure says what was renamed.
Helper visibility.GetParamsElementType is private and exists in three copies (analyzer, TupleArgumentHelper.GetTrailingElementType, and the engine's reflection collector). The Collecting_element_types_match_both_invocation_implementations test guards against drift between them, which is good. Longer term, consider whether the analyzer and generator can share one implementation, since they already share the linked inference helper. That would make the parity test unnecessary for those two.
Test readability. The Cases() rows carry eight positional strings and a bool. A small record or named-tuple type would make the table easier to review and extend. The #if NET9_0_OR_GREATER branch also means the params-collection cases only run on some target frameworks. Please confirm CI covers one of them.
Overall this looks good to merge once CI is green.
[Low impact] The PR appears safe to merge; the previous test weakness is fixed and no new actionable issues were found.
Summary
The PR extracts the analyzer’s existing typed-row binding logic into TypedRowParameterBinding and adds compiled checks against generated and reflection calls.
Typed-row checks now use a dedicated binding helper.
Pushed 3c52f4e to address the token identity coverage finding: generated and reflection invocation now distinguish injected, scalar-row and array-element tokens by CancellationToken equality. All 30 TypedRowParameterBindingTests passed on net10.0; git diff --check passed.
Disposition of Claude's nonblocking suggestions:
Keeping reflection for this extraction: the tests exercise internal/private helpers across assemblies, including the engine. Broadening visibility or refactoring lookup diagnostics is optional test maintenance and does not affect the asserted binding results.
Keeping the three collection-type implementations separate here preserves the existing production behavior. This PR adds parity coverage specifically to detect drift; sharing more production code is outside this behavior-preserving extraction.
Keeping the existing MethodDataSource rows avoids a separate test-data refactor. CI coverage is confirmed: RunSourceGeneratorTestsModule uses ModernAndNetFrameworks in TestBaseModule, which includes net10.0 and also net9.0 on Linux. The conditional params-collection cases therefore run in CI; they also passed locally on net10.0.
The draft remains awaiting fresh reviews for the new head.
The extraction in TypedRowParameterBinding is a faithful move of the analyzer logic. I compared the removed block with the new class line by line, including the declared-position trailing-parameter handling and the GetParamsElementType collection set. I found no behavior change. Having TestDataAnalyzer call one helper next to TypedDataSourceInference is a clear improvement.
The parity tests cover the cases the PR lists, and they compare analyzer, generated-code and reflection-invoker results against each other.
Suggestions, none blocking:
Reflection into internals. The tests look up GetExpectedTypes, GetParamsElementType, InferTypesFromTypedRow, GetTrailingElementType and CreateReflectionTestInvoker by string name. A rename turns these into runtime NullReferenceExceptions instead of compile errors. InternalsVisibleTo for the test assembly would let the analyzer helper be called directly. The engine types are probably harder to reach, so a shared nameof-based accessor or a helper that fails with a clear message when a lookup is null would at least make failures readable.
GetParamsElementType is private but tested. The test reaches it with BindingFlags.NonPublic. Making it internal documents that it is part of the parity contract.
Long test method.Expected_types_agree_with_generated_and_reflection_invocation runs analyzer, source-gen and reflection checks in one body. Splitting it into a shared fixture plus one assertion group per path would make a failure point to the diverging implementation sooner.
#if NET9_0_OR_GREATER cases. The five params collection interface cases only run on net9.0 and later. Check CI still exercises them on a TFM that satisfies this.
Promoted to ready at unchanged head 3c52f4e after positive Claude and Codex verdicts and resolution of the token-identity finding.
Disposition of the current Claude suggestions: retain the existing reflection-based test access and private helper visibility for this behavior-preserving extraction; these helpers are implementation details, and changing their visibility is unnecessary for parity coverage. Retain the combined parity test so each case compares all implementations together; splitting assertion groups is optional readability work. The net9.0/net10.0 CI coverage for the conditional collection cases is confirmed in the earlier disposition above.
Promotion triggers a fresh Claude review and full CI. Auto-merge must wait for the promotion Claude run to pass and its comment to be read; this single unit does not wait for runs.
This is a behaviour-preserving extraction: the closed-signature binding logic moves from TestDataAnalyzer.CheckDataGenerator into TypedRowParameterBinding.GetExpectedTypes, and GetParamsElementType moves with it. I compared the moved code line by line with the removed code. The only change is that context.Compilation and typeArguments become the compilation and argumentTypes parameters. The call site passes method.Construct(inferred) as before, so the diagnostics should not change.
The parity tests are a real improvement. They emit the generated argument expressions and run them, then run the reflection invoker, and compare both against the analyzer's expected types. That is the right way to keep the three implementations (analyzer, generator, reflection) in sync.
Suggestions, none blocking:
The tests reach private and internal members through string-based reflection (GetMethod("GetExpectedTypes"), GetTrailingElementType, InferTypesFromTypedRow and similar). A rename will fail at runtime with a null-reference error, not at compile time. If TUnit.Core.SourceGenerator.Tests can see the analyzer's internals (for example via InternalsVisibleTo), calling TypedRowParameterBinding.GetExpectedTypes directly would be sturdier. You could also give the GetMethod calls a shared helper that asserts non-null with a clear message.
Expected_types_agree_with_generated_and_reflection_invocation is long and mixes four concerns: inference, analyzer expected types, generated invocation and reflection invocation. Splitting it into helpers, or into separate assertions per mode, would make a failing case easier to diagnose. The case tuples are also positional (eight untyped fields), so a small record or named type would be more readable.
The NET9_0_OR_GREATER guard around the params collection cases means the five-interface coverage only runs on net9 and later. That seems intended, but please confirm CI actually exercises it.
I did not run the tests locally. Overall this looks good to merge, assuming CI passes.
Run 41657938516 completed successfully on head 3c52f4e; the earlier wait has ended. All current CI checks, including the required OS-specific engine and AOT checks, pass. Codex reports a completed review with no findings for this same head, and the promotion Claude review is positive.
The promotion review repeats the already-dispositioned nonblocking suggestions: retain private reflection-based test access and the combined per-case parity assertions for this behavior-preserving extraction. The net9/net10 CI coverage of conditional params-collection tests is confirmed in the previous disposition. CodeRabbit's docstring-coverage warning is nonblocking: the extracted helper already has an XML summary and explanatory comments, and the remaining touched functions are test coverage or existing code. Adding blanket docstrings is unnecessary for this extraction. CodeRabbit and Greptile report no remaining actionable findings; the token-identity thread is fixed and resolved with no later reviewer reply.
Proceeding through the repository merge gate on the unchanged head.
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}
Extract the analyzer's closed-signature typed-row binding calculation into
TypedRowParameterBinding, next to the linked inference helper. Keep the existing conversion, optional omission and declared-position token rules unchanged.Add compiled parity coverage that checks expected validation types, inference success versus null in both discovery modes, and concrete results from generated argument expressions and the reflection invoker. Cover omitted optional arguments and trailing arrays, packed and nested arrays, element conversions, loose trailing values, injected versus inferred CancellationTokens, and all five supported params collection interfaces. Compare collection support with both invocation implementations, including unsupported concrete collections and spans.
Closes #7465
Validation (SDK 10.0.403, net10.0, macOS arm64):
git diff --checkpassed.No generator output, shipped public API, reflection path or discovery behavior changes. Existing build warnings are tracked in #7410 and #7412.
Summary by CodeRabbit
paramsor array arguments.