fix(analyzers): decouple code fixers from Rules to prevent MissingFieldException in VS

[thomhurst]

Fixes #6157

Root cause

TUnit.Analyzers.CodeFixers.dll ships in the version-agnostic analyzers/dotnet/cs folder while TUnit.Analyzers.dll ships per-Roslyn (analyzers/dotnet/roslyn4.x/cs), and the code fixers reference the analyzers assembly at runtime (ProjectReference, resolved by simple name — the assemblies are not strong-named).

Visual Studio cannot unload analyzer assemblies. After a TUnit package update without a VS restart (or with mixed TUnit versions in one VS session), the new code fixers bind against a stale TUnit.Analyzers.dll. VS evaluates FixableDiagnosticIds eagerly on every lightbulb pass, which JITs the field reference Rules.MSTestMigration / Rules.NUnitMigration — fields added in #3195 that the stale assembly lacks — throwing MissingFieldException "at random parts of coding".

The heavy migration machinery (TUnit.Analyzers.Migrators.Base.*) is only touched lazily inside RegisterCodeFixesAsync, which can only run when the matching (new) analyzer reported the diagnostic — so decoupling the eager members removes the failure mode entirely.

The same latent landmine existed in TUnit.Assertions.Analyzers.CodeFixers.

Fix

• New DiagnosticIds classes with public const string per rule in TUnit.Analyzers and TUnit.Assertions.Analyzers; Rules.cs consumes the constants (single source of truth).
• All code fixers now use DiagnosticIds.X (consts are baked into the consuming IL at compile time) instead of Rules.X.Id; the two lazy Rules.X.Title usages replaced with literals. Both code fixer assemblies now carry zero references to the Rules type.
• IL-level regression tests (System.Reflection.Metadata) assert the built code fixer dlls contain no TypeReference to Rules — any future Rules.X.Id reintroduction fails the test.

No behavior change: same diagnostic IDs, same code-fix titles (copied from resx).

Testing

• TUnit.Analyzers.Tests: 705 passed, 0 failed, 1 skipped (pre-existing skip)
• TUnit.Assertions.Analyzers.CodeFixers.Tests: 19 passed, 0 failed
• All three Roslyn variant projects (4.4 / 4.7 / 4.14) build clean with the source-linked DiagnosticIds.cs

User-facing note

Existing affected sessions are environmental (stale assembly already loaded): restarting VS after updating TUnit clears it. This change makes future version-skew sessions benign.

[claude]

Code Review

This is a well-crafted fix for a subtle cross-assembly binding issue. The root cause analysis in the PR description is thorough, and the solution is architecturally correct.

What the fix does right

The const choice is the key design decision. Using public const string in DiagnosticIds (rather than static readonly) is deliberate and essential: the C# compiler inlines const values at the call site, so the consuming code fixer assembly ends up with zero IL TypeReferences to DiagnosticIds or Rules. static readonly would not have worked — it generates a runtime field load and keeps the type reference. This is exactly the right tool for this constraint.

IL-level regression tests are excellent. Using System.Reflection.Metadata / PEReader to assert the built assembly contains no TypeReference to Rules is a strong, code-path-independent guarantee. It prevents regressions that pass all behavioral tests but reintroduce the coupling (e.g., someone using Rules.X.Id inside a lazy helper that happens to be called only from RegisterCodeFixesAsync).

Both packages are fixed consistently. TUnit.Analyzers and TUnit.Assertions.Analyzers receive the same treatment.

Documentation in BaseMigrationCodeFixProvider is well-placed. The XML doc comment on DiagnosticId explains the constraint directly where a future contributor would override it.

---

One thing worth adding to DiagnosticIds.cs

The regression tests guard against Rules references, but they don't verify that DiagnosticIds constants remain const rather than drifting to static readonly. If someone later changes a field to static readonly (perhaps to derive a value), the compile-time baking guarantee silently breaks without failing the existing tests. Consider adding a comment near the top of each DiagnosticIds class:

// IMPORTANT: All members MUST be 'const', not 'static readonly'.
// consts are baked into consuming assemblies at compile time, eliminating
// the runtime TypeReference that causes MissingFieldException in VS after
// a package update (see issue #6157). Changing to 'static readonly' would
// silently reintroduce the coupling without failing the IL regression tests.

This is a low-cost addition that preserves the intent for future maintainers.

---

Minor pre-existing inconsistency (not introduced here, not a blocker)

Rules.cs has two fields without readonly:

• public static DiagnosticDescriptor InstanceTestMethod
• public static DiagnosticDescriptor MatrixDataSourceAttributeRequired

All other DiagnosticDescriptor fields use public static readonly. These mutable static fields are probably accidental. Worth fixing in a follow-up to close the door on mutation bugs.

---

Verdict

Approve. The fix is architecturally sound, the tests are creative and strong, and the change has no behavior impact (same IDs, same titles). The comment suggestion above is the only material addition worth considering.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{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.}

[claude]

Follow-up review

A previous review already approved this change and covered the design well. One additional finding worth flagging before merge:

Pre-existing diagnostic ID collision now surfaced in constants

The new DiagnosticIds classes make a pre-existing collision more visible. Both of these constants resolve to "TUnit0062":

• TUnit.Analyzers.DiagnosticIds.CancellationTokenMustBeLastParameter = "TUnit0062"
• TUnit.AspNetCore.Analyzers.DiagnosticIds.FactoryAccessedTooEarly = "TUnit0062"

This collision predates the PR (both Rules.cs files already used the literal "TUnit0062"), so it is not introduced here. But extracting to named constants now makes the duplication structurally visible in a way that invites a fix.

Depending on the intended ID space:

• If these are intended to live in separate namespaces with no overlap concern (i.e. TUnit.AspNetCore.Analyzers IDs are deliberately separate from TUnit.Analyzers IDs), the current state is fine and just needs a comment clarifying that.
• If they share a flat ID space, one of the two rules has the wrong ID and should be renumbered.

This is not a blocker for merging — the behavioral change from this PR is zero — but worth a follow-up issue to resolve the ambiguity.

[thomhurst]

Re follow-up review: confirmed the TUnit0062 collision (CancellationTokenMustBeLastParameter vs FactoryAccessedTooEarly) is real and pre-existing — both Rules.cs files shipped the literal before this PR. Not renumbering here: published IDs, .editorconfig severities, and resx resource keys (TUnit0062Title etc.) all depend on the ID, so that is a user-facing change deserving its own PR. Added a NOTE comment at the AspNetCore constant documenting the collision and the shared-severity implication for projects referencing both analyzers.

[claude]

Code Review

This is a well-targeted, correctly-reasoned fix for a subtle cross-assembly version-skew problem. Overall the PR is in excellent shape.

What's working well

The root cause analysis and fix are correct. const values are inlined into call-site IL at compile time (CIL ldstr / constexpr substitution), so the code fixer binary carries zero TypeReference entries pointing at Rules. That's the only reliable solution — any static readonly or property access would still produce a TypeReference, and any Rules.X.Id field access would be a FieldReference.

The IL-level regression test is excellent. Using System.Reflection.Metadata/PEReader to scan TypeReference handles in the compiled assembly is a low-level, unforgeable guard. It catches typeof(Rules), field access, and method calls all at once. Crucially, DiagnosticIds itself is also scanned — if anyone changes a const to static readonly, a TypeReference to DiagnosticIds appears and the test fails. That's a subtle but important self-check.

Opportunistic readonly fixes are correct. Rules.InstanceTestMethod and Rules.MatrixDataSourceAttributeRequired were missing readonly — the PR fixes that consistently with the rest of the class.

The TUnit0062 collision is correctly scoped out. The NOTE comment in TUnit.AspNetCore.Analyzers/DiagnosticIds.cs documents the pre-existing collision clearly and explains why renumbering is a separate, user-facing change.

---

Minor concerns (non-blocking)

Hardcoded title strings can drift from resx.

In InheritsTestsCodeFixProvider.cs and XUnitMigrationCodeFixProvider.cs, the lazy Rules.X.Title.ToString() usages were replaced with string literals:

// before
title: Rules.DoesNotInheritTestsWarning.Title.ToString()
// after
private const string CodeFixTitle = "Add [InheritsTests] attribute";

This is the correct trade-off for decoupling, but it means the title in the lightbulb and the title in the resource file (TUnit0030Title) can now diverge silently. If the resx title is ever updated (localization, rewording), the code fixer lightbulb won't track it.

A pattern worth considering for future cases: rather than duplicating the string in a private const, you could define a companion CodeFixTitles static class alongside DiagnosticIds, also with const strings, so there's still a single place to update both the analyzer and the fixer title. For this PR the two affected literals are stable enough that this isn't urgent — just worth noting for any future additions to the pattern.

RulesDecouplingVerifier XML doc verbosity.

The shared helper has unusually verbose XML docs for a test utility. That's fine here because the comment explains a non-obvious cross-assembly IL constraint that future maintainers will need to understand.

---

Summary

The fix is architecturally sound and the test coverage is thorough. The only open question is the long-term maintenance of the two hardcoded title strings — low risk for now, but worth tracking if more lazy Rules.X.Title usages need decoupling in the future. Ship it.