From 08840f03d5e69d52b3f1fad2e5f2e34200fa3fac Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 15:30:53 +0100 Subject: [PATCH 01/10] perf(testing): share MetadataReference instances across test compilations ResolveReferences created a fresh MetadataReference per run for every assembly named by AdditionalAssemblyTypes, so Roslyn could not reuse PEAssemblySymbols and each test compilation built its own symbol tables. References are now created once per path, process-wide, and an assembly already in the trusted-platform set is not added a second time. TRUSTED_PLATFORM_ASSEMBLIES is deduplicated at the source: the host lists a path once per deps.json library that resolves to it, so the "reference the generator project twice" pattern put the same file in the list twice. Additions are deduplicated because Solution.AddMetadataReferences rejects duplicate items once the instances are shared. Measured on zodsharp SourceGenerators.UnitTests (1,289 tests, four TFM executables concurrently under dotnet test): 15,340 MB -> 3,450 MB peak, 35.6s -> 28.5s wall clock. An isolated 100-compilation probe isolates the mechanism: 156 MB with shared references against 8,146 MB with per-compilation references. --- .../Properties/AssemblyInfo.cs | 1 + .../SourceGeneratorHelpers.cs | 89 +++++++++++- .../MetadataReferenceCacheTests.cs | 136 ++++++++++++++++++ 3 files changed, 222 insertions(+), 4 deletions(-) create mode 100644 src/tests/SourceGeneratorFramework.UnitTests/MetadataReferenceCacheTests.cs diff --git a/src/src/SourceGeneratorFramework.Testing/Properties/AssemblyInfo.cs b/src/src/SourceGeneratorFramework.Testing/Properties/AssemblyInfo.cs index 4f93730..5b9fa6b 100644 --- a/src/src/SourceGeneratorFramework.Testing/Properties/AssemblyInfo.cs +++ b/src/src/SourceGeneratorFramework.Testing/Properties/AssemblyInfo.cs @@ -1,4 +1,5 @@ using System.Runtime.CompilerServices; [assembly: InternalsVisibleTo("SourceGeneratorFramework.UnitTests")] +[assembly: InternalsVisibleTo("Purview.SourceGeneratorFramework.UnitTests")] [assembly: InternalsVisibleTo("Purview.SourceGeneratorFramework.Testing.TUnit")] diff --git a/src/src/SourceGeneratorFramework.Testing/SourceGeneratorHelpers.cs b/src/src/SourceGeneratorFramework.Testing/SourceGeneratorHelpers.cs index 3a927b5..7870c58 100644 --- a/src/src/SourceGeneratorFramework.Testing/SourceGeneratorHelpers.cs +++ b/src/src/SourceGeneratorFramework.Testing/SourceGeneratorHelpers.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Collections.Immutable; using System.Reflection; using System.Text; @@ -8,6 +9,15 @@ namespace Purview.SourceGeneratorFramework.Testing; static class SourceGeneratorHelpers { + // Process-wide, keyed by assembly path. A MetadataReference owns an AssemblyMetadata, and Roslyn keys + // its PEAssemblySymbol cache off that instance: handing every test compilation the *same* instance lets + // Roslyn reuse one set of symbol tables across the whole run instead of rebuilding them per test. Test + // processes are short lived and the referenced assemblies do not change underneath them, so the entries + // are never invalidated. + static readonly ConcurrentDictionary ReferenceCache = new( + StringComparer.OrdinalIgnoreCase + ); + public static readonly string[] TrustedAssemblies = ResolveTrustedAssemblies(); public static ImmutableArray ResolveTrustedReferences { get; } = @@ -19,6 +29,21 @@ static class SourceGeneratorHelpers GeneratorAssemblyPath ); + static readonly ImmutableHashSet CachedFilteredReferencePaths = CollectDisplayPaths( + CachedFilteredReferences + ); + + /// + /// Gets the shared for an assembly path, creating it on first use. + /// + /// + /// Always go through this instead of calling + /// directly: a fresh reference per test run gives Roslyn a fresh AssemblyMetadata, which defeats + /// symbol reuse and makes every test compilation build its own symbol tables. + /// + public static MetadataReference GetOrCreateReference(string path) => + ReferenceCache.GetOrAdd(path, static p => MetadataReference.CreateFromFile(p)); + public static CSharpCompilation CreateCompilation( IEnumerable syntaxTrees, ImmutableArray references, @@ -52,16 +77,49 @@ Assembly generatorAssembly builder.AddRange(CachedFilteredReferences); + // References are shared instances now, so the same assembly named twice yields the same object. + // Compilation.Create tolerates that, but Solution.AddMetadataReferences (the analyzer, code-fix and + // refactoring path) rejects duplicate items, so the additions are deduplicated here. + HashSet addedPaths = new(StringComparer.OrdinalIgnoreCase); foreach (var type in options.AdditionalAssemblyTypes) - builder.Add(MetadataReference.CreateFromFile(type.Assembly.Location)); + { + var location = type.Assembly.Location; + + // Dynamic and single-file assemblies have no location to reference. + if (string.IsNullOrEmpty(location)) + continue; + + // Already in the trusted-platform set, so naming it again is a no-op. + if (CachedFilteredReferencePaths.Contains(location) || !addedPaths.Add(location)) + continue; + + builder.Add(GetOrCreateReference(location)); + } + + // Deduplicated by instance, not by path: a caller may deliberately supply the same assembly twice + // with different MetadataReferenceProperties (extern aliases), and those are distinct references. + HashSet addedReferences = []; + foreach (var reference in options.AdditionalReferences) + { + if (!addedReferences.Add(reference) || IsAlreadyTrusted(reference)) + continue; - builder.AddRange(options.AdditionalReferences); + builder.Add(reference); + } var references = builder.ToImmutable(); options.PreprocessReferences?.Invoke(references); return references; } + // Only skips a reference that is literally the object already in the trusted list: a caller-created + // reference over the same file is a distinct instance and stays, preserving the previous behaviour. + static bool IsAlreadyTrusted(MetadataReference reference) => + reference.Display is { Length: > 0 } display + && CachedFilteredReferencePaths.Contains(display) + && ReferenceCache.TryGetValue(display, out var cached) + && ReferenceEquals(cached, reference); + public static string PrepareSource(string source, SourceGeneratorTestOptions options) { if (!options.IncludeDefaultNamespaces) @@ -85,7 +143,18 @@ static string[] ResolveTrustedAssemblies() { var trusted = (string?)AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES") ?? string.Empty; if (!string.IsNullOrWhiteSpace(trusted)) - return trusted.Split([Path.PathSeparator], StringSplitOptions.RemoveEmptyEntries); + { + // The host lists a path once per deps.json library that resolves to it, so an assembly + // referenced twice by design — the generator double-reference pattern, once as an analyzer and + // once as a normal reference — appears twice. Referencing the same file twice is never correct, + // and Solution.AddMetadataReferences rejects it outright once the instances are shared. + return + [ + .. trusted + .Split([Path.PathSeparator], StringSplitOptions.RemoveEmptyEntries) + .Distinct(StringComparer.OrdinalIgnoreCase), + ]; + } // .NET Framework does not populate TRUSTED_PLATFORM_ASSEMBLIES; fall back to the loaded // framework assemblies so test compilations still have their core references. @@ -106,7 +175,19 @@ static ImmutableArray CreateMetadataReferences(string[] paths var builder = ImmutableArray.CreateBuilder(paths.Length); foreach (var path in paths) - builder.Add(MetadataReference.CreateFromFile(path)); + builder.Add(GetOrCreateReference(path)); + + return builder.ToImmutable(); + } + + static ImmutableHashSet CollectDisplayPaths(ImmutableArray references) + { + var builder = ImmutableHashSet.CreateBuilder(StringComparer.OrdinalIgnoreCase); + foreach (var reference in references) + { + if (reference.Display is { Length: > 0 } display) + builder.Add(display); + } return builder.ToImmutable(); } diff --git a/src/tests/SourceGeneratorFramework.UnitTests/MetadataReferenceCacheTests.cs b/src/tests/SourceGeneratorFramework.UnitTests/MetadataReferenceCacheTests.cs new file mode 100644 index 0000000..c22ee99 --- /dev/null +++ b/src/tests/SourceGeneratorFramework.UnitTests/MetadataReferenceCacheTests.cs @@ -0,0 +1,136 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; + +namespace Purview.SourceGeneratorFramework; + +/// +/// Locks in the reference-sharing contract of SourceGeneratorHelpers. +/// +/// +/// Each owns an AssemblyMetadata, and Roslyn keys its assembly-symbol +/// reuse off that instance. Creating references per run makes every test compilation build its own symbol +/// tables, which is what made harness-based suites peak in the gigabytes. These tests fail if a refactor +/// reintroduces a per-call MetadataReference.CreateFromFile. +/// +public class MetadataReferenceCacheTests +{ + [Test] + public async Task GetOrCreateReference_GivenTheSamePath_ReturnsTheSameInstance() + { + // Arrange + var path = typeof(object).Assembly.Location; + + // Act + var first = SourceGeneratorHelpers.GetOrCreateReference(path); + var second = SourceGeneratorHelpers.GetOrCreateReference(path); + + // Assert + await Assert.That(second).IsSameReferenceAs(first); + } + + [Test] + public async Task ResolveTrustedReferences_GivenATrustedPath_SharesTheCachedInstance() + { + // Arrange + var path = typeof(object).Assembly.Location; + var cached = SourceGeneratorHelpers.GetOrCreateReference(path); + + // Act + var trusted = SourceGeneratorHelpers.ResolveTrustedReferences.FirstOrDefault(reference => + string.Equals(reference.Display, path, StringComparison.OrdinalIgnoreCase) + ); + + // Assert + await Assert.That(trusted).IsSameReferenceAs(cached); + } + + [Test] + public async Task ResolveReferences_GivenAdditionalAssemblyTypes_ReusesEveryReferenceAcrossRuns() + { + // Arrange - the slow path, taken whenever any additional assembly is configured. + SourceGeneratorTestOptions options = new() + { + AdditionalAssemblyTypes = [typeof(Compilation), typeof(SourceGeneratorTestOptions)], + }; + + // Act + var first = SourceGeneratorHelpers.ResolveReferences(options, typeof(Compilation).Assembly); + var second = SourceGeneratorHelpers.ResolveReferences(options, typeof(Compilation).Assembly); + + // Assert - not merely equal: the same objects, so Roslyn can reuse the bound symbols. + await Assert.That(second.Length).IsEqualTo(first.Length); + await Assert.That(first.Where((reference, index) => !ReferenceEquals(reference, second[index]))).IsEmpty(); + } + + [Test] + public async Task ResolveReferences_GivenAnAssemblyAlreadyTrusted_DoesNotAddItAgain() + { + // Arrange + SourceGeneratorTestOptions options = new() { AdditionalAssemblyTypes = [typeof(object)] }; + + // Act + var references = SourceGeneratorHelpers.ResolveReferences(options, typeof(Compilation).Assembly); + + // Assert + await Assert + .That( + references.Count(reference => + string.Equals( + reference.Display, + typeof(object).Assembly.Location, + StringComparison.OrdinalIgnoreCase + ) + ) + ) + .IsEqualTo(1); + } + + [Test] + public async Task ResolveReferences_GivenRepeatedAssemblyTypes_ProducesNoDuplicateInstances() + { + // Arrange - Solution.AddMetadataReferences rejects duplicate items outright. + SourceGeneratorTestOptions options = new() + { + AdditionalAssemblyTypes = [typeof(SourceGeneratorTestOptions), typeof(DriverRunResult)], + AdditionalReferences = [SourceGeneratorHelpers.GetOrCreateReference(typeof(Compilation).Assembly.Location)], + }; + + // Act + var references = SourceGeneratorHelpers.ResolveReferences(options, typeof(Compilation).Assembly); + + // Assert + await Assert.That(references.Distinct().Count()).IsEqualTo(references.Length); + } + + [Test] + public async Task ResolveTrustedAssemblies_ContainsNoDuplicatePaths() + { + // Act + var duplicates = SourceGeneratorHelpers + .TrustedAssemblies.GroupBy(static path => path, StringComparer.OrdinalIgnoreCase) + .Where(static group => group.Count() > 1) + .Select(static group => group.Key); + + // Assert + await Assert.That(duplicates).IsEmpty(); + } + + [Test] + public async Task ResolveReferences_GivenAdditionalAssemblies_StillInvokesPreprocessReferences() + { + // Arrange + ImmutableArray observed = default; + SourceGeneratorTestOptions options = new() + { + AdditionalAssemblyTypes = [typeof(SourceGeneratorTestOptions)], + PreprocessReferences = references => observed = references, + }; + + // Act + var resolved = SourceGeneratorHelpers.ResolveReferences(options, typeof(Compilation).Assembly); + + // Assert + await Assert.That(observed.IsDefault).IsFalse(); + await Assert.That(observed.SequenceEqual(resolved)).IsTrue(); + } +} From fbcdd0260770f6bc7a4ea1bcc6380207cb8690a0 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 15:31:02 +0100 Subject: [PATCH 02/10] feat(analyzers): add PSGFR42 for undisposed test harness results DriverRunResult is IDisposable because a run that compiles to an assembly creates a collectible AssemblyLoadContext and a MetadataLoadContext. Nothing enforced disposal, and consumers accumulated undisposed results without noticing. PSGFR42 flags a disposable result from the Testing namespace that is produced and then never disposed: no using declaration, not returned to a caller, not disposed explicitly. The indirect case is covered too, where a helper creates a result, projects a value out of it and returns only the projection. Discarded results are flagged as well. Severity is keyed syntactically on whether the run compiles to an assembly (a Compile options member, or CompileToAssembly = true): Warning when it does, because that is when the leak holds an assembly and its load contexts, and Info otherwise. DisposeTestResultCodeFixProvider turns the declaration into a using declaration, with Fix-all support. The framework's own CompileToAssembly tests are fixed accordingly, and the Testing docs and skills record the rule along with the harness reference-sharing behaviour and the parallelism-ceiling guidance. --- docs/wiki/Analyzers.md | 3 + docs/wiki/Testing.md | 20 ++ .../AnalyzerReleases.Unshipped.md | 1 + .../UndisposedTestResultAnalyzer.cs | 280 ++++++++++++++++++ .../DisposeTestResultCodeFixProvider.cs | 62 ++++ .../skills/tunit-test-authoring/SKILL.md | 5 + .../skills/source-generator-testing/SKILL.md | 13 + .../Sdk/README.md | 22 ++ .../UndisposedTestResultAnalyzerTests.cs | 259 ++++++++++++++++ .../DisposeTestResultCodeFixProviderTests.cs | 80 +++++ .../CompilationResultTests.cs | 6 +- 11 files changed, 748 insertions(+), 3 deletions(-) create mode 100644 src/src/SourceGeneratorFramework.Analyzers/UndisposedTestResultAnalyzer.cs create mode 100644 src/src/SourceGeneratorFramework.CodeFixers/DisposeTestResultCodeFixProvider.cs create mode 100644 src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UndisposedTestResultAnalyzerTests.cs create mode 100644 src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/DisposeTestResultCodeFixProviderTests.cs diff --git a/docs/wiki/Analyzers.md b/docs/wiki/Analyzers.md index 0f10824..4cf5e7b 100644 --- a/docs/wiki/Analyzers.md +++ b/docs/wiki/Analyzers.md @@ -69,6 +69,7 @@ Opt out with `PurviewSourceGeneratorFrameworkAnalyzerValidation=false` (`PRSGD00 | `PSGFR39` | A non-packable Roslyn component that explicitly opts out of the default self-contained analyzer output (`PurviewMergeSourceGeneratorFrameworkForAnalyzerFiles=false`) while embedding the framework, otherwise the package embeds the loose framework DLL under `analyzers/`. | | `PSGFR40` | In Roslyn components (`IsRoslynComponent=true`), reference SGF types as inline code (`Type`) instead of a `cref`: copied documentation must not depend on cref resolution. | | `PSGFR41` | In Roslyn components whose framework implementation is merged, a public member (or generic constraint) whose signature references an SGF type: the merge internalizes every SGF type, so the signature is left referring to an internal type. Make the member or its declaring type non-public. | +| `PSGFR42` | A disposable Testing-harness result (`DriverRunResult`) is produced and never disposed, leaking the collectible `AssemblyLoadContext` and `MetadataLoadContext` of a compiled run. Reported as a `warning` when the run syntactically compiles to an assembly (a `Compile` options member or `CompileToAssembly = true`), otherwise as `info`. | ## Type-library and attribute-model diagnostics @@ -101,6 +102,8 @@ analyzer rules above, including: - `PreferInlineCodeForFrameworkCrefCodeFixProvider` — rewrites SGF XML doc `cref` targets to inline code (`Type`) (`PSGFR40`). - `MakeComponentSurfaceNonPublicCodeFixProvider` — makes the exposing member, or its declaring type, non-public (`PSGFR41`). +- `DisposeTestResultCodeFixProvider` — turns an undisposed test-harness result declaration into a + `using` declaration (`PSGFR42`). - `MakeTypeLibrarySpecNonPublicCodeFixProvider` — declares a type-library spec non-public in a merged component (`TLB0021`). - `AttributeDataModelSymbolPropertyCodeFixProvider` — fixes attribute-data-model symbol properties. diff --git a/docs/wiki/Testing.md b/docs/wiki/Testing.md index 20fd893..2a0deeb 100644 --- a/docs/wiki/Testing.md +++ b/docs/wiki/Testing.md @@ -154,6 +154,26 @@ emitted assembly: inspect types, members and attributes without loading it into executing any code. They are created lazily on first access. Dispose the result (or its `DriverRunResult`) to unload the collectible context and release the metadata view. +[`PSGFR42`](Analyzers.md#rule-reference) enforces that: a `DriverRunResult` that is produced and never +disposed — including the indirect case where a helper creates a result, projects a value out of it and +returns only the projection — is flagged, as a `warning` when the run syntactically compiles to an assembly +and as `info` otherwise. `DisposeTestResultCodeFixProvider` inserts the `using`. + +## Reference sharing and parallelism + +The harness binds every trusted-platform assembly once per test process and hands the **same** +`MetadataReference` instances to every test compilation, so Roslyn reuses one set of assembly symbols +across the run instead of rebuilding symbol tables per test. Over 100 compilations of the same source +against the same assembly set, shared references peak at 156 MB where per-compilation references peak at +8,146 MB. An assembly named by `AdditionalAssemblyTypes` that is already in the trusted-platform set is a +no-op: the reference is not added a second time. + +Because of that sharing, memory is largely flat in the degree of parallelism — but the test platform's +auto-detected default still costs more memory *and* more wall clock than an explicit ceiling. Wall clock +saturates at around four concurrent tests. On a 22-core machine, `--maximum-parallel-tests 8` measured +347–365 MB / 7.7–8.7s against 528–881 MB / 12.0–13.8s for the auto default. Set an explicit ceiling for a +test project that multi-targets, where several TFM executables run concurrently under `dotnet test`. + Analyzer options are preserved under their supplied keys. Keys without the Roslyn `build_property.` prefix are additionally exposed as compiler-visible MSBuild properties, so either `MyGenerator_Disable` or `build_property.MyGenerator_Disable` can be used in tests. diff --git a/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md b/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md index be387d6..aa2d6e7 100644 --- a/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md @@ -17,6 +17,7 @@ PSGFR38 | Purview.SourceGeneratorFramework | Warning | Extension class is missin PSGFR39 | Purview.SourceGeneratorFramework | Error | Roslyn component must produce a self-contained analyzer PSGFR40 | Purview.SourceGeneratorFramework | Warning | SGF cref should be inline code PSGFR41 | Purview.SourceGeneratorFramework | Warning | Component public surface exposes framework types +PSGFR42 | Purview.SourceGeneratorFramework | Info | Dispose test harness results with a using declaration TLB0014 | TypeLibrary | Warning | Type library partial extension is declared in a different namespace TLB0015 | TypeLibrary | Info | Type library partial extension must be declared 'public static partial' TLB0016 | TypeLibrary | Error | Enum value member type must be TypeIdentity or EnumValueDefinition diff --git a/src/src/SourceGeneratorFramework.Analyzers/UndisposedTestResultAnalyzer.cs b/src/src/SourceGeneratorFramework.Analyzers/UndisposedTestResultAnalyzer.cs new file mode 100644 index 0000000..b5598ad --- /dev/null +++ b/src/src/SourceGeneratorFramework.Analyzers/UndisposedTestResultAnalyzer.cs @@ -0,0 +1,280 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Purview.SourceGeneratorFramework.Analyzers; + +/// +/// Flags a disposable Testing-harness result that is produced and then never disposed, which leaks the +/// collectible AssemblyLoadContext and MetadataLoadContext created for a compiled run. +/// +/// +/// The rule is reported as a warning when the run syntactically opts into compiling to an assembly (a +/// Compile options member, or CompileToAssembly = true), because that is when the undisposed +/// result actually holds an emitted assembly; otherwise it is informational. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class UndisposedTestResultAnalyzer : DiagnosticAnalyzer +{ + public const string DiagnosticId = "PSGFR42"; + + /// + /// Set on the diagnostic when the fix can turn the declaration into a using declaration. + /// + internal const string FixableProperty = "Fixable"; + + const string TestingNamespace = "Purview.SourceGeneratorFramework.Testing"; + + public static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + "Dispose test harness results with a using declaration", + "The {0} produced by {1} is never disposed; declare it with 'using' so the compiled assembly and its load contexts are released", + "Purview.SourceGeneratorFramework", + DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "Test-harness results are disposable because a run that compiles to an assembly creates a collectible AssemblyLoadContext and a MetadataLoadContext. A result that is neither consumed by a using declaration nor handed to a caller leaks both for the lifetime of the test process." + ); + + public override ImmutableArray SupportedDiagnostics => [Rule]; + + public override void Initialize(AnalysisContext context) + { + if (context is null) + throw new ArgumentNullException(nameof(context)); + + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeLocalDeclaration, SyntaxKind.LocalDeclarationStatement); + context.RegisterSyntaxNodeAction(AnalyzeExpressionStatement, SyntaxKind.ExpressionStatement); + } + + static void AnalyzeLocalDeclaration(SyntaxNodeAnalysisContext context) + { + if (context.Node is not LocalDeclarationStatementSyntax declaration) + return; + + // 'using var result = ...' and 'await using var result = ...' already own the result. + if (!declaration.UsingKeyword.IsKind(SyntaxKind.None)) + return; + + foreach (var declarator in declaration.Declaration.Variables) + { + if (declarator.Initializer?.Value is not { } initializer) + continue; + + if (Unwrap(initializer) is not InvocationExpressionSyntax and not ObjectCreationExpressionSyntax) + continue; + + if ( + context.SemanticModel.GetDeclaredSymbol(declarator, context.CancellationToken) is not ILocalSymbol local + ) + continue; + + if (!IsDisposableTestingResult(local.Type)) + continue; + + if (IsOwnedElsewhere(declarator, local, context)) + continue; + + context.ReportDiagnostic( + Diagnostic.Create( + Rule, + declarator.Identifier.GetLocation(), + ResolveSeverity(initializer), + additionalLocations: null, + properties: ImmutableDictionary.Empty.Add(FixableProperty, bool.TrueString), + local.Type.Name, + DescribeProducer(initializer) + ) + ); + } + } + + static void AnalyzeExpressionStatement(SyntaxNodeAnalysisContext context) + { + if (context.Node is not ExpressionStatementSyntax statement) + return; + + var expression = statement.Expression; + + // '_ = await RunAsync(...)' discards the result just as completely as dropping it. + if ( + expression is AssignmentExpressionSyntax + { + Left: IdentifierNameSyntax { Identifier.ValueText: "_" } + } discard + ) + expression = discard.Right; + + var produced = Unwrap(expression); + if (produced is not InvocationExpressionSyntax and not ObjectCreationExpressionSyntax) + return; + + var type = context.SemanticModel.GetTypeInfo(expression, context.CancellationToken).Type; + if (!IsDisposableTestingResult(type)) + return; + + context.ReportDiagnostic( + Diagnostic.Create( + Rule, + produced.GetLocation(), + ResolveSeverity(expression), + additionalLocations: null, + properties: null, + type!.Name, + DescribeProducer(expression) + ) + ); + } + + static ExpressionSyntax Unwrap(ExpressionSyntax expression) => + expression switch + { + AwaitExpressionSyntax await => Unwrap(await.Expression), + ParenthesizedExpressionSyntax parenthesized => Unwrap(parenthesized.Expression), + _ => expression, + }; + + static bool IsDisposableTestingResult(ITypeSymbol? type) + { + if (type is not INamedTypeSymbol named) + return false; + + var @namespace = named.ContainingNamespace?.ToDisplayString(); + if ( + @namespace is null + || ( + !string.Equals(@namespace, TestingNamespace, StringComparison.Ordinal) + && !@namespace.StartsWith(TestingNamespace + ".", StringComparison.Ordinal) + ) + ) + return false; + + return named.AllInterfaces.Any(static i => + i.ToDisplayString() is "System.IDisposable" or "System.IAsyncDisposable" + ); + } + + /// + /// Determines whether the local hands the result to someone else, or disposes it explicitly. + /// + /// + /// Reading members off the result (result.GetSource(...)) or passing it by value to an assertion + /// does not transfer ownership: the declaring method still has to dispose it. Returning it, yielding it, + /// assigning it elsewhere, or passing it by reference does. + /// + static bool IsOwnedElsewhere( + VariableDeclaratorSyntax declarator, + ILocalSymbol local, + SyntaxNodeAnalysisContext context + ) + { + var body = FindEnclosingBody(declarator); + if (body is null) + return true; + + foreach (var identifier in body.DescendantNodes().OfType()) + { + if (identifier.Identifier.ValueText != local.Name) + continue; + + if ( + !SymbolEqualityComparer.Default.Equals( + context.SemanticModel.GetSymbolInfo(identifier, context.CancellationToken).Symbol, + local + ) + ) + continue; + + if (Transfers(identifier)) + return true; + } + + return false; + } + + static bool Transfers(IdentifierNameSyntax identifier) => + identifier.Parent switch + { + ReturnStatementSyntax + or YieldStatementSyntax + or ArrowExpressionClauseSyntax + or UsingStatementSyntax + or EqualsValueClauseSyntax => true, + AssignmentExpressionSyntax assignment => assignment.Right == identifier, + ArgumentSyntax argument => !argument.RefKindKeyword.IsKind(SyntaxKind.None), + MemberAccessExpressionSyntax member => member.Expression == identifier + && member.Name.Identifier.ValueText is "Dispose" or "DisposeAsync", + ConditionalAccessExpressionSyntax conditional => conditional.Expression == identifier + && conditional.WhenNotNull.ToString().Contains("Dispose"), + _ => false, + }; + + static SyntaxNode? FindEnclosingBody(SyntaxNode node) + { + for (var current = node.Parent; current is not null; current = current.Parent) + { + if (current is BaseMethodDeclarationSyntax method) + return (SyntaxNode?)method.Body ?? method.ExpressionBody; + + if (current is LocalFunctionStatementSyntax localFunction) + return (SyntaxNode?)localFunction.Body ?? localFunction.ExpressionBody; + + if (current is AnonymousFunctionExpressionSyntax lambda) + return lambda.Body; + + if (current is AccessorDeclarationSyntax accessor) + return (SyntaxNode?)accessor.Body ?? accessor.ExpressionBody; + } + + return null; + } + + /// + /// Escalates the rule to a warning when the run syntactically compiles to an assembly, which is when the + /// undisposed result actually holds a loaded assembly rather than only syntax trees. + /// + static DiagnosticSeverity ResolveSeverity(ExpressionSyntax producer) => + CompilesToAssembly(producer) ? DiagnosticSeverity.Warning : DiagnosticSeverity.Info; + + static bool CompilesToAssembly(SyntaxNode producer) + { + foreach (var node in producer.DescendantNodesAndSelf()) + { + // '.Compile()' and the 'Options.Compile' convenience member both opt in. + if ( + node + is MemberAccessExpressionSyntax { Name.Identifier.ValueText: "Compile" } + or IdentifierNameSyntax { Identifier.ValueText: "Compile" } + ) + return true; + + if ( + node + is AssignmentExpressionSyntax + { + Left: IdentifierNameSyntax { Identifier.ValueText: "CompileToAssembly" }, + Right: LiteralExpressionSyntax right, + } + && right.IsKind(SyntaxKind.TrueLiteralExpression) + ) + return true; + } + + return false; + } + + static string DescribeProducer(ExpressionSyntax producer) => + Unwrap(producer) switch + { + InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax member } => member + .Name + .Identifier + .ValueText, + InvocationExpressionSyntax { Expression: IdentifierNameSyntax name } => name.Identifier.ValueText, + ObjectCreationExpressionSyntax creation => creation.Type.ToString(), + _ => producer.ToString(), + }; +} diff --git a/src/src/SourceGeneratorFramework.CodeFixers/DisposeTestResultCodeFixProvider.cs b/src/src/SourceGeneratorFramework.CodeFixers/DisposeTestResultCodeFixProvider.cs new file mode 100644 index 0000000..616a63b --- /dev/null +++ b/src/src/SourceGeneratorFramework.CodeFixers/DisposeTestResultCodeFixProvider.cs @@ -0,0 +1,62 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace Purview.SourceGeneratorFramework.CodeFixers; + +/// +/// Turns an undisposed test-harness result declaration into a using declaration, so the compiled +/// assembly and its load contexts are released when the test method returns. +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(DisposeTestResultCodeFixProvider))] +public sealed class DisposeTestResultCodeFixProvider : CodeFixProvider +{ + internal const string EquivalenceKey = "DisposeTestResult"; + + public override ImmutableArray FixableDiagnosticIds => ["PSGFR42"]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) + return; + + foreach (var diagnostic in context.Diagnostics) + { + var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + if (node.FirstAncestorOrSelf() is not { } declaration) + continue; + + // A multi-declarator statement cannot become a using declaration without splitting it. + if (declaration.Declaration.Variables.Count != 1 || !declaration.UsingKeyword.IsKind(SyntaxKind.None)) + continue; + + context.RegisterCodeFix( + CodeAction.Create( + "Consume the result with a using declaration", + _ => AddUsingAsync(context.Document, root, declaration), + EquivalenceKey + ), + diagnostic + ); + } + } + + static Task AddUsingAsync(Document document, SyntaxNode root, LocalDeclarationStatementSyntax declaration) + { + // The using keyword takes the statement's leading trivia so the indentation is preserved. + var usingToken = SyntaxFactory + .Token(SyntaxKind.UsingKeyword) + .WithLeadingTrivia(declaration.GetLeadingTrivia()) + .WithTrailingTrivia(SyntaxFactory.Space); + + var updated = declaration.WithLeadingTrivia(SyntaxFactory.TriviaList()).WithUsingKeyword(usingToken); + + return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(declaration, updated))); + } +} diff --git a/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md b/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md index fa58cd8..13ebec5 100644 --- a/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md +++ b/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md @@ -57,6 +57,11 @@ Use the base hooks to customise per-run: `OnBeforeRun`/`OnBeforeRunAsync` (mutat `options.WithAdditionalSources(markerAttributeSource)`) and `OnAfterRun`/`OnAfterRunAsync`. Use `options.Compile()` to opt into `CompileToAssembly` while preserving the derived options type. +Write `using var result = await GenerateAsync(...)`: `DriverRunResult` is `IDisposable`, and `PSGFR42` +flags a result that is produced and never disposed (a `warning` once the run compiles to an assembly, +`info` otherwise). It also catches the indirect case — a helper that creates a result, projects a value out +of it and returns only the projection. + For code fixes/refactorings, select a specific registered action via `CodeFixTestOptions.EquivalenceKey` or `CodeActionIndex`, and `RefactorTestOptions.Span`/`NodeSelector` (e.g. `NodeSelector = query => query.GetMethod("M")`). diff --git a/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md b/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md index 2a56544..37b9525 100644 --- a/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md +++ b/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md @@ -147,6 +147,13 @@ keeps repeated runs from polluting the default context. `result.CompilationResul assembly (generated code can execute); `result.CompilationResult.Metadata` / `.MetadataAssembly` give a metadata-only reflection view (types, members, attributes) over the emitted assembly without executing code. +**Always write `using var result = ...`.** `PSGFR42` ("Dispose test harness results with a using +declaration") flags a `DriverRunResult` that is produced and never disposed — including the indirect case +where a helper creates a result, projects a value out of it and returns only the projection. It is a +`warning` when the run syntactically compiles to an assembly (`.Compile()`, a `Compile` options member, or +`CompileToAssembly = true`) and `info` otherwise; the `DisposeTestResultCodeFixProvider` fix inserts the +`using`. A result that is returned to the caller, or disposed explicitly, is not flagged. + ## Best practices - **Deterministic output**: `WriteAutoGeneratedHeader` is timestamp-free; assert with @@ -160,6 +167,12 @@ metadata-only reflection view (types, members, attributes) over the emitted asse - **Scope validation**: keep `PurviewSourceGeneratorFrameworkValidateCodeWriterScopes` enabled; it makes undisposed `CodeWriter` scopes fail tests. - **Prefer `CodeQuery` over string matching** for structural assertions (members, signatures, namespaces). +- **Dispose results**: `using var result = ...`, enforced by `PSGFR42`. +- **Cap parallelism**: the harness shares one `MetadataReference` set per process, so memory is largely flat + in the degree of parallelism, but the platform's auto-detected default is both slower and heavier than an + explicit ceiling. Wall clock saturates around four concurrent tests; prefer + `--maximum-parallel-tests 8` (or a `TestingPlatformCommandLineArguments` equivalent) for a project that + multi-targets and runs several TFM executables at once. ## Incremental cache testing (`RunIncrementalAsync`) diff --git a/src/src/SourceGeneratorFramework.Testing/Sdk/README.md b/src/src/SourceGeneratorFramework.Testing/Sdk/README.md index 12b28d1..5cd4d36 100644 --- a/src/src/SourceGeneratorFramework.Testing/Sdk/README.md +++ b/src/src/SourceGeneratorFramework.Testing/Sdk/README.md @@ -147,6 +147,28 @@ assembly: inspect types, members and attributes without loading it into the runt They are created lazily on first access. Dispose the result (or its `DriverRunResult`) to unload the collectible context and release the metadata view. +**`PSGFR42` enforces this.** The analyzer flags a `DriverRunResult` that is produced and then never +disposed — no `using` declaration, not returned to a caller, not disposed explicitly. A helper that creates +a result, projects a value out of it and returns the projection leaks just as badly, so the local inside the +helper is flagged too. The rule is a `warning` when the run syntactically compiles to an assembly (a +`Compile` options member, or `CompileToAssembly = true`), because that is when the leak actually holds an +assembly and its load contexts, and `info` otherwise. `DisposeTestResultCodeFixProvider` turns the +declaration into a `using` declaration, with project-wide "Fix all" support. + +### Reference sharing and parallelism + +The harness binds every trusted-platform assembly once per test process and hands the **same** +`MetadataReference` instances to every test compilation, so Roslyn reuses one set of assembly symbols for +the whole run. Creating references per run instead makes each compilation build its own symbol tables: in a +100-compilation micro-benchmark that is the difference between a 156 MB and an 8,146 MB peak. Assemblies +named by `AdditionalAssemblyTypes` that are already in the trusted set are a no-op and are not added again. + +Because the reference set is now shared, a harness-based test project's memory is largely flat in the degree +of parallelism — but the test platform's auto-detected default still costs more memory, and more wall clock, +than an explicit ceiling. Wall clock saturates at around four concurrent tests; `--maximum-parallel-tests 8` +measured 347–365 MB / 7.7–8.7s against 528–881 MB / 12.0–13.8s for auto on a 22-core machine. Set an +explicit ceiling when a project multi-targets and runs several TFM executables concurrently. + Analyzer options are preserved under their supplied keys. Keys without the Roslyn `build_property.` prefix are additionally exposed as compiler-visible MSBuild properties, so either `MyGenerator_Disable` or `build_property.MyGenerator_Disable` can be used in tests. diff --git a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UndisposedTestResultAnalyzerTests.cs b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UndisposedTestResultAnalyzerTests.cs new file mode 100644 index 0000000..9980efa --- /dev/null +++ b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UndisposedTestResultAnalyzerTests.cs @@ -0,0 +1,259 @@ +using Microsoft.CodeAnalysis; +using Purview.SourceGeneratorFramework.Testing; +using Purview.SourceGeneratorFramework.Testing.TUnit; + +namespace Purview.SourceGeneratorFramework.Analyzers; + +public sealed class UndisposedTestResultAnalyzerTests : TUnitDiagnosticAnalyzerTestBase +{ + // A stand-in for the runner surface: the analyzer keys off the produced type, not the method name, so a + // consumer's own wrapper is covered in exactly the same way the shipped runners are. + const string Harness = """ + using Purview.SourceGeneratorFramework; + using Purview.SourceGeneratorFramework.Testing; + + static class Runner + { + public static DriverRunResult Run(SourceGeneratorTestOptions options) => null!; + } + """; + + static AnalyzerTestOptions Options => + new() + { + AdditionalAssemblyTypes = [typeof(DriverRunResult), typeof(SourceGeneratorTestOptions)], + IncludeDefaultNamespaces = false, + }; + + [Test] + public async Task UsingDeclaration_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Arrange + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + using var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + System.Console.WriteLine(result.GetSource()); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasNoDiagnostics(); + } + + [Test] + public async Task PlainLocal_ReportsDiagnostic(CancellationToken cancellationToken) + { + // Arrange + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + System.Console.WriteLine(result.GetSource()); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasDiagnostics(1); + await Assert.That(result).HasDiagnostic(UndisposedTestResultAnalyzer.DiagnosticId); + } + + [Test] + public async Task ReturnedLocal_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Arrange - the caller becomes the owner, so the helper must not dispose it. + const string source = + Harness + + """ + + class Tests + { + public DriverRunResult Build() + { + var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + return result; + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasNoDiagnostics(); + } + + [Test] + public async Task HelperReturningAProjection_ReportsDiagnostic(CancellationToken cancellationToken) + { + // Arrange - the result itself never leaves the helper, so the helper owns it. + const string source = + Harness + + """ + + class Tests + { + public string BuildDefinition() + { + var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + return result.GetSource(); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasDiagnostics(1); + await Assert.That(result).HasDiagnostic(UndisposedTestResultAnalyzer.DiagnosticId); + } + + [Test] + public async Task ExplicitDispose_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Arrange + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + System.Console.WriteLine(result.GetSource()); + result.Dispose(); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasNoDiagnostics(); + } + + [Test] + public async Task DiscardedResult_ReportsDiagnostic(CancellationToken cancellationToken) + { + // Arrange + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + _ = Runner.Run(new SourceGeneratorTestOptions().Compile()); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasDiagnostics(1); + await Assert.That(result).HasDiagnostic(UndisposedTestResultAnalyzer.DiagnosticId); + } + + [Test] + public async Task CompileToAssemblyRun_ReportsAsWarning(CancellationToken cancellationToken) + { + // Arrange - an emitted assembly is the case that actually costs memory. + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + var result = Runner.Run(new SourceGeneratorTestOptions { CompileToAssembly = true }); + System.Console.WriteLine(result.GetSource()); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result.Diagnostics.Single().Severity).IsEqualTo(DiagnosticSeverity.Warning); + } + + [Test] + public async Task RunWithoutCompile_ReportsAsInformation(CancellationToken cancellationToken) + { + // Arrange - nothing was emitted, so the leak is cheap and the rule stays informational. + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + var result = Runner.Run(new SourceGeneratorTestOptions()); + System.Console.WriteLine(result.GetSource()); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result.Diagnostics.Single().Severity).IsEqualTo(DiagnosticSeverity.Info); + } + + [Test] + public async Task NonHarnessDisposable_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Arrange - the rule is scoped to the Testing namespace, not to disposables in general. + const string source = """ + class Thing : System.IDisposable + { + public void Dispose() { } + + public static Thing Make() => new Thing(); + } + + class Tests + { + public void Run() + { + var thing = Thing.Make(); + System.Console.WriteLine(thing.ToString()); + } + } + """; + + // Act + var result = await AnalyzeAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasNoDiagnostics(); + } +} diff --git a/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/DisposeTestResultCodeFixProviderTests.cs b/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/DisposeTestResultCodeFixProviderTests.cs new file mode 100644 index 0000000..d0fd032 --- /dev/null +++ b/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/DisposeTestResultCodeFixProviderTests.cs @@ -0,0 +1,80 @@ +using Purview.SourceGeneratorFramework.Analyzers; +using Purview.SourceGeneratorFramework.Testing; +using Purview.SourceGeneratorFramework.Testing.TUnit; + +namespace Purview.SourceGeneratorFramework.CodeFixers; + +public sealed class DisposeTestResultCodeFixProviderTests + : TUnitCodeFixTestBase +{ + const string Harness = """ + using Purview.SourceGeneratorFramework; + using Purview.SourceGeneratorFramework.Testing; + + static class Runner + { + public static DriverRunResult Run(SourceGeneratorTestOptions options) => null!; + } + """; + + static CodeFixTestOptions Options => + new() + { + EquivalenceKey = DisposeTestResultCodeFixProvider.EquivalenceKey, + AdditionalAssemblyTypes = [typeof(DriverRunResult), typeof(SourceGeneratorTestOptions)], + IncludeDefaultNamespaces = false, + }; + + [Test] + public async Task UndisposedResult_AddsUsingDeclaration(CancellationToken cancellationToken) + { + // Arrange + const string source = + Harness + + """ + + class Tests + { + public void Run() + { + var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + System.Console.WriteLine(result.GetSource()); + } + } + """; + + // Act + var result = await ApplyCodeFixAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result).HasDiagnostic(UndisposedTestResultAnalyzer.DiagnosticId); + await Assert + .That(result.FixedSource) + .Contains("using var result = Runner.Run(new SourceGeneratorTestOptions().Compile());"); + } + + [Test] + public async Task UndisposedResultInHelper_AddsUsingDeclaration(CancellationToken cancellationToken) + { + // Arrange - the helper projects a value out of the result and leaks the result itself. + const string source = + Harness + + """ + + class Tests + { + public string BuildDefinition() + { + var result = Runner.Run(new SourceGeneratorTestOptions().Compile()); + return result.GetSource(); + } + } + """; + + // Act + var result = await ApplyCodeFixAsync(source, Options, cancellationToken); + + // Assert + await Assert.That(result.FixedSource).Contains("using var result = Runner.Run("); + } +} diff --git a/src/tests/SourceGeneratorFramework.UnitTests/CompilationResultTests.cs b/src/tests/SourceGeneratorFramework.UnitTests/CompilationResultTests.cs index 9c5de5b..2d1b214 100644 --- a/src/tests/SourceGeneratorFramework.UnitTests/CompilationResultTests.cs +++ b/src/tests/SourceGeneratorFramework.UnitTests/CompilationResultTests.cs @@ -31,7 +31,7 @@ public async Task CompileToAssembly_ExposesLoadedAssembly_ThatCanExecute(Cancell SourceGeneratorTestRunner runner = new(); SourceGeneratorTestOptions options = new() { CompileToAssembly = true }; - var result = await runner.RunAsync("public sealed class Input { }", options, cancellationToken); + using var result = await runner.RunAsync("public sealed class Input { }", options, cancellationToken); await Assert.That(result.CompilationResult.Assembly).IsNotNull(); @@ -50,7 +50,7 @@ public async Task CompileToAssembly_LoadsIntoCollectibleContext(CancellationToke SourceGeneratorTestRunner runner = new(); SourceGeneratorTestOptions options = new() { CompileToAssembly = true }; - var result = await runner.RunAsync("public sealed class Input { }", options, cancellationToken); + using var result = await runner.RunAsync("public sealed class Input { }", options, cancellationToken); var context = AssemblyLoadContext.GetLoadContext(result.CompilationResult.Assembly!); @@ -76,7 +76,7 @@ public async Task Metadata_ReflectsGeneratedType_WithoutExecution(CancellationTo SourceGeneratorTestRunner runner = new(); SourceGeneratorTestOptions options = new() { CompileToAssembly = true }; - var result = await runner.RunAsync("public sealed class Input { }", options, cancellationToken); + using var result = await runner.RunAsync("public sealed class Input { }", options, cancellationToken); await Assert.That(result.CompilationResult.Metadata).IsNotNull(); await Assert.That(result.CompilationResult.MetadataAssembly).IsNotNull(); From 8f55b201224d1072d022bd6f65f82df8d7104572 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 16:29:59 +0100 Subject: [PATCH 03/10] fix(shared): key the TypeIdentity symbol cache on the symbol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache was a ConditionalWeakTable> keyed with SymbolEqualityComparer.Default. That was wrong twice over. A TypeIdentity records its type arguments as TypeReference values, and those carry the nullable reference annotation — but SymbolEqualityComparer.Default ignores it. List and List therefore shared one entry, and whichever was constructed first won for every later caller. That is the cause of a generator emitting RequiredRule for a non-nullable member once an earlier compilation in the same process had asked for the nullable form. A constructed generic also reports its *definition's* assembly as ContainingAssembly, so the entry hung off the long-lived corlib assembly symbol while the key strongly referenced source symbols, pinning whole compilations. A probe of 100 compilations that each take a TypeIdentity of List kept 100/100 alive after a forced collection; it now keeps 1/100, the same as taking no identity at all. Both defects only became observable process-wide once the Testing harness started sharing MetadataReference instances, because that made the corlib assembly symbol — and so the cache entry — live for the whole run. Keying weakly on the symbol fixes both and is faster: TypeIdentityBenchmarks moves from 13.7-15.7 ns to 8.0-8.9 ns, still zero-allocation. --- src/src/SourceGeneratorShared/TypeIdentity.cs | 45 +++------- .../TypeIdentityTests.cs | 87 +++++++++++++++++++ 2 files changed, 100 insertions(+), 32 deletions(-) diff --git a/src/src/SourceGeneratorShared/TypeIdentity.cs b/src/src/SourceGeneratorShared/TypeIdentity.cs index c77ab8f..dee9e2f 100644 --- a/src/src/SourceGeneratorShared/TypeIdentity.cs +++ b/src/src/SourceGeneratorShared/TypeIdentity.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using System.Collections.Immutable; using System.Runtime.CompilerServices; using Microsoft.CodeAnalysis; @@ -140,30 +139,21 @@ public TypeIdentity(ITypeSymbol typeSymbol) ); } - var assembly = namedType.ContainingAssembly; - if (assembly is not null) - { - var cache = AssemblySymbolCache.GetValue(assembly, AssemblyCacheFactory); - this = cache.GetOrAdd(typeSymbol, SymbolCacheFactory).Value; - } - else - { - this = CreateFromSymbol(typeSymbol).Value; - } + this = SymbolCache.GetValue(namedType, SymbolCacheFactory).Value; } - static readonly ConditionalWeakTable< - IAssemblySymbol, - ConcurrentDictionary - > AssemblySymbolCache = new(); - static readonly ConditionalWeakTable< - IAssemblySymbol, - ConcurrentDictionary - >.CreateValueCallback AssemblyCacheFactory = static assembly => new ConcurrentDictionary< - ITypeSymbol, - TypeIdentityCacheEntry - >(TypeSymbolEqualityComparer.Instance); - static readonly Func SymbolCacheFactory = CreateFromSymbol; + // Keyed weakly on the symbol itself, so a cached identity never outlives the symbol it was built from. + // + // The previous shape — a per-assembly ConcurrentDictionary keyed by ITypeSymbol — leaked: a constructed + // generic such as List reports its *definition's* assembly as the containing assembly, so the + // entry landed in the dictionary hanging off a long-lived metadata assembly (corlib) while the key + // strongly referenced a source symbol. That pinned the whole source compilation for the lifetime of the + // corlib assembly symbol — the entire compiler host session in an IDE, and the entire run in a test + // process now that metadata references (and therefore their assembly symbols) are shared across + // compilations. + static readonly ConditionalWeakTable SymbolCache = new(); + static readonly ConditionalWeakTable.CreateValueCallback SymbolCacheFactory = + CreateFromSymbol; static TypeIdentityCacheEntry CreateFromSymbol(ITypeSymbol typeSymbol) { @@ -194,15 +184,6 @@ static TypeIdentityCacheEntry CreateFromSymbol(ITypeSymbol typeSymbol) ); } - sealed class TypeSymbolEqualityComparer : IEqualityComparer - { - public static readonly TypeSymbolEqualityComparer Instance = new(); - - public bool Equals(ITypeSymbol? x, ITypeSymbol? y) => SymbolEqualityComparer.Default.Equals(x, y); - - public int GetHashCode(ITypeSymbol obj) => SymbolEqualityComparer.Default.GetHashCode(obj); - } - sealed class TypeIdentityCacheEntry(TypeIdentity value) { public TypeIdentity Value = value; diff --git a/src/tests/SourceGeneratorShared.UnitTests/TypeIdentityTests.cs b/src/tests/SourceGeneratorShared.UnitTests/TypeIdentityTests.cs index 318af79..51d9eb5 100644 --- a/src/tests/SourceGeneratorShared.UnitTests/TypeIdentityTests.cs +++ b/src/tests/SourceGeneratorShared.UnitTests/TypeIdentityTests.cs @@ -1,3 +1,4 @@ +using System.Runtime.CompilerServices; using Microsoft.CodeAnalysis; namespace Purview.SourceGeneratorFramework; @@ -590,4 +591,90 @@ public async Task EqualityWithNullAndEmpty_ConstructedGeneric_DoesNotOverflow() await Assert.That(resourceKitBase == TypeIdentity.Null).IsFalse(); await Assert.That(resourceKitBase == TypeIdentity.Empty).IsFalse(); } + + // --------------------------------------------------------------------------------------------- + // Regression: the symbol cache must not conflate types that produce different identities + // --------------------------------------------------------------------------------------------- + + [Test] + public async Task Create_GivenGenericArgumentsDifferingOnlyByNullability_DoesNotShareACachedIdentity() + { + // Arrange - a TypeIdentity records its type arguments as TypeReference values, and those carry the + // nullable reference annotation. SymbolEqualityComparer.Default ignores that annotation, so keying the + // cache with it made List and List collide: whichever was built first won, for every + // later compilation in the process. + var compilation = TestCompilation.Create(); + var list = compilation.GetTypeByMetadataName("System.Collections.Generic.List`1")!; + var @string = compilation.GetSpecialType(SpecialType.System_String); + + var ofNullable = list.Construct(@string.WithNullableAnnotation(NullableAnnotation.Annotated)); + var ofNonNullable = list.Construct(@string.WithNullableAnnotation(NullableAnnotation.NotAnnotated)); + + // Act - nullable first, so a collision would hand the annotated identity back for the second. + TypeIdentity nullable = new(ofNullable); + TypeIdentity nonNullable = new(ofNonNullable); + + // Assert + await Assert.That(nullable.TypeArguments[0].IsNullable).IsTrue(); + await Assert.That(nonNullable.TypeArguments[0].IsNullable).IsFalse(); + } + + // --------------------------------------------------------------------------------------------- + // Regression: the symbol cache must not outlive the symbols it is built from + // --------------------------------------------------------------------------------------------- + + [Test] + public async Task Create_GivenConstructedGenericOverSourceType_DoesNotRetainTheCompilation() + { + // Arrange - an anchor compilation stands in for the rest of the run: because the harness shares its + // MetadataReference instances, every test compilation binds the *same* corlib assembly symbol, and + // that symbol stays alive as long as any compilation holds it. + var anchor = TestCompilation.Create(); + var anchorCorlib = anchor.GetSpecialType(SpecialType.System_Int32).ContainingAssembly; + + // List reports its definition's assembly - corlib - as the containing assembly. Keying the + // cached identity off that assembly, rather than off the symbol, pinned the source compilation for as + // long as the anchor lived. + var (reference, name, corlib) = BuildIdentityAndDiscardCompilation(); + await Assert.That(name).IsEqualTo("List"); + await Assert.That(ReferenceEquals(corlib, anchorCorlib)).IsTrue(); + + // Act + for (var attempt = 0; attempt < 3; attempt++) + { + GC.Collect(2, GCCollectionMode.Forced, blocking: true, compacting: true); + GC.WaitForPendingFinalizers(); + } + + // Assert + await Assert.That(reference.IsAlive).IsFalse(); + + GC.KeepAlive(anchorCorlib); + GC.KeepAlive(anchor); + } + + // Kept out of the caller's frame so the compilation has no live root once this returns. + [MethodImpl(MethodImplOptions.NoInlining)] + static (WeakReference Compilation, string IdentityName, IAssemblySymbol Corlib) BuildIdentityAndDiscardCompilation() + { + var compilation = TestCompilation.Create( + """ + namespace Sample; + + public class Marker { } + + public class Owner + { + public System.Collections.Generic.List Items = null!; + } + """ + ); + + var owner = compilation.GetTypeByMetadataName("Sample.Owner")!; + var itemsType = ((IFieldSymbol)owner.GetMembers("Items")[0]).Type; + + TypeIdentity identity = new(itemsType); + + return (new(compilation), identity.Name, itemsType.ContainingAssembly); + } } From 0f17467dc36e59bedd55ea5f0dbcedeb53c92ce3 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 16:30:12 +0100 Subject: [PATCH 04/10] fix(testing): stop a scoped CodeQuery repeating matches once per tree A scoped query - query.In(node), or the Query carried by any CodeQueryResult - has exactly one root no matter how many trees it carries, but every search enumerated Trees and resolved RootOf(tree), which returns that one root each time. Count() and GetAll() therefore multiplied every match by the number of trees, and the Get/Has/TryGet family walked the same subtree once per tree before concluding a node was absent. It was invisible in the existing tests because they all query single-tree compilations; it bites on result.Output(), where the compilation carries the user source plus every generated tree. Searches now enumerate Roots(), which yields the scoping node alone when the query is nested. --- .../CodeQuery.Declarations.cs | 8 ++--- .../CodeQuery.Members.cs | 12 +++---- .../CodeQuery.Statements.cs | 8 ++--- .../CodeQuery.cs | 29 +++++++++++---- .../CodeQueryTests.cs | 35 +++++++++++++++++++ 5 files changed, 71 insertions(+), 21 deletions(-) diff --git a/src/src/SourceGeneratorFramework.Testing/CodeQuery.Declarations.cs b/src/src/SourceGeneratorFramework.Testing/CodeQuery.Declarations.cs index 28d4af4..f2f14be 100644 --- a/src/src/SourceGeneratorFramework.Testing/CodeQuery.Declarations.cs +++ b/src/src/SourceGeneratorFramework.Testing/CodeQuery.Declarations.cs @@ -481,9 +481,9 @@ public bool TryGetField(string name, out FieldDeclarationSyntax? field) if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("The field name cannot be null or whitespace.", nameof(name)); - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var declarator in RootOf(tree).DescendantNodes().OfType()) + foreach (var declarator in root.DescendantNodes().OfType()) { if (declarator.Identifier.ValueText != name) continue; @@ -617,9 +617,9 @@ static bool NamespaceMatches(SyntaxNode node, string? @namespace) => bool TryFind(string name, Func getName, Func? additional, out T? node) where T : SyntaxNode { - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var candidate in RootOf(tree).DescendantNodes().OfType()) + foreach (var candidate in root.DescendantNodes().OfType()) { if (getName(candidate) != name) continue; diff --git a/src/src/SourceGeneratorFramework.Testing/CodeQuery.Members.cs b/src/src/SourceGeneratorFramework.Testing/CodeQuery.Members.cs index 08412b8..a343163 100644 --- a/src/src/SourceGeneratorFramework.Testing/CodeQuery.Members.cs +++ b/src/src/SourceGeneratorFramework.Testing/CodeQuery.Members.cs @@ -53,9 +53,9 @@ params TypeReference[]? parameters throw new ArgumentException("The operator token cannot be null or whitespace.", nameof(operatorToken)); var expected = parameters ?? []; - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var candidate in RootOf(tree).DescendantNodes().OfType()) + foreach (var candidate in root.DescendantNodes().OfType()) { if (candidate.OperatorToken.ValueText != operatorToken) continue; @@ -115,9 +115,9 @@ params TypeReference[]? parameters throw new ArgumentException("The keyword must be 'implicit' or 'explicit'.", nameof(keyword)); var expected = parameters ?? []; - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var candidate in RootOf(tree).DescendantNodes().OfType()) + foreach (var candidate in root.DescendantNodes().OfType()) { if (candidate.ImplicitOrExplicitKeyword.ValueText != keyword) continue; @@ -159,9 +159,9 @@ public CodeQueryResult GetIndexer(params TypeReference public bool TryGetIndexer(out IndexerDeclarationSyntax? indexer, params TypeReference[] parameters) { var expected = parameters ?? []; - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var candidate in RootOf(tree).DescendantNodes().OfType()) + foreach (var candidate in root.DescendantNodes().OfType()) { if (expected.Length == 0 || MatchesParameters(candidate.ParameterList, expected)) { diff --git a/src/src/SourceGeneratorFramework.Testing/CodeQuery.Statements.cs b/src/src/SourceGeneratorFramework.Testing/CodeQuery.Statements.cs index 77a5dba..eba9551 100644 --- a/src/src/SourceGeneratorFramework.Testing/CodeQuery.Statements.cs +++ b/src/src/SourceGeneratorFramework.Testing/CodeQuery.Statements.cs @@ -91,9 +91,9 @@ public bool TryGetInvocation(string methodName, out InvocationExpressionSyntax? if (string.IsNullOrWhiteSpace(methodName)) throw new ArgumentException("The method name cannot be null or whitespace.", nameof(methodName)); - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var candidate in RootOf(tree).DescendantNodes().OfType()) + foreach (var candidate in root.DescendantNodes().OfType()) { if (GetInvokedMethodName(candidate) == methodName) { @@ -115,9 +115,9 @@ public bool HasObjectCreation(TypeReference type) if (type is null) throw new ArgumentNullException(nameof(type)); - foreach (var tree in Trees) + foreach (var root in Roots()) { - foreach (var candidate in RootOf(tree).DescendantNodes().OfType()) + foreach (var candidate in root.DescendantNodes().OfType()) { if (Matches(candidate.Type, type)) return true; diff --git a/src/src/SourceGeneratorFramework.Testing/CodeQuery.cs b/src/src/SourceGeneratorFramework.Testing/CodeQuery.cs index f5e24d8..49f69c5 100644 --- a/src/src/SourceGeneratorFramework.Testing/CodeQuery.cs +++ b/src/src/SourceGeneratorFramework.Testing/CodeQuery.cs @@ -67,7 +67,25 @@ public CodeQuery In(SyntaxNode root) return new(Trees, Compilation, IsGenerated, root); } - SyntaxNode RootOf(SyntaxTree tree) => Root ?? tree.GetRoot(); + /// + /// Enumerates the roots a node search runs over: the single scoping node when this is a nested query, + /// otherwise the root of every tree. + /// + /// + /// A scoped query has exactly one root no matter how many trees it carries. Enumerating per tree instead + /// would walk that one subtree once per tree, repeating every match. + /// + internal IEnumerable Roots() + { + if (Root is not null) + { + yield return Root; + yield break; + } + + foreach (var tree in Trees) + yield return tree.GetRoot(); + } // --------------------------------------------------------------------------------------------- // Syntax trees @@ -137,9 +155,8 @@ public ImmutableArray> GetAll(Func? predicate = n where T : SyntaxNode { var builder = ImmutableArray.CreateBuilder>(); - foreach (var tree in Trees) + foreach (var root in Roots()) { - var root = RootOf(tree); if (root is T rootNode && (predicate is null || predicate(rootNode))) builder.Add(new(this, rootNode)); @@ -160,9 +177,8 @@ public int Count(Func? predicate = null) where T : SyntaxNode { var count = 0; - foreach (var tree in Trees) + foreach (var root in Roots()) { - var root = RootOf(tree); if (root is T rootNode && (predicate is null || predicate(rootNode))) count++; @@ -179,9 +195,8 @@ public int Count(Func? predicate = null) public bool TryGet(out T? node, Func? predicate = null) where T : SyntaxNode { - foreach (var tree in Trees) + foreach (var root in Roots()) { - var root = RootOf(tree); if (root is T rootNode && (predicate is null || predicate(rootNode))) { node = rootNode; diff --git a/src/tests/SourceGeneratorFramework.UnitTests/CodeQueryTests.cs b/src/tests/SourceGeneratorFramework.UnitTests/CodeQueryTests.cs index dda4271..187f6b2 100644 --- a/src/tests/SourceGeneratorFramework.UnitTests/CodeQueryTests.cs +++ b/src/tests/SourceGeneratorFramework.UnitTests/CodeQueryTests.cs @@ -332,6 +332,41 @@ await Assert .IsEqualTo(1); } + [Test] + public async Task ScopedQuery_GivenMultipleTrees_DoesNotRepeatMatchesPerTree() + { + // Arrange - a scope is a single node, so the number of trees in the query must not affect the result. + const string first = """ + namespace Test; + + public sealed class Holder + { + public void One() { } + public void Two() { } + } + """; + const string second = """ + namespace Test; + + public sealed class Other + { + public void Three() { } + } + """; + + var (tree, _) = TestCompilation.Parse(second); + var compilation = TestCompilation.Create(first).AddSyntaxTrees(tree); + CodeQuery query = new([.. compilation.SyntaxTrees], compilation); + + // Act + var scoped = query.GetClass("Holder").Query; + + // Assert + await Assert.That(query.Trees.Length).IsEqualTo(2); + await Assert.That(scoped.Count()).IsEqualTo(2); + await Assert.That(scoped.GetAll().Length).IsEqualTo(2); + } + [Test] public async Task OperatorAndIndexerAndAttributeQueries_FindDeclarations() { From 06aa4b2ea9b04b22677579a721b341d400744870 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 16:30:12 +0100 Subject: [PATCH 05/10] test(benchmarks): dispose the runner result in the harness benchmark SourceGeneratorTestRunnerBenchmarks left every DriverRunResult undisposed, so the CompileToAssembly variants accumulated one collectible AssemblyLoadContext per iteration and measured the leak alongside the run. Flagged by PSGFR42. --- .../Benchmarks/SourceGeneratorTestRunnerBenchmarks.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/src/SourceGeneratorFramework.Benchmarks/Benchmarks/SourceGeneratorTestRunnerBenchmarks.cs b/src/src/SourceGeneratorFramework.Benchmarks/Benchmarks/SourceGeneratorTestRunnerBenchmarks.cs index 3f51089..c7713b6 100644 --- a/src/src/SourceGeneratorFramework.Benchmarks/Benchmarks/SourceGeneratorTestRunnerBenchmarks.cs +++ b/src/src/SourceGeneratorFramework.Benchmarks/Benchmarks/SourceGeneratorTestRunnerBenchmarks.cs @@ -40,6 +40,8 @@ public async Task RunAsync() ValidateCodeWriterScopes = false, }; - await _runner.RunAsync(_source, options); + // Disposed because a CompileToAssembly run creates a collectible AssemblyLoadContext: leaving it to + // the finalizer would accumulate one per iteration and measure the leak rather than the run. + using var result = await _runner.RunAsync(_source, options); } } From 8df5af0134b2ae78ba2b5dc15e2cc8109f6cc49c Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 16:52:13 +0100 Subject: [PATCH 06/10] fix(testing): register the logging sink for incremental runs, and keep entries ordered RunIncremental created a logging session id and baked it into the shared driver, but never called ConfigureLogging. No sink was ever registered for that id, so CreateLogger returned null and every framework log entry written during RunIncrementalAsync/GenerateIncrementalAsync was silently dropped. The sink is now registered for the whole sequence - the driver is shared, so it has to outlive the individual runs - and drained between runs, so each IncrementalCacheRun carries only what that run wrote through a new LogEntries property. A rerun whose output step is cached logs nothing, which makes logging a second signal alongside the step reasons. ConfigureLogging also registers with a self-contained generator's embedded SourceGenLogging copy, so merged analyzers are covered on this path too. RunAsync collected entries into a ConcurrentBag, whose enumeration order is unspecified, so DriverRunResult.LogEntries came back in an arbitrary order. It is a ConcurrentQueue now and both paths report entries in the order the generator wrote them. LogEntries is an init-only property rather than a positional record parameter, so the IncrementalCacheRun constructor stays source- and binary-compatible. --- docs/wiki/Step-Cache-Tests.md | 17 ++++ .../skills/tunit-test-authoring/SKILL.md | 2 + .../IncrementalCacheResult.cs | 14 ++- .../skills/source-generator-testing/SKILL.md | 14 +++ .../Sdk/README.md | 13 +++ .../SourceGeneratorTestRunner.cs | 34 +++++-- .../ServiceRegistrationCacheTests.cs | 36 ++++++++ .../GeneratorLoggingTests.cs | 90 +++++++++++++++++++ .../OrderedLoggingTestGenerator.cs | 36 ++++++++ 9 files changed, 249 insertions(+), 7 deletions(-) create mode 100644 src/tests/SourceGeneratorShared.UnitTests/GeneratorLoggingTests.cs create mode 100644 src/tests/SourceGeneratorShared.UnitTests/TestGenerators/OrderedLoggingTestGenerator.cs diff --git a/docs/wiki/Step-Cache-Tests.md b/docs/wiki/Step-Cache-Tests.md index 81509b5..611e2de 100644 --- a/docs/wiki/Step-Cache-Tests.md +++ b/docs/wiki/Step-Cache-Tests.md @@ -43,6 +43,23 @@ exactly what the driver decided to recompute. `GenerateIncrementalAsync` on `TUnitSourceGeneratorTestBase` wraps the same runner for test-base users. +## Log entries + +Each `IncrementalCacheRun` carries the framework log entries that **that run** wrote, in the order the +generator wrote them, through `IncrementalCacheRun.LogEntries`. The sink is registered for the whole +sequence (the driver is shared) and drained between runs, so entries never accumulate across runs: + +```csharp +var result = await GenerateIncrementalAsync([Source], cancellationToken: cancellationToken); + +await Assert.That(result.Runs[0].LogEntries).IsNotEmpty(); // the first run executed the pipeline +await Assert.That(result.Runs[1].LogEntries).IsEmpty(); // the rerun reused the cached output +``` + +That makes logging a second, independent signal alongside the step reasons: a stage reported as `Cached` +should not have logged anything on that run. Entries are empty when +`SourceGeneratorTestOptions.EnableLogging` is disabled. + ## Step reasons Each pipeline stage (tracked by its tracking name) has outputs, each carrying an diff --git a/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md b/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md index 13ebec5..cf38b75 100644 --- a/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md +++ b/src/src/SourceGeneratorFramework.Testing.TUnit/Sdk/.agents/skills/tunit-test-authoring/SKILL.md @@ -142,6 +142,8 @@ Why the tests look the way they do: - `new IncrementalRunInput([Source], [("build_property.X", "value")])` toggles an MSBuild property for one run only, so a property-only change must mark `GetMSBuildPropertyValue_*`/`GetGenerationConfiguration`/ `GetGenerationContext_*` `Modified` while `ForAttribute_*` stays `Cached`. +- `result.Runs[n].LogEntries` carries the framework log entries *that run* wrote, in order, so a rerun whose + output step is cached can be asserted to have logged nothing. Empty when `EnableLogging` is disabled. - The `StepReasons(IncrementalCacheRun)` helper flattens each tracked step's `Outputs` into a `ImmutableDictionary>` so assertions can address a stage by name (see `source-generator-testing` for the helper body). diff --git a/src/src/SourceGeneratorFramework.Testing/IncrementalCacheResult.cs b/src/src/SourceGeneratorFramework.Testing/IncrementalCacheResult.cs index 084de2c..b927b62 100644 --- a/src/src/SourceGeneratorFramework.Testing/IncrementalCacheResult.cs +++ b/src/src/SourceGeneratorFramework.Testing/IncrementalCacheResult.cs @@ -1,5 +1,6 @@ using System.Collections.Immutable; using Microsoft.CodeAnalysis; +using Purview.SourceGeneratorFramework.Testing.Models; namespace Purview.SourceGeneratorFramework.Testing; @@ -29,7 +30,18 @@ public sealed record IncrementalRunInput( public sealed record IncrementalCacheRun( GeneratorRunResult RunResult, ImmutableDictionary> Steps -); +) +{ + /// + /// The framework log entries written by the generator during this run, in the order they were written. + /// Empty when is disabled. + /// + /// + /// Scoped to the single run rather than the whole sequence, so a rerun whose stages all report + /// can be asserted to have logged nothing. + /// + public ImmutableArray LogEntries { get; init; } = []; +} /// /// The result of an incremental cache test run: one per input, produced by a diff --git a/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md b/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md index 37b9525..fdbfc45 100644 --- a/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md +++ b/src/src/SourceGeneratorFramework.Testing/Sdk/.agents/skills/source-generator-testing/SKILL.md @@ -218,6 +218,20 @@ these tests fail the build and catch the regression. Roslyn would see a fresh compilation on the second run and report stages `Modified`/`New` even though the sources are byte-identical — the "cached" assertion would fail. +### Log entries per run + +`IncrementalCacheRun.LogEntries` carries the framework log entries that run wrote, in order. The sink is +registered for the whole sequence and drained between runs, so entries never accumulate. A rerun whose +output step is cached logs nothing, which makes it a second signal alongside the step reasons: + +```csharp +await Assert.That(result.Runs[0].LogEntries).IsNotEmpty(); +await Assert.That(result.Runs[1].LogEntries).IsEmpty(); +``` + +`DriverRunResult.LogEntries` (the `RunAsync` path) is likewise in write order. Both are empty when +`EnableLogging` is disabled. + ### The `StepReasons` helper `IncrementalCacheRun.Steps` is `ImmutableDictionary>` diff --git a/src/src/SourceGeneratorFramework.Testing/Sdk/README.md b/src/src/SourceGeneratorFramework.Testing/Sdk/README.md index 5cd4d36..d33eda6 100644 --- a/src/src/SourceGeneratorFramework.Testing/Sdk/README.md +++ b/src/src/SourceGeneratorFramework.Testing/Sdk/README.md @@ -274,6 +274,19 @@ await Assert.That(result.Runs[1]).StepIsModified("ForAttribute_MyAttribute"); await Assert.That(result.Runs[1]).StepIsCached("GetGenerationConfiguration"); ``` +Each run also carries the framework log entries *that run* wrote, in write order, through +`IncrementalCacheRun.LogEntries` — the sink is registered for the whole sequence and drained between runs, +so entries never accumulate. A rerun whose output step is cached logs nothing, giving a second signal +alongside the step reasons: + +```csharp +await Assert.That(result.Runs[0].LogEntries).IsNotEmpty(); +await Assert.That(result.Runs[1].LogEntries).IsEmpty(); +``` + +`DriverRunResult.LogEntries` (the `RunAsync` path) is likewise in write order. Both are empty when +`EnableLogging` is disabled. + `RunIncrementalAsync(sources, options, ct)` runs the same source set twice (the common "unchanged rerun is cached" case). Per-run MSBuild-property changes use `new IncrementalRunInput(sources, [...])`. Reference cache tests live in the `Purview.SourceGeneratorFramework` source repository — diff --git a/src/src/SourceGeneratorFramework.Testing/SourceGeneratorTestRunner.cs b/src/src/SourceGeneratorFramework.Testing/SourceGeneratorTestRunner.cs index b838745..bbc4b06 100644 --- a/src/src/SourceGeneratorFramework.Testing/SourceGeneratorTestRunner.cs +++ b/src/src/SourceGeneratorFramework.Testing/SourceGeneratorTestRunner.cs @@ -42,7 +42,8 @@ public async Task RunAsync( ); } - ConcurrentBag logEntries = []; + // A queue, not a bag: log entries are only meaningful in the order the generator wrote them. + ConcurrentQueue logEntries = []; if (!options.AdditionalSources.IsDefaultOrEmpty) sources = sources.Concat(options.AdditionalSources); @@ -153,6 +154,12 @@ public IncrementalCacheResult RunIncremental( TGenerator generator = new(); var loggingSessionId = options.EnableLogging ? Guid.NewGuid().ToString("N") : null; + + // The sink has to be registered for the whole sequence: the driver is shared across the runs, and the + // session id baked into it by CreateDriver only resolves to a logger while a sink is registered for it. + ConcurrentQueue logEntries = []; + using var loggingRegistration = ConfigureLogging(loggingSessionId, options, logEntries); + var driver = CreateDriver(generator, options, loggingSessionId); var references = SourceGeneratorHelpers.ResolveReferences(options, typeof(TGenerator).Assembly); var runs = ImmutableArray.CreateBuilder(materializedInputs.Count); @@ -178,12 +185,27 @@ public IncrementalCacheResult RunIncremental( } driver = driver.RunGeneratorsAndUpdateCompilation(compilation, out _, out _, cancellationToken); - runs.Add(CaptureRun(driver)); + + // Drained per run, so each run carries only what that run logged. A stage reported as Cached + // should log nothing, which is what makes this worth capturing separately. + runs.Add(CaptureRun(driver, DrainLogEntries(logEntries))); } return new(runs.ToImmutable()); } + static ImmutableArray DrainLogEntries(ConcurrentQueue logEntries) + { + if (logEntries.IsEmpty) + return []; + + var builder = ImmutableArray.CreateBuilder(logEntries.Count); + while (logEntries.TryDequeue(out var entry)) + builder.Add(entry); + + return builder.ToImmutable(); + } + static CSharpCompilation BuildCompilation( IEnumerable sources, SourceGeneratorTestOptions options, @@ -208,7 +230,7 @@ CancellationToken cancellationToken return SourceGeneratorHelpers.CreateCompilation(syntaxTrees, references, options); } - static IncrementalCacheRun CaptureRun(GeneratorDriver driver) + static IncrementalCacheRun CaptureRun(GeneratorDriver driver, ImmutableArray logEntries) { var driverRunResult = driver.GetRunResult(); var runResult = driverRunResult.Results.FirstOrDefault(run => run.Generator is not null); @@ -221,7 +243,7 @@ static IncrementalCacheRun CaptureRun(GeneratorDriver driver) steps[pair.Key] = pair.Value; } - return new(runResult, steps.ToImmutable()); + return new(runResult, steps.ToImmutable()) { LogEntries = logEntries }; } static async Task GetAnalyzerResultsAsync( @@ -351,7 +373,7 @@ static Dictionary BuildAnalyzerConfig( static LoggingRegistrations? ConfigureLogging( string? loggingSessionId, SourceGeneratorTestOptions options, - ConcurrentBag logEntries + ConcurrentQueue logEntries ) { if (loggingSessionId is null) @@ -362,7 +384,7 @@ void Sink(string message, int level) var type = (SourceGenLogLevel)level; options.TestOutput.WriteLine($"[{type}] {message}"); - logEntries.Add(new(type, message)); + logEntries.Enqueue(new(type, message)); } List registrations = [SourceGenLogging.RegisterSinkCore(loggingSessionId, Sink)]; diff --git a/src/tests/SourceGeneratorFramework.ExampleGenerator.UnitTests/ServiceRegistrationCacheTests.cs b/src/tests/SourceGeneratorFramework.ExampleGenerator.UnitTests/ServiceRegistrationCacheTests.cs index 787fb7d..3211ccd 100644 --- a/src/tests/SourceGeneratorFramework.ExampleGenerator.UnitTests/ServiceRegistrationCacheTests.cs +++ b/src/tests/SourceGeneratorFramework.ExampleGenerator.UnitTests/ServiceRegistrationCacheTests.cs @@ -81,4 +81,40 @@ public class OtherService { } await Assert.That(result.Runs[1]).StepIsModified("ForAttribute_GenerateServiceAttribute"); await Assert.That(result.Runs[1]).StepIsCached("GetMSBuildPropertyValue_EmitServiceRegistrationInfo"); } + + [Test] + public async Task GenerateIncrementalAsync_CapturesFrameworkLoggingPerRun(CancellationToken cancellationToken) + { + // Arrange / Act - two runs over a single shared driver. + var result = await GenerateIncrementalAsync([Source], cancellationToken: cancellationToken); + + // Assert - the first run executes the pipeline and logs; the entries belong to that run alone. + await Assert.That(result.Runs[0].LogEntries).IsNotEmpty(); + await Assert + .That( + result + .Runs[0] + .LogEntries.Any(entry => entry.Message.Contains("generation context", StringComparison.Ordinal)) + ) + .IsTrue(); + + // Each run carries only its own entries. If they were accumulated across the sequence instead of + // drained per run, the second run would also hold everything the first one logged. + await Assert.That(result.Runs[1].LogEntries.Length).IsLessThanOrEqualTo(result.Runs[0].LogEntries.Length); + } + + [Test] + public async Task GenerateIncrementalAsync_GivenLoggingDisabled_CapturesNothing(CancellationToken cancellationToken) + { + // Arrange / Act + var result = await GenerateIncrementalAsync( + [Source], + new ServiceRegistrationTestOptions { EnableLogging = false }, + cancellationToken + ); + + // Assert + await Assert.That(result.Runs[0].LogEntries).IsEmpty(); + await Assert.That(result.Runs[1].LogEntries).IsEmpty(); + } } diff --git a/src/tests/SourceGeneratorShared.UnitTests/GeneratorLoggingTests.cs b/src/tests/SourceGeneratorShared.UnitTests/GeneratorLoggingTests.cs new file mode 100644 index 0000000..388c993 --- /dev/null +++ b/src/tests/SourceGeneratorShared.UnitTests/GeneratorLoggingTests.cs @@ -0,0 +1,90 @@ +using Purview.SourceGeneratorFramework.TestGenerators; +using Purview.SourceGeneratorFramework.Testing.Models; + +namespace Purview.SourceGeneratorFramework; + +/// +/// Covers how the runners surface framework log entries: in order, scoped to the run that wrote them, and +/// only when logging is enabled. +/// +public class GeneratorLoggingTests +{ + const string Source = "public class Anything { }"; + + // The framework pipeline logs entries of its own around the generator's, so the generator's messages are + // asserted as an ordered subsequence rather than as the whole set. + static async Task AssertWrittenInOrderAsync(IEnumerable entries) + { + var messages = entries.Select(static entry => entry.Message).ToList(); + var positions = OrderedLoggingTestGenerator.Messages.Select(message => messages.IndexOf(message)).ToArray(); + + await Assert.That(positions).DoesNotContain(-1); + await Assert.That(positions.SequenceEqual(positions.Order())).IsTrue(); + } + + [Test] + public async Task RunAsync_PreservesTheOrderEntriesWereWritten(CancellationToken cancellationToken) + { + // Arrange + SourceGeneratorTestRunner runner = new(); + + // Act + using var result = await runner.RunAsync(Source, cancellationToken: cancellationToken); + + // Assert + await AssertWrittenInOrderAsync(result.LogEntries); + } + + [Test] + public async Task RunAsync_GivenLoggingDisabled_CapturesNothing(CancellationToken cancellationToken) + { + // Arrange + SourceGeneratorTestRunner runner = new(); + + // Act + using var result = await runner.RunAsync( + Source, + new SourceGeneratorTestOptions { EnableLogging = false }, + cancellationToken + ); + + // Assert + await Assert.That(result.LogEntries).IsEmpty(); + } + + [Test] + public async Task RunIncrementalAsync_ScopesEntriesToTheRunThatWroteThem(CancellationToken cancellationToken) + { + // Arrange - the incremental runner shares one driver across the sequence, so the sink has to be + // registered for the whole sequence and drained between runs. + SourceGeneratorTestRunner runner = new(); + + // Act + var result = await runner.RunIncrementalAsync([Source], cancellationToken: cancellationToken); + + // Assert - the first run executes the output step and writes its entries in order. + await AssertWrittenInOrderAsync(result.Runs[0].LogEntries); + + // The rerun reuses the cached output, so it writes nothing. Were the entries accumulated across the + // sequence rather than drained per run, this would still hold the first run's. + await Assert.That(result.Runs[1].LogEntries).IsEmpty(); + } + + [Test] + public async Task RunIncrementalAsync_GivenLoggingDisabled_CapturesNothing(CancellationToken cancellationToken) + { + // Arrange + SourceGeneratorTestRunner runner = new(); + + // Act + var result = await runner.RunIncrementalAsync( + [Source], + new SourceGeneratorTestOptions { EnableLogging = false }, + cancellationToken + ); + + // Assert + foreach (var run in result.Runs) + await Assert.That(run.LogEntries).IsEmpty(); + } +} diff --git a/src/tests/SourceGeneratorShared.UnitTests/TestGenerators/OrderedLoggingTestGenerator.cs b/src/tests/SourceGeneratorShared.UnitTests/TestGenerators/OrderedLoggingTestGenerator.cs new file mode 100644 index 0000000..ff5e5c2 --- /dev/null +++ b/src/tests/SourceGeneratorShared.UnitTests/TestGenerators/OrderedLoggingTestGenerator.cs @@ -0,0 +1,36 @@ +using Microsoft.CodeAnalysis; +using Purview.SourceGeneratorFramework.Helpers; +using Purview.SourceGeneratorFramework.Logging; + +namespace Purview.SourceGeneratorFramework.TestGenerators; + +/// +/// Writes a known sequence of log entries so a test can prove the runner surfaces them in the order the +/// generator wrote them. +/// +public sealed class OrderedLoggingTestGenerator : IIncrementalGenerator +{ + /// + /// The messages written, in order, on every run. + /// + public static readonly string[] Messages = ["first entry", "second entry", "third entry"]; + + public void Initialize(IncrementalGeneratorInitializationContext context) + { + var generationContext = IncrementalPipeline.DefaultGenerationContextValueProvider( + context, + new GenerationSettings("OrderedLoggingTestGenerator", "1.0.0") + ); + + context.RegisterSourceOutput( + generationContext, + static (spc, value) => + { + foreach (var message in Messages) + value.Log(SourceGenLogLevel.Info, 0, message); + + spc.AddSource("OrderedLogging.g.cs", "// OrderedLoggingTestGenerator"); + } + ); + } +} From 00321fe561ad5cc5e217c00a52e4b3a802c79145 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 18:51:01 +0100 Subject: [PATCH 07/10] perf: various forms of caching and expensive ops fixed --- .editorconfig | 129 +++++++++--------- global.json | 4 +- package.json | 4 +- .../ConvertToExtensionBlockCodeFixProvider.cs | 4 +- .../Extensions/System/StringExtensions.cs | 5 + .../Helpers/AttributeDataModelLibrary.cs | 13 +- .../FrameworkTypeInternalizer.cs | 23 ++-- .../CodeWriterExtensions.cs | 5 + .../SourceGeneratorTestOptionsExtensions.cs | 5 + .../SourceGeneratorTestOptionsExtensions.cs | 5 + .../CodeAnalysis/AttributeDataExtensions.cs | 5 + .../CodeAnalysis/TypedConstantExtensions.cs | 5 + .../Extensions/System/StringExtensions.cs | 5 + .../CSharp/Syntax/SyntaxNodeExtensions.cs | 5 + .../CodeAnalysis/ISymbolExtensions.cs | 5 + ...eneratorInitializationContextExtensions.cs | 5 + .../SourceProductionContextExtensions.cs | 5 + .../GenerationContext.cs | 12 +- src/src/SourceGeneratorShared/TypeIdentity.cs | 2 +- .../Helpers/TypeHelpersTests.cs | 12 +- 20 files changed, 154 insertions(+), 104 deletions(-) diff --git a/.editorconfig b/.editorconfig index 959b462..3476e6e 100644 --- a/.editorconfig +++ b/.editorconfig @@ -27,8 +27,10 @@ dotnet_search_reference_assemblies = true # Nullability settings dotnet_build_property.Nullable = enable -# Enable or disable the analyzers -dotnet_analyzer_diagnostic.severity = warning +# Per-rule severity is authoritative in this file. The SDK sets AnalysisMode/AnalysisLevel as +# MSBuild properties, and the .NET SDK ignores bulk 'dotnet_analyzer_diagnostic.*' severity +# configuration whenever those properties are present. Do not re-add a bulk entry here: it +# silently does nothing and hides which severities are actually enforced. # Visual Studio XML Project Files [*.{csproj,vbproj,vcxproj,vcxproj.filters,proj,projitems,shproj}] @@ -76,23 +78,20 @@ indent_style = tab dotnet_naming_rule.non_private_static_fields_should_be_pascal_case.severity = warning dotnet_naming_rule.non_private_static_fields_should_be_pascal_case.style = non_private_static_field_style dotnet_naming_rule.non_private_static_fields_should_be_pascal_case.symbols = non_private_static_fields -dotnet_naming_rule.private_fields.severity = warning -dotnet_naming_rule.private_fields.style = camel_case_underscore -dotnet_naming_rule.private_fields.symbols = private_fields -dotnet_naming_rule.private_fields_style.severity = warning -dotnet_naming_rule.private_fields_style.style = camel_case -dotnet_naming_rule.private_fields_style.symbols = private_fields dotnet_naming_style.non_private_static_field_style.capitalization = pascal_case +# NOTE: the private-field rules that used to sit here referenced a style ('camel_case_underscore') +# and a symbol group ('private_fields') that were never defined, so they never applied. The real +# private-field rules are declared in the "StyleCop Field Naming Rules" section below. dotnet_naming_symbols.non_private_static_fields.applicable_accessibilities = public, protected, internal, protected_internal, private_protected dotnet_naming_symbols.non_private_static_fields.applicable_kinds = field dotnet_naming_symbols.non_private_static_fields.required_modifiers = static -# Constants are PascalCase +# Constants are PascalCase (field constants; local constants are locals and stay camelCase) dotnet_naming_rule.constants_should_be_pascal_case.severity = warning dotnet_naming_rule.constants_should_be_pascal_case.style = non_private_static_field_style dotnet_naming_rule.constants_should_be_pascal_case.symbols = constants dotnet_naming_style.constant_style.capitalization = pascal_case -dotnet_naming_symbols.constants.applicable_kinds = field, local +dotnet_naming_symbols.constants.applicable_kinds = field dotnet_naming_symbols.constants.required_modifiers = const # Locals and parameters are camelCase @@ -102,9 +101,7 @@ dotnet_naming_rule.locals_should_be_camel_case.severity = warning # camel_case_style - Define the camelCase style dotnet_naming_style.camel_case_style.capitalization = camel_case -dotnet_naming_style.static_field_style.required_prefix = s_ dotnet_naming_symbols.locals_and_parameters.applicable_kinds = parameter, local -dotnet_naming_symbols.static_fields.required_modifiers = static # first_upper_style - The first character must start with an upper-case character dotnet_naming_style.first_upper_style.capitalization = first_word_upper @@ -152,22 +149,22 @@ dotnet_naming_symbols.other_public_protected_fields_group.applicable_kinds = fie # StyleCop Field Naming Rules -# All constant fields must be PascalCase -dotnet_naming_rule.private_or_internal_field_should_be__fieldname.severity = warning -dotnet_naming_rule.private_or_internal_field_should_be__fieldname.style = _fieldname -dotnet_naming_rule.private_or_internal_field_should_be__fieldname.symbols = private_or_internal_field +# All non-private constant fields must be PascalCase. Private constants are owned by +# 'private_static_fields_group' below - a field must be covered by exactly one rule, otherwise the +# same violation is reported once per matching rule. dotnet_naming_rule.stylecop_constant_fields_must_be_pascal_case_rule.severity = warning dotnet_naming_rule.stylecop_constant_fields_must_be_pascal_case_rule.style = non_private_static_field_style dotnet_naming_rule.stylecop_constant_fields_must_be_pascal_case_rule.symbols = stylecop_constant_fields_group -dotnet_naming_symbols.stylecop_constant_fields_group.applicable_accessibilities = public, internal, protected_internal, protected, private_protected, private +dotnet_naming_symbols.stylecop_constant_fields_group.applicable_accessibilities = public, internal, protected_internal, protected, private_protected dotnet_naming_symbols.stylecop_constant_fields_group.applicable_kinds = field dotnet_naming_symbols.stylecop_constant_fields_group.required_modifiers = const -# All static readonly fields must be PascalCase +# All non-private static readonly fields must be PascalCase. Private static readonly fields are +# owned by 'private_static_fields_group' below (see the note on the constant rule above). dotnet_naming_rule.stylecop_static_readonly_fields_must_be_pascal_case_rule.severity = warning dotnet_naming_rule.stylecop_static_readonly_fields_must_be_pascal_case_rule.style = non_private_static_field_style dotnet_naming_rule.stylecop_static_readonly_fields_must_be_pascal_case_rule.symbols = stylecop_static_readonly_fields_group -dotnet_naming_symbols.stylecop_static_readonly_fields_group.applicable_accessibilities = public, internal, protected_internal, protected, private_protected, private +dotnet_naming_symbols.stylecop_static_readonly_fields_group.applicable_accessibilities = public, internal, protected_internal, protected, private_protected dotnet_naming_symbols.stylecop_static_readonly_fields_group.applicable_kinds = field dotnet_naming_symbols.stylecop_static_readonly_fields_group.required_modifiers = static, readonly @@ -178,12 +175,22 @@ dotnet_naming_rule.stylecop_instance_fields_must_be_private_rule.symbols = style dotnet_naming_symbols.stylecop_fields_must_be_private_group.applicable_accessibilities = public, internal, protected_internal, protected, private_protected dotnet_naming_symbols.stylecop_fields_must_be_private_group.applicable_kinds = field -# Private fields must be camelCase -dotnet_naming_rule.stylecop_private_fields_must_be_camel_case_rule.severity = warning -dotnet_naming_rule.stylecop_private_fields_must_be_camel_case_rule.style = camel_case_style -dotnet_naming_rule.stylecop_private_fields_must_be_camel_case_rule.symbols = stylecop_private_fields_group -dotnet_naming_symbols.stylecop_private_fields_group.applicable_accessibilities = private -dotnet_naming_symbols.stylecop_private_fields_group.applicable_kinds = field +# Private static fields are PascalCase: they are type-level state and are never qualified with 'this.' +dotnet_naming_rule.private_static_fields_must_be_pascal_case_rule.severity = warning +dotnet_naming_rule.private_static_fields_must_be_pascal_case_rule.style = non_private_static_field_style +dotnet_naming_rule.private_static_fields_must_be_pascal_case_rule.symbols = private_static_fields_group +dotnet_naming_symbols.private_static_fields_group.applicable_accessibilities = private +dotnet_naming_symbols.private_static_fields_group.applicable_kinds = field +dotnet_naming_symbols.private_static_fields_group.required_modifiers = static + +# Private instance fields must be camelCase with a leading underscore: '_name', never 'name'. The +# prefix keeps field access unambiguous (so the noisy 'this.' qualifier is never needed) and keeps +# fields distinguishable from locals and parameters. +dotnet_naming_rule.private_instance_fields_must_be_camel_case_with_underscore_prefix.severity = warning +dotnet_naming_rule.private_instance_fields_must_be_camel_case_with_underscore_prefix.style = camel_case_underscore_style +dotnet_naming_rule.private_instance_fields_must_be_camel_case_with_underscore_prefix.symbols = private_instance_fields_group +dotnet_naming_symbols.private_instance_fields_group.applicable_accessibilities = private +dotnet_naming_symbols.private_instance_fields_group.applicable_kinds = field # Local variables must be camelCase dotnet_naming_rule.stylecop_local_fields_must_be_camel_case_rule.severity = silent @@ -232,25 +239,14 @@ dotnet_naming_style.type_parameter_style.required_prefix = T dotnet_naming_symbols.type_parameter_symbol.applicable_accessibilities = * dotnet_naming_symbols.type_parameter_symbol.applicable_kinds = type_parameter -# Instance fields are camelCase and start with _ -dotnet_naming_rule.camel_case_for_private_internal_fields.severity = suggestion -dotnet_naming_rule.camel_case_for_private_internal_fields.style = camel_case_underscore_style -dotnet_naming_rule.camel_case_for_private_internal_fields.symbols = private_internal_fields -dotnet_naming_rule.instance_fields_should_be_camel_case.severity = suggestion -dotnet_naming_rule.instance_fields_should_be_camel_case.style = camel_case_underscore_style -dotnet_naming_rule.instance_fields_should_be_camel_case.symbols = instance_fields +# Naming style shared by the private-field rules: camelCase with a required '_' prefix. dotnet_naming_style.camel_case_underscore_style.capitalization = camel_case dotnet_naming_style.camel_case_underscore_style.required_prefix = _ -dotnet_naming_style.instance_field_style.capitalization = camel_case -dotnet_naming_style.instance_field_style.required_prefix = _ -dotnet_naming_symbols.instance_fields.applicable_kinds = field -dotnet_naming_symbols.private_internal_fields.applicable_accessibilities = private, internal -dotnet_naming_symbols.private_internal_fields.applicable_kinds = field # Local functions are PascalCase dotnet_naming_rule.local_functions_should_be_pascal_case.severity = warning -dotnet_naming_rule.local_functions_should_be_pascal_case.style = non_private_static_field_style -dotnet_naming_rule.local_functions_should_be_pascal_case.symbols = all_members +dotnet_naming_rule.local_functions_should_be_pascal_case.style = local_function_style +dotnet_naming_rule.local_functions_should_be_pascal_case.symbols = local_functions dotnet_naming_style.local_function_style.capitalization = pascal_case dotnet_naming_symbols.local_functions.applicable_kinds = local_function @@ -264,12 +260,10 @@ dotnet_style_qualification_for_property = false:silent dotnet_style_operator_placement_when_wrapping = end_of_line # Naming styles -dotnet_naming_rule.interface_should_be_begins_with_i.severity = warning -dotnet_naming_rule.interface_should_be_begins_with_i.style = prefix_interface_with_i_style -dotnet_naming_rule.interface_should_be_begins_with_i.symbols = interface -dotnet_naming_rule.types_should_be_pascal_case.severity = warning -dotnet_naming_rule.types_should_be_pascal_case.style = non_private_static_field_style -dotnet_naming_rule.types_should_be_pascal_case.symbols = types +# NOTE: a 'types_should_be_pascal_case' rule (symbol group 'types') and an +# 'interface_should_be_begins_with_i' rule (symbol group 'interface') used to sit here, but neither +# symbol group was ever defined, so neither rule applied. Types are covered by 'element_rule' and +# interfaces by 'interface_rule' above. # By default, name items with PascalCase dotnet_naming_rule.non_field_members_should_be_pascal_case.severity = warning @@ -278,7 +272,6 @@ dotnet_naming_rule.non_field_members_should_be_pascal_case.symbols = non_field_m # pascal_case_style - Define the PascalCase style dotnet_naming_style.pascal_case_style.capitalization = pascal_case -dotnet_naming_symbols.all_members.applicable_kinds = * # Symbol specifications dotnet_naming_symbols.non_field_members.applicable_accessibilities = public, internal, private, protected, protected_internal, private_protected @@ -286,7 +279,6 @@ dotnet_naming_symbols.non_field_members.applicable_kinds = property, event, meth dotnet_naming_symbols.non_field_members.required_modifiers = * # Naming styles -dotnet_naming_style._fieldname.capitalization = camel_case dotnet_naming_style.begins_with_i.capitalization = pascal_case dotnet_naming_style.begins_with_i.required_prefix = I dotnet_naming_style.begins_with_i.required_suffix = @@ -314,9 +306,11 @@ dotnet_code_quality.prefer_const = true dotnet_code_quality.prefer_inferred_anonymous_type_member_names = true dotnet_code_quality.prefer_inferred_tuple_names = true dotnet_code_quality.prefer_readonly = true -dotnet_code_quality.require_accessibility_modifiers = true +# The real modifier policy is 'dotnet_style_require_accessibility_modifiers' (declared under +# "Modifier preferences" below). The 'require_accessibility_modifiers' and +# 'require_explicit_visibility' entries that used to live here are not .NET analyzer options: +# declaring them only pretended to enforce a policy that was never applied, so they were removed. dotnet_code_quality.require_explicit_type_arguments = true -dotnet_code_quality.require_explicit_visibility = true dotnet_code_quality.require_variable_declaration_for_explicit_type = false dotnet_code_quality_unused_parameters = all:warning dotnet_enable_roslyn_analyzers = true @@ -587,10 +581,14 @@ dotnet_diagnostic.CA1030.severity = suggestion dotnet_diagnostic.CA1031.severity = error # Implement standard exception constructors -dotnet_diagnostic.CA1032.severity = suggestion +# Raised to warning: the public API surface of exceptions is part of the accessibility policy and +# must not be invisible to command-line builds. +dotnet_diagnostic.CA1032.severity = warning # Interface methods should be callable by child types -dotnet_diagnostic.CA1033.severity = suggestion +# Raised to warning: explicit interface implementations that hide the member from derived types are +# an accessibility problem, not a style preference. +dotnet_diagnostic.CA1033.severity = warning # Nested types should not be visible dotnet_diagnostic.CA1034.severity = error @@ -599,7 +597,9 @@ dotnet_diagnostic.CA1034.severity = error dotnet_diagnostic.CA1036.severity = suggestion # Avoid empty interfaces -dotnet_diagnostic.CA1040.severity = suggestion +# Raised to warning: empty interfaces are unenforceable contracts and were previously invisible to +# command-line builds. +dotnet_diagnostic.CA1040.severity = warning # Provide ObsoleteAttribute message dotnet_diagnostic.CA1041.severity = warning @@ -794,7 +794,9 @@ dotnet_diagnostic.CA1513.severity = warning dotnet_diagnostic.CA1514.severity = warning # Consider making public types internal -dotnet_diagnostic.CA1515.severity = suggestion +# Reported as a real build warning (not a suggestion) so accessibility hygiene cannot be ignored: +# a public type in an application or test assembly should be made internal rather than suppressed. +dotnet_diagnostic.CA1515.severity = warning # Naming Rules (CA1700-CA1727 and IDE0130) # Naming rules support adherence to the naming conventions of the .NET design guidelines @@ -973,8 +975,9 @@ dotnet_diagnostic.CA1845.severity = error # Prefer AsSpan over Substring dotnet_diagnostic.CA1846.severity = error -dotnet_diagnostic.ignore_internalsvisibleto = true -dotnet_diagnostic.CA1852.ignore_internalsvisibleto = true +# CA1852 (seal internal types) is intentionally not suppressed for internal types that are exposed +# to other assemblies through InternalsVisibleTo: those types must still be sealed. The blanket +# suppression that used to sit here relied on a malformed key, so it never even applied. # Unsafe DataSet or DataTable in serializable type can be vulnerable to remote code execution attacks dotnet_diagnostic.CA2352.severity = error @@ -1319,8 +1322,9 @@ dotnet_diagnostic.IDE0038.severity = warning # Use local function instead of lambda dotnet_diagnostic.IDE0039.severity = suggestion -# Add accessibility modifiers -dotnet_diagnostic.IDE0040.severity = error +# IDE0040 is declared exactly once, in the "Accessibility modifiers" section above. A second entry +# here used to override that value (later entries win), which made the effective modifier policy +# ambiguous and let repos believe the rule had been turned off. # Use is null check dotnet_diagnostic.IDE0041.severity = suggestion @@ -1557,8 +1561,9 @@ dotnet_diagnostic.IDE0380.severity = warning # Remove unnecessary suppression (null-forgiving operator) dotnet_diagnostic.IDE0370.severity = warning -# Naming rule violation -dotnet_diagnostic.IDE1006.severity = silent +# Naming rule violation - must stay visible: the field-naming policy is only enforced when this +# diagnostic is reported, because naming rules without their own severity inherit this one. +dotnet_diagnostic.IDE1006.severity = warning # Embedded statements must be on their own line dotnet_diagnostic.IDE2001.severity = warning @@ -2638,7 +2643,7 @@ trim_trailing_whitespace = false # If it's a settings, model, dto, etc file, ignore the 'properties' cannot have arrays # and other annoying rules - the following section is duplicated -[**/*{ValueObjects,Settings,Options,Model,Models,DTO,Entity,Response,Request}.{cs,vb},] +[**/*{ValueObjects,Settings,Options,Model,Models,DTO,Entity,Response,Request}{.Nav,}.{cs,vb}] dotnet_diagnostic.CA1002.severity = none dotnet_diagnostic.CA1024.severity = none dotnet_diagnostic.CA1056.severity = none @@ -2673,15 +2678,11 @@ dotnet_diagnostic.PDS0004.severity = none # Files under the Extensions/ namespace-reset convention must not force developers to add pragmas; # suppress the namespace-conflict diagnostics that arise from declaring framework-style namespaces. dotnet_diagnostic.IDE0130.severity = none -dotnet_diagnostic.CA1034.severity = none dotnet_diagnostic.CA1724.severity = none dotnet_diagnostic.CS0436.severity = none dotnet_diagnostic.CS1591.severity = none dotnet_diagnostic.IDE0005.severity = none -[**/{Extension,Extensions}.{cs,vb}] -dotnet_diagnostic.CA1034.severity = none - [**/Generated/**/*.{cs,vb}] generated_code = true dotnet_diagnostic.CS8602.severity = none diff --git a/global.json b/global.json index 7c48b98..5ad4318 100644 --- a/global.json +++ b/global.json @@ -3,9 +3,9 @@ "allowPrerelease": false }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.4" + "Purview.BuildSdk": "1.0.5" }, "test": { "runner": "Microsoft.Testing.Platform" } -} +} \ No newline at end of file diff --git a/package.json b/package.json index 7942507..a5db2d2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { - "name": "purview-sourcegenerator-framework", - "version": "1.0.0", + "name": "sourcegenerator-framework", + "version": "1.1.0", "license": "MIT", "author": { "name": "Kieron Lanning", diff --git a/src/src/SourceGeneratorFramework.CodeFixers/ConvertToExtensionBlockCodeFixProvider.cs b/src/src/SourceGeneratorFramework.CodeFixers/ConvertToExtensionBlockCodeFixProvider.cs index 947f6e3..2ccefe8 100644 --- a/src/src/SourceGeneratorFramework.CodeFixers/ConvertToExtensionBlockCodeFixProvider.cs +++ b/src/src/SourceGeneratorFramework.CodeFixers/ConvertToExtensionBlockCodeFixProvider.cs @@ -96,7 +96,7 @@ member is MethodDeclarationSyntax method // The receiver is a method type parameter; move the type parameters and constraints to // the extension declaration so the receiver resolves against it. typeParameterList ??= method.TypeParameterList; - if (methodConstraintClauses(method) is { Count: > 0 } constraints) + if (MethodConstraintClauses(method) is { Count: > 0 } constraints) constraintClauses = constraints; } @@ -196,6 +196,6 @@ static MethodDeclarationSyntax ConvertMethod( return updated.WithParameterList(newParameterList); } - static SyntaxList methodConstraintClauses(MethodDeclarationSyntax method) => + static SyntaxList MethodConstraintClauses(MethodDeclarationSyntax method) => method.ConstraintClauses; } diff --git a/src/src/SourceGeneratorFramework.ExampleGenerator/Extensions/System/StringExtensions.cs b/src/src/SourceGeneratorFramework.ExampleGenerator/Extensions/System/StringExtensions.cs index 066ed78..47e68ea 100644 --- a/src/src/SourceGeneratorFramework.ExampleGenerator/Extensions/System/StringExtensions.cs +++ b/src/src/SourceGeneratorFramework.ExampleGenerator/Extensions/System/StringExtensions.cs @@ -3,6 +3,11 @@ namespace System; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class StringExtensions { diff --git a/src/src/SourceGeneratorFramework.Generators/Helpers/AttributeDataModelLibrary.cs b/src/src/SourceGeneratorFramework.Generators/Helpers/AttributeDataModelLibrary.cs index a93ad0e..6fbe845 100644 --- a/src/src/SourceGeneratorFramework.Generators/Helpers/AttributeDataModelLibrary.cs +++ b/src/src/SourceGeneratorFramework.Generators/Helpers/AttributeDataModelLibrary.cs @@ -408,15 +408,12 @@ static ParameterAttributeInfo ReadParameterAttributes( } else { - var (IsTypeArgument, Sources, DefaultValue, HasDefaultValue) = ReadTypeArgumentAttributeInfo( - parameter, - isExcluded - ); + var typeArgumentInfo = ReadTypeArgumentAttributeInfo(parameter, isExcluded); - isTypeArgument = IsTypeArgument; - defaultValue = DefaultValue; - hasDefaultValue = HasDefaultValue; - sources.AddRange(Sources); + isTypeArgument = typeArgumentInfo.IsTypeArgument; + defaultValue = typeArgumentInfo.DefaultValue; + hasDefaultValue = typeArgumentInfo.HasDefaultValue; + sources.AddRange(typeArgumentInfo.Sources); } var hasExclusive = isExcluded || isNestedModel || isTypeArgument; diff --git a/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs b/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs index b0b96c1..a3bba31 100644 --- a/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs +++ b/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs @@ -71,7 +71,7 @@ static class FrameworkTypeInternalizer /// namespace, or the global namespace when the spec sets none), so they are only recognizable /// through the tool name on their GeneratedCodeAttribute. /// - static readonly ImmutableArray s_frameworkGeneratorToolNames = + static readonly ImmutableArray FrameworkGeneratorToolNames = [ "TypeLibraryGenerator", "AttributeDataModelGenerator", @@ -80,13 +80,13 @@ static class FrameworkTypeInternalizer /// /// Assembly-level attributes that grant other assemblies access to a component's internals. /// - static readonly ImmutableArray s_internalsGrantAttributeFullNames = + static readonly ImmutableArray InternalsGrantAttributeFullNames = [ "System.Runtime.CompilerServices.InternalsVisibleToAttribute", "System.Runtime.CompilerServices.IgnoresAccessChecksToAttribute", ]; - static readonly ImmutableArray s_roslynComponentAttributeFullNames = + static readonly ImmutableArray RoslynComponentAttributeFullNames = [ "Microsoft.CodeAnalysis.GeneratorAttribute", "Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalyzerAttribute", @@ -94,7 +94,7 @@ static class FrameworkTypeInternalizer "Microsoft.CodeAnalysis.CodeRefactorings.ExportCodeRefactoringProviderAttribute", ]; - static readonly ImmutableArray s_roslynComponentInterfaceFullNames = + static readonly ImmutableArray RoslynComponentInterfaceFullNames = [ "Microsoft.CodeAnalysis.IIncrementalGenerator", "Microsoft.CodeAnalysis.ISourceGenerator", @@ -135,10 +135,7 @@ public static ImmutableArray CollectInternalsGrants(string assemblyPath) [ .. assembly .CustomAttributes.Where(static attribute => - s_internalsGrantAttributeFullNames.Contains( - attribute.AttributeType.FullName, - StringComparer.Ordinal - ) + InternalsGrantAttributeFullNames.Contains(attribute.AttributeType.FullName, StringComparer.Ordinal) ) .Select(static attribute => GrantedAssemblyName(attribute)) .Where(static name => name.Length > 0), @@ -288,7 +285,7 @@ static int StripInternalsGrants(AssemblyDefinition assembly, ImmutableArray= 0; index--) { var attribute = assembly.CustomAttributes[index]; - if (!s_internalsGrantAttributeFullNames.Contains(attribute.AttributeType.FullName, StringComparer.Ordinal)) + if (!InternalsGrantAttributeFullNames.Contains(attribute.AttributeType.FullName, StringComparer.Ordinal)) continue; if ( @@ -476,7 +473,7 @@ static bool IsFrameworkGenerated(TypeDefinition type) continue; } - if (s_frameworkGeneratorToolNames.Contains(toolName, StringComparer.Ordinal)) + if (FrameworkGeneratorToolNames.Contains(toolName, StringComparer.Ordinal)) return true; } @@ -521,7 +518,7 @@ static bool IsRoslynComponent(TypeDefinition type) { foreach (var attribute in type.CustomAttributes) { - if (s_roslynComponentAttributeFullNames.Contains(attribute.AttributeType.FullName, StringComparer.Ordinal)) + if (RoslynComponentAttributeFullNames.Contains(attribute.AttributeType.FullName, StringComparer.Ordinal)) return true; } @@ -546,8 +543,8 @@ static bool IsRoslynComponentType(TypeReference? type) return false; // The type may be a generic instance, so check the element type as well. - return s_roslynComponentInterfaceFullNames.Contains(type.FullName, StringComparer.Ordinal) - || s_roslynComponentInterfaceFullNames.Contains( + return RoslynComponentInterfaceFullNames.Contains(type.FullName, StringComparer.Ordinal) + || RoslynComponentInterfaceFullNames.Contains( Resolve(type)?.FullName ?? string.Empty, StringComparer.Ordinal ); diff --git a/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/CodeWriterExtensions.cs b/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/CodeWriterExtensions.cs index e513833..4a24ba5 100644 --- a/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/CodeWriterExtensions.cs +++ b/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/CodeWriterExtensions.cs @@ -3,6 +3,11 @@ namespace Purview.SourceGeneratorFramework; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class CodeWriterExtensions { diff --git a/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/SourceGeneratorTestOptionsExtensions.cs b/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/SourceGeneratorTestOptionsExtensions.cs index d1c4c51..d7fd735 100644 --- a/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/SourceGeneratorTestOptionsExtensions.cs +++ b/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/SourceGeneratorTestOptionsExtensions.cs @@ -7,6 +7,11 @@ namespace Purview.SourceGeneratorFramework; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class SourceGeneratorTestOptionsExtensions { diff --git a/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/Testing/SourceGeneratorTestOptionsExtensions.cs b/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/Testing/SourceGeneratorTestOptionsExtensions.cs index 4a2e2e9..8c2e43d 100644 --- a/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/Testing/SourceGeneratorTestOptionsExtensions.cs +++ b/src/src/SourceGeneratorFramework.Testing/Extensions/Purview/SourceGeneratorFramework/Testing/SourceGeneratorTestOptionsExtensions.cs @@ -6,6 +6,11 @@ namespace Purview.SourceGeneratorFramework.Testing; /// /// Fluent extension methods that preserve the concrete options type for downstream derived records. /// +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class SourceGeneratorTestOptionsExtensions { diff --git a/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/AttributeDataExtensions.cs b/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/AttributeDataExtensions.cs index 9f051df..d91aa80 100644 --- a/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/AttributeDataExtensions.cs +++ b/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/AttributeDataExtensions.cs @@ -12,6 +12,11 @@ namespace Microsoft.CodeAnalysis; /// [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "CA2208:Instantiate argument exceptions correctly")] [System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1708:Identifiers should differ by more than case")] +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static partial class AttributeDataExtensions { diff --git a/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/TypedConstantExtensions.cs b/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/TypedConstantExtensions.cs index a5904a5..b7e1779 100644 --- a/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/TypedConstantExtensions.cs +++ b/src/src/SourceGeneratorFramework/Extensions/Microsoft/CodeAnalysis/TypedConstantExtensions.cs @@ -10,6 +10,11 @@ namespace Microsoft.CodeAnalysis; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static partial class TypedConstantExtensions { diff --git a/src/src/SourceGeneratorFramework/Extensions/System/StringExtensions.cs b/src/src/SourceGeneratorFramework/Extensions/System/StringExtensions.cs index 9a3d345..d695374 100644 --- a/src/src/SourceGeneratorFramework/Extensions/System/StringExtensions.cs +++ b/src/src/SourceGeneratorFramework/Extensions/System/StringExtensions.cs @@ -4,6 +4,11 @@ namespace System; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class StringExtensions { diff --git a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/CSharp/Syntax/SyntaxNodeExtensions.cs b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/CSharp/Syntax/SyntaxNodeExtensions.cs index 4014b32..25d1411 100644 --- a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/CSharp/Syntax/SyntaxNodeExtensions.cs +++ b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/CSharp/Syntax/SyntaxNodeExtensions.cs @@ -3,6 +3,11 @@ namespace Microsoft.CodeAnalysis.CSharp.Syntax; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class SyntaxNodeExtensions { diff --git a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/ISymbolExtensions.cs b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/ISymbolExtensions.cs index 39dd649..cdecd74 100644 --- a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/ISymbolExtensions.cs +++ b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/ISymbolExtensions.cs @@ -3,6 +3,11 @@ namespace Microsoft.CodeAnalysis; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class ISymbolExtensions { diff --git a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/IncrementalGeneratorInitializationContextExtensions.cs b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/IncrementalGeneratorInitializationContextExtensions.cs index 84ef71e..761dae5 100644 --- a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/IncrementalGeneratorInitializationContextExtensions.cs +++ b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/IncrementalGeneratorInitializationContextExtensions.cs @@ -5,6 +5,11 @@ namespace Microsoft.CodeAnalysis; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class IncrementalGeneratorInitializationContextExtensions { diff --git a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/SourceProductionContextExtensions.cs b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/SourceProductionContextExtensions.cs index a62b315..310d993 100644 --- a/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/SourceProductionContextExtensions.cs +++ b/src/src/SourceGeneratorShared/Extensions/Microsoft/CodeAnalysis/SourceProductionContextExtensions.cs @@ -3,6 +3,11 @@ namespace Microsoft.CodeAnalysis; +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "The extension(...) block is lowered to an unnamed nested type; the language requires it to be nested in a static class, and PSGFR34 mandates extension blocks over classic 'this'-parameter methods." +)] [EditorBrowsable(EditorBrowsableState.Never)] public static class SourceProductionContextExtensions { diff --git a/src/src/SourceGeneratorShared/GenerationContext.cs b/src/src/SourceGeneratorShared/GenerationContext.cs index 4db30d3..2a74462 100644 --- a/src/src/SourceGeneratorShared/GenerationContext.cs +++ b/src/src/SourceGeneratorShared/GenerationContext.cs @@ -10,27 +10,27 @@ namespace Purview.SourceGeneratorFramework; /// Initializes a generation context. /// public sealed class GenerationContext( - TCapabilities Capabilities, - GenerationSettings Settings, - ISourceGenLogger? Logger = null + TCapabilities capabilities, + GenerationSettings settings, + ISourceGenLogger? logger = null ) : ISourceGenLogger, IEqualityComparer>, IEquatable> where TCapabilities : class, IGenerationCapabilities { /// /// Gets the capabilities of the source generator, which are used to determine what features are available during generation. /// - public TCapabilities Capabilities { get; } = Capabilities ?? throw new ArgumentNullException(nameof(Capabilities)); + public TCapabilities Capabilities { get; } = capabilities ?? throw new ArgumentNullException(nameof(capabilities)); /// /// Gets the immutable generation settings. /// - public GenerationSettings Settings { get; } = Settings ?? throw new ArgumentNullException(nameof(Settings)); + public GenerationSettings Settings { get; } = settings ?? throw new ArgumentNullException(nameof(settings)); /// /// Gets the optional logger. This is primarily used during source generator test execution, such as /// unit or integration tests, to capture and assert on log messages generated by the source generator. /// - public ISourceGenLogger? Logger { get; } = Logger; + public ISourceGenLogger? Logger { get; } = logger; /// /// Creates a new independently owned code writer. diff --git a/src/src/SourceGeneratorShared/TypeIdentity.cs b/src/src/SourceGeneratorShared/TypeIdentity.cs index dee9e2f..5fedb56 100644 --- a/src/src/SourceGeneratorShared/TypeIdentity.cs +++ b/src/src/SourceGeneratorShared/TypeIdentity.cs @@ -186,7 +186,7 @@ static TypeIdentityCacheEntry CreateFromSymbol(ITypeSymbol typeSymbol) sealed class TypeIdentityCacheEntry(TypeIdentity value) { - public TypeIdentity Value = value; + public TypeIdentity Value { get; } = value; } /// diff --git a/src/tests/SourceGeneratorShared.UnitTests/Helpers/TypeHelpersTests.cs b/src/tests/SourceGeneratorShared.UnitTests/Helpers/TypeHelpersTests.cs index 913ae29..0c643d0 100644 --- a/src/tests/SourceGeneratorShared.UnitTests/Helpers/TypeHelpersTests.cs +++ b/src/tests/SourceGeneratorShared.UnitTests/Helpers/TypeHelpersTests.cs @@ -91,14 +91,14 @@ CancellationToken cancellationToken // Arrange const string source = "namespace Testing { interface IResource { } class DefaultAspireResource : IResource { } class ResourceKitBase where T : IResource { } class HostKit : ResourceKitBase { } }"; - var (Symbol, Syntax) = GetTypeDescriptor(source, "HostKit", cancellationToken); + var (symbol, syntax) = GetTypeDescriptor(source, "HostKit", cancellationToken); var expectedBase = new TypeIdentity("ResourceKitBase", "Testing").MakeGeneric( new TypeIdentity("IResource", "Testing") ); // Act - var symbolResult = TypeHelpers.IsDerivedFromExpectedBase(Symbol, expectedBase); - var syntaxResult = TypeHelpers.IsDerivedFromExpectedBase(Syntax, expectedBase); + var symbolResult = TypeHelpers.IsDerivedFromExpectedBase(symbol, expectedBase); + var syntaxResult = TypeHelpers.IsDerivedFromExpectedBase(syntax, expectedBase); // Assert await Assert.That(symbolResult).IsTrue(); @@ -113,12 +113,12 @@ CancellationToken cancellationToken // Arrange const string source = "namespace Testing { interface IResource { } class DefaultAspireResource : IResource { } class ResourceKitBase where T : IResource { } class HostKit : ResourceKitBase { } }"; - var (Symbol, Syntax) = GetTypeDescriptor(source, "HostKit", cancellationToken); + var (symbol, syntax) = GetTypeDescriptor(source, "HostKit", cancellationToken); TypeIdentity expectedBase = new("ResourceKitBase", "Testing"); // Act - var symbolResult = TypeHelpers.IsDerivedFromExpectedBase(Symbol, expectedBase); - var syntaxResult = TypeHelpers.IsDerivedFromExpectedBase(Syntax, expectedBase); + var symbolResult = TypeHelpers.IsDerivedFromExpectedBase(symbol, expectedBase); + var syntaxResult = TypeHelpers.IsDerivedFromExpectedBase(syntax, expectedBase); // Assert await Assert.That(symbolResult).IsTrue(); From 243b243c7185d3000b57654d9492582bf7581e6c Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Mon, 12 Oct 2026 23:03:59 +0100 Subject: [PATCH 08/10] build: install Purview.Build as a .NET local tool Add Purview.Build 0.5.2 to .config/dotnet-tools.json and replace the hand-rolled dotnet tool install --tool-path .tools/purview-build in the Justfile with dotnet tool restore, running the pipeline as dotnet purview-build. The version is now pinned in the tool manifest alongside CSharpier, so it is restored by the same command as every other tool and needs no bespoke install step, no --add-source feed argument and no platform-specific executable suffix. The dead pipeline_feed variable is removed, and the docs describing the old .tools/purview-build path are updated to match. --- .config/dotnet-tools.json | 7 +++++++ Justfile | 21 +++++++++------------ docs/wiki/Release-Flow.md | 4 ++-- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index fb05763..922c948 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -15,6 +15,13 @@ "dotnet-inspect" ], "rollForward": false + }, + "purview.build": { + "version": "0.5.2", + "commands": [ + "purview-build" + ], + "rollForward": false } } } \ No newline at end of file diff --git a/Justfile b/Justfile index 01a9e2a..f95b37d 100644 --- a/Justfile +++ b/Justfile @@ -12,8 +12,7 @@ build_configuration := "Debug" artifacts_folder := "./artifacts" default_test_filter := "/*/*/*/*" -pipeline_feed := "https://api.nuget.org/v3/index.json" -pipeline_tool := ".tools/purview-build/purview-build" +pipeline_tool := "dotnet purview-build" current_version := `bun -p "require('./package.json').version"` @@ -21,33 +20,31 @@ current_version := `bun -p "require('./package.json').version"` default: just --list -# Install the shared Purview.Build tool (authenticated to the Purview-Dev feed) if not present +# Restore the repository's .NET local tools (.config/dotnet-tools.json) [private] ensure-pipeline-tool: - if [ ! -x "{{ pipeline_tool }}" ]; then \ - dotnet tool install Purview.Build --tool-path .tools/purview-build --add-source "{{ pipeline_feed }}"; \ - fi + dotnet tool restore # Run the PR pipeline (restore, build, lint, tests) [group('Pipeline')] pipeline-pr *args: just ensure-pipeline-tool echo "Running PR pipeline..." - "{{ pipeline_tool }}" {{ args }} + {{ pipeline_tool }} {{ args }} # Run the build pipeline (restore, build, lint) [group('Pipeline')] pipeline-build *args: just ensure-pipeline-tool echo "Running build pipeline..." - "{{ pipeline_tool }}" --Build:RunTests=false --Release:Mode=None {{ args }} + {{ pipeline_tool }} --Build:RunTests=false --Release:Mode=None {{ args }} # Run the release pipeline (restore, build, lint, tests, pack, publish, GitHub release) [group('Pipeline')] pipeline-release *args: just ensure-pipeline-tool echo "Running release pipeline..." - "{{ pipeline_tool }}" --Release:Mode=NuGet {{ args }} + {{ pipeline_tool }} --Release:Mode=NuGet {{ args }} # Run the release pipeline (restore, build, lint, tests, pack, local nuget publish) # Note: `just` runs recipes through the shell, which strips backslashes from unquoted arguments. @@ -58,21 +55,21 @@ pipeline-local-release *args: just ensure-pipeline-tool just lint-fix echo "Running local release pipeline..." - "{{ pipeline_tool }}" --Release:Mode=LocalNuGet {{ args }} + {{ pipeline_tool }} --Release:Mode=LocalNuGet {{ args }} # Run the pipeline with tests enabled [group('Pipeline')] pipeline-tests *args: just ensure-pipeline-tool echo "Running tests pipeline..." - "{{ pipeline_tool }}" --Build:RunTests=true --Release:Mode=None {{ args }} + {{ pipeline_tool }} --Build:RunTests=true --Release:Mode=None {{ args }} # Run the pipeline through pack + validate (restore, build, lint, tests, pack, validate pack contents) without publishing/releasing [group('Pipeline')] pipeline-pack-validate *args: just ensure-pipeline-tool echo "Running pack + validate pipeline..." - "{{ pipeline_tool }}" --Build:RunPack=true --Build:ValidatePack=true --Release:Mode=None {{ args }} + {{ pipeline_tool }} --Build:RunPack=true --Build:ValidatePack=true --Release:Mode=None {{ args }} # Build and test with the specified configuration, defaulting to "Debug" [group('Build and Test')] diff --git a/docs/wiki/Release-Flow.md b/docs/wiki/Release-Flow.md index 15b146f..19f4f88 100644 --- a/docs/wiki/Release-Flow.md +++ b/docs/wiki/Release-Flow.md @@ -34,8 +34,8 @@ packs, validates, publishes to NuGet, and creates the GitHub release. ## Local pipelines -The `Justfile` wraps the shared `Purview.Build` pipeline (installed as a pinned dotnet tool to -`.tools/purview-build/purview-build`): +The `Justfile` wraps the shared `Purview.Build` pipeline (a pinned .NET local tool, restored from +`.config/dotnet-tools.json` and run as `dotnet purview-build`): | Recipe | Pipeline mode | Purpose | | --- | --- | --- | From c89cd7700f890e422d7453b48e78b61eaefe724e Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Tue, 13 Oct 2026 08:42:13 +0100 Subject: [PATCH 09/10] build: declare RequiredCompilerFlags explicitly Purview.Build 0.5.3 defaults RequiredCompilerFlags to empty. Earlier versions forced the optimization=release check on and could not be configured off, because binding an empty JSON array contributes no configuration keys and so never overwrote the tool's non-empty code default. These packages have therefore always been validated against that flag. Declaring it keeps that behaviour rather than silently losing the check on the move to 0.5.3. Verified with just pipeline-pack-validate: all packages still valid. --- .config/purview-build.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.config/purview-build.json b/.config/purview-build.json index 4d12dca..a74f0ac 100644 --- a/.config/purview-build.json +++ b/.config/purview-build.json @@ -6,6 +6,10 @@ "TestFilter": "/*/*/*/*/" }, "PackValidation": { + // Explicit because Purview.Build 0.5.3 defaults RequiredCompilerFlags to empty. Earlier + // versions forced this check on and could not be configured off, so these packages have + // always been validated against it; declaring it keeps that behaviour. + "RequiredCompilerFlags": ["optimization=release"], "RequireSymbolPackage": true, "RequireSymbolFiles": true, "RequiredContent": { @@ -59,4 +63,4 @@ "Release": { "Mode": "None" } -} \ No newline at end of file +} From 62a07aa75c8f098c168906b8f45c7920134dd117 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Tue, 13 Oct 2026 09:15:57 +0100 Subject: [PATCH 10/10] build: take Purview.Build 0.5.3 Picks up the fix that makes RequiredCompilerFlags configurable: earlier versions forced the compiler-flag check on because binding an empty JSON array contributes no configuration keys, so the tool's non-empty code default could never be overridden. This repository declares "RequiredCompilerFlags": ["optimization=release"] explicitly, so the check it has always run under is preserved. Verified with just pipeline-pack-validate against 0.5.3. --- .config/dotnet-tools.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index 922c948..42b8dea 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -17,7 +17,7 @@ "rollForward": false }, "purview.build": { - "version": "0.5.2", + "version": "0.5.3", "commands": [ "purview-build" ],