Skip to content

fix(generator): infer generic class types from class MethodDataSource rows - #7422

Closed
thomhurst wants to merge 26 commits into
mainfrom
issue-7333-class-method-source
Closed

thomhurst wants to merge 26 commits into
mainfrom
issue-7333-class-method-source

Conversation

@thomhurst

@thomhurst thomhurst commented Nov 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

A generic test class whose types come only from a class-level [MethodDataSource] silently disappears in source-generated mode. Infer its types from the member's declared row type and the constructor parameters, using the same row-shape and constructor matching already used for own-class members. Both external and own-class members now instantiate the class, including async factory rows, multiple sources of the same type, and generic methods with their own rows.

Class sources join the instantiation they infer, alongside matching class [Arguments] rows. Sources already covered by runtime class-row partitioning remain covered. When an uncovered source cannot supply accessible class type arguments, report a discovery failure with guidance to declare concrete rows or use [GenerateGenericTest]. This also enables the existing constructor-selection and null-row regressions in both modes. Inferred class instantiations without matching method rows are omitted, while method-only constructor fallback remains unchanged.

Reviewed and updated the affected snapshots. Removed redundant nullable access on the non-null compilation argument in the touched inference code, which caused a compiler warning, and corrected an outdated fixture comment.

Validation

  • Full TUnit.Core.SourceGenerator.Tests, net10.0: 218 passed, 1 skipped. This machine has only the net10.0 runtime; other target frameworks remain for CI.
  • New _7333 fixtures: source-gen and reflection each run 17 tests: 16 pass and 1 expected uninferable-class discovery failure. Before the fix the six inferable classes produced zero tests in source-gen.
  • Existing _7208 fixtures: both modes run 43 tests, 42 pass and 1 expected discovery failure; source-gen previously omitted four tests. _7161: both modes 7 total, 6 pass and 1 intentional unmatched-row failure. _7210: both modes 19 total, 15 pass, 3 skipped and 1 intentional failure.
  • Targeted _7108, _7107, _7117, _7118, and _7156 checks retain their expected outcomes, including the intentional failures and private-row-type reporting difference in _7108.
  • Added engine integration assertions for CI; did not run TUnit.Engine.Tests locally per repository instructions.
  • A minimal Native AOT application using the new fixtures compiled and generated native code, but local publishing could not link because the machine lacks libssl. Full AOT validation remains for CI.

#7334 retains the separate reflection tuple-splitting and failed-row reporting work; its native prerequisite points to this issue.

Review follow-up: preserve method data sources on class-inferred entries even without method [Arguments], and retain runtime partitioning for polymorphic external class sources. Both regressions were reproduced on the previous generator. The class/method source fixture now runs all four row combinations; the interface-declared source joins both integer rows on the integer instantiation in both modes. Full net10.0 generator suite: 218 passed, 1 skipped. Engine integration assertions are updated for CI.

Closes #7333

Array/nullable review follow-up (425bb59): external fixed arrays infer their class types recursively; covariant arrays and scalar nullable values retain runtime-value inference. Added int[], string[], int[][], int?[], covariant object[]/string[], and nullable scalar fixtures. Full net10.0 generator suite: 218 passed, 1 skipped. Updated _7333 runs in both modes: 24 passed and 1 expected uninferable-class discovery failure (25 total). Engine integration counts updated for CI; engine suite remains unrun locally per repository instructions.

Latest review follow-up (5f43bb8): nullable array constructor rows now infer consistently in both modes; fixed external rows match constructor positions and preserve all-null-row filtering. Newly inferred MethodDataSource entries use typed construction. Full net10 generator suite: 218 passed, 1 skipped. Full net10 PublicAPI suite: 19 passed. Expanded _7333 source-generated, reflection and published native AOT execution each run 29 tests (28 passed, one existing intentional discovery failure). Native AOT publish and execution now succeed, superseding the earlier linker limitation. Reviewed additive generated-code FilterNullClassRows property baselines for all frameworks; other framework execution remains for CI.

Tuple-column follow-up (d158aea): only columns that determine class type arguments must have fixed runtime types. Added an unrelated object-column regression. Final _7333 source-generated/reflection/native AOT execution each run 30 tests: 29 passed and one existing intentional discovery failure. Full net10 generator suite remains 218 passed, 1 skipped; preceding API validation remains 19 passed.

Factory/constructor review follow-up (e0e6dc9): class sources preserve fresh factory objects, fixed value-type sources suppress empty fallback rows, and typed construction uses the inference constructor. Added three regression classes; _7333 source-generated/reflection/native AOT each run 34 total (33 passed, one existing intentional discovery failure). Native AOT publish succeeds; full net10 generator suite 218 passed, 1 skipped. Neighboring _7208, _7210 and _7161 outcomes remain unchanged. Engine integration counts updated for CI; that suite remains unrun locally.

Own-class empty-source review follow-up (df67d20): inferred own-class sources now suppress empty fallback rows while retaining valid null values. Cache source inference and its selected constructor through emission, and share the all-null predicate between discovery modes. Added own-empty, own-null, and unsealed-row regressions. Full net10 generator suite: 218 passed / 1 skipped; full net10 PublicAPI suite: 20 passed. Updated _7333 source-generated/reflection/native AOT each run 38 total (37 passed, one existing intentional failure); native publish succeeds. Neighboring _7208 and _7161 retain their expected outcomes in both modes. Fixed one incidental API path-scrubber case where the .cs extension wraps across string literals, with a regression. Engine.Tests remains for CI. The hidden additive generated-code property is now named FilterClassRows and remains public because generated consumer code lives in another assembly.

Explicit-instantiation follow-up (4b3353d): preserve external null rows when GenerateGenericTest explicitly supplies the class types; use a safe constructor-symbol fallback. FilterClassRows is deliberately public for generated consumer assemblies and is hidden with EditorBrowsable(Never). Expanded _7333 fixtures in both modes: 43 total, 42 passed, one existing intentional discovery failure. Full net10 generator suite: 218 passed / 1 skipped. Updated snapshot and CI integration counts. Engine.Tests, Native AOT and other framework runs remain for CI for this follow-up.

Method-only fallback follow-up (285eb28): filter class rows only when their source supplies the current entry's inferred class rows. Raw empty rows remain visible to method-only constructor fallback. Added a constructor-failure regression and CI integration assertions; reviewed Tests7333 and Tests7161 snapshots. Full net10 generator suite: 218 passed / 1 skipped. Expanded _7333 in generated, reflection and published Native AOT: 44 total, 42 passed, two intentional failures. Native AOT publish succeeds; _7161 retains 6 passed / 1 intentional failure in both managed modes. Engine.Tests and other framework runs remain for CI. The separate pre-existing missing-constructor-argument behavior difference is tracked in #7442.

Grouped explicit-instantiation follow-up (01cf701): retain external null-row filtering when method rows merge explicit class types into an inferred group; standalone explicit null rows remain supported. Extracted class-source inference/coverage caching, cached explicit types on instantiations, shared constructor preference and added a constructor-mapping debug assertion. Full net10 generator suite: 218 passed / 1 skipped; reviewed and updated Tests7333 snapshot. Expanded _7333 in generated, reflection and published Native AOT execution: 45 total, 43 passed and the same two intentional failures. The new regression alone passes with one case; before the fix generated mode ran two cases with one failure, while reflection ran one passing case. Neighboring _7208 remains 42 passed / 1 intentional discovery failure in both managed modes. Native AOT publish succeeds. Updated engine integration assertions for CI; Engine.Tests was not run locally per AGENTS.md. Other frameworks and hosted checks remain for CI.

Explicit null-only follow-up (615d7b4): Full net10 generator suite: 218 passed / 1 skipped. Full net10 PublicAPI suite: 20 passed. Expanded _7333 fixtures in generated, reflection and published Native AOT each run 49 tests: 47 passed and the same two intentional failures. Native AOT publish succeeds. Neighboring _7161 retains 6 passed / 1 intentional failure in both managed modes. Updated engine integration assertions; Engine.Tests and other framework execution remain for CI per AGENTS.md. Explicit grouped instantiations now decide from actual rows whether to use grouped or raw data. Added null-only, own-null, and array-row regressions. FilterClassRows is internal; generated code configures it through the existing generated-code-only partition helper. Nonblocking refactors are tracked in #7377 and #7443.

Runtime-grouping review follow-up (2d6821f): preserve constructor-mapped class Arguments, deferred own-null rows, explicit generic-method grouping, and matching-empty-source fallback. Cache the closed inference constructor and clarify coverage predicates, with an explicit/uninferable-source regression. Full net10 generator suite: 218 passed / 1 skipped. Full net10 PublicAPI: 20 passed. Expanded _7333 generated/reflection/published Native AOT each run 56 total, 53 passed and three intentional failures; native publish succeeds. _7208, _7161 and _7210 retain their expected outcomes in both managed modes. Engine.Tests and other framework execution remain for CI.

Mixed-partition review follow-up (9910c8c): inferred sources outside PartitionedIndices retain null/empty-row filtering when a sibling creates a runtime partition. Three new regressions reproduce seven cases with three failures before the fix and four passing cases after. Full net10 generator suite: 218 passed / 1 skipped. Full _7333 generated and reflection runs: 60 total, 57 passed and three existing intentional failures each. Reviewed snapshot changes and updated engine integration counts/cases for CI; Engine.Tests was not run locally. Native AOT and other framework execution remain for CI for this follow-up.

Constructor-selection follow-up (737b5eb): preserve the cached inferred constructor for grouped rows while retaining the missing-row guard. Added a marked-object-overload regression: two failures before, two passes after in both modes. Full net10 generator suite 218 passed / 1 skipped. Expanded _7333 generated/reflection runs each 63 total, 59 passed / four intentional failures; neighboring _7210 unchanged. Updated engine integration assertions; Engine.Tests, Native AOT and other frameworks remain for CI for this follow-up.

External constraint-row follow-up (35b418c): defer external source constraint violations until an eligible row exists in both discovery modes. Added empty, null-only and nonempty invalid regressions; reviewed generator and additive optional GenericConstraintViolationEntry.DataSources API baselines. Full net10 generator suite 218 passed / 1 skipped; PublicAPI 20 passed. Expanded _7333 generated/reflection/native AOT each run 70 total, 64 passed and six intentional failures. Native _7161 runs 6 passed / one intentional failure, confirming constructor fallback. Explicit comparer removes its incidental AOT assertion warning; final native publish has no IL warnings. Engine integration assertions updated for CI; Engine.Tests remains unrun locally per AGENTS.md.

Summary by CodeRabbit

  • New Features
    • Generic test classes can now infer their type arguments from class-level method data sources, including external and asynchronous sources.
    • Constructor selection and row grouping now reflect the inferred types, supporting mixed data sources and runtime row types.
  • Bug Fixes
    • Improved handling of empty, null, unmatched, and constraint-violating rows so test discovery and reported failures better reflect the available data.
    • Static data sources can provide fresh values on subsequent fetches.

@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Walkthrough

Class-level method data-source rows now contribute to generic class type inference in source generation and reflection discovery. Generated metadata partitions rows by inferred types, selects constructors, and associates some constraint failures with their source rows. Regression coverage and generated API snapshots are updated. Source-path matching also accepts split .cs extensions.

Changes: Generic class method-data-source inference

Layer / File(s) Summary
Infer class types and select constructors
src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs, src/TUnit.Core.SourceGenerator/CodeGenerators/Helpers/InstanceFactoryGenerator.cs, tests/TUnit.Engine.Tests/Issue7208Tests.cs
The generator infers class type arguments from class-level [MethodDataSource] rows, selects constructors for inferred rows, and records constraint violations with their originating sources. Constructor-inference tests now run in both modes.
Configure generated rows and report constraint violations
src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs, src/TUnit.Core/Attributes/TestData/*, src/TUnit.Core/GenericConstraintViolationEntry.cs, tests/TUnit.Core.SourceGenerator.Tests/Tests*.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/InheritedTestsFromDifferentProjectTests.Test.verified.txt, tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.*.verified.txt, tests/TUnit.TestProject/Bugs/_7161/*
Generated sources receive inferred-type and filtering settings. Row partitioning uses declared row types, and method-source factories are retained for later fetches. Constraint failures can use row-backed data sources. Generated output and API baselines reflect these changes.
Discover rows in reflection mode and validate inference
src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs, tests/TUnit.TestProject/Bugs/_7333/*, tests/TUnit.Engine.Tests/Issue7333Tests.cs
Reflection discovery infers types from fixed external method sources and retains row factories. Added fixtures and engine tests cover row types, constructor selection, filtering, empty sources, constraints, and generated/reflection parity.

Source path scrubbing

Layer / File(s) Summary
Match split source extensions
tests/TUnit.PublicAPI/Tests.cs, tests/TUnit.PublicAPI/SourcePathScrubberTests.cs
The source-path pattern accepts .cs extensions split across concatenated string literals. The added test checks that the combined path is scrubbed.

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant TestMetadataGenerator
  participant GeneratedMetadata
  participant GenericClassRowPartition
  participant MethodDataSourceAttribute
  TestMetadataGenerator->>GeneratedMetadata: Emit class type arguments and constructor selection
  GeneratedMetadata->>GenericClassRowPartition: Configure source row partitioning
  GenericClassRowPartition->>MethodDataSourceAttribute: Apply inferred types and filtering
  MethodDataSourceAttribute->>GenericClassRowPartition: Return accepted rows
Loading

Possibly related PRs

  • thomhurst/TUnit#7339: Adds the generic class row partitioning and inference code that this change extends.
  • thomhurst/TUnit#7336: Updates reflection discovery for generic class method sources and touches the constructor-inference tests revised here.
  • thomhurst/TUnit#7425: Changes generator reporting for uninferable generic class type arguments in the same code path.
Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Out of Scope Changes check Warning Most changes support #7333, including row partitioning, constructor selection, constraint reporting, generated snapshots, and regression tests. However, tests/TUnit.PublicAPI/Tests.cs changes `Sourc… Remove the source-path scrubber implementation and its test from this pull request, or move them to a separate pull request. Keep the public-API baselines only when they record API changes required by the #7333 implementation.
Docstring Coverage Warning Docstring coverage is 26.32% 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 Passed checks: 3 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: inferring generic class types from class-level MethodDataSource rows in the generator.
Linked Issues check Passed #7333 requires source generation to infer generic class types from class-level [MethodDataSource] rows and to report an outcome when inference is not possible. TestMetadataGenerator now collects c…
Full details: Linked Issues check

Explanation

#7333 requires source generation to infer generic class types from class-level [MethodDataSource] rows and to report an outcome when inference is not possible. TestMetadataGenerator now collects class method-data-source inference, maps declared row types and constructors, emits inferred instantiations, and emits class-constraint or uninferable-source failures. The changes cover external and own-class sources, async and multiple sources, arrays, nullable rows, explicit instantiations, runtime partitioning, and method-row matching. The _7333 fixtures and engine assertions provide automated coverage for generated and reflection modes. The reported generated, reflection, and Native AOT runs support parity.

Full details: Out of Scope Changes check

Explanation

Most changes support #7333, including row partitioning, constructor selection, constraint reporting, generated snapshots, and regression tests. However, tests/TUnit.PublicAPI/Tests.cs changes SourcePathPattern to handle a .cs extension split across string literals, and SourcePathScrubberTests.cs adds the corresponding scrubber test. These changes concern public-API path normalization and have no demonstrated connection to generic class type inference from class-level [MethodDataSource] rows.

Resolution

Remove the source-path scrubber implementation and its test from this pull request, or move them to a separate pull request. Keep the public-API baselines only when they record API changes required by the #7333 implementation.

Full details: Docstring Coverage

Explanation

Docstring coverage is 26.32% 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 38 functions across 14 files. (8 skipped: 8 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ast-grep (0.51.1)

ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/php/security/search-active-debug-php.yml

Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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 row in flight
And sorts the types by day and night
A constructor joins the stream
While factories return fresh beans
The empty rows are counted too
The scrubber splits the suffix through
Then hops away with tests in view

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: acab67f3-5186-429f-8985-8f8e4f8a168d
Commits

Reviewing files that changed from the base of the PR and between ca4437c and bfcc91e.

Files selected for processing: 23
  • src/TUnit.Core.SourceGenerator/CodeGenerators/Helpers/InstanceFactoryGenerator.cs
  • src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs
  • src/TUnit.Core/Attributes/TestData/GenericClassRowPartition.cs
  • src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
  • src/TUnit.Core/Attributes/TestData/StaticDataSourceAttribute.cs
  • src/TUnit.Core/GenericConstraintViolationEntry.cs
  • src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
  • tests/TUnit.Core.SourceGenerator.Tests/Bugs/7333/Tests7333.cs
  • tests/TUnit.Core.SourceGenerator.Tests/InheritedTestsFromDifferentProjectTests.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/Tests5118.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/Tests7161.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/Tests7210.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/Tests7333.Test.verified.txt
  • tests/TUnit.Engine.Tests/Issue7208Tests.cs
  • tests/TUnit.Engine.Tests/Issue7333Tests.cs
  • tests/TUnit.PublicAPI/SourcePathScrubberTests.cs
  • tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt
  • tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt
  • tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt
  • tests/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt
  • tests/TUnit.PublicAPI/Tests.cs
  • tests/TUnit.TestProject/Bugs/_7161/ClassDataForMethodRowInstantiationTests.cs
  • tests/TUnit.TestProject/Bugs/_7333/ClassMethodDataSourceInferenceTests.cs
Files not fully reviewed because their changes were unavailable: 1
  • tests/TUnit.Core.SourceGenerator.Tests/Tests7333.Test.verified.txt

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

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 2, 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-11-03T04:50:17.348622Z df168fa New commits
ℹ️ 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

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Review of #7422

The change fits the existing design. Class-level [MethodDataSource] is now a row-type-inferring class source alongside class [Arguments], reusing the existing own-member inference and accessibility check, so there is no parallel inference path. Uninferable sources now yield an actionable discovery failure instead of silently producing zero tests. Fixtures, the new _7333 snapshot and the redundant compilation?. cleanup look reasonable.

Concerns

  1. The _7161 snapshot changes in a way the PR text doesn't explain. ClassMethodDataSourceWithMethodRows<int> was created with a typed constructor call and now goes through Activator.CreateInstance(typeof(...), args). The GenericClassRowPartition.Create(...) wrapper is gone, and the class ArgumentsAttribute(1) row was dropped from the attributes. Please confirm the Activator path is intended and AOT/trim-safe (AGENTS.md asks for AOT verification when reflection paths change). A typed constructor call is cheaper and more trimmer-friendly. Please also confirm that dropping the partition keeps the every-class-row x every-method-row behaviour from Generic test classes drop [Arguments] rows that infer the same type arguments (both modes; reflection runs no method rows) #7106 when an [Arguments] row and a [MethodDataSource] row coexist.

  2. Repeated inference calls. InferClassTypesFromClassMethodDataSource is called in the pairing loop, the else branch, the later loop, the new violation loop and otherClassDataSources. Each call re-resolves the member and row type. A per-class cache built once next to inferringClassAttributes would remove that cost. It would also keep the instantiation and violation paths from diverging.

  3. The violation guard is dense. The continue condition in the new foreach (var source in classMethodDataSources) loop chains four disjuncts. entry.SpecificArgumentsAttribute is null means any entry without a specific attribute covers every source, which is broad and could mask an uncovered source. Extract a named helper such as IsClassMethodDataSourceCovered(...) and add a test for an uninferable source next to an unrelated no-attribute instantiation.

  4. The join condition widened. existing.AddRow is now reached for every isClassRow source, not just ArgumentsAttribute. That matches the stated intent, but please confirm other typed class sources reaching this branch don't double-count.

  5. Minor: in GetDeclaredMethodDataSourceRowType, var rowType = ...; return rowType; can be inlined.

Verdict: The direction is sound and both-mode test coverage is good. Please resolve point 1 (the Activator path and AOT) before merge, and consider the cache and helper refactors in points 2-3.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1c37a38de

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

greptile-apps Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The changes appear safe to merge; no new blocking issue was found.

Summary

Generic classes now infer their types from class-level MethodDataSource rows. The PR also preserves constructor choice, fresh factory values, and row grouping across discovery modes.

  • Generic test classes now get matching instances from class-level method rows.
  • The source-path scrubber now catches file extensions split across lines.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Class MethodDataSource] --> B[Match declared rows to constructor]
    B --> C[Infer concrete class types]
    C --> D[Generate test entries]
    D --> E{Source supplies inferred rows?}
    E -->|Yes| F[Filter empty and ineligible rows]
    E -->|Explicit types or fallback| G[Keep raw rows]
    F --> H[Combine class and method rows]
    G --> H
Loading

Reviews (24) · Last reviewed commit: "Correct successful TRX summary expectati..." · Reviewed by Greptile

@thomhurst

Copy link
Copy Markdown
Owner Author

Review dispositions for 9b59b31:

  1. The _7161 Activator path is intentional: the int class source now owns that instantiation, and the existing scoped-class-row constructor path uses Activator.CreateInstance(typeof(ClosedClass<int>), args). The concrete closed typeof supplies public constructor preservation for trimming. The partition disappears because the declared int source is now inferable; runtime partitioning is needed only for untyped sources. The removed ArgumentsAttribute(1) was method-row metadata, not a class row. Both-mode _7161 runs remain 6 passed / 1 expected unmatched-row failure. Re-published a minimal AOT project including _7161 and _7333: native code generation completed without constructor-related trim warnings, but linking failed on this machine's missing libssl. The _7161 array-equivalency assertion reports its existing IL2026 warning; full linked AOT validation remains for CI.
  2. Leaving the optional inference-cache refactor out of this correctness fix. Inference shares the existing declared-member row-type cache; no measured performance regression is established, and later partition emission is a separate caller.
  3. The no-specific-attribute coverage branch corresponds to emission: those entries receive all class data sources in EmitConcreteDataSources. They therefore cover the source rather than masking an omitted row. GetClassDataSourcePartition has the same explicit no-specific-attribute handling. No behavior change is needed for that branch.
  4. AddRow checks IsSpecificClassDataSource before adding, so the broadened join does not duplicate sources. The existing _7208 OwnClassConcreteMethodDataSourceWithArgumentsAndMethodRows regression passes all four class-row × method-row combinations in each mode.
  5. Inlined the redundant rowType return local in this commit.

Also fixed the Codex/Greptile empty-method-row finding with a both-mode regression. Full generator snapshots pass: 218 passed, 1 skipped. _7333 now has 10 passes and its one expected discovery failure in each mode. Engine integration assertions are updated for CI; the engine suite was not run locally per repository instructions.

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b59b313ac

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

Copy link
Copy Markdown
Owner Author

@codex review

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7422

I read the generator diff, the new _7333 fixtures and the snapshot changes. I did not run the tests.

What looks good

  • The change reuses the existing own-class row-shape and constructor matching (InferClassTypesFromOwnMemberRowType). It adds one entry point, InferClassTypesFromClassMethodDataSource, and InferClassRowTypes dispatches to it. That is better than a parallel inference path.
  • Splitting GetDeclaredMethodDataSourceRowType out of InferTypesFromMethodDataSourceRowType removes duplication.
  • The conservative rule for external members is sensible. They are only inferred when every row type is a value type or a sealed type, so runtime partitioning stays in charge where reflection mode would see runtime types. The explicit discovery failure with [GenerateGenericTest] guidance is better than silently dropping tests.
  • Coverage is good: async factories, multiple sources, generic methods, polymorphic external rows, and the unmatched-method-rows case. The ordering of class violations matches reflection mode, and the pre-existing comment says that matters for test ids.

Points to check (non-blocking)

  1. Tests7161 snapshot. The class-level [MethodDataSource] no longer goes through GenericClassRowPartition.Create(...AllRows...). The instance factory moved from a direct constructor call with CastHelper.Cast<int> to Activator.CreateInstance(typeof(Closed<int>), args). This is the same pattern already used elsewhere in the generator, and the closed type is statically referenced, so it should survive trimming. The path is still slower and less AOT-friendly than the direct constructor. Since AGENTS.md asks for AOT verification on reflection-path changes, please confirm an AOT publish of a fixture like _7333, or say why it isn't needed. A direct new is also possible when the constructor parameters are statically known.
  2. ArgumentsAttribute(1) removed from __Attributes_0. The 7161 snapshot also drops a new ArgumentsAttribute(1) from the attribute array. This is probably the method row now flowing only through methodDataSources, and the engine test is described as passing in both modes. A short note in the PR on why it is safe would help reviewers.
  3. Complexity of the new class-source filtering. The long results.Any(... || ... || ...) condition around line 1650 mixes three unrelated skip reasons. Extracting it into a named local function such as IsClassSourceCovered(source) would make the intent clearer and the next change safer. The isClassRow || ArgumentsAttribute || joinsDuplicateMethodSources condition is similar and would benefit from a comment on which cases each term covers.
  4. Tests7333.cs assertions. They check for substrings in the generated source (Contains("...UnmatchedMethodRows<int>")). That is brittle, but it is paired with the verified snapshot and the engine tests, so acceptable.

Overall this is a solid fix with strong test coverage. I'd like the AOT check from point 1 before merge, but nothing here looks like a correctness blocker.

🤖 Generated with Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c87a5ec3c

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

Copy link
Copy Markdown
Owner Author

The fixed-runtime-type guard now accepts arrays when their element types cannot vary (recursively), including value, sealed-reference, nested, and nullable-element arrays. Covariant arrays still use runtime partitioning. Scalar nullable values also retain runtime partitioning because their non-null values box as the underlying type.

Added fixtures and reviewed snapshots for int[], string[], int[][], int?[], object[] rows containing string[], and nullable scalar rows. Both review regressions were reproduced against the previous predicates. Full net10.0 generator suite: 218 passed, 1 skipped. Targeted _7333 runs in each discovery mode: 24 passed and one intentional uninferable-class discovery failure (25 total). Updated engine integration expectations for CI; did not run TUnit.Engine.Tests locally per repository instructions.

Claude's latest non-blocking points repeat the earlier review disposition in #7422 (comment): the closed-type Activator path completed native code generation; linking remains limited by missing local libssl. No new reflection path is introduced by this follow-up. The removed Arguments attribute was method-row metadata, and coverage/helper refactors remain optional follow-ups. Existing _7161 and cross-product validation remains documented in the PR.

Separate pre-existing problems found during validation are recorded in #7427 (explicit array type arguments), #7428 (snapshot test build warnings), and #7412 (vulnerable transitive package).

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 425bb59885

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

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7422. I read the generator diff and the snapshot changes. I didn't build or run anything. The approach looks sound. It reuses the existing own-class row inference, and it keeps runtime partitioning when a row type can't fix the runtime type. The HasFixedRuntimeType guard covers interfaces, unsealed types, covariant arrays and Nullable<T>, and the fixtures cover both the positive and negative cases. The two review follow-ups from the description are covered by the PolymorphicExternalRows and MethodRows fixtures.

Points to check

  1. __CreateInstance_* in the Tests7161 snapshot changed. It went from a typed new X<int>(CastHelper.Cast<int>(args[0])) to Activator.CreateInstance(typeof(X<int>), args). That is a perf and AOT-surface regression for a case that was already inferable, and AGENTS.md asks us to keep the hot paths and trimming clean. Please say why the typed constructor path is no longer chosen. I suspect constructor matching now fails for these class-method-source entries, so the generator falls back to reflection. If so, fixing the constructor resolution is better than accepting the fallback in the snapshots. Please also confirm the AOT CI job exercises this fixture, since you couldn't link locally.

  2. Tests7161 no longer emits GenericClassRowPartition.Create(... AllRows ...). Class sources are now a plain IDataSourceAttribute[], and the fixed ArgumentsAttribute(1) is dropped from __Attributes_0. I think this is intended, because the instantiation is now fully inferred. Please confirm that the string instantiation no longer receives the int rows. Today that depends on the engine matching rows to int or string, with no generator-side guarantee. A comment or an assertion in Tests7161 would pin it down.

  3. The inference logic is spread across TestMetadataGenerator.cs. The file is already very large and this PR adds more branching. InferClassRowTypes is a local function closing over testMethod and compilation, and classMethodDataSources and inferringClassAttributes are recomputed per call. Consider moving the class-level-data-source-to-type-args logic (InferClassTypesFromClassMethodDataSource, HasFixedRuntimeType, the external-member check) into one small static helper type. It could then be unit-tested without the whole generator, and the large method would be easier to read.

  4. The isExternal heuristic is fragile. ConstructorArguments is [{ Value: INamedTypeSymbol }, ..] || AttributeClass.IsGenericType infers external from the constructor-argument shape. A generic MethodDataSource<T> is always treated as external, even when T is the test class itself. Please add a fixture for that case, or resolve the provider type explicitly. GetDeclaredMethodDataSourceRowType already resolves it.

  5. The uninferable-class diagnostic. The condition that suppresses the new classViolations entry combines IsSpecificClassDataSource, SpecificArgumentsAttribute is null, a partition lookup and a GenerateGenericTest check. A short comment saying which of these means covered at runtime would help.

Minor: Tests7333.cs hard-codes 14 class names in one loop, so a failure won't say which name was missing. Putting the name in the assertion would give a better message.

Overall this is good work. The one thing I'd resolve before merge is point 1, the Activator.CreateInstance fallback, which looks like an unintended side effect.

🤖 Generated with Claude Code

@thomhurst

Copy link
Copy Markdown
Owner Author

Review follow-up in 1396417 and 5f43bb8:

  • Fixed nullable array constructor inference in both discovery modes, with a T[] parameter regression.
  • Fixed all-null external row filtering, including suppressing the empty-source fallback after filtering. The generated-code FilterNullClassRows property is additive; reviewed all framework API baselines.
  • Replaced the newly inferred MethodDataSource class entries' Activator fallback with the existing typed instance factory. Tests7161 now emits a direct new ClassMethodDataSourceWithMethodRows(CastHelper.Cast(args[0])).
  • The _7161 string instantiation intentionally still receives the class source's integer 5 converted to string "5": it is inferred only by a method row, and Generic class: source-gen builds the instance from a method [Arguments] row when class types come from a class-level data source or [GenerateGenericTest]; reflection uses the class data #7161's fallback semantics require every class source to supply constructor values. The fixture explicitly asserts both argument/value types and value.ToString()=="5". The removed Arguments(1) was method-row metadata, not a class row.
  • Fixed external declared rows now use the same constructor matcher in reflection mode; the positional id/value review example is covered explicitly. Untyped, polymorphic and scalar-nullable sources retain runtime inference.
  • Optional cache/helper refactors remain outside this correctness fix. Explicit provider attributes, including generic providers, remain external as in reflection's ClassProvidingDataSource rule, even when their provider names a concrete test class. Existing coverage-guard dispositions remain applicable.

Validation: full net10 generator suite 218 passed / 1 skipped; full net10 PublicAPI suite 19 passed. Expanded _7333 fixtures in source-generated mode, reflection mode and the published native AOT executable each run 29 tests: 28 passed and the existing intentional uninferable-class discovery failure. Neighboring reflection _7208, _7161 and _7210 retain their documented outcomes. Native AOT publish now succeeds and the executable runs, superseding the earlier local linker limitation; only the pre-existing _7161 collection-equivalency IL2026 warning remains. Engine.Tests was not run locally per repository instructions. Other runtime/framework validation remains for CI.

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7422

I read the generator diff, not every snapshot. I did not build the branch or run any tests.

What looks good

  • The approach matches the existing own-class inference: infer from the declared row type and the constructor parameters, and share the matching logic.
  • ClassRowSources computes the inference once and reuses it for both collection and emission. That removes a class of drift between the two passes.
  • The missing-constructor case throws an InvalidOperationException during generation. It does not emit a runtime throw into the generated code.
  • Uncovered sources get a clear discovery failure that points to [GenerateGenericTest] or concrete rows.
  • The PR description lists validation for both source-gen and reflection modes, and covers the _7333, _7208, _7161 and _7210 fixtures.

Concerns

  1. Complexity of TestMetadataGenerator. This file now has many interacting special cases: HasExplicitClassTypes, joinsDuplicateMethodSources, filterClassRows, isPartitionedSource, declaredClassTypes and the ClassRowFallback choice. Most are decided by inline boolean expressions in the emission code. The IsCovered check already shows a cleaner shape. I'd suggest going further and having ClassRowSources or ConcreteInstantiation expose one decision record per (entry, source). It could carry the role (partitioned, filtered, explicit, fallback), the declared types and the constructor. The emitter would then just render that record. That would make each rule testable on its own and stop the emitter from re-deriving it.
  2. Matching sources by attribute name. Several places compare AttributeClass?.Name == "MethodDataSourceAttribute" or "ArgumentsAttribute". This also matches a user-defined attribute that happens to share the name. Comparing against the known TUnit symbols, or a shared helper, would be safer. Some of the existing code already does the same, so a follow-up is acceptable.
  3. Behaviour change you should document. Class instantiations inferred from method sources that have no matching method rows are now omitted. The Arguments-only fallback is unchanged. A short note in the docs or release notes would help, because a user could see tests disappear in this narrow case.
  4. .Where(...).ToArray() over GetAttributesIncludingBaseTypes() in the generator. The cost is small, but the same inference runs once per test method. Caching per type symbol would avoid recomputing it for classes with many tests.
  5. Windows paths. The new _7333 fixtures and snapshots add many files. Check that the snapshot paths stay within MAX_PATH on Windows, since the repo notes this as a known risk.

None of these block the PR, assuming CI is green. My main suggestion is the refactor in point 1, either now or as a follow-up.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: bfcc91e665

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

Copy link
Copy Markdown
Owner Author

Disposition of current-head Claude review 6595448989:
1, 2, 4. Verified the inline partition decisions, simple attribute-name comparisons and per-method ClassRowSources enumeration in TestMetadataGenerator. Recorded these nonblocking consolidation/caching suggestions on #7377, extending its existing mapping work.
3. The PR description already documents the observable behavior: “Inferred class instantiations without matching method rows are omitted, while method-only constructor fallback remains unchanged.” This provides the release-facing note for this change.
5. Checked all changed _7333 fixture/snapshot paths: the longest repository-relative path is 73 characters (tests/TUnit.TestProject/Bugs/_7333/ClassMethodDataSourceInferenceTests.cs). The existing Tests7333 snapshot naming does not embed individual fixture names, so these additions do not introduce long snapshot filenames. Hosted Windows validation remains for CI.

Codex completed review of bfcc91e with no findings; Claude explicitly reports no blockers on the same head. All 33 review threads are resolved. Promoting the unchanged head for CI.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7422

I read the TestMetadataGenerator.cs and InstanceFactoryGenerator.cs changes. I did not read the Core/Engine changes or the tests in detail, and I ran nothing. Treat this as a partial review.

What looks good

  • The design is sound: class-level [MethodDataSource] types are inferred through the same row-shape and constructor matching already used for own-class members, so there is no second inference path.
  • ClassRowSources computes the inference once and shares it with every ConcreteInstantiation. Collection and emission therefore use the same decisions, which avoids drift.
  • ClassConstraintViolation carries the originating RowSource instead of only a string. The engine can then report per-row failures, and that matches reflection mode.
  • Making the GenerateInstanceFactoryBody overload take a selectedConstructor is a small, backward-compatible change.

Concerns

  1. Violation row-source condition: the check source.AttributeClass?.Name == "MethodDataSourceAttribute" && (source.ConstructorArguments is [{ Value: INamedTypeSymbol }, ..] || source.AttributeClass.IsGenericType) ? source : null is inline in TryAddInstantiation. It encodes which MethodDataSource forms can be re-emitted as external rows. Extract it into a named helper such as IsExternalMethodDataSource. GenerateDataSourceAttribute can then share it, and the two will not diverge silently.
  2. Name-based attribute matching: AttributeClass?.Name == "MethodDataSourceAttribute" matches by simple name only, so a user type with the same name would also match. This follows the file's existing convention, but the new code adds several more of these. A single helper that checks the full type would be safer.
  3. Emitting violation data sources: the code builds a throwaway CodeWriter, then calls .Trim().TrimEnd(',') on its output. That is fragile string surgery on generated code. Prefer a GenerateDataSourceAttribute variant that returns the expression without a trailing separator.
  4. Repeated attribute scans: GetAttributes() is filtered several times per method (classMethodDataSources, inferringClassAttributes, ClassRowSources). This runs in the generator hot path. Computing the filtered sets once inside ClassRowSources and reusing them would be cheaper and clearer.
  5. Dead local: classMethodDataSources in the @@ -1200 hunk looks unused after ClassRowSources was introduced. Please confirm and remove it if so.
  6. Coverage and AOT: per AGENTS.md, both modes need the same behaviour. The _7333 fixtures cover that. Please make sure the TUnit.PublicAPI snapshots for the TUnit.Core changes (GenericConstraintViolationEntry.DataSources, GenericConstraintViolationDataSource) are intentional, and that the new Core paths are trim and AOT safe.

None of these block the change. Overall the approach is good and the PR is well tested; I'd address 1, 3 and 5 before merging.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfcc91e665

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

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

@thomhurst

Copy link
Copy Markdown
Owner Author

Fixed Codex finding 4450986596 in a6b3a7a. Directly filtered inferred class sources now suppress empty-source skipped placeholders; explicit and constructor-fallback sources keep their original setting. The exact regression produced 1 pass + 1 unwanted skip before; the full _7333 fixtures now produce 67 passes, 8 existing intentional failures, 0 skips in both generated and reflection modes (75 total).

Validation: full net10 generator suite 218 passed / 1 existing skip. Reviewed and accepted the additive Tests7333 snapshot. Neighboring _7210 in both modes remains 15 passed / 3 skipped / 1 intentional failure. Engine integration assertions updated for CI; Engine.Tests was not run per AGENTS.md. This follow-up adds no public API or reflection paths; Native AOT and other framework runs remain for CI.

Disposition of Claude review 6595605926:
1–4. These are explicitly nonblocking suggestions already tracked on #7377: the existing follow-up comments cover the external-source helper, expression-only emission instead of trimming, symbol-based attribute matching, and cached attribute scans. Retaining that consolidation as one follow-up avoids mixing refactors into this parity correction.
5. classMethodDataSources is live: it guards class-only pruning and is enumerated to report uncovered sources in CollectConcreteInstantiations. Removing it would break those paths.
6. GenericConstraintViolationEntry.DataSources and GenericConstraintViolationDataSource baselines were intentionally reviewed in 35b418c. The prior bfcc91e follow-up validated generated/reflection/native _7333 (66 passes / 8 intentional failures) and the full generator suite. This correction preserves the same source-generated factory path and adds no reflection or trimming requirements.

The previous blocker, run 41579303929, completed successfully. The pushed fix now awaits current-head CI and review.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7422: infer generic class types from class-level [MethodDataSource]

I read the generator, core and engine changes and the new tests. I did not build or run anything. The fix looks correct for the cases it targets, and I found no bugs. I have two maintainability concerns and one smaller one.

What works well

  • Reuses the existing row-shape and constructor matching rather than adding a parallel inference path.
  • Passing the selected constructor through InstanceFactoryGenerator.GenerateInstanceFactoryBody(type, ctor) keeps the #7161 conversion behaviour while honouring the inferred constructor.
  • Uninferable sources now produce an explicit discovery failure with guidance instead of silently vanishing. That is the real user-facing fix.
  • Both modes have _7333 fixtures and updated snapshots. The reflection side (ReflectionTestDataCollector) is changed alongside the generator, as AGENTS.md requires.

Concerns

  1. Decision logic is spread across TestMetadataGenerator. CollectConcreteInstantiations was already very large. This PR adds ClassRowSources, IsCovered, the HasExplicitClassTypes and filterClassRows branching in EmitConcreteDataSources, a post-pass that removes class-only instantiations, and a second pass for the uninferable-source failure. The same question is answered in several places: does this source supply this entry's rows, and are type arguments inferred or explicit? ClassRowSources is a good start. I'd go further and give each ConcreteInstantiation a single resolved "class row plan", computed once during collection. It would say which sources feed it, whether they are filtered, and which constructor is used. Emission would then read the plan instead of re-deriving it from AreSameAttribute lookups and AttributeClass?.Name == "MethodDataSourceAttribute" string checks. That makes collection and emission hard to drift apart, and it makes the next #72xx-style fix cheaper. If that is too big for this PR, a follow-up issue would do.
  2. Public API surface. GenericConstraintViolationEntry.DataSources and GenericConstraintViolationDataSource.Create are new, and the PublicAPI snapshots changed. Please confirm they are meant to be public. If only generated code needs them, they could follow whatever the existing convention is for generator-only members (for example [EditorBrowsable(Never)]). Otherwise they become permanent API.
  3. Generator invariant throw. Throwing InvalidOperationException when closedConstructor is null is defensible, as the comment says. The consequence is that a bad edge case surfaces as a generator crash (CS8785) and suppresses all generated tests for the project. A user-visible diagnostic or the runtime-failure fallback might be kinder. A fixture that reaches that branch would also show whether it is truly unreachable.

Validation
The PR description reports the source generator tests and the fixtures in both modes passing. TUnit.Engine.Tests additions (Issue7333Tests, Issue7208Tests) rely on CI, as expected. Please make sure CI is green on all target frameworks. The description says only net10.0 was run locally.

Overall this is reasonable to merge once CI passes. I'd weigh item 2 before merging and treat item 1 as follow-up.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review of #7422: infer generic class types from class-level [MethodDataSource]

I read the diff of the generator and core changes. I did not build it or run the tests. The PR reports a clean full run of the generator tests and matching source-gen and reflection results for the new _7333 fixtures.

What works well

  • It fixes a real silent-drop bug: affected tests simply vanished in source-gen mode.
  • The inference is shared. ClassRowSources computes the types and constructor once, and both collection and emission reuse that result. This keeps the two stages from diverging.
  • The uninferable case now reports a discovery failure with actionable guidance (concrete rows or [GenerateGenericTest]) instead of dropping tests.
  • Snapshots are updated, and both modes get engine tests.

Concerns

  1. Complexity of TestMetadataGenerator. The pairing between collection, GetClassDataSourcePartition and EmitConcreteDataSources now depends on several interacting flags: HasExplicitClassTypes, filterClassRows, declaredClassTypes, ClassRowFallback, IsCovered, and the MethodParametersReferenceTypeParameters plus HasMethodRows gate.
    • Parts of this are hard to reason about locally. For example, EmitConcreteDataSources has three separate branches deciding how to emit one class source (partitioned, ConfigureInferredSource for [Arguments], or filtered/declared).
    • Suggestion: compute a small per-entry, per-source "class row plan" record once during collection (types, constructor, filter yes/no, fallback). Emission would then only read it instead of re-deriving decisions from attributes. That also makes the reflection-parity argument easier to check.
  2. Re-parsing attributes at emit time. EmitConcreteDataSources calls InferTypesFromClassArgumentsAttribute and GetDeclaredMethodDataSourceRowType again, and AreSameAttribute is used for identity. Storing the results from collection avoids both the repeated work and the risk of the two code paths disagreeing.
  3. New public surface. GenericClassRowPartition.ConfigureInferredSource and GenericConstraintViolationDataSource.Create appear in the PublicAPI snapshots. If they are only called by generated code, consider marking them [EditorBrowsable(Never)], or internal if the generated code's access allows it, so they don't become supported API.
  4. Fragile string handling. sourceWriter.ToString().Trim().TrimEnd(',') strips the trailing comma from a generated attribute expression. A GenerateDataSourceAttribute overload that returns the expression without the comma would be sturdier.
  5. AOT/trim. The new engine-side paths (ReflectionTestDataCollector, MethodDataSourceAttribute) should be confirmed AOT-safe. I did not see an AOT publish run mentioned, and the project guidelines ask for one when reflection paths change.
  6. Invariant failure. Throwing InvalidOperationException in the generator when closedConstructor is null takes down the whole generator (every test in the project) rather than one test. A ClassConstraintViolation or a diagnostic for that entry would keep the failure local.

None of these is a correctness bug I can demonstrate from reading alone. Item 1 is the main maintainability risk, and the rest are smaller. Overall the approach is sound and the tests are thorough.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df168fa58f

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

Copy link
Copy Markdown
Owner Author

The earlier blocker, run 41579303929, completed successfully. Current checks are running on df168fa, but the latest Codex review has a verified blocking finding: an own-class null enumerable is converted into a null constructor row. DataSourceFactories.FromEnumerable falls through to ToObjectArray(null), and MethodDataSourceAttribute retains it when ClassProvidingDataSource is null. A null collection must remain distinct from a null element in a non-null collection.

This PR has reached 21 automated review rounds and still has that blocking finding. Per the loop rework budget, no further fixes will be pushed here. Split into native children of #7333:

There have been no force-push conflict cycles; no files repeatedly conflict. The coupled review surface is TestMetadataGenerator.cs, MethodDataSourceAttribute.cs, GenericClassRowPartition.cs and ReflectionTestDataCollector.cs. Splitting runtime support from generator activation gives each a separately mergeable validation boundary. Preserve branch issue-7333-class-method-source at df168fa for extraction, not wholesale reapplication.

Disposition of Claude comments 6595697190 and 6595715302:

Closing this PR without merging, keeping its branch and parent #7333 open. No tests were run in this administrative split unit. Existing pending checks cannot establish approval of the unresolved finding.

This branch was previously deployed

1 inactive deployment
Pull Requests — df168fa5 Deployed Nov 3, 2026 by thomhurst via modularpipeline (macos-latest) #20806
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.

Source-gen: generic test class does not infer its type arguments from a class-level [MethodDataSource]; reflection does

1 participant