perf: filter generated attributes to TUnit-related types only

[thomhurst]

Summary

• Replace blacklist-based attribute filtering (ShouldSkipCompilerInternalAttribute, ShouldSkipFrameworkSpecificAttribute) with a whitelist approach that only emits attributes defined in, inheriting from, or implementing a type in a TUnit namespace
• Eliminates redundant attributes from generated source (e.g., AssemblyFileVersionAttribute, UnconditionalSuppressMessageAttribute, NullableContextAttribute, CompilerGeneratedAttribute, ParamArrayAttribute)
• Removes the WellKnownTypes dependency from AttributeWriter (was only used by the old filtering methods)

Test plan

• Source generator snapshot tests pass (460/460) across all TFMs (net8.0, net9.0, net10.0, net472)
• Updated 2 snapshot files where UnconditionalSuppressMessageAttribute was correctly removed from generated output
• Verify user-defined attributes that inherit from TUnit types (e.g., custom SkipAttribute subclass) still appear in generated code

[claude]

Code Review

Overall: Solid improvement. The whitelist approach is architecturally superior to the old blacklist — it's opt-in by design, self-documenting, and immune to the "what should I add to the blocklist next?" maintenance burden. The ~80-line reduction speaks for itself.

---

Design: Whitelist vs. Blacklist ✅

The previous approach had two layered blocklists (ShouldSkipCompilerInternalAttribute + ShouldSkipFrameworkSpecificAttribute) that grew organically as edge cases surfaced. The new approach inverts the question: "Is this attribute related to TUnit?" Any non-TUnit attribute is implicitly excluded, which is the correct default for a test metadata generator.

---

Potential Issue: Namespace Walking Direction

The IsInTUnitNamespace helper walks from the innermost namespace segment outward:

while (ns is { IsGlobalNamespace: false })
{
    if (ns.Name == TUnitRootNamespace)  // TUnit
        return true;
    ns = ns.ContainingNamespace;
}

This means a user-defined namespace like MyCompany.TUnit.Helpers would match, because traversal hits the TUnit segment mid-walk. In practice this is unlikely to cause problems, but if you want to be precise about "rooted at TUnit", you could check the outermost non-global ancestor:

private static bool IsInTUnitNamespace(INamedTypeSymbol type)
{
    var ns = type.ContainingNamespace;
    INamespaceSymbol? outermost = null;

    while (ns is { IsGlobalNamespace: false })
    {
        outermost = ns;
        ns = ns.ContainingNamespace;
    }

    return outermost?.Name == TUnitRootNamespace;
}

That said, anyone naming a namespace MyCompany.TUnit.X is making an unusual choice, and even if their attributes were included they'd probably work fine. Flagging this as low-priority but worth being aware of.

---

Unrelated BOM Change in Snapshot File

The diff for MethodDataSourceDrivenWithCancellationTokenTests.Test.verified.txt includes an encoding change:

-// <auto-generated/>
+// <auto-generated/>

The added  is a UTF-8 BOM character. This is unrelated to the attribute filtering and could cause noise in diffs. Worth checking if this was intentional or a side effect of a tool/editor setting.

---

Caching ✅

Using SymbolEqualityComparer.Default for the _tunitRelatedCache is exactly right — Roslyn symbols must not be compared by reference. The cache correctly avoids repeated hierarchy walks for the same attribute type.

Removed WellKnownTypes Dependency ✅

Clean separation: AttributeWriter no longer needs to resolve arbitrary framework types by name. The old approach was inherently fragile (string-based FQN prefix matching).

Interface Check (AllInterfaces) ✅

Including attributes that implement a TUnit interface (not just inherit a TUnit base class) is correct and future-proof — e.g., attributes that implement IDataSourceAttribute or similar contracts will be captured.

---

Summary

Architecture	Whitelist approach is the right call
Correctness	Handles inheritance and interface implementations correctly
Performance	Cache added with correct comparer
Risk	Low-priority namespace false-positive edge case; unrelated BOM change

The unchecked test plan item ("Verify user-defined attributes that inherit from TUnit types") is the most important functional validation — the code logic is correct for it, but it's worth ensuring there's a snapshot test covering a custom SkipAttribute subclass so this contract is locked in.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -11 complexity

Metric	Results
Complexity	-11

View in Codacy

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

[claude]

Follow-up Review

The unchecked test plan item from the original PR description ("Verify user-defined attributes that inherit from TUnit types") has been addressed — good. Here's a focused review of the additions.

---

New Test: ✅

The snapshot test correctly covers the multi-level inheritance case. Both attributes in the test exercise the whitelist logic through inherited chains:

• EngineTest → PropertyAttribute (TUnit.Core) → TUnitAttribute (TUnit.Core) — matched via GetSelfAndBaseTypes()
• SkipNetFrameworkAttribute → SkipAttribute (TUnit.Core) — same path

Both appear correctly in the snapshot output, confirming the IsTUnitRelatedAttribute logic handles transitive base-class inheritance as intended.

---

Empty Test Body

The test body has no explicit assertions:

async generatedFiles =>
{
});

This relies entirely on the snapshot comparison for correctness. That's consistent with the rest of the snapshot test suite, but worth noting: if someone accidentally regenerates the snapshot with broken output, the test will silently accept it. A minimal assertion like Assert.That(generatedFiles.Any(...)) would make the intent explicit. This is low-priority — the snapshot is self-documenting — but other snapshot tests in the project do add explicit generatedFiles assertions to catch regressions more clearly.

---

Previous BOM Concern — Resolved

The previous review flagged a UTF-8 BOM change in MethodDataSourceDrivenWithCancellationTokenTests.Test.verified.txt. It does not appear in the current diff, so either the concern was premature or it has been addressed.

---

Namespace Walk — Confirmed Correct

The IsInTUnitNamespace implementation correctly walks to the outermost non-global namespace segment, which was the approach suggested previously. No change needed there.

---

Overall: The PR is in good shape. The whitelist approach is correct, the cache is properly keyed, and the new snapshot test locks in the inheritance contract. The empty test body is the only minor remaining note.

[claude]

Follow-up Review

The unchecked test plan item from the original PR description ("Verify user-defined attributes that inherit from TUnit types") has been addressed — good. Here is a focused review of the additions.

---

New Test: CustomAttributeInheritanceTests ✅

The snapshot test correctly covers the multi-level inheritance case. Both attributes in the test exercise the whitelist logic through inherited chains:

• EngineTest → PropertyAttribute (TUnit.Core) → TUnitAttribute (TUnit.Core) — matched via GetSelfAndBaseTypes()
• SkipNetFrameworkAttribute → SkipAttribute (TUnit.Core) — same path

Both appear correctly in the snapshot output, confirming the IsTUnitRelatedAttribute logic handles transitive base-class inheritance as intended.

---

Empty Test Body

The test body has no explicit assertions:

async generatedFiles =>
{
});

This relies entirely on the snapshot comparison for correctness. That is consistent with the rest of the snapshot test suite, but worth noting: if someone accidentally regenerates the snapshot with broken output, the test will silently accept it. A minimal assertion like Assert.That(generatedFiles.Any(...)) would make the intent explicit. This is low-priority since the snapshot is self-documenting, but other snapshot tests in the project do add explicit generatedFiles assertions to catch regressions more clearly.

---

Previous BOM Concern — Resolved

The previous review flagged a UTF-8 BOM change in MethodDataSourceDrivenWithCancellationTokenTests.Test.verified.txt. It does not appear in the current diff, so either the concern was premature or it has been addressed.

---

Namespace Walk — Confirmed Correct

The IsInTUnitNamespace implementation correctly walks to the outermost non-global namespace segment, which was the approach suggested previously. No change needed there.

---

Overall: The PR is in good shape. The whitelist approach is correct, the cache is properly keyed, and the new snapshot test locks in the inheritance contract. The empty test body is the only minor remaining note.