[thomhurst]

Nullable generic class test parameters now accept compatible method data and remain discoverable. Analyzer validation honors explicit, constructor-inferred and inherited constructor types, while method rows still validate constraints when constructor arguments supply only values.

Both discovery modes infer class types from compatible method rows, partition sources by inferred types, and retain uninferred sources on explicitly supplied classes. Constructor-supplied types take precedence, including nullable parameters nested in arrays or generic types. Partial constructor type sets retain the existing row-value fallback when it closes the class; otherwise method rows infer the closed class and supply their constructor values in both modes. Repeated class type parameters use the last matching occurrence in both modes. Inference excludes unresolved generic rows, generic source methods, argument-taking sources and external generic providers.

Regression coverage includes null, factory, tuple and array rows, multiple sources, overloads, explicit types, type-free constructors and nested nullable constructor precedence. STAThread snapshots are refreshed after #7594 reduced repeats; the net472 update was reviewed from the Windows CI artifact.

Validation:

• All 31 TUnit0001 rejects an int method source for a nullable generic class parameter #7573 execution cases pass in source-generated and reflection modes.
• Full net10.0 generator suite: 340 passed, one skipped; snapshot changes reviewed.
• Method-data analyzer tests: 142 passed. Full analyzer suite: 1,120 passed, one skipped.
• Earlier validation: Native AOT publish and all 22 then-existing cases passed; 16 existing passing Generic class with a class [Arguments] row that infers no type argument: modes disagree on T and constructor values, source-gen drops [Arguments(null)] #7209 cases passed in each mode; netstandard2.0 Engine build passed without warnings/errors.
• Latest follow-up: partial constructor rows pass in both modes; all 16 expected-pass Generic class with a class [Arguments] row that infers no type argument: modes disagree on T and constructor values, source-gen drops [Arguments(null)] #7209 cases pass in each mode. Native AOT publish succeeded and all 31 TUnit0001 rejects an int method source for a nullable generic class parameter #7573 cases passed in the native executable. Existing fixture/dependency trim warnings remain. Engine tests remain for CI per repository instructions. Existing warning debt is tracked in Analyzer and code-fixer projects emit release-tracking and nullable build warnings #7410, Mock and core generator test restores report vulnerable System.Security.Cryptography.Pkcs versions #7412, Generated instance hooks emit IL2075 during Native AOT publishing #7430 and Analyzer test restore reports duplicate Microsoft.NETCore.App.Ref PackageDownload items #7585.

Closes #7573

Summary by CodeRabbit

• New Features
  • Generic test classes can now infer type arguments from method-data rows, including nullable values and arrays.
  • Data rows are better matched with class constructors, explicit generic types, and other data sources.
• Bug Fixes
  • Improved handling of nullable generic parameters, constraints, and overloaded data-source methods to ensure valid test cases are discovered and invoked.

Review follow-up: preserved partial-constructor row-value fallback in both collectors and added repeated-occurrence parity coverage. All 33 #7573 execution cases pass in source-generated and reflection modes. Full net10.0 generator suite: 340 passed, one skipped; the snapshot adds only the two intended regression entries. Engine tests remain for CI.

Native AOT publishing succeeded and all 33 cases also pass in the native executable. Existing fixture/dependency trim warnings remain.

Review follow-up: nullable constructed struct arrays now infer identically in both modes; partial constructor row-value fallback is used only when it satisfies class constraints. All 35 #7573 cases pass in generated and reflection execution. Full net10.0 generator suite: 340 passed, one skipped; both new snapshot entries reviewed. Native AOT publish succeeded and all 35 cases pass in the native executable.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Walkthrough

The analyzer, source generator, and reflection discovery now handle nullable generic class parameters and method-data rows in additional inference and source-selection cases. Tests cover analyzer validation, generated metadata, and runtime discovery scenarios. STA verified snapshots also record updated repeat counts and source line numbers.

Changes: Generic Method-Row Inference

Layer / File(s)	Summary
Nullable analyzer inference and validation src/TUnit.Analyzers/Extensions/CompilationExtensions.cs, src/TUnit.Analyzers/TestDataAnalyzer.cs, tests/TUnit.Analyzers.Tests/MethodDataSourceAnalyzerTests.cs	The analyzer now normalizes nullable class parameter types during inference and binding checks. Tests cover convertible data, independently supplied class types, constraint failures, peer inference, and overload matching.
Generated class-row inference and source selection src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs, tests/TUnit.TestProject/Bugs/_7573/NullableClassMethodRows.cs, tests/TUnit.Core.SourceGenerator.Tests/NullableClassMethodRowsTests.cs, tests/TUnit.Core.SourceGenerator.Tests/NullableClassMethodRowsTests.Generates_Nullable_Class_From_Method_Rows.verified.txt	The source generator uses eligible method-data row types to infer generic class types and scopes rows to matching types. It also retains sources when class types are supplied through other data or explicit generation. Regression tests and a generated snapshot cover nullable, array, tuple, external, instance, and untyped sources.
Reflection discovery from method rows src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	Reflection discovery now infers closed generic class types from eligible method-data row types and associates inferred rows with matching class instantiations.

STA Generated Snapshot Refresh

Layer / File(s)	Summary
Updated STA verified snapshots tests/TUnit.Core.SourceGenerator.Tests/STAThreadTests.Test.DotNet10_0.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/STAThreadTests.Test.DotNet8_0.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/STAThreadTests.Test.DotNet9_0.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/STAThreadTests.Test.Net4_7.verified.txt	The snapshots change repeat counts from 100 to 10 and update recorded source line numbers. The .NET 8, .NET 9, and .NET 10 snapshots also gain a UTF-8 BOM.

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

• thomhurst/TUnit#7224 concerns reflection inference from method-level data sources. This PR adds that inference path.
• thomhurst/TUnit#7377 describes drift among inference paths. This PR changes analyzer, source-generator, and reflection behavior, but does not add the requested shared helper.
• thomhurst/TUnit#7390 concerns type-free class rows with other data sources. This PR changes method-source partitioning and type-free row handling.
• thomhurst/TUnit#7597 concerns generic class discovery from method-source rows, including partitioning and tuple handling. This PR implements related inference and partitioning changes.
• thomhurst/TUnit#7445 concerns constructor-row semantics and source partitioning. This PR changes generic class method-source partitioning, but does not establish null-collection behavior.
• thomhurst/TUnit#7596 concerns row binding and constructor partitions for generic class method-source inference. This PR changes related discovery partitioning, but may not cover every runtime binding case.

Merge Risk: Low · previous review 51e59

Invalid method data can miss an expected analyzer diagnostic. This is a bounded issue, suitable for follow-up if accepted.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The four STAThreadTests verified snapshots change repeat counts from 100 to 10 and update line metadata. These changes address an independent repeat-count change after #7594 and have no connection t…	Remove the unrelated STAThreadTests.Test.*.verified.txt changes from this pull request, or move them to a separate pull request.
Docstring Coverage		Docstring coverage is 31.58% 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: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "Fix nullable generic class method data source validation" names the main change. The PR fixes analyzer validation (TUnit0001) for nullable generic class parameters (T?) when method data sou…
Linked Issues check		Issue #7573 requires TUnit0001 to accept IEnumerable<int> method data for T? on Tests<T> where T : struct, and to include analyzer coverage. CheckMethodDataSource now invokes nullable class ty…

Full details: Title check

Explanation

The title "Fix nullable generic class method data source validation" names the main change. The PR fixes analyzer validation (TUnit0001) for nullable generic class parameters (T?) when method data sources supply compatible types. The changeset updates CompilationExtensions.InferTypeArguments and CanBindToTestParameter, TestDataAnalyzer, and adds analyzer tests. It also changes source-generator and reflection discovery for the same scenario, which the title does not mention. Linked issue #7573 describes this same validation failure, so the title matches the stated objective.

Full details: Linked Issues check

Explanation

Issue #7573 requires TUnit0001 to accept IEnumerable<int> method data for T? on Tests<T> where T : struct, and to include analyzer coverage. CheckMethodDataSource now invokes nullable class type inference. CanBindToTestParameter compares the inferred underlying type and validates constraints. The tests Nullable_Generic_Class_Parameter_Accepts_Value_Data cover int and int? rows. Additional tests cover explicit, constructor-inferred, inherited, conflicting, and constraint-invalid cases. The source-generator and reflection changes preserve discovery for these rows.

Full details: Out of Scope Changes check

Explanation

The four STAThreadTests verified snapshots change repeat counts from 100 to 10 and update line metadata. These changes address an independent repeat-count change after #7594 and have no connection to issue #7573 or nullable generic class method data. The nullable method-row source-generator and reflection changes are connected to discovery parity and are in scope.

Resolution

Remove the unrelated STAThreadTests.Test.*.verified.txt changes from this pull request, or move them to a separate pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 31.58% 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 19 functions across 7 files. (5 skipped: 5 unsupported.)

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each row at dawn
And finds the types that fit
Nullable paths now join the trail
While sources sort and split
The snapshots count to ten
Then hop away content}

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-04T21:54:05.814249Z	36c8723	Manual request

ℹ️ 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.

[github-actions]

Review

The fix is small and targeted, and the tests cover it well.

• Fix: HasImplicitConversionOrGenericParameter now handles a parameter of type Nullable<T> where T is a class-level type parameter. It unwraps Nullable<> from the argument type and checks the underlying type against T's constraints with CanSatisfyConstraints. The DeclaringMethod: null check keeps method-level generics on their existing inference path.
• Tests: they cover int and int? rows (accepted) and a string row or an unsatisfied IDisposable constraint (rejected). That confirms the fix doesn't just turn the diagnostic off.
• Scope: only the analyzer changes, with no runtime or generator output changes, so the dual-mode metadata rule doesn't apply.

Two optional notes, neither blocking:

1. The new branch returns early, so a parameter like T? never reaches the later array and type-parameter inference logic. That looks fine, because Nullable<T> can't contain arrays. A short comment saying so would help the next reader.
2. A row type of int? for where T : struct is accepted. Make sure the runtime handles null rows the same way, or the analyzer is more lenient than discovery. The PR states discovery infers the underlying type from either T or T?, so this is probably consistent.

LGTM.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06ef942217

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Confidence Score: 4/5

[Medium impact] The changes affect generic test discovery and data validation across TUnit's build-time and runtime paths.

The PR needs a matching constructor-precedence check before merging.

Findings

1.  Constructor types get overridden ▶

Summary

This PR lets nullable generic class parameters use compatible method data in both discovery modes.

• Compatible method rows can now close nullable generic test classes.
• The analyzer checks nullable generic method rows against the class type.
• STA thread snapshots now reflect the lower repeat count.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Constructor row] --> B{Can its types close the class?}
  B -->|Yes| C[Keep constructor-supplied types]
  B -->|No| D[Infer class types from method rows]
  D --> E[Attach constructor values]
  C --> F[Run matching method sources]
  E --> F

Loading

_{Reviews (16) · Last reviewed commit: "Validate partial constructor fallback co..." · Reviewed by Greptile}

[thomhurst]

@codex review

Fixed both constraint-validation findings in 6e85ddb. All 129 method data source tests pass on net10.0, including int and int? explicit-instantiation regressions that reproduced TUnit0001 before the fix. The separate mutable-reference warning is tracked in #7598 and explicitly expected by these regressions.

Claude's optional array-path comment does not require a change: Nullable requires a value type, and the branch is restricted to that shape. The nullable-row runtime concern is a follow-up observation rather than a blocking finding; this change only preserves the analyzer's existing acceptance of int? rows while adding non-nullable support. No runtime conversion code changes.

Keeping the PR draft pending fresh Claude and Codex reviews of this head.

[github-actions]

Review

The fix is focused and correct. It adds an early branch in HasImplicitConversionOrGenericParameter for Nullable<T> parameters whose T is a class-level type parameter. The branch unwraps a nullable argument type and validates the underlying type against T's constraints through the existing CanSatisfyConstraints helper. It also reuses HasGenerateGenericTestAttribute for the explicit-instantiation case.

• The tests cover int, int?, a reference type, an unsatisfied interface constraint, and the [GenerateGenericTest] path. That covers the main behaviours.
• The DeclaringMethod: null pattern correctly keeps method-level type parameters on their existing path.
• I did not build the PR or run the tests. I only read the diff.

Two non-blocking suggestions:

1. The nullable-unwrapping pattern (Nullable<T> → T) now appears here as well as elsewhere in the analyzers. A small shared helper such as UnwrapNullableValueType() would cut the duplication and keep the semantics consistent.
2. In the [GenerateGenericTest] test, the analyzer reports ReturnFunc on the method rather than accepting the row silently. If that is intended, a short comment in the test would help the next reader.

No blocking issues. LGTM.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e85ddb73c

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

Fixed constructor-inferred nullable class type handling in 117ba5f. All 131 method data source tests pass on net10.0; both new regressions reproduced TUnit0001 before the fix. Resolved the finding with a commit-linked reply.

Claude's optional test-comment suggestion is addressed: the expected TUnit0046 warning is explained and linked to #7598. Leaving nullable unwrapping local keeps this focused fix small; a shared helper is optional and does not change behavior.

Keeping this draft pending fresh reviews of the current head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 117ba5f927

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

Fixed the repeated nullable inference and data-source overload findings in 9624fd6, with commit-linked replies and resolved threads. Both reported regressions failed before the fix. All 134 method data source tests pass; the full net10.0 analyzer suite passes (1,112 passed, one skipped). Existing build/restore warning debt is tracked by #7410, #7412 and #7585. Keeping this draft pending fresh reviews.

[github-actions]

Review of #7591

The change looks correct and well scoped. It is analyzer-only, with no runtime or generator impact. The new tests cover the paths I traced: explicit [GenerateGenericTest] types, constructor-supplied types, conflicting and certain repeated inference, and constraint violations.

What I checked

• Nullable class parameters (T? where T is a class-level type parameter and T : struct) now unwrap to T for constraint checks, and an int? row unwraps to int. This matches the discovery behavior the PR describes.
• When T is not in inferredTypeArguments, binding is accepted. That is the conservative choice, since the generator converts values to the final inferred type.
• Bare class type parameters keep their deferred validation, so there is no regression there.

Suggestions (non-blocking)

• The Nullable<T>-of-class-type-parameter pattern is written out three times: in CanBindToTestParameter, in InferTypeArguments, and in TestDataAnalyzer. A small helper such as TryGetNullableClassTypeParameter(ITypeSymbol, out ITypeParameterSymbol) would keep the three sites in step if the rule changes. The same goes for the int? to int unwrap, which appears twice.
• In InferTypeArguments, parameterType is reassigned inside the loop after the nullable check. That works, but it makes the later isNullableClassParameter branch harder to follow. A separate local such as effectiveParameterType would be clearer.
• There is no test for the unconstrained-reference case (class Tests<T> with T? value, where T? is just an annotation and not Nullable<T>). It should be unaffected, but one test would pin that down.

Nothing here blocks the merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9624fd62bb

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

Fixed the independently supplied class type / peer constraint findings in e457634, with commit-linked replies and resolved threads. Four new explicit/constructor and int/int? regressions reproduced TUnit0001 before the fix. All 138 method data source tests pass; full net10.0 analyzer suite: 1,116 passed, one skipped. Existing warnings remain tracked by #7410, #7412 and #7585.

Claude's helper and naming suggestions are optional cleanup; the shared independently-supplied guard now keeps binding and inference aligned. The unconstrained annotated-reference case uses the existing bare-parameter path, which this change preserves. Keeping this draft pending fresh reviews of the new head.

[github-actions]

Review of #7591

The fix looks correct and well scoped. I found no blocking issues.

• Behaviour: T? class parameters with struct constraints now accept both T and T? row data. Constraint checks only run when the inference is certain. Independently supplied class types ([GenerateGenericTest] or constructor data sources) are excluded, so peer method constraints are not checked against the wrong type.
• Callers: InferTypeArguments gained a Compilation parameter. The only call site in the PR head is TestDataAnalyzer.cs, and it is updated.
• Tests: the new cases cover int and int?, explicit and constructor-inferred types, conflicting and certain repeated inference, overload matching, and rejection of constraint violations.

Suggestions (non-blocking):

1. The pattern INamedTypeSymbol { OriginalDefinition.SpecialType: System_Nullable_T, TypeArguments: [ITypeParameterSymbol { DeclaringMethod: null }] } appears three times, in CanBindToTestParameter, InferTypeArguments and TestDataAnalyzer. A small helper such as TryGetNullableClassTypeParameter(ITypeSymbol?, out ITypeParameterSymbol) would keep the definition in one place.
2. Nullable unwrapping of the argument type is done in two places. Moving it into the same helper (or a GetUnderlyingNullable extension) would avoid the two copies drifting apart.
3. HasIndependentlySuppliedClassType re-reads attributes on each call. This is fine for an analyzer, but the check could be computed once per type parameter if it ever shows up in profiles.

Overall this is a good fix.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: e457634095

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Both Claude and Codex completed positive reviews of e457634; all review threads have commit-linked dispositions and are resolved. The latest helper extraction and attribute caching suggestions are optional cleanup, so the focused fix remains unchanged; caching should follow profiling evidence. Promoting this head to ready. Auto-merge must wait for the new ready_for_review Claude run to pass and its comment to be read.

[thomhurst]

Ready promotion confirmed with unchanged head e457634. Waiting for ready_for_review Claude run 41887370518: https://github.com/thomhurst/TUnit/actions/runs/41887370518 . Next unit should read its review and refreshed review surfaces, then arm auto-merge if the gate passes. CI is now starting; no on-hold label is needed.

[github-actions]

Review of #7591: nullable generic class method data source validation

I found no blocking issues.

• Fix: T? class parameters with value-type constraints now bind to int and int? row data. Independently supplied class types ([GenerateGenericTest] or constructor data) skip the row constraint check. Repeated occurrences are only checked when the inferred type is certain. This matches the generator's behaviour.
• API change: InferTypeArguments gained a Compilation parameter. The only caller, in TestDataAnalyzer.cs, is updated in this PR.
• Tests: they cover int, int?, explicit and constructor-supplied types, conflicting and certain inference, peer method constraints and overload matching. I only read the diff and did not run the tests.

Suggestions (non-blocking):

1. The Nullable<T> with class type parameter pattern is written out three times: in CanBindToTestParameter, InferTypeArguments and the TestDataAnalyzer condition. A small helper, for example TryGetNullableClassTypeParameter(ITypeSymbol?, out ITypeParameterSymbol), would keep the three sites from drifting apart.
2. The same applies to unwrapping argumentType from Nullable<T>, which is written twice.
3. HasIndependentlySuppliedClassType calls GetAttributes().Any(...) on each invocation. It runs per row parameter, so caching it per type parameter would be cheap if it ever shows up in analyzer profiles.

I read the diff only, so I did not check how this behaves on larger code bases.

Overall this is a good, well-tested fix.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e457634095

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

Fixed both new findings in 7b441ea, replied with evidence and resolved their threads. Earlier run 41911277540 passed, so that blocker is gone. Nested nullable constructor precedence and type-free constructor analyzer constraints are now covered by regressions that failed before the fix.

All 30 #7573 cases pass in each discovery mode. Generator suite: 340 passed/one skipped. Analyzer suite: 1,120 passed/one skipped (142 method-data cases). Reviewed the verified snapshot; only the constructor-supplied double class is emitted. No reflection-path changes; AOT was not republished. Existing warning debt remains tracked in #7410, #7412, #7430 and #7585.

Claude’s shared-helper, parity-table, caching, naming and docstring observations remain optional cleanup. This fix preserves the existing non-nullable inference policy and existing source exclusions. The PR body now consolidates scope and validation. Keeping this draft pending fresh reviews of this head.

[github-actions]

Review of #7591 (nullable generic class parameters with method-data rows)

I read the diff but did not build or run the tests. I found no blocking bugs. The approach is coherent: both discovery modes use the same rules to infer class types from declared row shapes, and constructor-supplied types take precedence.

What I checked

• The analyzer, the source generator and reflection discovery follow the same rules for excluding generic source methods, argument-taking sources and external providers. Reflection mode uses DeclaredOnly lookup, and the generator now only considers members declared on the class.
• Nullable class parameters are handled consistently. The analyzer, ProcessTypeForGenerics and the reflection matcher all treat T and T? data as inferring T.
• Reflection mode guards its trimming-sensitive paths with UnconditionalSuppressMessage, and the #if NET fallback for netstandard2.0 looks fine.
• Test coverage is broad: null, factory, tuple and array rows, overloads, explicit types and constructor precedence, in both modes.

Design concerns (non-blocking)

1. The same rule is written three times. ClassRowSuppliesNoTypeArguments exists in the analyzer and in the generator. The analyzer copy carries a comment saying "keep in step with" the generator. Reflection mode has its own version of the same rule. A shared helper would remove that drift risk. GenericMethodInferencePolicy and GenericClassInferencePolicy already do this for other rules, so a similar policy class for "does a class row supply type arguments" would fit. Without it, a future change to the constructor-row rule can silently diverge between analyzer and generator, and you'd only see it as a spurious or missing diagnostic.
2. The nullable-unwrap pattern is duplicated. The Nullable<T> with a class type parameter check appears in CanBindToTestParameter, InferTypeArguments, the analyzer call site and ContainsNullableClassParameter. A small TryGetNullableClassTypeParameter(ITypeSymbol, out ITypeParameterSymbol) extension would make the intent clearer and shrink the call sites.
3. scopesClassMethodSources makes CollectConcreteInstantiations harder to follow. It is already a very large method. Behaviour now depends on this flag, on uninferredMethodDataSources and on typedDataSourceBucket. Extracting the source-partitioning step into its own method with a short doc comment on the invariants would help the next maintainer.
4. InferTypeArguments now takes a Compilation. That is only needed for the attribute check. If the independently-supplied check were computed once per test class and passed in, the helper could stay a pure function of symbols.

Process notes

• The PR says reflection paths are unchanged, but ReflectionTestDataCollector.cs has new reflection code. The PR description's AOT note is therefore not accurate for this change. Please re-run the Native AOT publish check, or rely on CI for the trim warnings.
• Updating the STAThread snapshots is fine, but they belong in a separate commit or a separate note so the semantic diff stays reviewable.

Overall this looks good to merge once CI is green, including the Engine tests that can't be run locally.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 7b441ea21d

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

Fixed the new partial-constructor finding in 8eda804 and resolved its thread with before/after evidence. Complete constructor type sets retain precedence; partial sets allow method inference and retain constructor values in both discovery modes. All 31 #7573 cases pass generated, reflection and native AOT execution; Native AOT publish succeeded. Generator suite: 340 passed, one skipped. All 16 expected-pass #7209 cases pass in both modes. Existing warning debt is unchanged and remains tracked by the issues listed in the PR body.

Earlier waiting run 41911277540 succeeded. No open prerequisite, superseding PR, stale linked issue or owner-reserved decision prevents this PR; it still delivers #7573. Keeping the new head in draft pending fresh reviews. Optional earlier helper extraction, caching and naming observations remain follow-up cleanup.

[github-actions]

Review of #7591. I read the analyzer, generator and reflection-collector changes; I did not build or run anything. The change is coherent and handles the dual-mode requirement well: the generator and reflection collector get matching changes (partial-row handling, ContainsTypeParameter, method-source inference, DeclaredOnly and non-generic-method restrictions), and the regression coverage across both modes is broad. Concerns: (1) Possible cross-mode divergence when one class type parameter matches several row positions. Reflection's MatchClassRowType is last-wins (match = candidate, and ?? match for generic arguments). The generator goes through ProcessTypeForGenerics and genericParamMap; I only saw its call sites, so I don't know its tie-break. If it is first-wins or widening, a method like Test(T a, T? b) fed (int, long) would infer different class types per mode. Please add a conflicting-occurrence test in both modes, ideally by moving the tie-break rule into the existing GenericClassInferencePolicy so parity is structural. (2) In TestMetadataGenerator, the predicate 'class-level data source that is not a type-free [Arguments] row' is written out twice (scopesClassMethodSources and the nullable-constructor-precedence check), and includePartialRows: true must stay in step in both. A small helper such as ClassSuppliesAllTypeArguments would keep them from drifting. (3) The includePartialRows flag changes the meaning of ClassRowSuppliesNoTypeArguments to 'supplies no or not all type arguments', in both the generator and reflection copies. A separately named method such as ClassRowLeavesTypeArgumentsUninferred would read more clearly than a boolean that flips semantics. (4) Minor: reflection inference uses LINQ (parameters.Where(...).ToArray()) per source during discovery; please confirm it is not on a hot path or cache it per method. The #if NET / #else GetMethod lookup could be one loop with a single code path to test. None of this is blocking; (1) is the one I would want answered before merge because it is a cross-mode correctness risk. I am relying on CI for the Engine tests.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 8eda8047cc

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

Fixed Greptile's partial-constructor fallback finding in 32d2a95, with a before/after reproduction and paired generator/reflection coverage. The thread has a commit-linked disposition.

Claude's repeated-occurrence concern is answered by the new LastOccurrenceInfersClassType fixture: Run(T first, T? second) receives (int, long), infers long, and passes in both modes. ProcessTypeForGenerics assigns the dictionary entry on each match; MatchClassRowType and its caller likewise retain the last match. The reviewed snapshot pins the long instantiation. No inference-policy change is needed. Helper extraction and renaming are optional cleanup. Caching should follow profiling; this patch makes no performance claim and retains the existing discovery path.

All 33 #7573 cases pass generated and reflection execution; full net10.0 generator suite: 340 passed, one skipped. Engine tests remain for CI. Existing warnings remain tracked by the issues in the PR body.

Earlier run 41911277540 succeeded. No prerequisite, superseding PR, stale linked issue, finished parent or reserved decision prevents #7591; it still closes #7573. Keeping the new head draft for fresh reviews. This is pushed work awaiting review, not a deferral.

Native AOT publishing succeeded and all 33 cases also pass in the native executable. Existing fixture/dependency trim warnings remain.

[github-actions]

Review of #7591

I read the file list, the description and the diff structure. I did not trace every hunk or run the tests, so this is a partial review.

What looks good

• The change covers all three layers: the analyzer, the source-generator path and the reflection path in ReflectionTestDataCollector. That follows the repo rule that metadata collection must behave the same in both modes.
• The tests are broad. They include a runtime fixture in tests/TUnit.TestProject/Bugs/_7573, a generator snapshot and analyzer tests. The description says 31 cases pass in both modes and that Native AOT publish was checked.
• The STAThread snapshot refresh looks intentional, and the net472 update was reviewed.

Concerns

1. The inference rules are duplicated and hard to keep in sync. The rules are: constructor types take precedence, the last occurrence wins for a repeated type parameter, and generic source methods, sources that take arguments and external providers are excluded. They are implemented separately in TestMetadataGenerator (Roslyn symbols), ReflectionTestDataCollector (System.Type) and the analyzer. The generator alone changes by hundreds of lines. Any drift between them will show up as a mode-dependent discovery bug. I'd put the pure decision logic (which rows can infer, which sources are retained) into one small, documented rule set. Each layer would then only adapt its type representation to it. I'd also add a parity test that runs the same fixture matrix through both modes and compares the discovered class and method type arguments.
2. TestMetadataGenerator.cs keeps growing. It is already well over 8k lines, and this adds more nested local helpers. Moving the method-row inference into its own internal class would make it testable and reviewable.
3. The 'last matching occurrence' rule for repeated type parameters is surprising. Please document it in the user docs, or have the analyzer report a diagnostic when occurrences disagree. As it stands, a silent last-wins choice could hide a user mistake.
4. Nullable unwrapping needs a test for value types. T? for an unconstrained T behaves differently from T? where T : struct. The tests cover nullable reference types well. Please confirm that Nullable<int> parameters and int rows infer int and not int? in both modes. If a case is missing, add it.
5. A silent fallback can hide an unrecognised pattern. When a source is excluded from inference, such as an argument-taking or generic source, the test falls back to the old behaviour without saying so. A debug-level log or a diagnostic would make 'why was my class not inferred' much easier to answer.

None of this is a blocker. Item 1 is the one I'd address before merge, or track in a follow-up issue.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 32d2a955e7

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Review of #7591 (nullable generic class parameters with method-data rows)

I read the analyzer, generator and reflection-collector changes. I didn't build or run anything. The PR description reports 33 execution cases passing in both modes, plus snapshot, analyzer and AOT runs. I couldn't see the tail of the reflection-collector diff, so my comments on that file are limited.

What looks good

• Both discovery modes get the same inference rules, which matches the AGENTS.md requirement. Coverage is broad: null, factory, tuple and array rows, overloads, explicit types, and constructor precedence.
• Inference now skips generic source methods, argument-taking sources and unresolved generic rows. That avoids wrong instantiations.
• Matching reflection's DeclaredOnly lookup in the generator is the right call.

Concerns

1. The same precedence rule is written three times. "Do constructor rows fully supply the class types?" now lives in the analyzer's CompilationExtensions.ClassRowSuppliesNoTypeArguments, the generator's TestMetadataGenerator.ClassRowSuppliesNoTypeArguments and the reflection collector. The code only says "keep in step with…" in comments. The decision is already factored into GenericClassInferencePolicy, but the new partial-row logic (includePartialRows, ClassRowHasValueTypeFallback, ContainsTypeParameter) bypasses that policy and sits inline in the generator. Moving it into GenericClassInferencePolicy as pure functions of counts and flags would let the generator and reflection share one tested implementation. Only the analyzer would keep a mirrored copy.
2. A boolean flag changes what the method means. With includePartialRows: true, ClassRowSuppliesNoTypeArguments no longer answers its name, and it is called with true at three sites and false at others. Two separately named methods would be clearer, so a future call site can't pick the wrong mode.
3. InferClassTypesFromMethodDataSource is hard to follow. It now has an early-exit chain: generic attribute, Arguments named argument, generic method, and a constructor-precedence check that re-runs GetAttributesIncludingBaseTypes for every source. That check doesn't depend on the source, so compute it once per test method and pass it in.
4. Rules that only look at the last occurrence are fragile. "Last matching occurrence wins" is written into the analyzer, the generator and reflection. The analyzer deliberately returns true when inference is uncertain, so constraint violations can slip through when a type parameter appears more than once. Please confirm this is intended, and cover it with an analyzer test that has both an uncertain and a violating case.
5. The nullable handling is spread over several helpers. There are ContainsNullableClassParameter, the ProcessTypeForGenerics nullable branch, the analyzer's isNullableClassParameter, and a separate unwrap of the argument's Nullable. A single UnwrapNullableValueType helper per project would cut the repeated pattern matches.
6. HasIndependentlySuppliedClassType re-walks the type hierarchy for every parameter. It uses LINQ on the analyzer hot path. Please cache it per type parameter, or per symbol action.

Minor

• The STAThread snapshots are churn from Cut STAThreadTests repeats so the Windows reflection pass run fits its timeout #7594. Fine, but landing them separately would mean a revert of this PR doesn't also revert unrelated snapshot refreshes.

Overall the behaviour looks correct and well tested. My main request is to centralise the precedence and inference policy (points 1 and 2) so the three copies can't drift apart over time.

[thomhurst]

@codex review

Fixed nullable constructed-struct inference parity in 0492f2b and Greptile's constraint-invalid partial constructor fallback in 36c8723. Both threads have commit-linked dispositions and are resolved. All 35 #7573 cases pass in generated and reflection execution; full net10.0 generator suite: 340 passed, one skipped. Reviewed both added snapshot entries. Native AOT publish succeeded; all 35 cases also pass in the native executable.

Earlier waiting run 41911277540 succeeded. No linked prerequisite, superseding PR, stale issue, finished parent or owner-reserved decision blocks this PR; it still closes #7573. Keeping draft for fresh reviews of this head.

Claude's non-blocking policy consolidation and repeated-occurrence concerns overlap the existing follow-up #7377 (and broader #7371). Helper extraction, naming, caching and diagnostic suggestions remain follow-up improvements; this patch makes no performance claim. Existing NullableClassMethodRows where T : struct covers int and int? rows and asserts typeof(T) == typeof(int) in both modes. The repeated-occurrence fixture pins the existing last-wins behavior; changing that policy or introducing diagnostics is beyond this fix. STA snapshots remain a documented validation repair after #7594. Existing warning debt remains tracked in the issues named in the PR body. Engine tests remain for CI.

[github-actions]

Review of #7591

I read the diff, the PR description and the earlier review comments. I did not run the tests or build the branch.

Prior feedback

• The earlier concerns were about precedence and inference policy being duplicated across the analyzer, the generator and reflection discovery. They are already tracked in Share class-row type inference between the source generator, analyzer and reflection #7377 and Generic type inference rules are implemented twice (source-gen and reflection) and keep drifting #7371, and the author's replies cover them.
• Their reasoning for deferring is reasonable. The duplication is a maintainability risk, but the PR pins the current last-occurrence-wins behavior with regression tests in both execution modes, so drift would show up as failures.

Assessment

• The change matches the project constraint that metadata collection must behave the same in the source generator and the engine. The PR reports all 35 TUnit0001 rejects an int method source for a nullable generic class parameter #7573 cases passing in both modes and in the Native AOT executable.
• Analyzer and generator snapshot tests are updated, and the STAThread snapshot refresh is documented.

Suggestions, non-blocking

• Put the nullable unwrap and class-type inference policy in one shared helper per project. Today that policy is spread over several helpers, so a change to it means editing three places.
• Compute HasIndependentlySuppliedClassType once per symbol action instead of re-walking the type hierarchy for each parameter.
• Add an analyzer test with a repeated type parameter that has both an uncertain case and a violating case. That would document the intended behavior where the analyzer returns true because inference is uncertain.

I found nothing that should block the merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36c8723ae1

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Constructor types get overridden

The wider nullable recursion can override constructor-supplied class types because ContainsNullableClassParameter still recognizes only bare T?.

For example, [Arguments("seed")] class C<T>(T seed) can have a source returning IEnumerable<Func<KeyValuePair<int, int>[]>> with a null row for Run(KeyValuePair<T, int>?[] values). Previously, the constructor supplied T = string, and the null row ran on C<string>. This change also infers C<int>. The constructor-precedence check misses this shape, so the extra entry receives "seed" for an int constructor parameter and fails.

Extend the precedence check to cover the constructed nullable shapes now visited here.

Knowledge Base Used:

• Test source generation and registration
• Engine discovery and initialization

[thomhurst]

Run 41911277540 completed successfully; that earlier wait no longer blocks this PR. The current head has completed Claude review with no blockers, but Codex found a valid constructor-precedence discrepancy: #7591 (comment). ContainsNullableClassParameter misses class parameters inside nullable constructed structs even though ProcessTypeForGenerics now infers through them.

This PR has 21 automated review rounds and a new blocking finding, exceeding the loop's 20-round rework budget. Closing it without merging and preserving branch issue-7573-nullable-analyzer at 36c8723ae14871d458537bdc8f3daf4dee479808 for selective reuse:

• Accept nullable generic class method rows in analyzer validation #7610: analyzer correction and analyzer regressions.
• Preserve nullable class discovery parity and constructed-struct constructor precedence #7611: generator/reflection discovery parity and shared execution/snapshot regressions, including the outstanding Codex finding.
  Both are verified native sub-issues of TUnit0001 rejects an int method source for a nullable generic class parameter #7573. No recurring file conflicts drove this split; the repeated changes were in TestMetadataGenerator.cs and ReflectionTestDataCollector.cs as new inference edge cases were found. Analyzer files belong only to Accept nullable generic class method rows in analyzer validation #7610.

Checked resolution alternatives: #7573 remains unimplemented on main; no replacement open PR delivers its full scope. It had no native children before this split and is not a finished parent. Its original analyzer-only report remains accurate, with the discovery acceptance criteria now explicit in #7611. No human decision, credentials, external release or pending run needs to park this work. The required review-budget split supplies actionable replacements instead of deferring it again. #7377 retains policy consolidation and #7598 retains the independent warning fix.