fix(sourcegen): merge generic parameter lists in [AssertionExtension] emit#5921
Conversation
… emit AssertionExtensionGenerator emitted the covariant receiver-type parameter and the assertion class's own type parameter as two adjacent generic-parameter blocks, e.g. `<TActual><T>` rather than `<TActual, T>`. This is invalid C# syntax and fails to compile for any [AssertionExtension]-decorated class that derives from Assertion<ConcreteNonSealedType> while also declaring its own generic type parameter. Merge both lists into a single generic-parameter block before emitting the method signature. Add a ConcreteReceiverWithExtraGenericAssertion test fixture (anchored on System.Exception as the BCL non-sealed receiver) and a companion test method. The test pins the absence of adjacent generic-parameter blocks (DoesNotContain `"><"`) and adds an inline compile-clean gate: parse + compile the generated source through Roslyn and assert no error-severity diagnostic. The compile-clean check catches the whole class of emit-syntax bugs regardless of specific diagnostic id, so this regression cannot return silently.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
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
The fix is correct and the approach is sound. This is a clean, minimal change that directly addresses the root cause.
What the fix does
AssertionExtensionGenerator previously emitted two separate angle-bracket blocks when a class had both a covariant receiver type parameter (TActual) and its own generic parameters (T), producing invalid C# like Method<TActual><T>(...). The fix collects all generic parameters into a single List<string> and emits one merged <TActual, T> block. All four edge cases are handled correctly:
genericTypeParam |
genericParams |
Before | After |
|---|---|---|---|
TActual |
[T] |
<TActual><T> ❌ |
<TActual, T> ✅ |
TActual |
[] |
<TActual> ✅ |
<TActual> ✅ |
null |
[T] |
<T> ✅ |
<T> ✅ |
null |
[] |
`` ✅ | `` ✅ |
The genericParamsString variable is still correctly used for the return type (assertionType.Name + genericParamsString), so that path is unaffected.
Minor observations
DoesNotContain("><") assertion — this catches the bug, but the check is slightly broader than needed and could theoretically be confused by other generated content. The positive assertion (that the snapshot contains ConcreteReceiverWithExtraGenericMatches<TActual, T>() would be a more precise regression guard. That said, the snapshot file itself already pins the exact merged signature, so the compile gate + snapshot together are the real guarantees here; DoesNotContain("><") is a belt-and-suspenders check. No action required.
Comment verbosity — the inline comments in the generator and test are more detailed than this project typically uses (CLAUDE.md: "Only add one when the WHY is non-obvious"). The WHY is actually non-obvious here (a subtle codegen bug), so some comment is warranted — but the multi-paragraph blocks could be trimmed in a future pass. Not a blocker.
#if NETFRAMEWORK CS0122 filter — well-documented workaround; acceptable.
Summary
The fix is minimal, correct, and well-tested. The snapshot files across all four TFMs confirm the output is identical and valid. Approving.
|
Thanks! |
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.44.0 to 1.45.8. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.45.8 <!-- Release notes generated using configuration in .github/release.yml at v1.45.8 --> ## What's Changed ### Other Changes * fix(aspire): route CreateHttpClient through IHttpClientFactory by @thomhurst in thomhurst/TUnit#5957 ### Dependencies * chore(deps): update tunit to 1.45.0 by @thomhurst in thomhurst/TUnit#5949 * chore(deps): update dependency dompurify to v3.4.5 by @thomhurst in thomhurst/TUnit#5951 * chore(deps): update dependency microsoft.testing.extensions.codecoverage to 18.7.0 by @thomhurst in thomhurst/TUnit#5953 * chore(deps): update dependency coverlet.collector to 10.0.1 by @thomhurst in thomhurst/TUnit#5952 * chore(deps): update dependency polyfill to 10.6.0 by @thomhurst in thomhurst/TUnit#5955 * chore(deps): update dependency polyfill to 10.6.0 by @thomhurst in thomhurst/TUnit#5954 **Full Changelog**: thomhurst/TUnit@v1.45.0...v1.45.8 ## 1.45.0 <!-- Release notes generated using configuration in .github/release.yml at v1.45.0 --> ## What's Changed ### Other Changes * fix(generator): place CallerArgumentExpression before params in [GenerateAssertion] emit by @JohnVerheij in thomhurst/TUnit#5940 * fix(sourcegen): drop covariant TActual when [GenerateAssertion] method has its own type parameters by @JohnVerheij in thomhurst/TUnit#5935 * feat(assertions): add CancellationToken overload to WaitsFor and Eventually by @JohnVerheij in thomhurst/TUnit#5938 * fix(html-report): extract categories using MTP Key=name convention by @thomhurst in thomhurst/TUnit#5946 * feat(html-report): rewrite as split-pane design template by @thomhurst in thomhurst/TUnit#5947 ### Dependencies * chore(deps): update microsoft.testing to 2.2.3 by @thomhurst in thomhurst/TUnit#5927 * chore(deps): update mstest to 4.2.3 by @thomhurst in thomhurst/TUnit#5928 * chore(deps): update tunit to 1.44.39 by @thomhurst in thomhurst/TUnit#5929 * chore(deps): update aspire to 13.3.3 by @thomhurst in thomhurst/TUnit#5933 * chore(deps): update dependency dompurify to v3.4.4 by @thomhurst in thomhurst/TUnit#5944 * chore(deps): update dependency qs to v6.15.2 by @thomhurst in thomhurst/TUnit#5941 **Full Changelog**: thomhurst/TUnit@v1.44.39...v1.45.0 ## 1.44.39 <!-- Release notes generated using configuration in .github/release.yml at v1.44.39 --> ## What's Changed ### Other Changes * fix(tests): retry trx read to dodge MTP post-exit flush race on Windows by @thomhurst in thomhurst/TUnit#5888 * fix(pipeline): timeout + retry InstallPlaywrightModule so a hung download fails fast by @thomhurst in thomhurst/TUnit#5889 * fix(otel): require two consecutive idle windows in DrainAsync to catch in-transit POSTs by @thomhurst in thomhurst/TUnit#5890 * test(assertions): drop flaky wall-clock upper bound on WaitsFor timeout test by @thomhurst in thomhurst/TUnit#5886 * fix(sourcegen): drop spurious ')' in MethodAssertionGenerator Task<bool> emit by @JohnVerheij in thomhurst/TUnit#5920 * fix(sourcegen): merge generic parameter lists in [AssertionExtension] emit by @JohnVerheij in thomhurst/TUnit#5921 * fix(aspnetcore): scope correlation processor per-factory to stop cross-factory tag leak by @thomhurst in thomhurst/TUnit#5891 * Changed FSharp.Core version to 10.1.300 by @licon4812 in thomhurst/TUnit#5909 * feat(mocks): add Mock.HttpClientFactory() helper by @thomhurst in thomhurst/TUnit#5894 * Harden WaitsFor timeout test by @thomhurst in thomhurst/TUnit#5926 * fix(sourcegen): emit `default` literal for value-type assertion parameters by @JohnVerheij in thomhurst/TUnit#5919 ### Dependencies * chore(deps): update dependency nunit to 4.6.0 by @thomhurst in thomhurst/TUnit#5826 * chore(deps): update tunit to 1.44.0 by @thomhurst in thomhurst/TUnit#5882 * chore(deps): update dependency mockolate to 3.2.0 by @thomhurst in thomhurst/TUnit#5892 * chore(deps): update dependency yaml to v2.9.0 by @thomhurst in thomhurst/TUnit#5887 * chore(deps): update dependency nuget.protocol to 7.6.0 by @thomhurst in thomhurst/TUnit#5897 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.8 by @thomhurst in thomhurst/TUnit#5898 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.300 by @thomhurst in thomhurst/TUnit#5899 * chore(deps): update microsoft.extensions by @thomhurst in thomhurst/TUnit#5905 * chore(deps): update microsoft.aspnetcore to 10.0.8 by @thomhurst in thomhurst/TUnit#5904 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.300 by @thomhurst in thomhurst/TUnit#5902 * chore(deps): update aspire to 13.3.1 by @thomhurst in thomhurst/TUnit#5900 * chore(deps): update dependency system.commandline to 2.0.8 by @thomhurst in thomhurst/TUnit#5903 * chore(deps): update dependency azure.storage.blobs to 12.28.0 by @thomhurst in thomhurst/TUnit#5910 * chore(deps): update dependency dotnet-sdk to v10.0.300 by @thomhurst in thomhurst/TUnit#5901 * chore(deps): update dependency stackexchange.redis to 2.13.1 by @thomhurst in thomhurst/TUnit#5906 * chore(deps): update aspire to 13.3.2 by @thomhurst in thomhurst/TUnit#5924 * chore(deps): bump mermaid from 11.12.2 to 11.15.0 in /docs by @dependabot[bot] in thomhurst/TUnit#5893 * chore(deps): update dependency streamjsonrpc to 2.24.92 by @thomhurst in thomhurst/TUnit#5915 * chore(deps): update dependency dompurify to v3.4.3 by @thomhurst in thomhurst/TUnit#5913 * chore(deps): update microsoft.build to 18.6.3 by @thomhurst in thomhurst/TUnit#5914 **Full Changelog**: thomhurst/TUnit@v1.44.0...v1.44.39 Commits viewable in [compare view](thomhurst/TUnit@v1.44.0...v1.45.8). </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>
Description
AssertionExtensionGeneratoremitted the covariant receiver-type parameter (when applied) and the assertion class's own generic parameter as two adjacent angle-bracket blocks in the generated extension method signature, for exampleMatches<TActual><T>(this IAssertionSource<TActual> source, ...). That's not valid C#; the generated file fails to parse for any[AssertionExtension]-decorated class that derives fromAssertion<ConcreteNonSealedType>while also declaring its own generic type parameter.Fix: collect both the covariant receiver parameter and the class's own generic parameters into a single list and emit one
<...>block before the method signature opens.Test coverage:
ConcreteReceiverWithExtraGenericAssertiontest fixture inTestData/. UsesSystem.Exceptionas the receiver. Exception is a BCL non-sealed class that the generator's existing covariance-candidate check recognises, so the fixture trips the affected branch without adding a stand-in type to the test surface.ConcreteReceiverWithExtraGenerictest onAssertionExtensionGeneratorTests. Pins the absence of adjacent generic-parameter blocks (DoesNotContain "><") and runs an inline Roslyn compile-clean gate so the whole class of emit-syntax bugs is pinned regardless of which specific diagnostic id the parser produces.Related Issue
Fixes #5918
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.TActualat every site) is a separate design call. It's tracked as a follow-up issue rather than expanded here.Polyfillassembly 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. 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.defaultliteral for value-type assertion parameters") introduces a sharedCompileChecker.AssertNoErrorstest helper. Both PRs were authored in parallel, so the helper isn't onmainyet, and the inline form here matches the helper's body. Can rebase this branch and replace the inline copy with the shared helper once Bug 1 merges, whichever order you prefer to land them in.