From 5594bceff8a6a1fd19d32308ca6015169b19ceba Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Tue, 6 Oct 2026 23:20:07 +0100 Subject: [PATCH] fix: keep a component's own internals grants through the merge, and release 1.0.0 A code-fix component that reads its generator's internal diagnostic identity compiled, passed its in-repo tests, and then threw FieldAccessException in the compiler host. The merge tool's StripInternalsGrants removed every assembly-level InternalsVisibleTo from the merged artifact, so the merged generator the IDE loads no longer granted access to the merged code fix. In-repo tests bind the unmerged bin output, which keeps the grant, so nothing caught it. Verified on this repository's own ComponentClosureFixture - built for exactly this scenario, with a non-const internal field so the assembly reference survives: Fixture.Generator.dll (bin, unmerged) 116 grants, including Fixture.Generator.CodeFixers Fixture.Generator.dll (merged analyzer) 0 The grants are now stripped by provenance. The tool already receives the framework assembly separately, so it strips only framework-authored grants and keeps the ones the component wrote. That preserves the documented intent - a shipped analyzer is not the framework's assembly, and a leftover framework grant would let the framework's own test assemblies reach the internalized framework types - without breaking the supported component-to-component arrangement. End to end on the fixture the merged artifact goes from 0 grants to 24 including Fixture.Generator.CodeFixers, with 0 framework grants. CollectInternalsGrants reads an assembly's grants, Apply takes them as frameworkInternalsGrants, and omitting the argument strips everything as before, so the existing behaviour and its test are unchanged. The namespace workarounds this repository needed are gone: Purview.BuildSdk 1.0.3 no longer strips Roslyn component names from RootNamespace, so SourceGeneratorFramework.Analyzers, .Generators, .CodeFixers and .ExampleGenerator.CodeFixers - and their test projects - build clean with no NamespaceRemoveSuffix opt-outs at all. That is what confirms the SDK change was the right fix rather than a workaround moved upstream. First stable release. Adopts Purview.BuildSdk 1.0.3, up from 1.0.0-prerelease.60. Tests: 3752 passing, including the closure-fixture build integration tests that previously failed. --- .agents/agents/sdk-consumer-setup.md | 34 -------- .../prompts/sdk-diagnose-agent-folder-copy.md | 26 ------ .../project-placement-defaults/.gitignore | 8 -- .../sdk-configuration-reference/.gitignore | 8 -- .../.gitignore | 8 -- global.json | 2 +- package.json | 2 +- .../FrameworkTypeInternalizer.cs | 84 ++++++++++++++++--- .../MergeToolRunner.cs | 7 +- .../FrameworkTypeInternalizerTests.cs | 55 ++++++++++++ 10 files changed, 136 insertions(+), 98 deletions(-) delete mode 100644 .agents/agents/sdk-consumer-setup.md delete mode 100644 .agents/prompts/sdk-diagnose-agent-folder-copy.md delete mode 100644 .agents/skills/project-placement-defaults/.gitignore delete mode 100644 .agents/skills/sdk-configuration-reference/.gitignore delete mode 100644 .agents/skills/sdk-project-behavior-and-detection/.gitignore diff --git a/.agents/agents/sdk-consumer-setup.md b/.agents/agents/sdk-consumer-setup.md deleted file mode 100644 index 8e5066c..0000000 --- a/.agents/agents/sdk-consumer-setup.md +++ /dev/null @@ -1,34 +0,0 @@ -# sdk-consumer-setup (generic agent spec) - -## Goal - -Help a consuming repository adopt or troubleshoot `Purview.BuildSdk` correctly, without breaking existing build behaviour. - -## Workflow - -1. Confirm the SDK is imported in `Directory.Build.props`/`Directory.Build.targets` via - `` and the matching `Sdk.targets` import. -2. Check pre-import bootstrap properties are set **before** the `Sdk.props` import when they must affect - evaluation: `NamespacePrefix`, `UsePackageJsonVersion`, `RootPackageJson`. -3. If version resolution looks wrong, verify `package.json` discovery: explicit `RootPackageJson`, then CI - variables, `.git` root, or a nearby `package.json`. `UsePackageJsonVersion=Strict` fails fast instead of - silently skipping resolution. -4. If the bundled `.agents/**` content isn't appearing in the repo root, check `EnableAgentFolderInPackage` - (default `true`) and `AgentPackDestinationFolder` (default `.agents`) — the copy runs before build via - `EnsureAgentFolderInPackageTarget`. -5. For test-framework or project-shape questions, confirm the project follows repo naming and placement - conventions the SDK expects, rather than introducing bespoke structure. -6. Re-run `dotnet build` (or the repo's canonical build command) after each configuration change to confirm - the fix. - -## Constraints - -- Prefer minimal, targeted property changes over broad `Directory.Build.props` rewrites. -- Do not disable `PurviewAutoSdkPack` or `EnableAgentFolderInPackage` unless the consumer explicitly asks to - opt out. -- Do not duplicate SDK-managed properties in individual project files unless the scenario is intentionally - project-specific. - -## Related skill - -See `../skills/sdk-configuration-reference/SKILL.md` for the full property reference. diff --git a/.agents/prompts/sdk-diagnose-agent-folder-copy.md b/.agents/prompts/sdk-diagnose-agent-folder-copy.md deleted file mode 100644 index 85ad444..0000000 --- a/.agents/prompts/sdk-diagnose-agent-folder-copy.md +++ /dev/null @@ -1,26 +0,0 @@ -# sdk-diagnose-agent-folder-copy (generic prompt spec) - -Diagnose why the bundled `.agents/**` folder from `Purview.BuildSdk` did not appear at the expected -destination in a consuming repository. - -## Required behaviour - -1. Confirm the NuGet package actually contains `.agents/**` content (inspect the `.nupkg` if available). -2. Confirm the consuming project is packable/buildable and imports the SDK via - `Sdk.props`/`Sdk.targets`, since the copy runs in `EnsureAgentFolderInPackageTarget` before build. -3. Check `EnableAgentFolderInPackage` is not set to `false` anywhere in the build (project file, - `Directory.Build.props`, or command-line `-p:` overrides). -4. Confirm the destination folder: default is `.agents` at the repo root, overridable per-build with - `-p:AgentPackDestinationFolder=`. -5. Verify repo-root discovery succeeded: explicit `RepoRoot`, then a nearby `AGENTS.md`, then source-control - root metadata. -6. Re-run the build and confirm the destination folder now contains the copied files (including the - generated `.gitignore` for skill/prompt/agent subfolders). - -## Suggested output - -- A short root-cause explanation (missing import, disabled flag, wrong destination override, or repo-root - discovery miss). -- The exact command used to reproduce/verify the fix (for example - `dotnet build -p:AgentPackDestinationFolder=`). -- Confirmation that the expected files exist at the resolved destination path. diff --git a/.agents/skills/project-placement-defaults/.gitignore b/.agents/skills/project-placement-defaults/.gitignore deleted file mode 100644 index 2799754..0000000 --- a/.agents/skills/project-placement-defaults/.gitignore +++ /dev/null @@ -1,8 +0,0 @@ -# Ignore all files -* - -# Don't ignore directories, so Git can traverse them -!*/ - -# Keep this file -!.gitignore \ No newline at end of file diff --git a/.agents/skills/sdk-configuration-reference/.gitignore b/.agents/skills/sdk-configuration-reference/.gitignore deleted file mode 100644 index 2799754..0000000 --- a/.agents/skills/sdk-configuration-reference/.gitignore +++ /dev/null @@ -1,8 +0,0 @@ -# Ignore all files -* - -# Don't ignore directories, so Git can traverse them -!*/ - -# Keep this file -!.gitignore \ No newline at end of file diff --git a/.agents/skills/sdk-project-behavior-and-detection/.gitignore b/.agents/skills/sdk-project-behavior-and-detection/.gitignore deleted file mode 100644 index 2799754..0000000 --- a/.agents/skills/sdk-project-behavior-and-detection/.gitignore +++ /dev/null @@ -1,8 +0,0 @@ -# Ignore all files -* - -# Don't ignore directories, so Git can traverse them -!*/ - -# Keep this file -!.gitignore \ No newline at end of file diff --git a/global.json b/global.json index b476ad4..6cefa81 100644 --- a/global.json +++ b/global.json @@ -3,7 +3,7 @@ "allowPrerelease": false }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.0-prerelease.60" + "Purview.BuildSdk": "1.0.3" }, "test": { "runner": "Microsoft.Testing.Platform" diff --git a/package.json b/package.json index f579167..7942507 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "purview-sourcegenerator-framework", - "version": "1.0.0-prerelease.54", + "version": "1.0.0", "license": "MIT", "author": { "name": "Kieron Lanning", diff --git a/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs b/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs index 092057c..b0b96c1 100644 --- a/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs +++ b/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs @@ -120,6 +120,46 @@ public static ImmutableArray CollectTypeFullNames(string assemblyPath) return [.. assembly.MainModule.Types.SelectMany(MergeToolRunner.Flatten).Select(static type => type.FullName)]; } + /// + /// Reads the assembly names an assembly grants internals access to. The merge uses the framework + /// assembly's grants to decide which grants in the merged component were copied in by the merge + /// (and must go) and which the component itself authored (and must stay) - see + /// . + /// + /// The assembly to read grants from. + public static ImmutableArray CollectInternalsGrants(string assemblyPath) + { + using var assembly = AssemblyDefinition.ReadAssembly(assemblyPath); + + return + [ + .. assembly + .CustomAttributes.Where(static attribute => + s_internalsGrantAttributeFullNames.Contains( + attribute.AttributeType.FullName, + StringComparer.Ordinal + ) + ) + .Select(static attribute => GrantedAssemblyName(attribute)) + .Where(static name => name.Length > 0), + ]; + } + + /// + /// Reads the granted assembly's simple name from an internals-grant attribute, discarding any + /// , PublicKey=... suffix. + /// + static string GrantedAssemblyName(CustomAttribute attribute) + { + if (attribute.ConstructorArguments.Count == 0) + return string.Empty; + + var value = attribute.ConstructorArguments[0].Value as string ?? string.Empty; + var comma = value.IndexOf(',', StringComparison.Ordinal); + + return (comma < 0 ? value : value[..comma]).Trim(); + } + /// /// Internalizes every framework-owned type in the merged component, strips the assembly-level /// internals grants the merge copied in, and returns a report of the changes plus anything that @@ -130,12 +170,19 @@ public static ImmutableArray CollectTypeFullNames(string assemblyPath) /// Optional sink for non-blocking findings, such as component members that expose framework types. /// Namespace prefixes to internalize; defaults to the framework's own namespaces. /// Additional type full names to internalize (normally the framework assembly's own type names). + /// + /// The internals grants declared by the framework assembly. Only these are stripped from the merged + /// component, so a grant the component itself authored - the one that lets a companion code-fix + /// component read the generator's internal diagnostic identity - survives the merge. When + /// , every grant is stripped. + /// public static FrameworkInternalizationReport Apply( string assemblyPath, IEnumerable searchDirectories, Action? warn = null, ImmutableArray? ownedNamespaces = null, - ImmutableArray? ownedTypeFullNames = null + ImmutableArray? ownedTypeFullNames = null, + ImmutableArray? frameworkInternalsGrants = null ) { var namespaces = ownedNamespaces ?? DefaultOwnedNamespaces; @@ -190,7 +237,7 @@ public static FrameworkInternalizationReport Apply( CollectExposingMembers(type, namespaces, typeFullNames, exposingMembers); } - var strippedGrantCount = StripInternalsGrants(assembly); + var strippedGrantCount = StripInternalsGrants(assembly, frameworkInternalsGrants); if (internalizedTypeCount > 0 || strippedGrantCount > 0) assembly.Write(assemblyPath, new WriterParameters { WriteSymbols = hasSymbols }); @@ -217,21 +264,36 @@ [.. exposingMembers.Select(static entry => $"{entry.Owner}.{entry.Member}")], } /// - /// Removes the assembly-level internals grants the merge copied into the artifact. The merged - /// component is a shipped analyzer, not the component's own assembly: a leftover grant would let - /// an unrelated assembly (such as the framework's own test assemblies) reach the internalized - /// framework types, while the component's bin output keeps its grants for in-repo tests. + /// Removes the assembly-level internals grants the merge copied into the artifact from the + /// framework assembly. The merged component is a shipped analyzer, not the framework's own + /// assembly: a leftover framework grant would let an unrelated assembly (such as the framework's + /// own test assemblies) reach the internalized framework types. + /// + /// Grants the component itself authored are kept. Stripping those broke the supported + /// component-to-component arrangement: a code-fix component that reads its generator's internal + /// diagnostic identity compiles and passes in-repo tests against the generator's unmerged bin + /// output, which keeps the grant, and then fails at runtime in the compiler host with a + /// FieldAccessException because the merged analyzer it actually loads had the grant + /// removed. Keeping the component's own grants makes the merged artifact behave like the assembly + /// the author compiled against. + /// /// - static int StripInternalsGrants(AssemblyDefinition assembly) + /// The merged component. + /// + /// Grants declared by the framework assembly, or to strip every grant. + /// + static int StripInternalsGrants(AssemblyDefinition assembly, ImmutableArray? frameworkInternalsGrants) { var removed = 0; for (var index = assembly.CustomAttributes.Count - 1; index >= 0; index--) { + var attribute = assembly.CustomAttributes[index]; + if (!s_internalsGrantAttributeFullNames.Contains(attribute.AttributeType.FullName, StringComparer.Ordinal)) + continue; + if ( - !s_internalsGrantAttributeFullNames.Contains( - assembly.CustomAttributes[index].AttributeType.FullName, - StringComparer.Ordinal - ) + frameworkInternalsGrants is { } frameworkGrants + && !frameworkGrants.Contains(GrantedAssemblyName(attribute), StringComparer.Ordinal) ) { continue; diff --git a/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs b/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs index d889222..76be836 100644 --- a/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs +++ b/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs @@ -132,7 +132,12 @@ logger is not null ownedTypeFullNames: Extend( FrameworkTypeInternalizer.DefaultOwnedTypeFullNames, [.. FrameworkTypeInternalizer.CollectTypeFullNames(frameworkPath), .. options.OwnedTypeFullNames] - ) + ), + // Only the framework's own grants are stripped. A grant the component authored - the one + // that lets a companion code-fix component read the generator's internal diagnostic + // identity - has to survive, or the merged analyzer the compiler host loads behaves + // differently from the assembly the author compiled and tested against. + frameworkInternalsGrants: FrameworkTypeInternalizer.CollectInternalsGrants(frameworkPath) ); if (internalization.PublicFrameworkTypesRemaining.Length > 0) diff --git a/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs b/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs index aefbf0c..aa3882b 100644 --- a/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs +++ b/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs @@ -244,6 +244,24 @@ public sealed class PublicType } """; + /// + /// A component carrying both a grant the merge copied in from the framework and the component's own + /// grant to its companion code-fix assembly. + /// + const string ComponentWithMixedInternalsGrantsSource = """ + using System.Runtime.CompilerServices; + + [assembly: InternalsVisibleTo("Purview.SourceGeneratorFramework.UnitTests")] + [assembly: InternalsVisibleTo("Fixture.Component.CodeFixers")] + + namespace Fixture.Component + { + public sealed class PublicType + { + } + } + """; + [Test] public async Task Apply_GivenOwnedPublicTypes_InternalizesThemAndKeepsComponentsPublic( CancellationToken cancellationToken @@ -551,6 +569,43 @@ await Assert .IsFalse(); } + // The merge copies the framework assembly's own grants into the artifact, and those must go: a + // shipped analyzer is not the framework's assembly. A grant the *component* authored is different - + // it is what lets a companion code-fix component read the generator's internal diagnostic identity. + // Stripping it made the merged analyzer behave differently from the unmerged bin output the author + // compiled and tested against, so the code fix threw FieldAccessException only in the compiler host. + [Test] + public async Task Apply_GivenFrameworkGrants_StripsOnlyThoseAndKeepsTheComponentsOwn( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var componentPath = workspace.Compile("Fixture.Component", ComponentWithMixedInternalsGrantsSource); + + // Act + var report = FrameworkTypeInternalizer.Apply( + componentPath, + [workspace.GetPath("Fixture.Component")], + ownedNamespaces: ["Purview.SourceGeneratorFramework"], + frameworkInternalsGrants: ["Purview.SourceGeneratorFramework.UnitTests"] + ); + + // Assert + await Assert.That(report.StrippedInternalsGrantCount).IsEqualTo(1); + + using var component = AssemblyDefinition.ReadAssembly(componentPath); + var grants = component + .CustomAttributes.Where(static attribute => + attribute.AttributeType.FullName == "System.Runtime.CompilerServices.InternalsVisibleToAttribute" + ) + .Select(static attribute => (string)attribute.ConstructorArguments[0].Value) + .ToArray(); + + await Assert.That(grants).IsEquivalentTo(["Fixture.Component.CodeFixers"]); + } + [Test] public async Task Apply_GivenGenericConstraintsExposingFrameworkTypes_ReportsThem( CancellationToken cancellationToken