You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix class argument literals to use constructor parameter types - #7496
Class-level [Arguments] now emit literals using the selected constructor parameter types. This fixes decimal method parameters changing boxed class arguments and breaking constructor binding (#7440).
Generic class factories use the selected constructor with runtime argument conversions. They preserve required-member values and constructor identity after generic substitution, and emit locals with the required modifiers for by-reference arguments. Generated and reflection discovery retain explicit arrays/null, pack trailing rows, preserve optional array defaults, and try whole-array user conversions before scalar packing. Referenced generic attributes preserve array and enum values; floating-point constants retain runtime conversion semantics.
Validation (SDK 10.0.403/net10.0): full core generator suite 327 passed, 1 skipped; all 42 targeted #7440 tests passed in generated and reflection discovery. Reviewed the affected verified snapshot. The new generic ref-constructor regression fails with CS1620 before the fix. Native AOT validation is recorded in the latest disposition comment. Engine.Tests remains CI-only per repository instructions. No public API changes.
Broader pre-existing required-member initialization behavior is tracked in #7516. Existing AOT/build warning categories remain tracked in #7499, #7410, #7412, #7419, #7428, #7430, #7452 and #7493.
Improved how test class constructor arguments are matched and converted, including generic and numeric values, arrays, optional and params parameters, and required members.
Test instances now use the constructor selected for their data row, avoiding incorrect argument values or unintended constructor calls.
Improved handling of decimal and enum values when generating tests, including cases where test data is shared across compilations.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Class-row generation now uses selected constructors and formats row values against constructor parameters. Generated and reflection-based factories bind trailing collections, conversions, optional arrays, and required members. Regression tests and generated snapshots cover these cases.
Changes: Class-row constructor binding
Layer / File(s)
Summary
Select constructors and format class-row arguments src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs, src/TUnit.Core.SourceGenerator/CodeGenerators/Formatting/TypedConstantFormatter.cs, tests/TUnit.Core.SourceGenerator.Tests/CrossCompilationAttributeTests.cs
Class-level data-source generation passes selected constructors and formats generic and non-generic row arguments according to their respective parameter types. Cross-compilation tests cover emitted argument values.
Generated factories use selected constructors and handle required properties and fields, optional arrays, trailing arguments, and conversions. Regression tests and snapshots cover constructor selection and direct construction.
Bind trailing collections at runtime src/TUnit.Core/Helpers/CastHelper.cs, src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs
The conversion helper tries whole-array conversions before scalar packing. Reflection-based factories separately bind final array or params parameters, preserving compatible values and converting supplied values into collections.
thomhurst/TUnit#7422 — Both changes pass a selected constructor to GenerateInstanceFactoryBody for generic class instantiation.
thomhurst/TUnit#7336 — Both changes use selected constructors and class-row arguments in generic class construction.
thomhurst/TUnit#7420 — Both changes affect construction of generic instances from distinct class-row arguments.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: class argument literals now use constructor parameter types.
Linked Issues check
Issue #7440 requires class-level [Arguments] values to use constructor parameter types, plus a generator snapshot and execution coverage in both discovery modes. TestMetadataGenerator.cs now passe…
Out of Scope Changes check
The changes remain connected to issue #7440. InstanceFactoryGenerator.cs adds typed constructor calls and conversion handling needed for class rows. ReflectionTestDataCollector.cs keeps array and …
Full details: Linked Issues check
Explanation
Issue #7440 requires class-level [Arguments] values to use constructor parameter types, plus a generator snapshot and execution coverage in both discovery modes. TestMetadataGenerator.cs now passes the selected class-row constructor to factory generation. The generated snapshot keeps the class value as ArgumentsAttribute(1) and constructs ClassArgumentParameterTypesTests<int> with CastHelper.Cast<int>. The new _7440/ClassArgumentParameterTypesTests.cs and its source-generator wrapper cover the reported decimal binding case and related constructor cases. The PR reports 37 targeted tests passing in both generated and reflection discovery.
Full details: Out of Scope Changes check
Explanation
The changes remain connected to issue #7440. InstanceFactoryGenerator.cs adds typed constructor calls and conversion handling needed for class rows. ReflectionTestDataCollector.cs keeps array and optional-parameter behavior consistent with generated discovery. CastHelper.cs, TypedConstantFormatter.cs, required-member handling, cross-compilation tests, and updated snapshots support constructor binding, conversion, or validation. No unrelated change is demonstrated.
Full details: Docstring Coverage
Explanation
Docstring coverage is 28.57% 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 28 functions across 9 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each row with care And finds the constructor waiting there Arrays gather, values flow Required fields get defaults to show The test rows hop into their place While snapshots keep a record of the chase
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: 96103ad0-02fe-4236-a747-63b6918f87c7
Commits
Reviewing files that changed from the base of the PR and between 790ab90 and b68ac37.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.
Generated class-row literals keep their declared values for constructor conversion.
No new actionable issues were found. The numbered previous finding is fixed.
thomhurst acknowledged and deferred the broader non-generic/default-constructor initializer behavior to #7516 because it follows the existing default-value policy.
Review: Fix class argument literals to use constructor parameter types
I found no blocking issues. The change is correct and well scoped.
What I checked
Class-level [Arguments] used the test method's parameters to choose literal types. That is the root cause of Class-level [Arguments] numeric literals are rewritten using test method parameter types #7440. All five class data-source emission paths now call GetClassRowConstructor(...): concrete data sources, GenerateDataSources, the optional factory parameters, the filtered-data-sources path and the non-generic path. I grepped for the old methodSymbol call sites and found none left.
Using the same helper that reflection mode mirrors (ReflectionGenericTypeResolver.FindRowConstructor) keeps source-gen and reflection discovery consistent. This matches the AGENTS.md rule that both modes must behave identically. The PR reports 10 passing tests in each mode.
The nullable IMethodSymbol? plus ImmutableArray<IParameterSymbol>.Empty fallback handles a class with no public constructor without throwing.
TryGetDecimalLiteralText is a clear improvement on the old TrimEnd('d','D','f','F','m','M') approach. That approach produced invalid code for 3L, hex literals and string constants. The new helper rejects hex and binary prefixes and integral suffixes, and handles signed literals. Anything it rejects falls back to the typed-constant formatter, which keeps the runtime conversion.
The snapshots show the intended output: 3m for a decimal ctor with an int method parameter, 3L and 2147483647 preserved for the other rows, and 2m still applied on the method side.
Minor suggestions, non-blocking
The two decimal-literal emission sites (the syntax path around L3115 and the typed-constant fallback around L3227) duplicate the SpecialType == Decimal && TryGetDecimalLiteralText check. A small TryEmitDecimalLiteral(...) helper would keep them from drifting.
GenerateDataSourceAttribute now takes the target constructor or method. The parameter is still called methodSymbol, and the same name is used downstream. Renaming it to something like targetSymbol would make the new class-versus-method meaning obvious to future readers.
A decimal literal such as 1e2 is emitted as 1e2m, which is valid C#. A test for an exponent literal would pin that down, since the PR lists floating-point constants as covered but I did not see an exponent case.
Overall this is a good fix with solid fixtures and snapshots. Approving from my side.
Fixed the generic decimal constructor finding from #7496 (comment) in 4fc3575. Reproduced all five rows failing with MissingMethodException before the fix. Generic factories consuming runtime rows now use typed constructor calls with CastHelper conversions, selecting the constructor used by class-row inference. This also preserves first-public/[TestConstructor] selection when constructors are overloaded.
Added generic multiple-row regressions for long, double/float constants, and an escaped string constant. Reviewed and updated eight generator snapshots. Validation on SDK 10.0.403/net10.0: generator suite 250 passed, 1 skipped; #7440 execution 15 passed in each discovery mode. Selected passing fixtures for #7106, #7118, #7161, #7209, #7210, and #7335 also passed in both modes (existing explicit/skipped cases remain skipped). #7222 has intentionally failing fixtures and no EngineTest=Pass matches; its generator snapshot passed, and engine coverage remains for CI per repository instructions. No reflection path or shipped public API was added.
Claude's three minor suggestions are non-blocking cleanup/coverage suggestions; leaving the existing helper layout, parameter naming, and exponent coverage unchanged keeps this fix scoped to constructor conversion.
The change looks correct. I read the diff but did not build it or run any tests.
What it does well
The root cause is addressed where it lives. Every class-level data-source emission path now passes GetClassRowConstructor(type) instead of the test method symbol. That covers the concrete-instantiation path, the generic path and the class-source path. GenerateDataSourceAttribute and GenerateArgumentsAttributeWithParameterTypes take a nullable IMethodSymbol, and the targetParameters fallback to an empty array is safe.
Decimal literal text is now kept only for supported numeric literal syntax. Integral suffixes and constant expressions are formatted from their values, which avoids emitting wrong literals such as 1Lm.
The new fixtures cover the failing shapes: a generic class with mixed class sources, a decimal constructor with an int method parameter, and a new snapshot test. I read the .verified.txt snapshots and they match the intended output. No .received.txt files are included.
Reflection mode is exercised too, since the PR description reports 15 tests passing in each mode.
Suggestions (non-blocking)
Method parameters and class-constructor parameters now share one literal-emission routine. A small ParameterSet type, or at least a comment on the parameter, would make it harder to pass the wrong symbol again. The bug was exactly that mix-up. A short doc comment on GetClassRowConstructor explaining that class rows must always bind to constructor parameters would also help.
The generic instance factory snapshot still uses Activator.CreateInstance(typeof(X<int>), args) when several class rows share a closed type. That's fine for correctness, and the PR text says typed constructor calls were adopted elsewhere. If the constructor can be resolved statically, a typed call with CastHelper.Cast would be trim and AOT safer, and consistent with the other branches.
The PR description is long and mixes validation notes with the design. Splitting it into a short summary and a validation section would make it easier to read.
Nothing blocking. Good fix, and the tests cover it well.
Both blocking constructor-factory findings are fixed in 6b1389b and their threads resolved. Claude's latest non-blocking suggestions: retaining the existing parameter-symbol API keeps this fix scoped; the multi-row factory already uses typed calls (now covered by required-members and params snapshots); the PR body has been shortened and validation refreshed. Reflection constructor params packing was corrected alongside source generation to preserve discovery parity. Native AOT publish and all 21 targeted native tests passed; pre-existing generic-property dynamic metadata warnings are tracked in #7499.
Review of #7496. The approach looks sound: class-level Arguments literals are typed from the selected constructor, the generated factory replaces Activator.CreateInstance, and trailing params packing matches in both discovery modes. Limiting the decimal m-suffix path to real numeric literals is a good fix. I only read the diff; I have not built it or run tests. Concern: constructor choice is now fixed per class. EmitConcreteInstanceCreation now calls GenerateInstanceFactoryBody(constructedType, GetClassRowConstructor(constructedType)). Activator picked a constructor from runtime arg count and types; the new code always uses the TestConstructor constructor or the first public one. A class with several public constructors, e.g. C(int) and C(int, string), with rows for the second would previously work and would now index args for the wrong constructor. This also affects the non-generic HasParameterizedConstructor branch. Suggest a test with two public constructors of different arity, or keeping an Activator fallback when more than one eligible constructor exists. Smaller points: (1) GetClassRowConstructor only considers public constructors and GenerateInstanceFactoryBody silently falls back to GetPrimaryConstructor otherwise; worth a comment. (2) Trailing array/params logic is now duplicated between GenerateTypedConstructorCallBody and ReflectionTestDataCollector; a shared runtime helper would stop the two modes drifting. (3) The SetsRequiredMembers check compares a display string; a well-known-name constant would match the rest of the generator. Only the multi-constructor behaviour change warrants a test before merge.
Fixed the omitted optional-array findings in fab6440. The fix preserves declared defaults for omitted optional plain arrays in reflection factories and generated runtime-row factories; empty params arrays still stay empty. Added a generic two-row regression and a zero-argument row regression.
For the constructor-selection concern in the latest Claude comment: class rows already select the public [TestConstructor], otherwise the first public constructor, in TestMetadataGenerator.GetClassRowConstructor and ReflectionGenericTypeResolver.FindRowConstructor; ReflectionTestDataCollector.SelectInstanceConstructor uses the same rule. TUnit0052 asks ambiguous multi-constructor tests to select [TestConstructor]. Falling back to Activator overload selection would disagree with class metadata/type inference and reflection discovery, and reintroduce the numeric conversion problem addressed here. Retaining that selection is intentional. Added a regression with constructors of different arities where [TestConstructor] selects the second and the other throws; it passes in both discovery modes. The remaining minor cleanup suggestions are non-blocking and remain outside this constructor conversion fix.
Validation on SDK 10.0.403/net10.0: 26 targeted tests passed in each discovery mode and in the Native AOT executable; Native AOT publishing succeeded. Core generator suite: 250 passed, 1 skipped, with one reviewed snapshot update in this commit. Existing warnings remain tracked in #7410, #7412, #7419, #7428, #7430, #7452, #7493 and #7499. No shipped API changes; Engine.Tests remains for CI as required.
Review of #7496 (class-level [Arguments] typed to constructor parameters)
I read the source changes in InstanceFactoryGenerator, TestMetadataGenerator and ReflectionTestDataCollector. I did not build or run anything. I skimmed the first part of the generated snapshots. I did not read the TUnit.TestProject tests or the full snapshot set.
What looks good
The Activator.CreateInstance(..., args) fallback in the generic concrete paths is replaced with the same typed CastHelper.Cast<T> constructor call that ordinary factories use. The generated __CreateInstance_0 snapshots show this. That removes the boxed long/double to decimal binder failure from Class-level [Arguments] numeric literals are rewritten using test method parameter types #7440 and keeps the AOT path reflection-free.
Decimal literal preservation now goes through TryGetDecimalLiteralText, which accepts only numeric literal syntax. The old code stripped d/f/m suffixes from arbitrary expression text. That was fragile, and non-literal expressions now take the runtime conversion path.
[SetsRequiredMembers] constructors no longer get an object initializer that overwrites their values.
The reflection path handles the trailing array/params row (explicit array, null, loose values, omitted optional array via Type.Missing) the same way as the generated (args.Length == N ? ... : ...) expression.
Suggestions
Row packing is implemented twice. The generator builds the trailing-array expression as a string, and ReflectionTestDataCollector repeats the same decisions in about 45 lines of runtime code. Both discovery modes must behave identically, and this is the kind of logic that drifts. The tests do cover both modes, which helps. Moving the packing into one shared runtime helper in TUnit.Core (for example ConstructorArgumentBinder.Bind(ParameterInfo[], object?[])) would let reflection call it directly. The generated code could call it too, if the allocation cost is acceptable. That gives one definition of the semantics. If the generator must stay allocation-free, at least cross-reference the two sites in comments.
Constructor selection can diverge.GetClassRowConstructor is used to pick the constructor for binding, and GetPrimaryConstructor is used for the factory. The PR passes the selected constructor into GenerateInstanceFactoryBody, which fixes the split in the places it is called. GenerateInstanceFactory and GenerateInstanceFactoryAsMethod still use GetPrimaryConstructor independently. A single selector with one [TestConstructor] rule would remove the risk. The different-arity [TestConstructor] test covers part of this.
AOT and trimming in the reflection path. The new code calls Array.CreateInstance(trailingElementType, ...) and CastHelper.Cast(Type, object) with runtime types. Please confirm these do not add new IL2xxx/IL3050 warnings in the engine. The PR description says AOT publishing succeeded, so this is probably fine. The array creation should still be annotated or justified, in line with the AOT constraint in AGENTS.md.
Minor:Type.Missing only works for optional parameters that have a default value. The check uses HasDefaultValue, so it is correct. Add a short comment saying why Type.Missing is used instead of DefaultValue.
Nothing blocking. Please address suggestion 2, and consider suggestion 1 in a follow-up.
Rebased onto current main (9d6a435), preserving its one-element tuple data-source binding option alongside the nullable class-constructor target. The integration also makes method-data-source tuple inspection accept a missing public constructor instead of introducing a nullable warning or dereference.
Latest Claude suggestions are non-blocking. Constructor selection across the other factory entry points is already tracked by #7365; this PR keeps its selected-constructor conversion fix and the different-arity [TestConstructor] regression scoped to #7440. Consolidating all selectors belongs with that issue’s row-compatibility rule. Shared row-packing cleanup, cross-reference comments, and the Type.Missing explanatory comment are optional follow-up cleanup. Native AOT publishing and the 26 native regressions were already verified on the pre-rebase head; this integration adds no reflection operations or trimming requirements.
Rebase validation: core generator snapshots 251 passed, 1 skipped, with no snapshot differences; all 26 #7440 tests passed in each discovery mode. Engine.Tests remains for CI per repository instructions. Existing warnings remain tracked in the issues listed in the PR body.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Class-level
[Arguments]now emit literals using the selected constructor parameter types. This fixes decimal method parameters changing boxed class arguments and breaking constructor binding (#7440).Generic class factories use the selected constructor with runtime argument conversions. They preserve required-member values and constructor identity after generic substitution, and emit locals with the required modifiers for by-reference arguments. Generated and reflection discovery retain explicit arrays/null, pack trailing rows, preserve optional array defaults, and try whole-array user conversions before scalar packing. Referenced generic attributes preserve array and enum values; floating-point constants retain runtime conversion semantics.
Validation (SDK 10.0.403/net10.0): full core generator suite 327 passed, 1 skipped; all 42 targeted #7440 tests passed in generated and reflection discovery. Reviewed the affected verified snapshot. The new generic ref-constructor regression fails with CS1620 before the fix. Native AOT validation is recorded in the latest disposition comment. Engine.Tests remains CI-only per repository instructions. No public API changes.
Broader pre-existing required-member initialization behavior is tracked in #7516. Existing AOT/build warning categories remain tracked in #7499, #7410, #7412, #7419, #7428, #7430, #7452 and #7493.
Closes #7440
Summary by CodeRabbit
paramsparameters, and required members.