feat(analyzers): code-fix for TUnit0015 to insert CancellationToken (#5613 D)

[thomhurst]

Summary

Part D of #5613 (final). Adds a Roslyn code-fix for TUnit0015 (MissingTimeoutCancellationTokenAttributes) with three user-selectable actions, mirroring the shape suggested in the issue:

1. Add CancellationToken parameter — bare parameter, user wires it up.
2. Add CancellationToken parameter with ThrowIfCancellationRequested — prepends the call so the test actually observes cancellation.
3. Add CancellationToken parameter as discard — prepends _ = cancellationToken; to silence IDE0060 / CA1801 under TreatWarningsAsErrors.

All three add using System.Threading; if it's missing and re-format the affected method so indentation matches surrounding code.

Test plan

• Adds_CancellationToken_Parameter — bare parameter
• Adds_CancellationToken_Parameter_With_ThrowIfCancellationRequested — prepends the call
• Adds_CancellationToken_Parameter_As_Discard — prepends _ = cancellationToken;
• Manual: trigger TUnit0015 in an IDE and apply each of the three code-fixes

[claude]

Code Review: feat(analyzers): code-fix for TUnit0015

Overall this is a clean, well-structured code fix provider. The three-action design (bare param / ThrowIfCancellationRequested / discard) directly maps to the real use cases users will encounter. A few issues worth addressing before merging.

---

Issue 1: Expression-bodied methods silently ignored (bug)

// AddCancellationTokenAsync — only handles block bodies:
if (bodyMode != BodyMode.None && updated.Body is { } body)
{
    // ... only runs for block-bodied methods
}

If the test method uses an expression body (e.g. public Task MyTest() => SomethingAsync();), the ThrowIfCancellationRequested and Discard modes silently skip the statement insertion. The user picks one of those actions expecting a statement to be prepended, and it isn't. The bare-param fix still works, but only that one.

Better approach — either convert the expression body to a block body first, or skip registering the body-modifying actions entirely when method.ExpressionBody != null. Silently doing nothing is worse than not offering the action:

// Option A: skip body-mode actions for expression-bodied methods
if (method.Body != null)
{
    context.RegisterCodeFix(/* ThrowIfCancellationRequested action */, diagnostic);
    context.RegisterCodeFix(/* Discard action */, diagnostic);
}

// Option B: rewrite expression body → block body then prepend statement
var blockBody = SyntaxFactory.Block(
    SyntaxFactory.ReturnStatement(method.ExpressionBody!.Expression));
updated = updated.WithBody(blockBody).WithExpressionBody(null).WithSemicolonToken(default);
// then prepend statement as normal

Option A is simpler and safer; Option B gives the user the richer fix.

---

Issue 2: Using directive appended at end, not sorted

private static CompilationUnitSyntax EnsureSystemThreadingUsing(CompilationUnitSyntax compilationUnit)
{
    // ...
    return compilationUnit.AddUsings(newUsing); // always appends at the end
}

AddUsings appends to the end of the using list, producing:

using TUnit.Core;
using System.Threading.Tasks;
using System.Threading;   // ← wrong position under alphabetical or System-first ordering

Projects using StyleCop, Roslyn's own IDE0210, or .editorconfig dotnet_separate_import_directive_groups will immediately re-flag this. The fix should insert the using in sorted position (or at minimum within the System.* group). The Roslyn workspace Formatter won't reorder usings.

Better approach — find the insertion index by comparing directive names:

var newUsing = SyntaxFactory.UsingDirective(SyntaxFactory.ParseName(SystemThreadingNamespace))
    .WithTrailingTrivia(SyntaxFactory.ElasticCarriageReturnLineFeed);
var insertAt = compilationUnit.Usings
    .TakeWhile(u => string.Compare(u.Name?.ToString(), SystemThreadingNamespace, StringComparison.Ordinal) < 0)
    .Count();
return compilationUnit.WithUsings(compilationUnit.Usings.Insert(insertAt, newUsing));

---

Issue 3: Missing test coverage for edge cases

The three existing tests are good, but they all share the same method shape. Worth adding:

1. Already has using System.Threading; — verify no duplicate is added.
2. Method already has parameters — verify CancellationToken is appended after existing params, not prepended.
3. Expression-bodied method — either verifies the action is not offered, or verifies the expression body is converted (depending on the fix chosen for Issue 1 above).

---

Minor: ParseTypeName for a known simple identifier

// Current:
.WithType(SyntaxFactory.ParseTypeName(CancellationTokenTypeName).WithTrailingTrivia(SyntaxFactory.Space))

// Slightly cleaner:
.WithType(SyntaxFactory.IdentifierName(CancellationTokenTypeName).WithTrailingTrivia(SyntaxFactory.Space))

ParseTypeName invokes the full parser; IdentifierName is direct. Minor, but since the type name is a known simple identifier this avoids unnecessary work.

---

What's good

• Three-action design maps cleanly to real user intent — this is exactly what the IDE should offer.
• GetFixAllProvider returning BatchFixer is correct for this fix shape.
• Formatter.Annotation + Formatter.FormatAsync is the right pattern to let the workspace normalise indentation.
• EnsureSystemThreadingUsing is clean and avoids adding duplicates for the common case.
• Tests use the correct CSharpCodeFixVerifier pattern with equivalenceKey to target specific actions.

The expression-body silent-skip (Issue 1) and the using sort order (Issue 2) are both user-visible bugs worth fixing before this lands.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 33 complexity

Metric	Results
Complexity	33

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Follow-up Review: All Previous Issues Addressed ✅

All four concerns from the first review have been resolved — this is ready to merge.

Previous Issues — Status

Issue 1 (Expression-bodied methods silently ignored) — Fixed ✅
Body-modifying code fixes are now only registered when method.Body is not null, so expression-bodied methods only see the bare-parameter action. Correct choice (Option A) and now covered by Expression_Bodied_Method_Only_Offers_Bare_Parameter_Action.

Issue 2 (Using directive appended unsorted) — Fixed ✅
EnsureSystemThreadingUsing now performs a sorted insertion within the System.* group. The logic is correct for the common cases.

Issue 3 (Missing test coverage) — Fixed ✅
Three new tests added: Does_Not_Duplicate_Existing_System_Threading_Using, Appends_CancellationToken_After_Existing_Parameters, and Expression_Bodied_Method_Only_Offers_Bare_Parameter_Action. All edge cases from the prior review are now exercised.

Minor (ParseTypeName → IdentifierName) — Fixed ✅
Uses SyntaxFactory.IdentifierName as suggested.

---

One Minor Edge Case to Be Aware Of

EnsureSystemThreadingUsing has an uncommon edge case: if non-System usings are interspersed between System.* usings (e.g. using System.Text;, using Z;, using System.Threading.Tasks;), the inserted System.Threading can land adjacent to the non-System using rather than grouped with the other System ones. Example result:

using System.Text;
using Z;
using System.Threading; // ← inserted between Z and System.Threading.Tasks
using System.Threading.Tasks;

This is a very contrived scenario (no real project organises usings this way), so it does not need to block the PR — just noting it for awareness if a future report comes in.

---

What's Well Done

• Three-action design (bare / ThrowIfCancellationRequested / discard) maps exactly to real user intent.
• Guard on method.Body is not null before registering body-modifying actions is clean and prevents confusing UX.
• Formatter.Annotation + Formatter.FormatAsync round-trip is idiomatic Roslyn.
• Test matrix is now comprehensive for a code-fix provider of this scope.

LGTM. 🟢

[claude]

Code Review: TUnit0015 Code-Fix Provider

This is a well-structured addition that follows the established patterns in the codebase. The three-action UX design is the right call — users have genuinely different intents (use the token, discard-silence it, or wire it themselves).

Good

• Mirrors the shape of VirtualHookOverrideCodeFixProvider cleanly.
• 7 test cases with good coverage: bare, throw, discard, no-dup-using, existing params, sorted-insertion, expression-bodied guard.
• method.Body is not null guard for expression-bodied methods is correct and well-explained.
• GetFixAllProvider() returning BatchFixer is the right default.
• ConfigureAwait(false) throughout.

Issue: EnsureSystemThreadingUsing is blind to global using

// Only checks file-level usings:
foreach (var usingDirective in compilationUnit.Usings)

If the consuming project has global using System.Threading; (e.g., in a GlobalUsings.cs), the fix will redundantly add using System.Threading; to the file. The semantic model is the right tool here:

private static async Task<bool> IsSystemThreadingAlreadyImported(Document document, CancellationToken ct)
{
    var semanticModel = await document.GetSemanticModelAsync(ct).ConfigureAwait(false);
    if (semanticModel is null) return false;
    var ns = semanticModel.Compilation.GetTypeByMetadataName("System.Threading.CancellationToken");
    // Check if any using alias or global using makes CancellationToken accessible without qualification
    // Simpler: check SemanticModel's available symbols or rely on compilation's GetUsedNamespaces
    return false; // fall back to syntactic check if semantic unavailable
}

A lighter-weight workaround that avoids the perf cost of a full semantic model: scan for global using System.Threading in compilationUnit.Usings (they have GlobalKeyword set) in addition to the normal usings. This at least covers the same-file global using, though cross-file global usings would still require the semantic model.

Minor: hardcoded "\n" line ending

.WithTrailingTrivia(SyntaxFactory.ElasticEndOfLine("\n"))

On CRLF files the Formatter will likely normalise this anyway, but SyntaxFactory.ElasticLineFeed (or detecting the document's line ending via document.Project.Solution.Workspace.Options) would be more correct. Low risk in practice since Formatter.FormatAsync follows through.

Suggestion: consider offering the fix for the CancellationTokenMustBeLastParameter diagnostic too

The analyzer reports two separate diagnostics:

• MissingTimeoutCancellationTokenAttributes — token absent (handled here)
• CancellationTokenMustBeLastParameter — token present but not last (no fix exists)

A companion fix that reorders the existing parameter to last would complete the pair and use almost identical infrastructure. Not a blocker for this PR, but worth a follow-up issue.

Verdict

Approved. The global-using gap is a real correctness issue but narrow in practice (requires a project-wide global using System.Threading while also lacking a file-level one). Worth a quick follow-up, but it should not block merging.

[claude]

Code Review: Final Pass — All Previous Issues Addressed ✅

This is now in excellent shape. The fourth commit addresses the global-using gap raised in the third review, and the fix is well-chosen.

Previous Issues — Final Status

Issue	Status
Expression-bodied method silent no-op	✅ Fixed (review 1)
Using directive appended unsorted	✅ Fixed (review 1)
Missing test coverage (dup-using, existing params, expression-bodied)	✅ Fixed (review 2)
ParseTypeName → IdentifierName	✅ Fixed (review 2)
EnsureSystemThreadingUsing blind to global using	✅ Fixed (this commit)
Hardcoded ElasticEndOfLine("\n")	✅ Fixed (this commit) — ElasticLineFeed used

---

What the Latest Commit Gets Right

Semantic scope check is the correct design. Using semanticModel.LookupSymbols(method.SpanStart, name: CancellationTokenTypeName) uniformly covers file-level usings, same-file global usings, cross-file global usings, and SDK ImplicitUsings — all via one call. The syntactic loop that remains inside EnsureSystemThreadingUsing now serves as a defence-in-depth fallback for when the semantic model is unavailable (null path returns false, so that branch still runs and the syntactic check prevents duplicates). That layering is correct.

New test is appropriately scoped. Does_Not_Add_Using_When_System_Threading_Is_Global_Using_In_Other_File exercises the cross-file case explicitly; the interspersed-namespace regression test from the previous commit locks in the sorted-insertion edge case.

---

One Observation (not a blocker)

EnsureSystemThreadingUsing uses SyntaxFactory.ParseName(SystemThreadingNamespace) for the using directive's name. For a dotted namespace this works correctly (produces a QualifiedNameSyntax), but it does invoke the full parser for a string that could be constructed directly:

// Current — invokes full parser:
SyntaxFactory.ParseName(SystemThreadingNamespace)

// Direct equivalent:
SyntaxFactory.QualifiedName(
    SyntaxFactory.IdentifierName(System),
    SyntaxFactory.IdentifierName(Threading))

This is purely a micro-consistency note (you already swapped ParseTypeName → IdentifierName for the parameter type) — the parser result is identical and FormatAsync normalises either way. Not worth a follow-up commit.

---

What's Well Done

• Three-action UX (bare / throw / discard) maps exactly to real user intent.
• method.Body is not null guard prevents confusing no-ops on expression-bodied methods.
• Sorted insertion within System.* group is correct for the common cases and regression-tested.
• Semantic scope check covers all import mechanisms with a single call.
• 8 test cases across a well-chosen matrix; each exercises a distinct behaviour.
• ConfigureAwait(false) throughout; GetFixAllProvider returns BatchFixer.

LGTM. 🟢