fix(sourcegen): emit default literal for value-type assertion parameters#5919
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics -7 complexity
Metric Results Complexity -7
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This is a clean, well-scoped fix for a genuine Roslyn edge case. Here's my assessment:
What's Good
Correct root cause diagnosis. Roslyn reports null as the constant value for = default on non-nullable value types, but = null is invalid C# for those types (CS1750). The fix correctly branches on IsValueType and the Nullable<T> carve-out is right — Nullable<T> should stay on the null path since null is a valid default for it.
Sensible deduplication. The two FormatDefaultValue private methods were nearly identical; pulling them into DefaultValueFormatter is the right call and the useFullyQualifiedEnumName parameter neatly encodes the one intentional semantic difference between the two call sites.
Snapshot hygiene. .verified.txt files committed; .received.txt not committed. All four TFMs covered.
CompileChecker is a good addition. Structurally-correct-but-compile-broken output would otherwise slip past content-only snapshot tests. The #if NETFRAMEWORK CS0122 filter is well-documented.
Suggestions
1. Boolean trap on useFullyQualifiedEnumName
// Current — caller must know which bool means what
DefaultValueFormatter.FormatDefaultValue(value, type, useFullyQualifiedEnumName: true)Named arguments mitigate this, but with only two call sites it's also fine to keep two thin static helpers instead:
internal static class DefaultValueFormatter
{
public static string FormatDefaultValue(object? defaultValue, ITypeSymbol type)
=> FormatDefaultValueCore(defaultValue, type, useFullyQualifiedEnumName: false);
public static string FormatDefaultValueFullyQualified(object? defaultValue, ITypeSymbol type)
=> FormatDefaultValueCore(defaultValue, type, useFullyQualifiedEnumName: true);
private static string FormatDefaultValueCore(...) { ... }
}This makes the intent at each call site self-documenting without needing to read the parameter name. Not a blocker — the named-argument form is readable enough — but worth considering if more generators are added later.
2. Pre-existing: incomplete string escaping
return $"\"{str.Replace("\"", "\\\"")}\"";This only escapes ". A default value of e.g. "line1\nline2" would emit as a literal newline inside a string literal. Since this PR didn't introduce the issue and it's in a code path for test-assertion generators where string defaults are rare, it's fine to leave it, but worth tracking separately.
3. CompileChecker compiles all generated files together
This is correct behaviour (multi-file generators can have cross-file dependencies), but it means a missing reference will silently drop the error if ReferencesHelper.References doesn't include the missing assembly. If CI ever shows a false-green, this is the first place to check.
Verdict
Approving. The fix is correct, well-tested, and the deduplication is a genuine improvement. The suggestions above are minor and non-blocking.
…hods Per review feedback on thomhurst#5919: replace the `useFullyQualifiedEnumName` boolean with two thin public wrappers (`FormatDefaultValue` for the short enum form and `FormatDefaultValueFullyQualified` for the namespace-prefixed form), each delegating to a private `FormatDefaultValueCore` that retains the original parameterised body. Removes the boolean trap at call sites without changing emit behaviour. Existing snapshots compare identically.
|
Thanks! |
…hods Per review feedback on thomhurst#5919: replace the `useFullyQualifiedEnumName` boolean with two thin public wrappers (`FormatDefaultValue` for the short enum form and `FormatDefaultValueFullyQualified` for the namespace-prefixed form), each delegating to a private `FormatDefaultValueCore` that retains the original parameterised body. Removes the boolean trap at call sites without changing emit behaviour. Existing snapshots compare identically.
9ef34a6 to
024a415
Compare
…hods Per review feedback on thomhurst#5919: replace the `useFullyQualifiedEnumName` boolean with two thin public wrappers (`FormatDefaultValue` for the short enum form and `FormatDefaultValueFullyQualified` for the namespace-prefixed form), each delegating to a private `FormatDefaultValueCore` that retains the original parameterised body. Removes the boolean trap at call sites without changing emit behaviour. Existing snapshots compare identically.
024a415 to
038d561
Compare
Description
Both source generators in
TUnit.Assertions.SourceGeneratorcarried a near-duplicate privateFormatDefaultValuehelper, and both returned"null"when the parameter's Roslyn-reported default-value constant wasnull. For a non-nullable value-type parameter declared with thedefaultliteral (e.g.CancellationToken ct = default), Roslyn reports the default asnull, so the generator emitted= null. That literal is invalid as the default for a value type and produces CS1750.Fix: when the constant is
null, branch on the parameter type. Non-nullable value types emit the baredefaultliteral (the compiler infers the target type from the parameter, matching the user's original declaration). Reference types andNullable<T>continue to emitnull.Unified the two private helpers into a single shared
DefaultValueFormatter.FormatDefaultValue(new fileGenerators/DefaultValueFormatter.cs, sibling to the existingCovarianceHelper). The helper takes abool useFullyQualifiedEnumNameparameter so each generator preserves its intentional enum-emit style:MethodAssertionGeneratorpassestrue(emitsNamespace.Enum.Memberto match its fully-qualified-everywhere style), andAssertionExtensionGeneratorpassesfalse(emitsEnum.Memberbecause it relies onusingdirectives at the top of its generated files). Both generators now share a single source of truth for default-value emit while keeping the call-site-specific behaviour explicit.Test coverage:
ValueTypeDefaultParametertest onMethodAssertionGeneratorTests([GenerateAssertion]path).ValueTypeDefaultParametertest onAssertionExtensionGeneratorTests([AssertionExtension]path).token = default,) and run a structural compile-clean gate.CompileChecker.AssertNoErrors(generatedFiles)test helper (sibling toReferencesHelper) so the compile-clean assertion isn't duplicated across the two new tests.Related Issue
Fixes #5917
Type of Change
Checklist
Required
TUnit-Specific Requirements
TUnit.Core.SourceGenerator)TUnit.Engine)TUnit.Core.SourceGenerator.Testsand/orTUnit.PublicAPItests.received.txtfiles and accepted them as.verified.txt.verified.txtfiles[DynamicallyAccessedMembers]annotationsdotnet publish -p:PublishAot=trueTesting
dotnet test)Additional Notes
.claude/docs/mandatory-rules.mdRule 1, dual-mode does not apply to assertion library changes. This PR fixes an emit defect inTUnit.Assertions.SourceGeneratoronly.useFullyQualifiedEnumNameparameter encodes the previously-duplicated semantic difference between the two generators, so neither generator's output for any existing case changes.CompileCheckertest helper: on the net472 leg of CI, the test project'sPolyfillassembly marksCallerArgumentExpressionAttributeasinternal, producing a CS0122 ('inaccessible due to its protection level') false positive when Roslyn compiles the generator's output through the test reference set. Conditional#if NETFRAMEWORKfilter pre-empts this in the helper. On modern TFMs the BCL attribute is public, so CS0122 (if it ever appears) remains a real failure signal. All four TFMs (net472 + net8.0 + net9.0 + net10.0) verified green locally before push.