Skip to content

fix(engine): use last inferred type for repeated generic parameters - #7134

Merged
thomhurst merged 1 commit into
thomhurst:mainfrom
theluckystrike:fix/7121-reflection-mode-generic-test
Oct 19, 2026
Merged

thomhurst merged 1 commit into
thomhurst:mainfrom
theluckystrike:fix/7121-reflection-mode-generic-test

Conversation

@theluckystrike

@theluckystrike theluckystrike commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Description

Reflection mode inferred a repeated generic type parameter from its first non-assignable argument while naming the test from its last. A test displayed as Repeated<int> could therefore be invoked with T = string and fail its where T : struct constraint. The reflection invoker now takes the last declared argument type for a directly declared generic parameter, matching TestGenericTypeResolver and source-generated mode. Nested inference, such as the element type of a params T[], keeps the previous guard so it cannot replace a more specific leading type.

Related Issue

Fixes #7121

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Performance improvement
  • Refactoring (no functional changes)

Checklist

Required

  • I have read the Contributing Guidelines
  • If this is a new feature, I started a discussion first and received agreement
  • My code follows the project's code style (modern C# syntax, proper naming conventions)
  • I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

  • Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
    • Source Generator path (TUnit.Core.SourceGenerator)
    • Reflection path (TUnit.Engine)
  • Snapshot Tests: If I changed source generator output or public APIs:
    • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
    • I reviewed the .received.txt files and accepted them as .verified.txt
    • I committed the updated .verified.txt files
  • Performance: If this change affects hot paths (test discovery, execution, assertions):
    • I minimized allocations and avoided LINQ in hot paths
    • I cached reflection results where appropriate
  • AOT Compatibility: If this change uses reflection:
    • I added appropriate [DynamicallyAccessedMembers] annotations
    • I verified the change works with dotnet publish -p:PublishAot=true

Testing

Source-generated mode already infers the last occurrence, so only the reflection path needed changing; the fixture is compiled and run in both modes by the existing engine suite.

dotnet test --project tests/TUnit.Engine.Tests -c Release -f net10.0 --treenode-filter "/*/TUnit.Engine.Tests/Issue7121Tests/*"
dotnet test --project tests/TUnit.Engine.Tests -c Release -f net10.0

Additional Notes

The where T : struct row ("1", 2) is the issue repro. Before the fix the test was reported as Repeated<int>(1, 2) but invoked with T = string, throwing ArgumentException ... violates the constraint of type 'T'; after the fix it passes. The _7099 params T[] inference tests are the regression guard for the nested-inference guard.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed generic type reporting for tests that use the same type parameter across multiple arguments. Test results now reflect the inferred type consistently.
  • Tests
    • Added regression coverage verifying that a repeated-type-parameter test completes successfully and reports the expected test name.

Reflection mode named a repeated-type-parameter test after its last
argument type but instantiated the method from its first non-assignable
type. A test shown as Repeated<int> could then run with T = string and
fail its where T : struct constraint. Directly declared generic
parameters now take the last argument type, matching
TestGenericTypeResolver and source-generated mode. Nested inference, such
as the element type of a params T[], still preserves a more specific
leading type so the _7099 fixtures keep passing.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T04:28:33.402183Z cba38d4 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a3ec0b34-e0b3-4802-98bb-3740a98885f4
📥 Commits

Reviewing files that changed from the base of the PR and between ae74298 and cba38d4.

📒 Files selected for processing: 3
  • src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
  • tests/TUnit.Engine.Tests/Issue7121Tests.cs
  • tests/TUnit.TestProject/Bugs/_7121/RepeatedTypeParameterTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Reflection-mode inference now uses the last top-level argument type for repeated direct generic parameters. Nested inference keeps its assignability-based rule. Regression tests check execution and the reported test name for a repeated-type-parameter test.

Changes: Reflection Generic Inference

Layer / File(s) Summary
Top-level generic inference
src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
Direct generic parameter mappings inferred from top-level arguments now use the current argument type. Nested inference retains the existing assignability-based rule.
Repeated parameter regression coverage
tests/TUnit.TestProject/Bugs/_7121/RepeatedTypeParameterTests.cs, tests/TUnit.Engine.Tests/Issue7121Tests.cs
The regression test provides "1" and 2 to repeated T parameters. The engine test checks that the test passes and reports a name beginning with Repeated<int>.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: thomhurst

Merge Risk: ⚪ Minimal · up to cba38

No actionable risk is established for the repeated-parameter inference change; it is mergeable after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: using the last inferred type for repeated generic parameters.
Linked Issues check Passed Issue #7121 requires reflection mode to use the same generic instantiation that it reports. ReflectionTestDataCollector.InferGenericTypeMapping now overwrites a direct generic parameter mapping for …
Out of Scope Changes check Passed The changes stay within issue #7121. The production change fixes reflection generic inference. The added engine test and test-project fixture provide regression coverage for the reported mismatch. The…
Full details: Linked Issues check

Explanation

Issue #7121 requires reflection mode to use the same generic instantiation that it reports. ReflectionTestDataCollector.InferGenericTypeMapping now overwrites a direct generic parameter mapping for each top-level argument, so the last non-null argument selects T. The added ("1", 2) fixture asserts T == int and verifies both converted values. Issue7121Tests verifies one passing test named Repeated&lt;int&gt;.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #7121. The production change fixes reflection generic inference. The added engine test and test-project fixture provide regression coverage for the reported mismatch. The nested-inference condition preserves related existing behavior and does not add unrelated functionality.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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.

❤️ Share

A rabbit checks each type in line,
The last one sets the mapping fine.
Two values hop into the test,
The name and run now match their best.
One green result, then off I go!

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Review

The fix looks correct and is narrow. isTopLevel: true is passed only from the invoker's direct-parameter loop. Nested recursion keeps the default false, so the params T[] guard from #7099 is unchanged. Null and Type.Missing args are already skipped by the caller, so "last non-null" matches the comment. The new fixture reproduces the issue repro (("1", 2) with where T : struct), and the engine test runs in both modes.

Minor, non-blocking suggestions:

  • A bool flag on a recursive helper is a bit of a smell. A small separate step for the top-level case would make the two policies (last-wins vs. most-specific-wins) explicit and avoid the flag. This is optional.
  • Consider one more row, such as (1, "x") with an unconstrained T, to pin down the last-wins behaviour when the earlier type is not assignable.

I couldn't run the tests locally, so this review is based on reading the diff. LGTM.

@greptile-apps

greptile-apps Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Reflection invocation now uses the last non-null argument type for repeated direct generic parameters.

  • Reflection calls use the last direct argument to choose a repeated generic type.

Reviews (1) · Last reviewed commit: "fix(engine): use last inferred type for ..." · Reviewed by Greptile

@thomhurst

Copy link
Copy Markdown
Owner

Thanks!

@thomhurst
thomhurst enabled auto-merge (squash) October 19, 2026 20:14
@thomhurst
thomhurst merged commit 78c9c14 into thomhurst:main Oct 19, 2026
19 checks passed
@theluckystrike

theluckystrike commented Oct 20, 2026 via email •

Copy link
Copy Markdown
Contributor Author

This was referenced Oct 28, 2026

This branch was previously deployed

1 inactive deployment
Pull Requests — cba38d45 Deployed Oct 19, 2026 by theluckystrike via modularpipeline (macos-latest) #20196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reflection mode: generic test with a repeated type parameter is named Repeated<int> but runs with a different type argument

2 participants