Skip to content

Commit 1cf11e4

Browse files
committed
refactor: consolidate IL decoupling guard, cover AspNetCore codefixer, address review
- Extract duplicated PEReader scan into SharedTestHelpers/RulesDecouplingVerifier (linked into all three codefixer test projects) - Cover the third vulnerable codefixer family: TUnit.AspNetCore.Analyzers gets DiagnosticIds consts + IL guard test (same #6157 pattern, was missed) - Verifier also flags DiagnosticIds type refs so const -> static readonly drift fails the test (review feedback) - Add missing readonly on InstanceTestMethod / MatrixDataSourceAttributeRequired - Collapse the repeated rationale doc comment to one canonical home
1 parent 4da0e0a commit 1cf11e4

14 files changed

Lines changed: 134 additions & 84 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
using System.Collections.Generic;
2+
using System.IO;
3+
using System.Reflection;
4+
using System.Reflection.Metadata;
5+
using System.Reflection.PortableExecutable;
6+
7+
namespace TUnit.Tests.Shared;
8+
9+
/// <summary>
10+
/// Verifies that a code fixer assembly carries no IL reference to its analyzer project's
11+
/// <c>Rules</c> type. Guards against https://github.com/thomhurst/TUnit/issues/6157.
12+
/// </summary>
13+
/// <remarks>
14+
/// Code fixer assemblies ship in the version-agnostic <c>analyzers/dotnet/cs</c> folder while the
15+
/// analyzer assemblies ship per-Roslyn (<c>analyzers/dotnet/roslyn4.x/cs</c>), and the dependency
16+
/// resolves at runtime by simple name. Visual Studio cannot unload analyzer assemblies, so after a
17+
/// package update (or with mixed TUnit versions in one VS session) a new code fixer can bind
18+
/// against a stale analyzer assembly. Any IL reference to the <c>Rules</c> type — e.g.
19+
/// <c>Rules.X.Id</c> inside the eagerly-evaluated <c>FixableDiagnosticIds</c> — then throws
20+
/// <see cref="System.MissingFieldException"/> for rules the stale assembly doesn't have. Code
21+
/// fixers must use the compile-time-baked <c>DiagnosticIds</c> constants instead, which this
22+
/// helper enforces at the IL level: a <c>TypeReference</c> to <c>Rules</c> appears for any usage
23+
/// (field access, <c>typeof</c>, method call), so an empty result proves full decoupling.
24+
/// <c>DiagnosticIds</c> itself is also scanned — its members must stay <c>const</c>; changing one
25+
/// to <c>static readonly</c> would silently reintroduce a runtime type reference, which surfaces
26+
/// here as a <c>TypeReference</c> to <c>DiagnosticIds</c>.
27+
/// <para>
28+
/// Linked into each code fixer test project via
29+
/// <c>&lt;Compile Include="..\SharedTestHelpers\RulesDecouplingVerifier.cs"&gt;</c>.
30+
/// </para>
31+
/// </remarks>
32+
internal static class RulesDecouplingVerifier
33+
{
34+
/// <summary>
35+
/// Returns the fully-qualified names of all <c>Rules</c> or <c>DiagnosticIds</c> type
36+
/// references in <paramref name="codeFixersAssembly"/> whose namespace is
37+
/// <paramref name="rulesNamespace"/>. An empty list means the assembly is fully decoupled.
38+
/// </summary>
39+
public static List<string> FindRulesTypeReferences(Assembly codeFixersAssembly, string rulesNamespace)
40+
{
41+
using var stream = File.OpenRead(codeFixersAssembly.Location);
42+
using var peReader = new PEReader(stream);
43+
var metadata = peReader.GetMetadataReader();
44+
45+
var rulesReferences = new List<string>();
46+
47+
foreach (var handle in metadata.TypeReferences)
48+
{
49+
var typeReference = metadata.GetTypeReference(handle);
50+
var name = metadata.GetString(typeReference.Name);
51+
var typeNamespace = metadata.GetString(typeReference.Namespace);
52+
53+
if (name is "Rules" or "DiagnosticIds" && typeNamespace == rulesNamespace)
54+
{
55+
rulesReferences.Add($"{typeNamespace}.{name}");
56+
}
57+
}
58+
59+
return rulesReferences;
60+
}
61+
}

TUnit.Analyzers.CodeFixers/Base/BaseMigrationCodeFixProvider.cs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,8 @@ public abstract class BaseMigrationCodeFixProvider : CodeFixProvider
1515
protected abstract string FrameworkName { get; }
1616

1717
/// <summary>
18-
/// The fixable diagnostic ID. Implementations MUST return a <see cref="DiagnosticIds"/> constant
19-
/// (never <c>Rules.X.Id</c>): VS evaluates <see cref="FixableDiagnosticIds"/> eagerly, and a runtime
20-
/// field reference into TUnit.Analyzers can bind against a stale copy already loaded in the IDE,
21-
/// throwing MissingFieldException. See https://github.com/thomhurst/TUnit/issues/6157.
18+
/// The fixable diagnostic ID. Implementations MUST return a <see cref="DiagnosticIds"/> constant,
19+
/// never <c>Rules.X.Id</c> — see <see cref="DiagnosticIds"/> remarks (issue #6157).
2220
/// </summary>
2321
protected abstract string DiagnosticId { get; }
2422

TUnit.Analyzers.Tests/CodeFixerRulesDecouplingTests.cs

Lines changed: 5 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,19 @@
1-
using System.Reflection.Metadata;
2-
using System.Reflection.PortableExecutable;
31
using TUnit.Analyzers.CodeFixers;
2+
using TUnit.Tests.Shared;
43

54
namespace TUnit.Analyzers.Tests;
65

76
/// <summary>
8-
/// Guards against https://github.com/thomhurst/TUnit/issues/6157.
9-
///
10-
/// TUnit.Analyzers.CodeFixers.dll ships in the version-agnostic analyzers/dotnet/cs folder and
11-
/// resolves its TUnit.Analyzers dependency at runtime by simple name. Visual Studio cannot unload
12-
/// analyzer assemblies, so after a package update (or with mixed TUnit versions in one VS session)
13-
/// the new code fixers can bind against a stale TUnit.Analyzers.dll. Any IL reference to the
14-
/// <c>Rules</c> type (e.g. <c>Rules.MSTestMigration.Id</c> inside the eagerly-evaluated
15-
/// <c>FixableDiagnosticIds</c>) then throws MissingFieldException for rules the stale assembly
16-
/// doesn't have. Code fixers must use the compile-time-baked <c>DiagnosticIds</c> constants instead.
7+
/// Code fixers must use <c>DiagnosticIds</c> constants, never <c>Rules.X</c> — see
8+
/// <see cref="RulesDecouplingVerifier"/> for the full rationale (issue #6157).
179
/// </summary>
1810
public class CodeFixerRulesDecouplingTests
1911
{
2012
[Test]
2113
public async Task CodeFixers_Assembly_Has_No_Reference_To_Rules_Type()
2214
{
23-
var assemblyPath = typeof(MSTestMigrationCodeFixProvider).Assembly.Location;
24-
25-
using var stream = File.OpenRead(assemblyPath);
26-
using var peReader = new PEReader(stream);
27-
var metadata = peReader.GetMetadataReader();
28-
29-
var rulesReferences = new List<string>();
30-
31-
foreach (var handle in metadata.TypeReferences)
32-
{
33-
var typeReference = metadata.GetTypeReference(handle);
34-
35-
if (metadata.GetString(typeReference.Name) == "Rules" &&
36-
metadata.GetString(typeReference.Namespace) == "TUnit.Analyzers")
37-
{
38-
rulesReferences.Add($"{metadata.GetString(typeReference.Namespace)}.{metadata.GetString(typeReference.Name)}");
39-
}
40-
}
15+
var rulesReferences = RulesDecouplingVerifier.FindRulesTypeReferences(
16+
typeof(MSTestMigrationCodeFixProvider).Assembly, "TUnit.Analyzers");
4117

4218
await TUnit.Assertions.Assert.That(rulesReferences)
4319
.IsEmpty()

TUnit.Analyzers.Tests/TUnit.Analyzers.Tests.csproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
Visible="false" />
4848
<Compile Include="..\SharedTestHelpers\AnalyzerTestCompatibility.cs"
4949
Link="Shared\AnalyzerTestCompatibility.cs" />
50+
<Compile Include="..\SharedTestHelpers\RulesDecouplingVerifier.cs"
51+
Link="Shared\RulesDecouplingVerifier.cs" />
5052
</ItemGroup>
5153

5254
<!-- Reference the VSTHRD analyzer assembly (Analyzer="false") so its analyzer types are

TUnit.Analyzers/DiagnosticIds.cs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,12 @@ namespace TUnit.Analyzers;
22

33
/// <summary>
44
/// Diagnostic ID constants for all TUnit analyzer rules.
5+
/// Code fix providers MUST reference these constants instead of <c>Rules.X.Id</c> — consts are
6+
/// baked into the consuming IL at compile time, avoiding a runtime bind against a stale
7+
/// analyzer assembly in Visual Studio. See https://github.com/thomhurst/TUnit/issues/6157.
8+
/// Members MUST stay <c>const</c>: <c>static readonly</c> would reintroduce the runtime
9+
/// reference (and fails the IL regression tests).
510
/// </summary>
6-
/// <remarks>
7-
/// Code fix providers (TUnit.Analyzers.CodeFixers) MUST reference these constants instead of
8-
/// <c>Rules.X.Id</c>. Constants are baked into the consuming assembly at compile time, so the
9-
/// code fixers carry no runtime reference to the <see cref="Rules"/> type. A runtime reference
10-
/// can bind against a stale TUnit.Analyzers.dll already loaded in Visual Studio (analyzers
11-
/// cannot be unloaded), throwing <see cref="System.MissingFieldException"/> for newly added
12-
/// rules. See https://github.com/thomhurst/TUnit/issues/6157.
13-
/// </remarks>
1411
public static class DiagnosticIds
1512
{
1613
public const string WrongArgumentTypeTestData = "TUnit0001";

TUnit.Analyzers/Rules.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,10 @@ public static class Rules
111111
public static readonly DiagnosticDescriptor AsyncLocalCallFlowValues =
112112
CreateDescriptor(DiagnosticIds.AsyncLocalCallFlowValues, UsageCategory, DiagnosticSeverity.Warning);
113113

114-
public static DiagnosticDescriptor InstanceTestMethod =
114+
public static readonly DiagnosticDescriptor InstanceTestMethod =
115115
CreateDescriptor(DiagnosticIds.InstanceTestMethod, UsageCategory, DiagnosticSeverity.Error);
116116

117-
public static DiagnosticDescriptor MatrixDataSourceAttributeRequired =
117+
public static readonly DiagnosticDescriptor MatrixDataSourceAttributeRequired =
118118
CreateDescriptor(DiagnosticIds.MatrixDataSourceAttributeRequired, UsageCategory, DiagnosticSeverity.Error);
119119

120120
public static readonly DiagnosticDescriptor TooManyArguments =

TUnit.AspNetCore.Analyzers.CodeFixers/UseTestWebApplicationFactoryCodeFixProvider.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public class UseTestWebApplicationFactoryCodeFixProvider : CodeFixProvider
2020
private const string TestWebApplicationFactoryNamespace = "TUnit.AspNetCore";
2121

2222
public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } =
23-
ImmutableArray.Create(Rules.DirectWebApplicationFactoryInheritance.Id);
23+
ImmutableArray.Create(DiagnosticIds.DirectWebApplicationFactoryInheritance);
2424

2525
public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
2626

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
using TUnit.AspNetCore.Analyzers.CodeFixers;
2+
using TUnit.Tests.Shared;
3+
4+
namespace TUnit.AspNetCore.Analyzers.Tests;
5+
6+
/// <summary>
7+
/// Code fixers must use <c>DiagnosticIds</c> constants, never <c>Rules.X</c> — see
8+
/// <see cref="RulesDecouplingVerifier"/> for the full rationale (issue #6157).
9+
/// </summary>
10+
public class CodeFixerRulesDecouplingTests
11+
{
12+
[Test]
13+
public async Task CodeFixers_Assembly_Has_No_Reference_To_Rules_Type()
14+
{
15+
var rulesReferences = RulesDecouplingVerifier.FindRulesTypeReferences(
16+
typeof(UseTestWebApplicationFactoryCodeFixProvider).Assembly, "TUnit.AspNetCore.Analyzers");
17+
18+
await Assert.That(rulesReferences)
19+
.IsEmpty()
20+
.Because("TUnit.AspNetCore.Analyzers.CodeFixers must not reference TUnit.AspNetCore.Analyzers.Rules at runtime - " +
21+
"use DiagnosticIds constants instead (see issue #6157)");
22+
}
23+
}

TUnit.AspNetCore.Analyzers.Tests/TUnit.AspNetCore.Analyzers.Tests.csproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
Visible="false" />
3030
<Compile Include="..\SharedTestHelpers\AnalyzerTestCompatibility.cs"
3131
Link="Shared\AnalyzerTestCompatibility.cs" />
32+
<Compile Include="..\SharedTestHelpers\RulesDecouplingVerifier.cs"
33+
Link="Shared\RulesDecouplingVerifier.cs" />
3234
</ItemGroup>
3335

3436
<Import Project="..\TestProject.targets" />
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
namespace TUnit.AspNetCore.Analyzers;
2+
3+
/// <summary>
4+
/// Diagnostic ID constants for all TUnit ASP.NET Core analyzer rules.
5+
/// Code fix providers MUST reference these constants instead of <c>Rules.X.Id</c> — consts are
6+
/// baked into the consuming IL at compile time, avoiding a runtime bind against a stale
7+
/// analyzer assembly in Visual Studio. See https://github.com/thomhurst/TUnit/issues/6157.
8+
/// Members MUST stay <c>const</c>: <c>static readonly</c> would reintroduce the runtime
9+
/// reference (and fails the IL regression tests).
10+
/// </summary>
11+
public static class DiagnosticIds
12+
{
13+
public const string FactoryAccessedTooEarly = "TUnit0062";
14+
public const string GlobalFactoryMemberAccess = "TUnit0063";
15+
public const string DirectWebApplicationFactoryInheritance = "TUnit0064";
16+
}

0 commit comments

Comments
 (0)