Fix implicit conversion operators bypassed in property injection casts#5317
Fix implicit conversion operators bypassed in property injection casts#5317
Conversation
…t in source generators When ClassDataSource<T> is used on a property with a different type than T, the generated SetProperty code now emits (PropertyType)(DataSourceType)value instead of (PropertyType)value, allowing user-defined implicit/explicit conversion operators to be invoked correctly. Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/60f08ff4-ae2d-463a-979e-7a47008ada0c Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
|
@copilot is there a way to make it work at runtime, even when the source generator doesn't know the type information? E.g. custom data sources that yield different results |
…ype methods Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/60f08ff4-ae2d-463a-979e-7a47008ada0c Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
When the source generator doesn't know the data source type (e.g., custom data sources yielding different types), the PropertyInjector now uses CastHelper.Cast() at runtime to convert values via implicit/explicit operators before setting them on properties. This works in both source-gen and reflection injection paths. Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/fb5a76be-48dd-461b-a68c-d5a357abcdf4 Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Yes — in |
|
@copilot you need to add thorough test coverage around the runtime conversion fallback |
…version; revert source-gen two-step cast Reverts the source generator two-step cast approach which breaks when values are already converted (e.g., cached property values set through SetCachedPropertiesOnInstance). Instead, relies entirely on the runtime CastHelper.Cast() fallback in PropertyInjector and ObjectLifecycleService to handle implicit/explicit conversion operators. Adds 6 new test classes covering runtime conversion scenarios: - Custom UntypedDataSource with implicit operator on source type - Custom UntypedDataSource with explicit operator - Custom UntypedDataSource with implicit operator on target type - Custom UntypedDataSource with struct (value type) implicit operator - Custom UntypedDataSource yielding same type (no conversion baseline) - MethodDataSource returning different type with implicit operator Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/08b79cfb-b2f5-46c2-a10c-f7c34f8beef4 Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Added in
Also found and fixed a bug: |
There was a problem hiding this comment.
Code Review
The approach here — adding a runtime conversion fallback via CastHelper.Cast() before calling the generated setter — is the right design. The source generator can't know at compile time what type a custom UntypedDataSourceGeneratorAttribute will produce, so a runtime fallback is the correct place to handle this. The fix for ObjectLifecycleService.SetCachedPropertiesOnInstance (applying the same conversion to cached values) is also a good catch. Test coverage is thorough across operator directions, value types, and data source varieties.
Issues
1. Duplicated ConvertPropertyValueIfNeeded — should live in CastHelper
ConvertPropertyValueIfNeeded is copy-pasted identically into both PropertyInjector and ObjectLifecycleService. These two static methods will inevitably drift. The method is a thin wrapper over CastHelper.Cast() and belongs there:
// In CastHelper:
public static object? CastIfNeeded(Type targetType, object? value)
{
if (value is null) return null;
if (value.GetType().IsAssignableTo(targetType)) return value;
return Cast(targetType, value);
}Then both call sites become CastHelper.CastIfNeeded(metadata.PropertyType, resolvedValue). This keeps the CastHelper as the single source of truth for all conversion logic and means trimming/AOT annotations only need to be applied once.
2. Missing [DynamicallyAccessedMembers] on targetType parameter
ConvertPropertyValueIfNeeded passes targetType into CastHelper.Cast(Type type, ...), which requires [DynamicallyAccessedMembers(PublicParameterlessConstructor | Interfaces | PublicMethods)] on its first argument. Rather than suppressing IL2067, the parameter should carry the annotation:
private static object? ConvertPropertyValueIfNeeded(
object? value,
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor
| DynamicallyAccessedMemberTypes.Interfaces
| DynamicallyAccessedMemberTypes.PublicMethods)] Type targetType)The IL2067 suppressions with justifications like "PropertyType is preserved through source generation or reflection discovery" are correct in practice, but annotations are preferable to suppressions when the annotation can flow naturally — they give the compiler and trimmer actual information rather than telling them to be quiet.
3. AOT correctness gap
In AOT mode, CastHelper.Cast() skips reflection-based conversions and falls back to returning the value unconverted (or throwing a helpful InvalidCastException). In the case where it returns unconverted, the generated setter (PropertyType)value will still throw InvalidCastException since CLR reference casts bypass user-defined operators.
The [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "CastHelper handles AOT scenarios with proper fallbacks")] is technically accurate — CastHelper does handle AOT by throwing an informative error rather than silently corrupting state. But this means the fix does not work in AOT mode for custom data sources. That's an acceptable known limitation (the reflection path was never AOT-safe) but it should be documented, either in an XML doc comment or a code comment on the call site, so future readers don't assume it works everywhere.
4. Unnecessary re-conversion in retry paths
In SetCachedPropertiesOnInstance, the conversion is re-applied every time a new test instance is created (e.g., for each retry). The conversion will always produce the same result — the cached value type doesn't change between retries.
The better fix is to store the already-converted value in the cache at the point of first injection (PropertyInjector), so that SetCachedPropertiesOnInstance simply reads the correctly-typed value and applies it without any conversion. If the cache key stores the post-conversion object, the ConvertPropertyValueIfNeeded call in ObjectLifecycleService becomes redundant. This is a correctness simplification, not just a micro-optimization.
5. Unhelpful comment
// Convert the value if needed (same as above) in ObjectLifecycleService.cs:212 gives no useful information. It should either explain why the conversion is needed here (like the comment above it does) or be removed.
Minor
The test file RuntimeConversionPropertyInjectionTests.cs is long but well-structured. The inline data source attribute classes (ImplicitSourceDataSourceAttribute, etc.) are clear for their purpose. No issues there.
|
@copilot address the code reviews |
Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/1b851ce1-e6f8-4181-b0d3-fddf7c573cf1 Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Head branch was pushed to by a user without write access
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.23.7 to 1.24.13. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.24.13 <!-- Release notes generated using configuration in .github/release.yml at v1.24.13 --> ## What's Changed ### Other Changes * perf(mocks): optimize MockEngine for lower allocation and faster verification by @thomhurst in thomhurst/TUnit#5319 * Remove defunct `UseTestingPlatformProtocol` reference for vscode by @erwinkramer in thomhurst/TUnit#5328 * perf(aspnetcore): prevent thread pool starvation during parallel WebApplicationTest server init by @thomhurst in thomhurst/TUnit#5329 * fix TUnit0073 for when type from from another assembly by @SimonCropp in thomhurst/TUnit#5322 * Fix implicit conversion operators bypassed in property injection casts by @Copilot in thomhurst/TUnit#5317 * fix(mocks): skip non-virtual 'new' methods when discovering mockable members by @thomhurst in thomhurst/TUnit#5330 * feat(mocks): IFoo.Mock() discovery with generic fallback and ORP resolution by @thomhurst in thomhurst/TUnit#5327 ### Dependencies * chore(deps): update tunit to 1.24.0 by @thomhurst in thomhurst/TUnit#5315 * chore(deps): update aspire to 13.2.1 by @thomhurst in thomhurst/TUnit#5323 * chore(deps): update verify to 31.14.0 by @thomhurst in thomhurst/TUnit#5325 ## New Contributors * @erwinkramer made their first contribution in thomhurst/TUnit#5328 **Full Changelog**: thomhurst/TUnit@v1.24.0...v1.24.13 ## 1.24.0 <!-- Release notes generated using configuration in .github/release.yml at v1.24.0 --> ## What's Changed ### Other Changes * perf: optimize TUnit.Mocks hot paths by @thomhurst in thomhurst/TUnit#5304 * fix: resolve System.Memory version conflict on .NET Framework (net462) by @thomhurst in thomhurst/TUnit#5303 * fix: resolve CS0460/CS0122/CS0115 when mocking concrete classes from external assemblies by @thomhurst in thomhurst/TUnit#5310 * feat(mocks): parameterless Returns() and ReturnsAsync() for async methods by @thomhurst in thomhurst/TUnit#5309 * Fix typo in NUnit manual migration guide by @aa-ko in thomhurst/TUnit#5312 * refactor(mocks): unify Mock.Of<T>() and Mock.OfPartial<T>() into single API by @thomhurst in thomhurst/TUnit#5311 * refactor(mocks): clean up Mock API surface by @thomhurst in thomhurst/TUnit#5314 * refactor(mocks): remove generic/untyped overloads from public API by @thomhurst in thomhurst/TUnit#5313 ### Dependencies * chore(deps): update tunit to 1.23.7 by @thomhurst in thomhurst/TUnit#5305 * chore(deps): update dependency mockolate to 2.1.1 by @thomhurst in thomhurst/TUnit#5307 ## New Contributors * @aa-ko made their first contribution in thomhurst/TUnit#5312 **Full Changelog**: thomhurst/TUnit@v1.23.7...v1.24.0 Commits viewable in [compare view](thomhurst/TUnit@v1.23.7...v1.24.13). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Generated property setters cast
(PropertyType)valuewherevalueisobject. This is a CLR reference cast that bypasses user-defined implicit/explicit operators, causingInvalidCastExceptionwhenClassDataSource<T>typeTdiffers from the property type.Approach
Rather than modifying source generators to emit two-step casts (which breaks when cached values are already converted), this PR adds runtime conversion fallbacks using
CastHelper.Cast()before calling the generated setters.CastHelperdiscovers and invokesop_Implicit/op_Explicitoperators via reflection at runtime, handling both known and unknown data source types.Changes
PropertyInjector(runtime conversion): AddedConvertPropertyValueIfNeeded()that usesCastHelper.Cast()at runtime when the resolved value's type doesn't match the property type. Applied to both source-generated and reflection injection paths. This handles all data sources — including custom ones where the source generator can't know the produced type at compile time.ObjectLifecycleService: Added the sameConvertPropertyValueIfNeeded()conversion inSetCachedPropertiesOnInstance, which sets cached property values on new test instances (e.g., during retries). Without this, cached values that haven't been converted would fail the generated setter's direct cast.Tests:
ImplicitOperatorPropertyInjectionTests— reproduces the original scenario (ClassDataSource<ImplicitDbFixture>on arequired ImplicitDbContextproperty with an implicit operator).RuntimeImplicitConversionFromCustomDataSourceTests— customUntypedDataSourceGeneratorAttributewith implicit operator on source type.RuntimeExplicitConversionFromCustomDataSourceTests— custom data source with explicit operator.RuntimeImplicitOnTargetTypeConversionTests— implicit operator defined on the target type (not the source).RuntimeStructImplicitConversionTests— struct (value type) with implicit operator, exercising the boxing/unboxing path.RuntimeSameTypeNoConversionTests— same-type baseline verifying no regression when types already match.RuntimeMethodDataSourceImplicitConversionTests—MethodDataSourcereturning a different type with implicit operator.