feat: TUnit0074 analyzer for redundant hook attributes on overrides

[thomhurst]

Summary

Replaces the runtime hook deduplication merged in #5441 with a compile-time analyzer that surfaces the duplication to the author and also catches the [InheritsTests] variant from #5450.

When a method declares [Before(Test)] / [After(Test)] and overrides a base method that already declares the same hook attribute, both registrations are invoked via virtual dispatch — the override's body runs twice per test. The previous fix deduplicated this silently at runtime via MethodInfo.GetBaseDefinition(); this PR errors at compile time instead.

• New analyzer TUnit0074 (Error, Usage): walks the full IMethodSymbol.OverriddenMethod chain so transitive cases (gap in the middle of the chain) are also caught. The [InheritsTests] + abstract intermediate shape from [Bug]: Virtual hook methods are called for base classes in inherited tests #5450 fails to compile at the intermediate's override.
• Code fix: removes the redundant attribute. If it was the only attribute in the list, the whole list goes too (with KeepNoTrivia so we don't leave a stray blank line above the method).
• Reverted runtime dedup: dropped InstanceHookMethod.BaseDefinition, ResolveBaseDefinition / IsOverriddenByMoreDerivedHook in HookDelegateBuilder, and the GetBaseDefinition() init in ReflectionHookDiscoveryService. PublicAPI snapshots updated for all 4 TFMs.
• Regression tests (VirtualHookOverrideTests + Bugs/5450/InheritsTestsVirtualHookOverrideTests) rewritten to the "attribute only on base, override without attribute" shape — the only shape TUnit0074 still allows. Both run with [EngineTest(ExpectedResult.Pass)] so they're exercised in source-gen and reflection modes. Assertions are inlined inside the hook bodies because After-hooks are sorted derived-class-first across the type hierarchy, so a separate verification hook on the derived class would run before the base teardown.

Closes #5450. Supersedes the runtime fix from #5441.

Test plan

• TUnit.Analyzers.Tests — 8 new test cases under VirtualHookOverrideAnalyzerTests covering: Before+Before error, After+After error, attribute only on base (ok), attribute only on override (ok), abstract intermediate + InheritsTests (error on intermediate), chain with gap A(attr)→B(no)→C(attr) (error on C via transitive ancestor), mismatched hook types (ok), new method (ok). Full analyzer suite passes.
• TUnit.PublicAPI snapshot tests pass on all 4 TFMs after removing the BaseDefinition line.
• VirtualHookOverrideTests.DerivedTestClass.Override_Runs_Exactly_Once passes in source-gen and reflection modes on net10.0.
• Bugs/5450/InheritsTestsVirtualHookOverrideTests (both ConcreteTestClassA and ConcreteTestClassB) pass in source-gen and reflection modes on net10.0.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 17 complexity

Metric	Results
Complexity	17

View in Codacy

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

[claude]

Code Review: TUnit0074 — Compile-time analyzer for redundant hook attributes on overrides

Overall this is a well-reasoned improvement: promoting a silent runtime workaround (#5441) to a loud compile-time error is strictly better UX and removes the complexity from the hot path. The implementation is clean and the test coverage is comprehensive. A few observations below.

---

Architecture concern: runtime dedup removal leaves a gap for pre-compiled assemblies

The most significant concern is that the runtime dedup is removed unconditionally, while the analyzer only guards against newly compiled code.

Consider the scenario:

1. A test helper library TestBase.dll was compiled before TUnit0074 ships, and it contains the double-attribute pattern (base + override both declare [Before(Test)]).
2. A consuming project upgrades TUnit to the version that includes this PR.
3. The consuming project gets no compiler error (the offending attribute is in the DLL, not in their source).
4. At runtime, TUnit discovers both hook registrations and — without the runtime dedup — invokes the override's body twice per test.

The previous approach handled this transparently for any inheritance shape, regardless of where the attribute was compiled. The new approach only prevents it for source the user can recompile. This tradeoff is probably acceptable for the common case (people typically recompile when upgrading TUnit), but it's worth at least a note in the analyzer description or a migration guide entry, and possibly a [Obsolete] warning in a transitional release rather than hard removal.

An alternative that keeps both layers would be to retain the runtime dedup as a silent fallback but also add the analyzer error. That way, code that slips through at compile time (pre-compiled libraries, generated code) is still protected at runtime. The runtime cost is minimal since well-behaved code exits the loop immediately.

---

hookType!.Value — the suppression is technically correct but fragile

In VirtualHookOverrideAnalyzer.cs:

var conflictingBase = FindBaseWithMatchingHook(methodSymbol, hookType!.Value, context.Compilation);

IsStandardHook is annotated [NotNullWhen(true)] on out HookType? hookType, so the ! is correct — the compiler just can't flow-analyse through the subsequent continue on hookLevel. This works but is relying on a null-forgiving operator rather than the normal control flow. A cleaner alternative:

if (!attributeData.IsStandardHook(compilation, out _, out var hookLevel, out var hookType)
    || hookLevel != HookLevel.Test
    || hookType is null)
{
    continue;
}

var conflictingBase = FindBaseWithMatchingHook(methodSymbol, hookType.Value, context.Compilation);

Combining the guards into one if lets the compiler track nullability without needing !, making the intent clearer.

---

Code fix — KeepNoTrivia silently discards leading comments

In VirtualHookOverrideCodeFixProvider.cs:

newRoot = root.RemoveNode(attributeList, SyntaxRemoveOptions.KeepNoTrivia)!;

KeepNoTrivia is the right choice to avoid leaving a stray blank line, but it also silently discards any leading trivia (e.g. a // Setup for test X comment placed directly above the attribute). KeepLeadingTrivia would preserve that comment but re-attach it to the next node (the method declaration), which is arguably more correct. Worth considering which behaviour is preferred here — losing a comment is surprising.

---

Positives worth noting

• Transitive chain detection (FindBaseWithMatchingHook walks the full OverriddenMethod chain) correctly handles the A→B(no attr)→C(attr) gap shape that a simple OverriddenMethod check would miss.
• new method exclusion is handled by the IsOverride: true guard at the top — the test case confirms this.
• Mismatched hook types (Before on base, After on override) are correctly allowed since they register distinct lifecycle phases.
• [InheritsTests] regression (Bugs/5450) is tested through the concrete subclass shape — the inline assertion pattern is a smart workaround for the After-hook ordering constraint.
• Public API snapshots updated across all four TFMs — no drift.
• Analyzer release notes updated (AnalyzerReleases.Unshipped.md) — good hygiene.

The resource strings give a clear, actionable error message that explains why the attribute is redundant and what to do instead.

---

Summary

The compile-time approach is the right long-term direction. The main architectural question is whether to fully remove the runtime fallback (acceptable if you're comfortable with the pre-compiled-assembly gap) or retain it as a backstop alongside the analyzer error. The other two items (hookType!.Value cleanup, trivia handling in the code fix) are minor quality improvements but not blockers.

[claude]

Follow-up Review: TUnit0074 (after commit ed5c622)

The follow-up commit addresses two of the three items from the previous review — good turnaround.

Previous concerns: status

✅ Addressed — null guard cleanup in analyzer
The hookType!.Value null-forgiving operator is gone. The combined guard now reads:

if (!attributeData.IsStandardHook(context.Compilation, out _, out var hookLevel, out var hookType)
    || hookLevel != HookLevel.Test
    || hookType is null)
{
    continue;
}

The compiler can track non-nullability into hookType.Value without any suppression. Clean.

✅ Addressed — KeepLeadingTrivia in code fix
The single-attribute removal path now uses KeepLeadingTrivia, preserving any comment placed directly above the attribute list and re-attaching it to the method declaration. The inline comment explains the intent clearly.

⚠️ Still open — runtime dedup removed, pre-compiled assemblies unprotected
The concern from the previous review remains: if a library compiled against an older TUnit version ships the double-attribute pattern in a DLL, consumers who upgrade to this version of TUnit will hit the double-invocation bug at runtime with no compiler error to guide them, because the analyzer only fires on source being compiled.

This is a deliberate tradeoff (compile-time is the right long-term home for this guard), but it deserves explicit documentation or a migration note — ideally in the TUnit0074 description or a CHANGELOG entry — so library authors know they need to recompile and fix their sources, not just upgrade the package.

---

No new concerns

The rest of the implementation looks solid:

• The full-chain walk in FindBaseWithMatchingHook correctly handles the A→B(no attr)→C(attr) transitive case.
• BatchFixer for GetFixAllProvider is the right choice; it handles multiple diagnostics in the same document atomically.
• The analyzer correctly skips non-Test hook levels (Assembly/Class hooks are enforced static elsewhere, so virtual dispatch isn't a factor for them).
• Test coverage for all 8 distinct shapes (error cases, no-error cases, abstract intermediate, transitive chain, mismatched types, new shadow) is thorough.

The architecture is sound. The only remaining action item is documenting the pre-compiled-assembly gap so library authors are aware of it.