diff --git a/docs/wiki/Analyzers.md b/docs/wiki/Analyzers.md index 5ae0c2a..0f10824 100644 --- a/docs/wiki/Analyzers.md +++ b/docs/wiki/Analyzers.md @@ -29,6 +29,8 @@ These are MSBuild diagnostics rather than compiler analyzers, so they are not tr | `PSGF0001` | `Purview.BuildSdk` | A Roslyn component did not produce (or did not declare) a source-generator analyzer file. | | `PSGF0003` | `Purview.SourceGeneratorFramework` | A `PurviewGeneratorVisibleProperty` is not compiler-visible in the declaring project or its `Sdk/build`/`Sdk/buildTransitive` assets, so consumers cannot read `build_property.`. | | `PRSGD0005` | `Purview.BuildSdk` | A file in the returned analyzer closure references an assembly that is neither part of the closure nor a compiler-host assembly. | +| `PSGFR41` | `Purview.SourceGeneratorFramework` merge tool | A component's public surface exposes framework types that the merge internalizes. Raised by the compiler analyzer of the same id at design time and by the merge pass as a `message` (default), `warning` or `error`. | +| `PSGFR42` | `Purview.SourceGeneratorFramework` merge tool | The merged analyzer still exposes a public framework type, so it is not self-contained. Always an error; the merge fails with exit code 5. | Opt out with `PurviewSourceGeneratorFrameworkAnalyzerValidation=false` (`PRSGD0005`) or `PurviewSourceGeneratorFrameworkGeneratorPropertyValidation=false` (`PSGF0003`). @@ -66,6 +68,7 @@ Opt out with `PurviewSourceGeneratorFrameworkAnalyzerValidation=false` (`PRSGD00 | `PSGFR38` | Extension classes should carry `[EditorBrowsable(EditorBrowsableState.Never)]`. | | `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. | ## Type-library and attribute-model diagnostics @@ -96,6 +99,10 @@ analyzer rules above, including: to the structured `IfBlock`/`ElseIf`/`Else` APIs (`PSGFR23`). - `CodeWriterToStringCodeFixProvider` — replaces embedded `CodeWriter` string interpolation (`PSGFR29`). - `PreferInlineCodeForFrameworkCrefCodeFixProvider` — rewrites SGF XML doc `cref` targets to inline code (`Type`) (`PSGFR40`). +- `MakeComponentSurfaceNonPublicCodeFixProvider` — makes the exposing member, or its declaring type, + non-public (`PSGFR41`). +- `MakeTypeLibrarySpecNonPublicCodeFixProvider` — declares a type-library spec non-public in a merged + component (`TLB0021`). - `AttributeDataModelSymbolPropertyCodeFixProvider` — fixes attribute-data-model symbol properties. - `ReorganizeExtensionClassCodeFixProvider` — renames (`PSGFR35`), splits multi-receiver classes (`PSGFR37`), moves the class under `Extensions/{ReceiverNamespace}/`, and updates referencing files diff --git a/docs/wiki/Packaging.md b/docs/wiki/Packaging.md index d8ed517..c5427f0 100644 --- a/docs/wiki/Packaging.md +++ b/docs/wiki/Packaging.md @@ -215,21 +215,53 @@ The merge tool therefore runs a deterministic internalization pass over the merg (`FrameworkTypeInternalizer`) after `ILRepack` finishes: - every type in a framework-owned namespace (`Purview.SourceGeneratorFramework` and its children, - including the generated `Generators` attribute set) becomes non-public; + including the generated `Generators` attribute set) becomes non-public. Ownership is evaluated + through the **declaring chain**, because Mono.Cecil reports an empty namespace for a nested type: + the framework's nested containers (the generated type-library namespace classes, the nested + operator/enum groups, the `CodeWriter` scopes) are only reachable through their declaring type; +- the types the framework's own generators emit into the component — recognized by the tool name on + their `System.CodeDom.Compiler.GeneratedCodeAttribute` (`TypeLibraryGenerator`, + `AttributeDataModelGenerator`) — become non-public as well, so a generated type library can never + leak a `TypeIdentity`/`TypeReference` member as public API in the shipped analyzer; - `Microsoft.CodeAnalysis.EmbeddedAttribute` (the framework-emitted marker) becomes non-public; - **Roslyn component entry points are never internalized** — a generator, analyzer, code fix provider, or refactoring provider that is reachable from the framework namespace stays public, because Roslyn only instantiates public components (PSGFR27). This is what keeps the framework's own bundled analyzers working after the pass; - the pass reports the merge result: leftover public framework types fail the merge (exit code `5`), - and public component members whose signature exposes a framework type are logged as warnings so the - component author can make them (or their declaring type) non-public. - -The component's own generated types (the type library, attribute data models) keep their accessibility -in the component assembly: TLB0015 requires a hand-written partial to be declared + and public component members whose signature exposes a framework type are logged so the component + author can make them (or their declaring type) non-public. The report follows visibility through the + declaring chain too: a public nested type inside an internal type — for example the + compiler-synthesised `$`/`$` extension containers emitted for an extension class — is not + reachable from outside the component, so it is not part of the public surface and is not reported. + +Ownership is seeded with the framework assembly's own type identities, so a framework type that lives +outside the framework namespace (the `Microsoft.CodeAnalysis.*Extensions` and `System.StringExtensions` +extension classes, for example) is internalized as well, and the artifact's assembly-level +`InternalsVisibleTo` / `IgnoresAccessChecksTo` grants are removed because a shipped analyzer is not the +component's own assembly. + +The pass is configurable from the component's project: + +| Property | Default | Effect | +|----------|---------|--------| +| `PurviewMergeOwnedNamespaces` | empty | Extra namespace prefixes (semicolon-separated) treated as framework-owned. | +| `PurviewMergeOwnedTypeFullNames` | empty | Extra type full names (semicolon-separated) treated as framework-owned. | +| `PurviewMergePublicSurfaceSeverity` | `message` | How a finding is reported: `message` (plain log text), `warning`/`error` (MSBuild diagnostics with code `PSGFR41`), or `none` (dropped). | +| `PurviewMergePublicSurfaceValidation` | `true` | `false` sets the severity to `none`. | +| `PurviewMergePublicSurfaceOrigin` | the merged component assembly | The origin reported with an MSBuild finding; set it to a source or project path for IDE navigation. | + +All five participate in the merge's content tag, so changing one re-runs the merge (and re-emits its +findings) instead of reusing a cached artifact. The compiler analyzer of the same id (`PSGFR41`) reports +the same condition while you edit, so the surface is fixed before the merge runs. + +The component's own generated types (the type library, attribute data models) keep their generated +accessibility in the component's **bin output**: TLB0015 requires a hand-written partial to be declared `public static partial` so it can merge with the generated library, and in-repo consumers such as code -fixers and sibling assemblies compile against it. Self-containment is therefore enforced at the merge -boundary rather than by rewriting generated accessibility. See [Type-Library.md](Type-Library.md). +fixers and sibling assemblies compile against it. Only the merged analyzer artifact internalizes them +(see above), so self-containment is enforced at the merge boundary without rewriting the generated +accessibility an in-repo consumer compiles against. A merged artifact is never referenced as a +compile-time dependency, which is what makes that split safe. See [Type-Library.md](Type-Library.md). > A merged component is an analyzer artifact and must never be referenced as a compile-time > dependency. Tests that need to run a *packaged* generator load it out of band — see diff --git a/docs/wiki/Type-Library.md b/docs/wiki/Type-Library.md index 3d0d611..6430498 100644 --- a/docs/wiki/Type-Library.md +++ b/docs/wiki/Type-Library.md @@ -346,7 +346,7 @@ Set the MSBuild property `DisablePurviewTypeLibraryGenerator` to `true` to disab ## Validation -`TypeLibraryValidationAnalyzer` reports `TLB0001`–`TLB0020` for invalid specs, enum value members, and +`TypeLibraryValidationAnalyzer` reports `TLB0001`–`TLB0021` for invalid specs, enum value members, and type library partial extensions (non-static class, member type that is not `TypeIdentity`/`TypeReference`, unresolvable type/namespace, duplicate members, invalid class name, invalid namespace, invalid member accessibility, value members without an initializer, marker members without an explicit `= default`, a spec diff --git a/package.json b/package.json index 2fa2581..f579167 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "purview-sourcegenerator-framework", - "version": "1.0.0-prerelease.53", + "version": "1.0.0-prerelease.54", "license": "MIT", "author": { "name": "Kieron Lanning", diff --git a/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md b/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md index cafe263..be387d6 100644 --- a/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/src/SourceGeneratorFramework.Analyzers/AnalyzerReleases.Unshipped.md @@ -16,6 +16,7 @@ PSGFR37 | Purview.SourceGeneratorFramework | Warning | Extension class extends m PSGFR38 | Purview.SourceGeneratorFramework | Warning | Extension class is missing EditorBrowsable 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 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 @@ -23,3 +24,4 @@ TLB0017 | TypeLibrary | Error | Enum value member references an enum type that i TLB0018 | TypeLibrary | Error | Duplicate enum value member TLB0019 | TypeLibrary | Info | Duplicate enum value TLB0020 | TypeLibrary | Error | Enum values member references a type that is not an enum +TLB0021 | TypeLibrary | Info | Type-library spec should not be public in a merged component diff --git a/src/src/SourceGeneratorFramework.Analyzers/ComponentPublicSurfaceAnalyzer.cs b/src/src/SourceGeneratorFramework.Analyzers/ComponentPublicSurfaceAnalyzer.cs new file mode 100644 index 0000000..78a0dd7 --- /dev/null +++ b/src/src/SourceGeneratorFramework.Analyzers/ComponentPublicSurfaceAnalyzer.cs @@ -0,0 +1,263 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Purview.SourceGeneratorFramework.Analyzers; + +/// +/// Flags public members of a merged Roslyn component whose signature references a +/// Purview.SourceGeneratorFramework type. The merge internalizes every framework type in the +/// shipped analyzer, so such a member becomes a public signature over an internal type: unusable, and +/// the merge reports it (as PSGFR41) after a full build. This analyzer reports the same finding +/// while the author is editing, so the surface is fixed before the merge ever runs. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class ComponentPublicSurfaceAnalyzer : DiagnosticAnalyzer +{ + public const string DiagnosticId = "PSGFR41"; + + internal const string FrameworkAssemblyName = "Purview.SourceGeneratorFramework"; + + public static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + "Component public surface exposes framework types", + "'{0}' is public and exposes the Purview.SourceGeneratorFramework type '{1}', which the merge internalizes in the shipped analyzer; make the member or its declaring type non-public", + "Purview.SourceGeneratorFramework", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "A merged Roslyn component internalizes every Purview.SourceGeneratorFramework type it ships, so a public member whose signature references one leaves a public signature over an internal type in the analyzer. The merge reports the same finding as PSGFR41." + ); + + 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.RegisterCompilationStartAction(startContext => + { + // Only a component whose framework implementation is merged loses the framework types, so + // only its public surface can be left unusable. + if (!FrameworkMergeFacts.WillBeMerged(startContext.Options)) + return; + + if (GetFrameworkAssembly(startContext.Compilation) is not { } frameworkAssembly) + return; + + startContext.RegisterSymbolAction( + context => AnalyzeNamedType(context, frameworkAssembly), + SymbolKind.NamedType + ); + }); + } + + static void AnalyzeNamedType(SymbolAnalysisContext context, IAssemblySymbol frameworkAssembly) + { + if (context.Symbol is not INamedTypeSymbol type) + return; + + // The framework's own types are what the merge internalizes; the component's are the surface. + if (SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, frameworkAssembly)) + return; + + // Generated code is not hand-editable: the merge internalizes generator-emitted type libraries + // and attribute sets itself. + if (IsGenerated(type)) + return; + + if (!RoslynComponentDiscovery.IsEffectivelyPublic(type)) + return; + + foreach (var (symbol, frameworkType) in FindExposingMembers(type, frameworkAssembly)) + { + if (symbol.Locations.FirstOrDefault(static location => location.IsInSource) is not { } location) + continue; + + context.ReportDiagnostic( + Diagnostic.Create( + Rule, + location, + symbol.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), + frameworkType.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat) + ) + ); + } + } + + static IAssemblySymbol? GetFrameworkAssembly(Compilation compilation) + { + foreach (var reference in compilation.References) + { + if ( + compilation.GetAssemblyOrModuleSymbol(reference) is IAssemblySymbol assembly + && string.Equals(assembly.Identity.Name, FrameworkAssemblyName, StringComparison.OrdinalIgnoreCase) + ) + { + return assembly; + } + } + + return null; + } + + /// + /// Enumerates the parts of the type's public surface that reference a framework type: the base type, + /// the implemented interfaces, the generic constraints, and the public or protected members. + /// + static IEnumerable<(ISymbol Symbol, ITypeSymbol FrameworkType)> FindExposingMembers( + INamedTypeSymbol type, + IAssemblySymbol frameworkAssembly + ) + { + if (FindFrameworkType(type.BaseType, frameworkAssembly) is { } fromBase) + yield return (type, fromBase); + + foreach (var @interface in type.Interfaces) + { + if (FindFrameworkType(@interface, frameworkAssembly) is { } fromInterface) + yield return (type, fromInterface); + } + + foreach (var typeParameter in type.TypeParameters) + { + if (FindFrameworkType(typeParameter, frameworkAssembly) is { } fromConstraint) + yield return (typeParameter, fromConstraint); + } + + foreach (var member in type.GetMembers()) + { + if (!IsPublicSurfaceMember(member) || IsGenerated(member)) + continue; + + if (FindExposedType(member, frameworkAssembly) is { } fromMember) + yield return (member, fromMember); + } + } + + static ITypeSymbol? FindExposedType(ISymbol member, IAssemblySymbol frameworkAssembly) => + member switch + { + IFieldSymbol field => FindFrameworkType(field.Type, frameworkAssembly), + IPropertySymbol property => FindFrameworkType(property.Type, frameworkAssembly) + ?? FirstFrameworkType(property.Parameters, frameworkAssembly), + IEventSymbol @event => FindFrameworkType(@event.Type, frameworkAssembly), + IMethodSymbol method => FindFrameworkType(method.ReturnType, frameworkAssembly) + ?? FirstFrameworkType(method.Parameters, frameworkAssembly) + ?? method + .TypeParameters.Select(typeParameter => FindFrameworkType(typeParameter, frameworkAssembly)) + .FirstOrDefault(static type => type is not null), + _ => null, + }; + + static ITypeSymbol? FirstFrameworkType( + ImmutableArray parameters, + IAssemblySymbol frameworkAssembly + ) => + parameters + .Select(parameter => FindFrameworkType(parameter.Type, frameworkAssembly)) + .FirstOrDefault(static type => type is not null); + + /// + /// Returns the framework type a signature references, if any: the type itself, one of its generic + /// arguments, an array element type, or a generic constraint. + /// + static ITypeSymbol? FindFrameworkType(ITypeSymbol? type, IAssemblySymbol frameworkAssembly) + { + switch (type) + { + case null: + return null; + case IArrayTypeSymbol array: + return FindFrameworkType(array.ElementType, frameworkAssembly); + case IPointerTypeSymbol pointer: + return FindFrameworkType(pointer.PointedAtType, frameworkAssembly); + case ITypeParameterSymbol typeParameter: + foreach (var constraint in typeParameter.ConstraintTypes) + { + if (FindFrameworkType(constraint, frameworkAssembly) is { } fromConstraint) + return fromConstraint; + } + + return null; + case INamedTypeSymbol named: + if (IsFrameworkType(named, frameworkAssembly)) + return named; + + foreach (var argument in named.TypeArguments) + { + if (FindFrameworkType(argument, frameworkAssembly) is { } fromArgument) + return fromArgument; + } + + return null; + default: + return IsFrameworkType(type, frameworkAssembly) ? type : null; + } + } + + static bool IsFrameworkType(ITypeSymbol type, IAssemblySymbol frameworkAssembly) => + SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, frameworkAssembly); + + /// + /// Detects a type the framework's generators emitted into the component (the generated type library, + /// attribute data models, marker attribute sets). Those members are not hand-editable, and the merge + /// internalizes them itself. + /// + static bool IsGenerated(ISymbol symbol) + { + for (var current = symbol; current is not null; current = current.ContainingType) + { + foreach (var attribute in current.GetAttributes()) + { + var attributeName = attribute.AttributeClass?.ToDisplayString(); + + if ( + string.Equals( + attributeName, + "System.CodeDom.Compiler.GeneratedCodeAttribute", + StringComparison.Ordinal + ) + || string.Equals( + attributeName, + "System.Runtime.CompilerServices.CompilerGeneratedAttribute", + StringComparison.Ordinal + ) + ) + { + return true; + } + } + } + + return false; + } + + static bool IsPublicSurfaceMember(ISymbol member) + { + if (member.IsImplicitlyDeclared) + return false; + + if ( + member.DeclaredAccessibility + is not (Accessibility.Public or Accessibility.Protected or Accessibility.ProtectedOrInternal) + ) + { + return false; + } + + // Property and event accessors are reported through their property or event. + return member switch + { + IFieldSymbol or IPropertySymbol or IEventSymbol => true, + IMethodSymbol method => method.MethodKind + is MethodKind.Ordinary + or MethodKind.Constructor + or MethodKind.UserDefinedOperator + or MethodKind.Conversion, + _ => false, + }; + } +} diff --git a/src/src/SourceGeneratorFramework.Analyzers/FrameworkMergeFacts.cs b/src/src/SourceGeneratorFramework.Analyzers/FrameworkMergeFacts.cs new file mode 100644 index 0000000..eaa6481 --- /dev/null +++ b/src/src/SourceGeneratorFramework.Analyzers/FrameworkMergeFacts.cs @@ -0,0 +1,46 @@ +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Purview.SourceGeneratorFramework.Analyzers; + +/// +/// Decides whether the project's framework implementation is merged into the shipped analyzer. That is +/// what makes a public surface over framework types a problem: the merge internalizes every framework +/// type, so a public member that references one is left with an unusable signature. +/// +static class FrameworkMergeFacts +{ + public const string IsRoslynComponentProperty = "IsRoslynComponent"; + public const string EmbedProperty = "PurviewEmbedSourceGeneratorFramework"; + public const string IsPackableProperty = "IsPackable"; + public const string MergeAnalyzerFilesProperty = "PurviewMergeSourceGeneratorFrameworkForAnalyzerFiles"; + + /// + /// Determines whether the project under analysis produces a merged, self-contained analyzer. This + /// mirrors the gating the merge targets use, so the analyzers that depend on it agree with the merge. + /// + public static bool WillBeMerged(AnalyzerOptions options) + { + var analyzerOptions = options.AnalyzerConfigOptionsProvider.GlobalOptions; + + if (!IsExplicitlyTrue(analyzerOptions, IsRoslynComponentProperty)) + return false; + + // Merge disabled: only the framework's compile-time library opts out of embedding. + if (IsExplicitlyFalse(analyzerOptions, EmbedProperty)) + return false; + + // A non-packable component that does not return a merged analyzer file is never merged. + return !( + IsExplicitlyFalse(analyzerOptions, IsPackableProperty) + && IsExplicitlyFalse(analyzerOptions, MergeAnalyzerFilesProperty) + ); + } + + public static bool IsExplicitlyTrue(AnalyzerConfigOptions options, string propertyName) => + options.TryGetValue("build_property." + propertyName, out var value) + && string.Equals(value, "true", StringComparison.OrdinalIgnoreCase); + + public static bool IsExplicitlyFalse(AnalyzerConfigOptions options, string propertyName) => + options.TryGetValue("build_property." + propertyName, out var value) + && string.Equals(value, "false", StringComparison.OrdinalIgnoreCase); +} diff --git a/src/src/SourceGeneratorFramework.Analyzers/TypeLibraryValidationAnalyzer.cs b/src/src/SourceGeneratorFramework.Analyzers/TypeLibraryValidationAnalyzer.cs index b6189ac..73c4862 100644 --- a/src/src/SourceGeneratorFramework.Analyzers/TypeLibraryValidationAnalyzer.cs +++ b/src/src/SourceGeneratorFramework.Analyzers/TypeLibraryValidationAnalyzer.cs @@ -62,6 +62,8 @@ public sealed class TypeLibraryValidationAnalyzer : DiagnosticAnalyzer public static DiagnosticDescriptor EnumValuesTypeNotEnum => TypeLibraryDiagnosticRules.EnumValuesTypeNotEnum; + public static DiagnosticDescriptor SpecShouldBeNonPublic => TypeLibraryDiagnosticRules.SpecShouldBeNonPublic; + public override ImmutableArray SupportedDiagnostics => [ SpecNotStaticClass, @@ -83,6 +85,7 @@ public sealed class TypeLibraryValidationAnalyzer : DiagnosticAnalyzer EnumValueDuplicateMember, EnumValueDuplicateValue, EnumValuesTypeNotEnum, + SpecShouldBeNonPublic, ]; public override void Initialize(AnalysisContext context) @@ -124,6 +127,10 @@ public override void Initialize(AnalysisContext context) generateTypeLibraryAttributeType ); + // A public spec only matters when the framework implementation is merged into the shipped + // analyzer, because that is when the framework type identities it exposes are internalized. + var mergesFramework = FrameworkMergeFacts.WillBeMerged(context.Options); + context.RegisterSymbolAction( context => AnalyzeNamedType( @@ -135,7 +142,8 @@ public override void Initialize(AnalysisContext context) enumValueAttributeType, enumValuesAttributeType, enumValueDefinitionType, - generatedTypeLibraries + generatedTypeLibraries, + mergesFramework ), SymbolKind.NamedType ); @@ -151,7 +159,8 @@ static void AnalyzeNamedType( INamedTypeSymbol? enumValueAttributeType, INamedTypeSymbol? enumValuesAttributeType, INamedTypeSymbol? enumValueDefinitionType, - IReadOnlyList generatedTypeLibraries + IReadOnlyList generatedTypeLibraries, + bool mergesFramework ) { if (context.Symbol is not INamedTypeSymbol typeSymbol) @@ -203,6 +212,10 @@ IReadOnlyList generatedTypeLibraries if (outputNamespace is not null && !IsValidNamespace(outputNamespace)) context.ReportDiagnostic(Diagnostic.Create(InvalidNamespace, typeLocation, outputNamespace)); + // The generated TypeRefMarkers member is always public and typed as framework identities, so a + // public spec leaves a public signature over a type the merge internalizes. + ReportSpecAccessibility(context, typeSymbol, typeLocation, mergesFramework); + Dictionary> memberNamesByPath = new(StringComparer.Ordinal); Dictionary> enumMemberNamesByGroup = new(StringComparer.Ordinal); Dictionary> enumValuesByGroup = new(StringComparer.Ordinal); @@ -244,6 +257,22 @@ IReadOnlyList generatedTypeLibraries } } + /// + /// Reports TLB0021 when the spec is public in a component whose framework implementation is + /// merged: the generated marker member is public and typed as framework identities, which the merge + /// internalizes. + /// + static void ReportSpecAccessibility( + SymbolAnalysisContext context, + INamedTypeSymbol typeSymbol, + Location typeLocation, + bool mergesFramework + ) + { + if (mergesFramework && typeSymbol.DeclaredAccessibility == Accessibility.Public) + context.ReportDiagnostic(Diagnostic.Create(SpecShouldBeNonPublic, typeLocation, typeSymbol.Name)); + } + static void AnalyzeMember( SymbolAnalysisContext context, IFieldSymbol field, diff --git a/src/src/SourceGeneratorFramework.CodeFixers/MakeComponentSurfaceNonPublicCodeFixProvider.cs b/src/src/SourceGeneratorFramework.CodeFixers/MakeComponentSurfaceNonPublicCodeFixProvider.cs new file mode 100644 index 0000000..d1ac8dc --- /dev/null +++ b/src/src/SourceGeneratorFramework.CodeFixers/MakeComponentSurfaceNonPublicCodeFixProvider.cs @@ -0,0 +1,94 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Purview.SourceGeneratorFramework.Analyzers; + +namespace Purview.SourceGeneratorFramework.CodeFixers; + +/// +/// Makes the member (or the declaring type) that exposes a framework type non-public, so the merged +/// analyzer does not carry a public signature over an internalized framework type (fixes +/// PSGFR41). +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(MakeComponentSurfaceNonPublicCodeFixProvider))] +public sealed class MakeComponentSurfaceNonPublicCodeFixProvider : CodeFixProvider +{ + internal const string MemberEquivalenceKey = "MakeMemberNonPublic"; + internal const string TypeEquivalenceKey = "MakeTypeNonPublic"; + + public override ImmutableArray FixableDiagnosticIds => [ComponentPublicSurfaceAnalyzer.DiagnosticId]; + + 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); + if (node.FirstAncestorOrSelf() is not { } declaration) + continue; + + // A base type, interface or generic constraint finding points at the type itself. + if (declaration is BaseTypeDeclarationSyntax) + { + context.RegisterCodeFix( + CodeAction.Create( + "Make the type non-public", + _ => MakeNonPublicAsync(context.Document, declaration, context.CancellationToken), + TypeEquivalenceKey + ), + diagnostic + ); + continue; + } + + context.RegisterCodeFix( + CodeAction.Create( + "Make the member non-public", + _ => MakeNonPublicAsync(context.Document, declaration, context.CancellationToken), + MemberEquivalenceKey + ), + diagnostic + ); + + if ( + declaration.FirstAncestorOrSelf(static node => + node is BaseTypeDeclarationSyntax + ) is + { } containingType + ) + { + context.RegisterCodeFix( + CodeAction.Create( + "Make the containing type non-public", + _ => MakeNonPublicAsync(context.Document, containingType, context.CancellationToken), + TypeEquivalenceKey + ), + diagnostic + ); + } + } + } + + static async Task MakeNonPublicAsync( + Document document, + MemberDeclarationSyntax declaration, + CancellationToken cancellationToken + ) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + return document; + + // The declaration may have been removed or replaced by another code fix, so find the current node. + return document.WithSyntaxRoot( + root.ReplaceNode(declaration, RoslynComponentFixHelpers.MakeInternal(declaration)) + ); + } +} diff --git a/src/src/SourceGeneratorFramework.CodeFixers/MakeTypeLibrarySpecNonPublicCodeFixProvider.cs b/src/src/SourceGeneratorFramework.CodeFixers/MakeTypeLibrarySpecNonPublicCodeFixProvider.cs new file mode 100644 index 0000000..aa5b78f --- /dev/null +++ b/src/src/SourceGeneratorFramework.CodeFixers/MakeTypeLibrarySpecNonPublicCodeFixProvider.cs @@ -0,0 +1,63 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Purview.SourceGeneratorFramework.Analyzers; + +namespace Purview.SourceGeneratorFramework.CodeFixers; + +/// +/// Declares a type-library spec non-public in a component whose framework implementation is merged, so +/// the generated marker member does not leave a public signature over an internalized framework type +/// (fixes TLB0021). +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(MakeTypeLibrarySpecNonPublicCodeFixProvider))] +public sealed class MakeTypeLibrarySpecNonPublicCodeFixProvider : CodeFixProvider +{ + internal const string EquivalenceKey = "MakeTypeLibrarySpecNonPublic"; + + public override ImmutableArray FixableDiagnosticIds => + [TypeLibraryValidationAnalyzer.SpecShouldBeNonPublic.Id]; + + 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); + if (node.FirstAncestorOrSelf() is not { } typeDeclaration) + continue; + + context.RegisterCodeFix( + CodeAction.Create( + "Make the spec non-public", + _ => MakeNonPublicAsync(context.Document, typeDeclaration, context.CancellationToken), + EquivalenceKey + ), + diagnostic + ); + } + } + + static async Task MakeNonPublicAsync( + Document document, + TypeDeclarationSyntax typeDeclaration, + CancellationToken cancellationToken + ) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + return document; + + // The type declaration is already non-public, so no fix is needed. + return document.WithSyntaxRoot( + root.ReplaceNode(typeDeclaration, RoslynComponentFixHelpers.MakeInternal(typeDeclaration)) + ); + } +} diff --git a/src/src/SourceGeneratorFramework.CodeFixers/PreferInlineCodeForFrameworkCrefCodeFixProvider.cs b/src/src/SourceGeneratorFramework.CodeFixers/PreferInlineCodeForFrameworkCrefCodeFixProvider.cs index 468c396..c418b77 100644 --- a/src/src/SourceGeneratorFramework.CodeFixers/PreferInlineCodeForFrameworkCrefCodeFixProvider.cs +++ b/src/src/SourceGeneratorFramework.CodeFixers/PreferInlineCodeForFrameworkCrefCodeFixProvider.cs @@ -78,14 +78,17 @@ CancellationToken cancellationToken return document; // The cref attribute lives on the / element; inline code replaces the element. - var owner = + if ( crefAttribute .AncestorsAndSelf() .FirstOrDefault(static ancestor => ancestor is XmlElementSyntax or XmlEmptyElementSyntax) - as XmlNodeSyntax; - if (owner is null) + is not XmlNodeSyntax owner + ) + { return document; + } + // Replace the or element with an inline ... element. return document.WithSyntaxRoot(root.ReplaceNode(owner, BuildInlineCodeElement(owner, inlineText))); } @@ -118,6 +121,7 @@ sealed class InlineCodeFixAllProvider : FixAllProvider if (document is null) return Task.FromResult(null); + // The fix-all action is a single rewrite of the document, so the equivalence key is the same as for a single fix. return Task.FromResult( CodeAction.Create( "Use inline code for SGF cref references", @@ -161,6 +165,7 @@ CancellationToken cancellationToken if (owners.Count == 0) return document; + // Replace all the and elements with inline ... elements. return document.WithSyntaxRoot( root.ReplaceNodes( owners, diff --git a/src/src/SourceGeneratorFramework.CodeFixers/RoslynComponentFixHelpers.cs b/src/src/SourceGeneratorFramework.CodeFixers/RoslynComponentFixHelpers.cs index 565d174..4b8861e 100644 --- a/src/src/SourceGeneratorFramework.CodeFixers/RoslynComponentFixHelpers.cs +++ b/src/src/SourceGeneratorFramework.CodeFixers/RoslynComponentFixHelpers.cs @@ -145,4 +145,44 @@ or SyntaxKind.InternalKeyword or SyntaxKind.PrivateKeyword or SyntaxKind.ProtectedKeyword or SyntaxKind.FileKeyword; + + /// + /// Makes a declaration non-public. A top-level type falls back to internal by removing its + /// accessibility modifier, because the repository convention is to omit the default accessibility; a + /// nested type or a member defaults to private, so internal is written explicitly. + /// + public static MemberDeclarationSyntax MakeInternal(MemberDeclarationSyntax declaration) + { + var modifiers = declaration.Modifiers; + var accessibilityIndex = IndexOfAccessibilityModifier(modifiers); + + if ( + declaration is BaseTypeDeclarationSyntax + && declaration.Parent is CompilationUnitSyntax or BaseNamespaceDeclarationSyntax + ) + { + return accessibilityIndex >= 0 + ? declaration.WithModifiers(modifiers.RemoveAt(accessibilityIndex)) + : declaration; + } + + var internalToken = SyntaxFactory.Token(SyntaxKind.InternalKeyword); + var updated = + accessibilityIndex >= 0 + ? modifiers.Replace(modifiers[accessibilityIndex], internalToken) + : modifiers.Insert(0, internalToken); + + return declaration.WithModifiers(updated); + } + + static int IndexOfAccessibilityModifier(SyntaxTokenList modifiers) + { + for (var index = 0; index < modifiers.Count; index++) + { + if (IsAccessibilityModifier(modifiers[index])) + return index; + } + + return -1; + } } diff --git a/src/src/SourceGeneratorFramework.ExampleGenerator/Models.cs b/src/src/SourceGeneratorFramework.ExampleGenerator/Models.cs index 21d4450..7bdb8fa 100644 --- a/src/src/SourceGeneratorFramework.ExampleGenerator/Models.cs +++ b/src/src/SourceGeneratorFramework.ExampleGenerator/Models.cs @@ -43,7 +43,7 @@ public sealed class GenerateServiceAttribute(ServiceLifetime lifetime = ServiceL /// name argument or as a named argument ([GenerateService(Name = "…")]); the named /// argument wins when both are supplied because it is assigned after the constructor runs. /// - public string? Name { get; set; } = name; + public string? Name { get; init; } = name; } /// diff --git a/src/src/SourceGeneratorFramework.Generators/Helpers/FrameworkCrefRewriter.cs b/src/src/SourceGeneratorFramework.Generators/Helpers/FrameworkCrefRewriter.cs index c9fc556..d22c65d 100644 --- a/src/src/SourceGeneratorFramework.Generators/Helpers/FrameworkCrefRewriter.cs +++ b/src/src/SourceGeneratorFramework.Generators/Helpers/FrameworkCrefRewriter.cs @@ -163,6 +163,7 @@ static string StripIdPrefix(string text) if (text.Length < 2 || text[1] != ':') return text; + // The prefix is a single character, so the first two characters are dropped and the rest is trimmed return text[0] switch { 'N' or 'T' or 'F' or 'P' or 'M' or 'E' or 'O' or 'C' or '!' => text.Substring(2).TrimStart(), diff --git a/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs b/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs index 1608f52..092057c 100644 --- a/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs +++ b/src/src/SourceGeneratorFramework.MergeTool/FrameworkTypeInternalizer.cs @@ -20,6 +20,29 @@ /// never internalized, including the framework's own bundled components whose entry points live under /// the framework namespace: Roslyn can only instantiate public component types. /// +/// +/// Ownership is evaluated through the declaring chain. Mono.Cecil reports an empty namespace for a +/// nested type, so matching on the type's own namespace alone never recognizes the framework's nested +/// containers (the generated type-library namespace classes, nested operator/enum groups, the +/// CodeWriter scopes) as owned, and never internalizes them. A nested type is therefore owned +/// when it or any of its declaring types is owned. +/// +/// +/// Public visibility is likewise evaluated through the declaring chain. A public nested type of an +/// internal type is unreachable from outside the component, so it is not part of the public surface +/// and must not be reported. This matters for the compiler-synthesised extension containers +/// (<G>$/<M>$ types) the C# compiler emits for extension blocks: they are +/// nested public inside an internal static class, and every component that extends a framework type +/// has several. +/// +/// +/// The types the framework's own generators emit into a component (the generated type library and its +/// namespace classes, the attribute data models) are not part of the merged framework assembly, so +/// ILRepack never sees them; they are recognized by their +/// System.CodeDom.Compiler.GeneratedCodeAttribute tool name and internalized with the rest of +/// the framework surface. The component's unmerged output keeps the generated accessibility, so +/// in-repo consumers (code fixers, sibling assemblies, test harnesses) are unaffected. +/// /// static class FrameworkTypeInternalizer { @@ -37,6 +60,32 @@ static class FrameworkTypeInternalizer "Microsoft.CodeAnalysis.EmbeddedAttribute", ]; + /// + /// The attribute the framework's generators stamp on every type they emit into a component. + /// + const string GeneratedCodeAttributeFullName = "System.CodeDom.Compiler.GeneratedCodeAttribute"; + + /// + /// Generator tool names whose emitted types are framework-generated. Those types live in the + /// component's own namespace (the framework's generated type library is emitted into the spec's + /// 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 = + [ + "TypeLibraryGenerator", + "AttributeDataModelGenerator", + ]; + + /// + /// Assembly-level attributes that grant other assemblies access to a component's internals. + /// + static readonly ImmutableArray s_internalsGrantAttributeFullNames = + [ + "System.Runtime.CompilerServices.InternalsVisibleToAttribute", + "System.Runtime.CompilerServices.IgnoresAccessChecksToAttribute", + ]; + static readonly ImmutableArray s_roslynComponentAttributeFullNames = [ "Microsoft.CodeAnalysis.GeneratorAttribute", @@ -55,14 +104,32 @@ static class FrameworkTypeInternalizer ]; /// - /// Internalizes every framework-owned type in the merged component and returns a report of the - /// changes plus anything that could not be internalized. + /// Reads every type full name (nested types included) declared by an assembly. The merge uses it + /// to treat every type that came from the framework assembly as framework-owned, whatever + /// namespace it lives in: the framework's public surface is not confined to + /// Purview.SourceGeneratorFramework (for example the + /// Microsoft.CodeAnalysis.*Extensions and System.StringExtensions extension + /// classes), and ILRepack's internalize is best effort, so ownership cannot rely on the namespace + /// alone. + /// + /// The assembly to read type names from. + public static ImmutableArray CollectTypeFullNames(string assemblyPath) + { + using var assembly = AssemblyDefinition.ReadAssembly(assemblyPath); + + return [.. assembly.MainModule.Types.SelectMany(MergeToolRunner.Flatten).Select(static type => type.FullName)]; + } + + /// + /// 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 + /// could not be internalized. /// /// The merged component to rewrite in place. /// Assembly resolution paths for the merged component. /// 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. + /// Additional type full names to internalize (normally the framework assembly's own type names). public static FrameworkInternalizationReport Apply( string assemblyPath, IEnumerable searchDirectories, @@ -106,70 +173,104 @@ public static FrameworkInternalizationReport Apply( List remainingPublicTypes = []; foreach (var type in allTypes) { - if (IsOwned(type, namespaces, typeFullNames) && IsPubliclyVisible(type) && !IsRoslynComponent(type)) + // A framework type reachable only through a non-public declaring type is not part of the + // public surface, so it cannot leak and must not fail the merge. + if (IsOwned(type, namespaces, typeFullNames) && IsExternallyVisible(type) && !IsRoslynComponent(type)) remainingPublicTypes.Add(type.FullName); } - List exposingMembers = []; + List<(string Owner, string Member)> exposingMembers = []; foreach (var type in allTypes) { - if (IsOwned(type, namespaces, typeFullNames) || !IsPubliclyVisible(type)) + // Framework types and types reachable only through a non-public declaring type are not + // part of the component's public surface, so they cannot expose anything to a consumer. + if (IsOwned(type, namespaces, typeFullNames) || !IsExternallyVisible(type)) continue; CollectExposingMembers(type, namespaces, typeFullNames, exposingMembers); } - if (internalizedTypeCount > 0) + var strippedGrantCount = StripInternalsGrants(assembly); + + if (internalizedTypeCount > 0 || strippedGrantCount > 0) assembly.Write(assemblyPath, new WriterParameters { WriteSymbols = hasSymbols }); if (warn is not null) { // Group by declaring type: a component whose type library exposes framework types has many // such members, and one actionable line per type is more useful than one per member. - foreach (var group in exposingMembers.GroupBy(ExposingMemberOwner, StringComparer.Ordinal)) + foreach (var group in exposingMembers.GroupBy(static entry => entry.Owner, StringComparer.Ordinal)) { - var samples = group.Take(3).Select(ExposingMemberName); + var samples = group.Take(3).Select(static entry => entry.Member); warn( $"'{group.Key}' is public and exposes Purview.SourceGeneratorFramework types ({group.Count()} member(s), e.g. {string.Join(", ", samples)}). The exposed types were internalized in the merged component, so make the declaring type or its members non-public to keep the component's public surface self-contained." ); } } - return new(internalizedTypeCount, [.. remainingPublicTypes], [.. exposingMembers]); + return new( + internalizedTypeCount, + [.. remainingPublicTypes], + [.. exposingMembers.Select(static entry => $"{entry.Owner}.{entry.Member}")], + strippedGrantCount + ); } - static string ExposingMemberOwner(string 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. + /// + static int StripInternalsGrants(AssemblyDefinition assembly) { - var separator = member.LastIndexOf('.'); - return separator <= 0 ? member : member[..separator]; - } + var removed = 0; + for (var index = assembly.CustomAttributes.Count - 1; index >= 0; index--) + { + if ( + !s_internalsGrantAttributeFullNames.Contains( + assembly.CustomAttributes[index].AttributeType.FullName, + StringComparer.Ordinal + ) + ) + { + continue; + } - static string ExposingMemberName(string member) - { - var separator = member.LastIndexOf('.'); - return separator <= 0 ? member : member[(separator + 1)..]; + assembly.CustomAttributes.RemoveAt(index); + removed++; + } + + return removed; } static void CollectExposingMembers( TypeDefinition type, ImmutableArray ownedNamespaces, ImmutableArray ownedTypeFullNames, - List exposingMembers + List<(string Owner, string Member)> exposingMembers ) { if (ReferencesOwnedType(type.BaseType, ownedNamespaces, ownedTypeFullNames)) - exposingMembers.Add(type.FullName); + exposingMembers.Add((type.FullName, $"base type {type.BaseType.FullName}")); foreach (var @interface in type.Interfaces) { if (ReferencesOwnedType(@interface.InterfaceType, ownedNamespaces, ownedTypeFullNames)) - exposingMembers.Add($"{type.FullName} : {@interface.InterfaceType.FullName}"); + exposingMembers.Add((type.FullName, $"interface {@interface.InterfaceType.FullName}")); + } + + // A generic constraint is part of the public surface even though it is not a member. + foreach (var genericParameter in type.GenericParameters) + { + if (ReferencesOwnedType(genericParameter, ownedNamespaces, ownedTypeFullNames)) + exposingMembers.Add((type.FullName, $"generic parameter {genericParameter.Name}")); } foreach (var field in type.Fields) { if (IsPubliclyVisible(field) && ReferencesOwnedType(field.FieldType, ownedNamespaces, ownedTypeFullNames)) - exposingMembers.Add($"{type.FullName}.{field.Name}"); + exposingMembers.Add((type.FullName, field.Name)); } foreach (var property in type.Properties) @@ -184,32 +285,36 @@ List exposingMembers ) ) { - exposingMembers.Add($"{type.FullName}.{property.Name}"); + exposingMembers.Add((type.FullName, property.Name)); } } foreach (var @event in type.Events) { if (IsPubliclyVisible(@event) && ReferencesOwnedType(@event.EventType, ownedNamespaces, ownedTypeFullNames)) - exposingMembers.Add($"{type.FullName}.{@event.Name}"); + exposingMembers.Add((type.FullName, @event.Name)); } foreach (var method in type.Methods) { + if (!IsPubliclyVisible(method)) + continue; + if ( - !IsPubliclyVisible(method) - || ( - !ReferencesOwnedType(method.ReturnType, ownedNamespaces, ownedTypeFullNames) - && !method.Parameters.Any(parameter => - ReferencesOwnedType(parameter.ParameterType, ownedNamespaces, ownedTypeFullNames) - ) + ReferencesOwnedType(method.ReturnType, ownedNamespaces, ownedTypeFullNames) + || method.Parameters.Any(parameter => + ReferencesOwnedType(parameter.ParameterType, ownedNamespaces, ownedTypeFullNames) ) ) { - continue; + exposingMembers.Add((type.FullName, method.Name)); } - exposingMembers.Add($"{type.FullName}.{method.Name}"); + foreach (var genericParameter in method.GenericParameters) + { + if (ReferencesOwnedType(genericParameter, ownedNamespaces, ownedTypeFullNames)) + exposingMembers.Add((type.FullName, $"{method.Name}<{genericParameter.Name}>")); + } } } @@ -253,19 +358,68 @@ ImmutableArray ownedTypeFullNames if (ownedTypeFullNames.Contains(type.FullName, StringComparer.Ordinal)) return true; + // Mono.Cecil reports an empty namespace for nested types, so the check walks the declaring chain: a nested type is owned when any of its declaring types is. return IsOwnedNamespace(type.Namespace, ownedNamespaces); } return false; } + /// + /// Determines whether a type belongs to the framework, or was emitted into the component by the + /// framework's generators. Mono.Cecil reports an empty namespace for nested types, so the check + /// walks the declaring chain: a nested type is owned when any of its declaring types is. + /// static bool IsOwned( TypeDefinition type, ImmutableArray ownedNamespaces, ImmutableArray ownedTypeFullNames - ) => - ownedTypeFullNames.Contains(type.FullName, StringComparer.Ordinal) - || IsOwnedNamespace(type.Namespace, ownedNamespaces); + ) + { + for (var current = type; current is not null; current = current.DeclaringType) + { + if ( + ownedTypeFullNames.Contains(current.FullName, StringComparer.Ordinal) + || IsOwnedNamespace(current.Namespace, ownedNamespaces) + || IsFrameworkGenerated(current) + ) + { + return true; + } + } + + return false; + } + + /// + /// Detects a type the framework's generators emitted into the component (the generated type + /// library, attribute data models). Those types are not part of the merged framework assembly, so + /// ILRepack cannot internalize them; the tool name on the generated-code attribute is the only + /// marker that survives the merge and identifies them. + /// + static bool IsFrameworkGenerated(TypeDefinition type) + { + foreach (var attribute in type.CustomAttributes) + { + if ( + !string.Equals( + attribute.AttributeType.FullName, + GeneratedCodeAttributeFullName, + StringComparison.Ordinal + ) + || attribute.ConstructorArguments.Count == 0 + || attribute.ConstructorArguments[0].Value is not string toolName + ) + { + continue; + } + + if (s_frameworkGeneratorToolNames.Contains(toolName, StringComparer.Ordinal)) + return true; + } + + return false; + } static bool IsOwnedNamespace(string? @namespace, ImmutableArray ownedNamespaces) => @namespace is not null @@ -274,6 +428,29 @@ @namespace is not null || @namespace.StartsWith(prefix + ".", StringComparison.Ordinal) ); + /// + /// Determines whether a type is reachable from outside the component. A nested type is reachable + /// only when it, and every type that declares it, is public: a public nested type inside an + /// internal type is not part of the component's public surface. + /// + static bool IsExternallyVisible(TypeDefinition type) + { + for (var current = type; current is not null; current = current.DeclaringType) + { + if (current.DeclaringType is null) + { + if (!current.IsPublic) + return false; + } + else if (!current.IsNestedPublic) + { + return false; + } + } + + return true; + } + /// /// Detects Roslyn component entry points so they are never internalized. Roslyn discovers /// components through their attributes and only instantiates public types. @@ -306,6 +483,7 @@ static bool IsRoslynComponentType(TypeReference? type) if (type is null) 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( Resolve(type)?.FullName ?? string.Empty, @@ -353,8 +531,10 @@ static void MakeNonPublic(TypeDefinition type) => /// The number of types rewritten to non-public visibility. /// Framework-owned types that are still public; a non-empty value must fail the merge. /// Public members of non-framework types whose signature references a framework-owned type. +/// The number of assembly-level internals grants removed from the merged artifact. sealed record FrameworkInternalizationReport( int InternalizedTypeCount, ImmutableArray PublicFrameworkTypesRemaining, - ImmutableArray PublicMembersExposingFrameworkTypes + ImmutableArray PublicMembersExposingFrameworkTypes, + int StrippedInternalsGrantCount = 0 ); diff --git a/src/src/SourceGeneratorFramework.MergeTool/IsExternalInitNormalizer.cs b/src/src/SourceGeneratorFramework.MergeTool/IsExternalInitNormalizer.cs index 1eb3019..6577c40 100644 --- a/src/src/SourceGeneratorFramework.MergeTool/IsExternalInitNormalizer.cs +++ b/src/src/SourceGeneratorFramework.MergeTool/IsExternalInitNormalizer.cs @@ -72,9 +72,7 @@ sealed class RequiredModifierRewriter( TypeReference frameworkMarker ) { - int _remainingComponentMarkerReferences; - - public int RemainingComponentMarkerReferences => _remainingComponentMarkerReferences; + public int RemainingComponentMarkerReferences { get; private set; } public void Rewrite() { @@ -246,7 +244,7 @@ void Rewrite(TypeReference? type) if (IsComponentMarker(type)) { - _remainingComponentMarkerReferences++; + RemainingComponentMarkerReferences++; } Rewrite(type.DeclaringType); diff --git a/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs b/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs index a77b9e5..d889222 100644 --- a/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs +++ b/src/src/SourceGeneratorFramework.MergeTool/MergeToolRunner.cs @@ -1,3 +1,4 @@ +using System.Collections.Immutable; using System.Security.Cryptography; using System.Text; using ILRepacking; @@ -8,22 +9,36 @@ static class MergeToolRunner const string IsExternalInitName = "IsExternalInit"; const string IsExternalInitNamespace = "System.Runtime.CompilerServices"; const string TagCommand = "--tag"; + const string InputValueSwitch = "--input-value"; + const string OwnedNamespaceSwitch = "--owned-namespace"; + const string OwnedTypeSwitch = "--owned-type"; + const string PublicSurfaceSeveritySwitch = "--public-surface-severity"; + const string OriginSwitch = "--origin"; - public static int Run(string[] args, TextWriter error, ILogger? logger = null) + /// + /// Reported when a component's public surface exposes framework types that the merge internalizes; + /// advisory, because the merge still produces a self-contained analyzer. + /// + const string PublicSurfaceFindingCode = "PSGFR41"; + + /// + /// Reported when the merged artifact still exposes framework types; the merge fails. + /// + const string PublicSurfaceBlockedCode = "PSGFR42"; + + const string DefaultDiagnosticOrigin = "Purview.SourceGeneratorFramework"; + + public static int Run(string[] args, TextWriter error, ILogger? logger = null, TextWriter? output = null) { if (args.Length > 0 && string.Equals(args[0], TagCommand, StringComparison.Ordinal)) - return WriteContentTag(args, error); + return WriteContentTag(args, error, output); - if (args.Length < 3) - { - error.WriteLine("Usage: Purview.SourceGeneratorFramework.MergeTool "); - error.WriteLine(" Purview.SourceGeneratorFramework.MergeTool --tag [...]"); + if (!TryParseMergeArguments(args, error, out var options)) return 2; - } - var componentPath = Path.GetFullPath(args[0]); - var frameworkPath = Path.GetFullPath(args[1]); - var outputPath = Path.GetFullPath(args[2]); + var componentPath = Path.GetFullPath(options.ComponentPath); + var frameworkPath = Path.GetFullPath(options.FrameworkPath); + var outputPath = Path.GetFullPath(options.OutputPath); if (!File.Exists(componentPath)) { @@ -54,7 +69,7 @@ public static int Run(string[] args, TextWriter error, ILogger? logger = null) Path.GetDirectoryName(frameworkPath)!, }; - foreach (var searchPath in args.Skip(3)) + foreach (var searchPath in options.SearchPaths) { var fullSearchPath = Path.GetFullPath(searchPath); searchDirectories.Add( @@ -76,7 +91,7 @@ public static int Run(string[] args, TextWriter error, ILogger? logger = null) searchDirectories.Add(Path.GetDirectoryName(normalizedComponent.AssemblyPath)!); } - RepackOptions options = new() + RepackOptions repackOptions = new() { InputAssemblies = [normalizedComponent?.AssemblyPath ?? componentPath, frameworkPath], OutputFile = stagingPath, @@ -93,28 +108,42 @@ public static int Run(string[] args, TextWriter error, ILogger? logger = null) if (logger is null) { - new ILRepack(options).Repack(); + new ILRepack(repackOptions).Repack(); } else { - new ILRepack(options, logger).Repack(); + new ILRepack(repackOptions, logger).Repack(); } RestoreCanonicalIsExternalInit(stagingPath, searchDirectories); // ILRepack's internalize is best-effort and cannot reach framework types the component's // own generators emit, so force self-containment deterministically and fail the build - // rather than shipping an analyzer that leaks framework types. + // rather than shipping an analyzer that leaks framework types. Ownership is seeded with + // the framework assembly's own type identities: the framework's public surface is not + // confined to the framework namespace, so the namespace heuristic alone is not enough. var internalization = FrameworkTypeInternalizer.Apply( stagingPath, searchDirectories, - logger is not null ? logger.Warn : message => error.WriteLine(message) + logger is not null + ? logger.Warn + : CreateFindingWriter(error, options.PublicSurfaceSeverity, options.Origin), + ownedNamespaces: Extend(FrameworkTypeInternalizer.DefaultOwnedNamespaces, options.OwnedNamespaces), + ownedTypeFullNames: Extend( + FrameworkTypeInternalizer.DefaultOwnedTypeFullNames, + [.. FrameworkTypeInternalizer.CollectTypeFullNames(frameworkPath), .. options.OwnedTypeFullNames] + ) ); if (internalization.PublicFrameworkTypesRemaining.Length > 0) { error.WriteLine( - $"The merged component '{stagingPath}' still exposes public Purview.SourceGeneratorFramework types: {string.Join(", ", internalization.PublicFrameworkTypesRemaining)}." + FormatDiagnostic( + options.Origin ?? outputPath, + PublicSurfaceSeverity.Error, + PublicSurfaceBlockedCode, + $"The merged component '{stagingPath}' still exposes public Purview.SourceGeneratorFramework types: {string.Join(", ", internalization.PublicFrameworkTypesRemaining)}. Only the component's own entry points may stay public; make the listed types or their declaring types non-public." + ) ); return 5; } @@ -131,14 +160,31 @@ public static int Run(string[] args, TextWriter error, ILogger? logger = null) /// Writes a deterministic content tag (SHA-256 over the per-input SHA-256 values) for the given /// inputs to standard output. The build uses it to give the merged artifact a content-addressed /// directory, so a rebuild with unchanged inputs skips the merge and changed inputs never - /// overwrite a merged assembly the compiler host has loaded. + /// overwrite a merged assembly the compiler host has loaded. Configuration values participate + /// through --input-value, so changing the ownership lists or the reporting options re-runs + /// the merge instead of reusing an artifact (and a report) produced with other options. /// - static int WriteContentTag(string[] args, TextWriter error) + static int WriteContentTag(string[] args, TextWriter error, TextWriter? output = null) { StringBuilder builder = new(); for (var index = 1; index < args.Length; index++) { - var path = Path.GetFullPath(args[index]); + var argument = args[index]; + + if (string.Equals(argument, InputValueSwitch, StringComparison.Ordinal)) + { + if (++index >= args.Length) + { + error.WriteLine($"The {InputValueSwitch} option requires a value."); + return 2; + } + + builder.Append(Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(args[index])))); + builder.Append('\n'); + continue; + } + + var path = Path.GetFullPath(argument); if (!File.Exists(path)) { error.WriteLine($"Merge input was not found: {path}"); @@ -149,10 +195,213 @@ static int WriteContentTag(string[] args, TextWriter error) builder.Append('\n'); } - Console.Out.WriteLine(Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(builder.ToString())))); + (output ?? Console.Out).WriteLine( + Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(builder.ToString()))) + ); return 0; } + /// + /// Parses the merge command line. Positional arguments are the component, the framework assembly, + /// the output path and any additional assembly search paths; options extend the internalization + /// ownership and control how findings are reported back to the build. + /// + static bool TryParseMergeArguments(string[] args, TextWriter error, out MergeArguments options) + { + List positional = []; + List ownedNamespaces = []; + List ownedTypeFullNames = []; + var severity = PublicSurfaceSeverity.Message; + string? origin = null; + + for (var index = 0; index < args.Length; index++) + { + var argument = args[index]; + + if (argument.StartsWith("--", StringComparison.Ordinal)) + { + if ( + !string.Equals(argument, OwnedNamespaceSwitch, StringComparison.Ordinal) + && !string.Equals(argument, OwnedTypeSwitch, StringComparison.Ordinal) + && !string.Equals(argument, PublicSurfaceSeveritySwitch, StringComparison.Ordinal) + && !string.Equals(argument, OriginSwitch, StringComparison.Ordinal) + ) + { + error.WriteLine( + string.Equals(argument, InputValueSwitch, StringComparison.Ordinal) + ? $"The {InputValueSwitch} option is only valid with {TagCommand}." + : $"Unknown option '{argument}'." + ); + WriteUsage(error); + options = default!; + return false; + } + + if (++index >= args.Length) + { + error.WriteLine($"The {argument} option requires a value."); + WriteUsage(error); + options = default!; + return false; + } + + var value = args[index]; + if (string.Equals(argument, OwnedNamespaceSwitch, StringComparison.Ordinal)) + ownedNamespaces.Add(value); + else if (string.Equals(argument, OwnedTypeSwitch, StringComparison.Ordinal)) + ownedTypeFullNames.Add(value); + else if (string.Equals(argument, OriginSwitch, StringComparison.Ordinal)) + origin = value; + else if (!TryParseSeverity(value, out severity)) + { + error.WriteLine( + $"The {PublicSurfaceSeveritySwitch} option must be 'message', 'warning' or 'error', but was '{value}'." + ); + WriteUsage(error); + options = default!; + return false; + } + + continue; + } + + positional.Add(argument); + } + + if (positional.Count < 3) + { + error.WriteLine("The merge requires the component, the framework assembly and the output path."); + WriteUsage(error); + options = default!; + return false; + } + + options = new( + positional[0], + positional[1], + positional[2], + [.. positional.Skip(3)], + [.. ownedNamespaces], + [.. ownedTypeFullNames], + severity, + origin + ); + return true; + } + + static void WriteUsage(TextWriter error) + { + error.WriteLine( + "Usage: Purview.SourceGeneratorFramework.MergeTool [...] [options]" + ); + error.WriteLine( + $" Purview.SourceGeneratorFramework.MergeTool {TagCommand} [--input-value ]..." + ); + error.WriteLine( + $" Options: {OwnedNamespaceSwitch} , {OwnedTypeSwitch} , {PublicSurfaceSeveritySwitch} , {OriginSwitch} " + ); + } + + static bool TryParseSeverity(string value, out PublicSurfaceSeverity severity) + { + switch (value.ToUpperInvariant()) + { + case "MESSAGE": + severity = PublicSurfaceSeverity.Message; + return true; + case "WARNING": + severity = PublicSurfaceSeverity.Warning; + return true; + case "ERROR": + severity = PublicSurfaceSeverity.Error; + return true; + case "NONE": + severity = PublicSurfaceSeverity.None; + return true; + default: + severity = PublicSurfaceSeverity.Message; + return false; + } + } + + /// + /// Builds the sink the internalizer reports findings through. With a logger (in-repo tests) the + /// findings stay structured; from MSBuild they are written in the canonical + /// origin : warning CODE: text form so the build surfaces them in the Error List and in CI + /// annotations, or as plain text when the caller keeps the default message severity. + /// + static Action CreateFindingWriter(TextWriter error, PublicSurfaceSeverity severity, string? origin) + { + if (severity == PublicSurfaceSeverity.None) + return static _ => { }; + + if (severity == PublicSurfaceSeverity.Message && origin is null) + return message => error.WriteLine(message); + + // The build's error stream is the only channel the merge has to report findings, so it must + return message => error.WriteLine(FormatDiagnostic(origin, severity, PublicSurfaceFindingCode, message)); + } + + static string FormatDiagnostic(string? origin, PublicSurfaceSeverity severity, string code, string message) + { +#pragma warning disable IDE0072 // Add missing cases + var category = severity switch + { + PublicSurfaceSeverity.Error => "error", + PublicSurfaceSeverity.Warning => "warning", + _ => "message", + }; +#pragma warning restore IDE0072 // Add missing cases + + return $"{origin ?? DefaultDiagnosticOrigin} : {category} {code}: {message}"; + } + + static ImmutableArray Extend(ImmutableArray defaults, ImmutableArray extras) => + extras.IsDefaultOrEmpty ? defaults : [.. defaults, .. extras]; + + /// + /// Parsed merge command line. + /// + /// The component assembly to merge. + /// The framework assembly to merge in. + /// The merged artifact to produce. + /// Additional assembly search paths. + /// Namespace prefixes the build adds to the framework-owned set. + /// Type full names the build adds to the framework-owned set. + /// How public-surface findings are reported to the build. + /// The build origin reported with a finding, so the IDE and CI can attribute it. + sealed record MergeArguments( + string ComponentPath, + string FrameworkPath, + string OutputPath, + ImmutableArray SearchPaths, + ImmutableArray OwnedNamespaces, + ImmutableArray OwnedTypeFullNames, + PublicSurfaceSeverity PublicSurfaceSeverity, + string? Origin + ); + + /// + /// How the merge reports a component whose public surface exposes framework types. + /// + enum PublicSurfaceSeverity + { + /// Plain text on the error stream; the historical behaviour. + Message, + + /// An MSBuild warning with a diagnostic code. + Warning, + + /// An MSBuild error; used by the merge for a leaked public framework type. + Error, + + /// + /// Findings are dropped. A leaked public framework type still fails the merge with an error, + /// because that artifact is not self-contained. + /// + None, + } + /// /// Publishes the staged merged assembly by renaming its staging directory onto the /// content-addressed destination directory. When a concurrent invocation has already produced diff --git a/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets b/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets index 2b16606..30a9197 100644 --- a/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets +++ b/src/src/SourceGeneratorFramework/Sdk/build/Purview.SourceGeneratorFramework.targets @@ -10,6 +10,19 @@ true + + none + message true @@ -158,6 +171,14 @@ <_PurviewMergedAnalyzerStagingDirectoryFullPath>$([System.IO.Path]::GetFullPath('$(_PurviewMergedAnalyzerStagingDirectory)')) + + <_PurviewMergePublicSurfaceOrigin Condition="'$(PurviewMergePublicSurfaceOrigin)' == ''" + >$(TargetPath) + + <_PurviewMergeConfigurationTagValues>$(PurviewMergeOwnedNamespaces)|$(PurviewMergeOwnedTypeFullNames)|$(PurviewMergePublicSurfaceSeverity)|$(_PurviewMergePublicSurfaceOrigin) .dll. --> @@ -207,10 +228,30 @@ + + <_PurviewMergeOwnedNamespace + Include="$(PurviewMergeOwnedNamespaces)" + Condition="'$(PurviewMergeOwnedNamespaces)' != ''" + /> + <_PurviewMergeOwnedType + Include="$(PurviewMergeOwnedTypeFullNames)" + Condition="'$(PurviewMergeOwnedTypeFullNames)' != ''" + /> + + + <_PurviewMergeOwnedNamespaceArgs Condition="'@(_PurviewMergeOwnedNamespace)' != ''" + >@(_PurviewMergeOwnedNamespace->'--owned-namespace "%(Identity)"', ' ') + <_PurviewMergeOwnedTypeArgs Condition="'@(_PurviewMergeOwnedType)' != ''" + >@(_PurviewMergeOwnedType->'--owned-type "%(Identity)"', ' ') + <_PurviewMergePublicSurfaceArgs>--public-surface-severity "$(PurviewMergePublicSurfaceSeverity)" --origin "$(_PurviewMergePublicSurfaceOrigin)" + diff --git a/src/src/SourceGeneratorShared/TypeLibraryDiagnosticRules.cs b/src/src/SourceGeneratorShared/TypeLibraryDiagnosticRules.cs index d31ba67..0402c5b 100644 --- a/src/src/SourceGeneratorShared/TypeLibraryDiagnosticRules.cs +++ b/src/src/SourceGeneratorShared/TypeLibraryDiagnosticRules.cs @@ -243,4 +243,20 @@ public static class TypeLibraryDiagnosticRules DiagnosticSeverity.Error, isEnabledByDefault: true ); + + /// + /// Diagnostic raised when a type-library spec is public in a component whose framework + /// implementation is merged into the shipped analyzer. The generated TypeRefMarkers member is + /// always public and typed as framework identities, so a public spec leaves a public signature over + /// a type the merge internalizes. + /// + public static readonly DiagnosticDescriptor SpecShouldBeNonPublic = new( + "TLB0021", + "Type-library spec should not be public in a merged component", + "Type-library spec '{0}' is public in a component whose framework implementation is merged; the generated marker member exposes framework type identities that the merged analyzer internalizes, so declare the spec non-public", + "TypeLibrary", + DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "The merge internalizes every Purview.SourceGeneratorFramework type in the shipped analyzer, so a public type-library spec leaves the generated public marker member typed over an internal type. Declaring the spec non-public keeps the merged analyzer's public surface self-contained." + ); } diff --git a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/AnalyzerTestHelpers.cs b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/AnalyzerTestHelpers.cs index 86f0917..adfdd17 100644 --- a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/AnalyzerTestHelpers.cs +++ b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/AnalyzerTestHelpers.cs @@ -42,12 +42,12 @@ public static async Task> GetAnalyzerDiagnosticsAsync( static CSharpCompilation CreateTestCompilation(string source, bool referenceSourceGeneratorFramework) { var syntaxTree = CSharpSyntaxTree.ParseText(source); - List references = new() - { + List references = + [ MetadataReference.CreateFromFile(typeof(object).Assembly.Location), MetadataReference.CreateFromFile(typeof(Compilation).Assembly.Location), MetadataReference.CreateFromFile(typeof(CSharpCompilation).Assembly.Location), - }; + ]; if (referenceSourceGeneratorFramework) { references.Add(MetadataReference.CreateFromFile(typeof(CodeWriter).Assembly.Location)); @@ -66,7 +66,7 @@ static CSharpCompilation CreateTestCompilation(string source, bool referenceSour static class TestAnalyzerConfigOptions { public static AnalyzerOptions CreateAnalyzerOptions(IReadOnlyDictionary buildProperties) => - new(ImmutableArray.Empty, CreateProvider(buildProperties)); + new([], CreateProvider(buildProperties)); public static AnalyzerConfigOptionsProvider CreateProvider(IReadOnlyDictionary buildProperties) => new Provider(new Options(ImmutableDictionary.CreateRange(StringComparer.Ordinal, buildProperties))); diff --git a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/ComponentPublicSurfaceAnalyzerTests.cs b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/ComponentPublicSurfaceAnalyzerTests.cs new file mode 100644 index 0000000..e466893 --- /dev/null +++ b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/ComponentPublicSurfaceAnalyzerTests.cs @@ -0,0 +1,238 @@ +namespace Purview.SourceGeneratorFramework.Analyzers; + +public sealed class ComponentPublicSurfaceAnalyzerTests +{ + /// + /// The merge runs for a Roslyn component that embeds the framework and is either packable or + /// returns a merged analyzer artifact. + /// + static readonly Dictionary MergedComponentProperties = new() + { + ["build_property.IsRoslynComponent"] = "true", + ["build_property.PurviewEmbedSourceGeneratorFramework"] = "true", + ["build_property.IsPackable"] = "true", + }; + + const string ExposingSurfaceSource = """ + namespace Fixture.Component + { + public sealed class Consumer + { + public Purview.SourceGeneratorFramework.TypeIdentity Identity => default; + } + } + """; + + [Test] + public async Task GivenPublicMemberExposingFrameworkType_ReportsDiagnostic(CancellationToken cancellationToken) + { + // Arrange + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + ExposingSurfaceSource, + MergedComponentProperties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Select(static diagnostic => diagnostic.Id).ToArray()) + .Contains(ComponentPublicSurfaceAnalyzer.DiagnosticId); + } + + [Test] + public async Task GivenPublicGenericConstraintExposingFrameworkType_ReportsDiagnostic( + CancellationToken cancellationToken + ) + { + // Arrange + const string source = """ + namespace Fixture.Component + { + public sealed class Constrained + where T : Purview.SourceGeneratorFramework.IGenerationCapabilities + { + } + } + """; + + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + source, + MergedComponentProperties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Select(static diagnostic => diagnostic.Id).ToArray()) + .Contains(ComponentPublicSurfaceAnalyzer.DiagnosticId); + } + + [Test] + public async Task GivenFrameworkTypeUsedOnlyInsideMethodBody_DoesNotReportDiagnostic( + CancellationToken cancellationToken + ) + { + // Arrange + const string source = """ + namespace Fixture.Component + { + public sealed class Consumer + { + public string Describe() + { + var identity = new Purview.SourceGeneratorFramework.TypeIdentity("Name", "Namespace"); + return identity.Name; + } + } + } + """; + + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + source, + MergedComponentProperties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Any(static diagnostic => diagnostic.Id == ComponentPublicSurfaceAnalyzer.DiagnosticId)) + .IsFalse(); + } + + [Test] + public async Task GivenPublicNestedTypeInsideInternalType_DoesNotReportDiagnostic( + CancellationToken cancellationToken + ) + { + // Arrange + const string source = """ + namespace Fixture.Component + { + internal static class InternalContainer + { + public sealed class Nested + { + public Purview.SourceGeneratorFramework.TypeIdentity Identity => default; + } + } + } + """; + + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + source, + MergedComponentProperties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Any(static diagnostic => diagnostic.Id == ComponentPublicSurfaceAnalyzer.DiagnosticId)) + .IsFalse(); + } + + [Test] + public async Task GivenGeneratedTypeExposingFrameworkType_DoesNotReportDiagnostic( + CancellationToken cancellationToken + ) + { + // Arrange + // The merge internalizes generator-emitted type libraries and attribute sets itself, so the + // author has nothing to fix. + const string source = """ + using System.Runtime.CompilerServices; + + namespace Fixture.Component + { + [CompilerGenerated] + public static class GeneratedTypeLibrary + { + public static Purview.SourceGeneratorFramework.TypeIdentity Identity => default; + } + } + """; + + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + source, + MergedComponentProperties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Any(static diagnostic => diagnostic.Id == ComponentPublicSurfaceAnalyzer.DiagnosticId)) + .IsFalse(); + } + + [Test] + public async Task GivenEmbeddingDisabled_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Arrange + Dictionary properties = new(MergedComponentProperties) + { + ["build_property.PurviewEmbedSourceGeneratorFramework"] = "false", + }; + + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + ExposingSurfaceSource, + properties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Any(static diagnostic => diagnostic.Id == ComponentPublicSurfaceAnalyzer.DiagnosticId)) + .IsFalse(); + } + + [Test] + public async Task GivenNotARoslynComponent_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Arrange + Dictionary properties = new(MergedComponentProperties) + { + ["build_property.IsRoslynComponent"] = "false", + }; + + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + ExposingSurfaceSource, + properties, + referenceSourceGeneratorFramework: true, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Any(static diagnostic => diagnostic.Id == ComponentPublicSurfaceAnalyzer.DiagnosticId)) + .IsFalse(); + } + + [Test] + public async Task GivenNoFrameworkReference_DoesNotReportDiagnostic(CancellationToken cancellationToken) + { + // Act + var diagnostics = await new ComponentPublicSurfaceAnalyzer().GetAnalyzerDiagnosticsAsync( + "public sealed class Consumer { }", + MergedComponentProperties, + referenceSourceGeneratorFramework: false, + cancellationToken + ); + + // Assert + await Assert + .That(diagnostics.Any(static diagnostic => diagnostic.Id == ComponentPublicSurfaceAnalyzer.DiagnosticId)) + .IsFalse(); + } +} diff --git a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/TypeLibraryValidationAnalyzerTests.cs b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/TypeLibraryValidationAnalyzerTests.cs index c76b6c0..bfeb785 100644 --- a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/TypeLibraryValidationAnalyzerTests.cs +++ b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/TypeLibraryValidationAnalyzerTests.cs @@ -1,3 +1,4 @@ +using Purview.SourceGeneratorFramework.Testing; using Purview.SourceGeneratorFramework.Testing.TUnit; namespace Purview.SourceGeneratorFramework.Analyzers; @@ -487,6 +488,90 @@ static partial class TypeLibraryModel await Assert.That(result).HasNoDiagnostics(); } + [Test] + public async Task Generate_PublicSpecInMergedComponent_ReportsSpecShouldBeNonPublic( + CancellationToken cancellationToken + ) + { + // Arrange + var source = + AttributeDefinition + + """ + [GenerateTypeLibrary] + public static partial class TypeLibraryModel + { + [TypeRef("Test")] + static readonly TypeIdentity MyAttribute = default!; + } + """; + var options = new AnalyzerTestOptions().WithAnalyzerConfigOptions( + ("build_property.IsRoslynComponent", "true"), + ("build_property.PurviewEmbedSourceGeneratorFramework", "true"), + ("build_property.IsPackable", "true") + ); + + // Act + var result = await AnalyzeAsync(source, options, cancellationToken); + + // Assert + await Assert.That(result).HasDiagnostic(TypeLibraryValidationAnalyzer.SpecShouldBeNonPublic.Id); + } + + [Test] + public async Task Generate_NonPublicSpecInMergedComponent_DoesNotReportSpecShouldBeNonPublic( + CancellationToken cancellationToken + ) + { + // Arrange + var source = + AttributeDefinition + + """ + [GenerateTypeLibrary] + static partial class TypeLibraryModel + { + [TypeRef("Test")] + static readonly TypeIdentity MyAttribute = default!; + } + """; + var options = new AnalyzerTestOptions().WithAnalyzerConfigOptions( + ("build_property.IsRoslynComponent", "true"), + ("build_property.PurviewEmbedSourceGeneratorFramework", "true"), + ("build_property.IsPackable", "true") + ); + + // Act + var result = await AnalyzeAsync(source, options, cancellationToken); + + // Assert + await Assert.That(result).HasNoDiagnostics(); + } + + [Test] + public async Task Generate_PublicSpecOutsideAMergedComponent_DoesNotReportSpecShouldBeNonPublic( + CancellationToken cancellationToken + ) + { + // Arrange + // Without the build properties the project does not merge the framework implementation, so a + // public spec keeps its framework-typed markers public in the component's own assembly. + var source = + AttributeDefinition + + """ + [GenerateTypeLibrary] + public static partial class TypeLibraryModel + { + [TypeRef("Test")] + static readonly TypeIdentity MyAttribute = default!; + } + """; + + // Act + var result = await AnalyzeAsync(source, cancellationToken); + + // Assert + await Assert.That(result).HasNoDiagnostics(); + } + [Test] public async Task Generate_PartialExtensionInDifferentNamespace_ReportsNamespaceMismatch( CancellationToken cancellationToken diff --git a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UnqualifiedFrameworkCrefAnalyzerTests.cs b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UnqualifiedFrameworkCrefAnalyzerTests.cs index 7a5f994..03784bf 100644 --- a/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UnqualifiedFrameworkCrefAnalyzerTests.cs +++ b/src/tests/SourceGeneratorFramework.Analyzers.UnitTests/UnqualifiedFrameworkCrefAnalyzerTests.cs @@ -16,13 +16,13 @@ public sealed class UnqualifiedFrameworkCrefAnalyzerTests }.ToImmutableDictionary(), AdditionalAssemblyTypes = [ - typeof(Purview.SourceGeneratorFramework.CodeWriter), - typeof(Purview.SourceGeneratorFramework.GenerationSettings), - typeof(Purview.SourceGeneratorFramework.TypeIdentity), - typeof(Purview.SourceGeneratorFramework.TypeReference), - typeof(Purview.SourceGeneratorFramework.XmlCommentWriter), - typeof(Purview.SourceGeneratorFramework.Helpers.IncrementalPipeline), - typeof(Purview.SourceGeneratorFramework.CodeWriterScopeValidationException), + typeof(CodeWriter), + typeof(GenerationSettings), + typeof(TypeIdentity), + typeof(TypeReference), + typeof(XmlCommentWriter), + typeof(Helpers.IncrementalPipeline), + typeof(CodeWriterScopeValidationException), ], }; diff --git a/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs b/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs index c71f31b..cbb9b20 100644 --- a/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs +++ b/src/tests/SourceGeneratorFramework.BuildIntegrationTests/AnalyzerClosureTests.cs @@ -35,7 +35,7 @@ public async Task MergedComponentBinOutputs_AreSelfSufficient(CancellationToken { await BuildAsync(ProjectPath("Fixture.Consumer"), cancellationToken); - List problems = new(); + List problems = []; foreach (var component in new[] { "Fixture.Generator", "Fixture.Generator.CodeFixers" }) { var binDirectory = BinDirectory(component); @@ -55,7 +55,7 @@ public async Task MergedComponentBinOutputs_AreSelfSufficient(CancellationToken [Test] public async Task ComponentClosure_ContainsNoUnresolvableAssemblyReferences(CancellationToken cancellationToken) { - List problems = new(); + List problems = []; foreach (var component in new[] { "Fixture.Generator", "Fixture.Generator.CodeFixers" }) { @@ -178,11 +178,11 @@ public async Task ConsumerAnalyzerSet_ExcludesUnmergedComponentAnalyzer(Cancella .GetProperty("GetFixtureAnalyzerItems") .GetProperty("Items"); - List analyzerPaths = new(); + List analyzerPaths = []; foreach (var item in items.EnumerateArray()) analyzerPaths.Add(item.GetProperty("Identity").GetString()!); - List problems = new(); + List problems = []; if (!analyzerPaths.Any(static path => path.Contains("purview-merged", StringComparison.OrdinalIgnoreCase))) problems.Add("The analyzer set does not contain the merged artifact."); @@ -220,7 +220,7 @@ static List ReadAssemblyReferenceNames(string path) using PEReader peReader = new(stream); var metadata = peReader.GetMetadataReader(); - List names = new(); + List names = []; foreach (var handle in metadata.AssemblyReferences) names.Add(metadata.GetString(metadata.GetAssemblyReference(handle).Name)); @@ -258,7 +258,7 @@ static async Task> ResolveAnalyzerClosureAsync(string projectPath, .GetProperty("GetSourceGeneratorAnalyzerFiles") .GetProperty("Items"); - List files = new(); + List files = []; foreach (var item in items.EnumerateArray()) files.Add(item.GetProperty("Identity").GetString()!); @@ -326,6 +326,7 @@ CancellationToken cancellationToken ); } + // Return the standard output only, since the standard error may contain warnings that are not relevant to the test. return output; } diff --git a/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/MakeComponentSurfaceNonPublicCodeFixProviderTests.cs b/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/MakeComponentSurfaceNonPublicCodeFixProviderTests.cs new file mode 100644 index 0000000..b432bef --- /dev/null +++ b/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/MakeComponentSurfaceNonPublicCodeFixProviderTests.cs @@ -0,0 +1,68 @@ +using Purview.SourceGeneratorFramework.Analyzers; +using Purview.SourceGeneratorFramework.Testing; +using Purview.SourceGeneratorFramework.Testing.TUnit; + +namespace Purview.SourceGeneratorFramework.CodeFixers; + +public sealed class MakeComponentSurfaceNonPublicCodeFixProviderTests + : TUnitCodeFixTestBase +{ + const string ExposingSurfaceSource = """ + namespace Fixture.Component + { + public sealed class Consumer + { + public Purview.SourceGeneratorFramework.TypeIdentity Identity => default; + } + } + """; + + static readonly (string, string)[] MergedComponentProperties = + [ + ("build_property.IsRoslynComponent", "true"), + ("build_property.PurviewEmbedSourceGeneratorFramework", "true"), + ("build_property.IsPackable", "true"), + ]; + + static CodeFixTestOptions Options(string equivalenceKey) => + new CodeFixTestOptions + { + EquivalenceKey = equivalenceKey, + AdditionalAssemblyTypes = [typeof(TypeIdentity)], + }.WithAnalyzerConfigOptions(MergedComponentProperties); + + [Test] + public async Task PublicMemberExposingFrameworkType_BecomesInternal(CancellationToken cancellationToken) + { + // Arrange + // Act + var result = await ApplyCodeFixAsync( + ExposingSurfaceSource, + Options(MakeComponentSurfaceNonPublicCodeFixProvider.MemberEquivalenceKey), + cancellationToken + ); + + // Assert + await Assert.That(result).HasDiagnostic(ComponentPublicSurfaceAnalyzer.DiagnosticId); + await Assert + .That(result.FixedSource) + .Contains("internal Purview.SourceGeneratorFramework.TypeIdentity Identity"); + } + + [Test] + public async Task PublicTypeExposingFrameworkType_BecomesInternal(CancellationToken cancellationToken) + { + // Arrange + // Act + var result = await ApplyCodeFixAsync( + ExposingSurfaceSource, + Options(MakeComponentSurfaceNonPublicCodeFixProvider.TypeEquivalenceKey), + cancellationToken + ); + + // Assert + await Assert.That(result).HasDiagnostic(ComponentPublicSurfaceAnalyzer.DiagnosticId); + await Assert.That(result.FixedSource).Contains("sealed class Consumer"); + await Assert.That(result.FixedSource).DoesNotContain("public sealed class Consumer"); + } +} diff --git a/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/MakeTypeLibrarySpecNonPublicCodeFixProviderTests.cs b/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/MakeTypeLibrarySpecNonPublicCodeFixProviderTests.cs new file mode 100644 index 0000000..6873426 --- /dev/null +++ b/src/tests/SourceGeneratorFramework.CodeFixers.UnitTests/MakeTypeLibrarySpecNonPublicCodeFixProviderTests.cs @@ -0,0 +1,78 @@ +using Purview.SourceGeneratorFramework.Analyzers; +using Purview.SourceGeneratorFramework.Testing; +using Purview.SourceGeneratorFramework.Testing.TUnit; + +namespace Purview.SourceGeneratorFramework.CodeFixers; + +public sealed class MakeTypeLibrarySpecNonPublicCodeFixProviderTests + : TUnitCodeFixTestBase +{ + const string AttributeDefinition = """ + using System; + using Microsoft.CodeAnalysis; + using Purview.SourceGeneratorFramework; + using Purview.SourceGeneratorFramework.Generators; + + namespace Purview.SourceGeneratorFramework.Generators + { + [AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)] + public sealed class GenerateTypeLibraryAttribute : Attribute + { + public string? ClassName { get; set; } + public string? Namespace { get; set; } + } + + [AttributeUsage(AttributeTargets.Field, Inherited = false, AllowMultiple = false)] + public sealed class TypeRefAttribute : Attribute + { + public TypeRefAttribute(string @namespace, int arity = 0) + { + Namespace = @namespace; + Arity = arity; + } + + public string? Namespace { get; set; } + public int Arity { get; set; } + } + } + + namespace Purview.SourceGeneratorFramework + { + public readonly record struct TypeIdentity; + } + """; + + const string PublicSpecSource = """ + [GenerateTypeLibrary] + public static partial class TypeLibraryModel + { + [TypeRef("Test")] + static readonly TypeIdentity MyAttribute = default!; + } + """; + + static readonly (string, string)[] MergedComponentProperties = + [ + ("build_property.IsRoslynComponent", "true"), + ("build_property.PurviewEmbedSourceGeneratorFramework", "true"), + ("build_property.IsPackable", "true"), + ]; + + [Test] + public async Task PublicSpecInMergedComponent_BecomesNonPublic(CancellationToken cancellationToken) + { + // Arrange + var options = new CodeFixTestOptions + { + EquivalenceKey = MakeTypeLibrarySpecNonPublicCodeFixProvider.EquivalenceKey, + }.WithAnalyzerConfigOptions(MergedComponentProperties); + + // Act + var result = await ApplyCodeFixAsync(AttributeDefinition + PublicSpecSource, options, cancellationToken); + + // Assert + await Assert.That(result).HasDiagnostic(TypeLibraryValidationAnalyzer.SpecShouldBeNonPublic.Id); + await Assert.That(result.FixedSource).Contains("static partial class TypeLibraryModel"); + await Assert.That(result.FixedSource).DoesNotContain("public static partial class TypeLibraryModel"); + } +} diff --git a/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs b/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs index 0994e54..aefbf0c 100644 --- a/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs +++ b/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/FrameworkTypeInternalizerTests.cs @@ -92,6 +92,158 @@ public sealed class ComponentEntryPoint : global::Microsoft.CodeAnalysis.IIncrem } """; + /// + /// A framework shape with nested namespace containers, mirroring the generated type library and + /// the nested operator/enum groups the framework ships: every container below the root type is a + /// public nested type, and Mono.Cecil reports an empty namespace for it. + /// + const string NestedFrameworkSource = """ + namespace Purview.SourceGeneratorFramework + { + public sealed class TypeIdentity + { + public TypeIdentity(string name, string @namespace) { } + } + + public static class PurviewTypeLibrary + { + public static class System + { + public static readonly TypeIdentity String = new("String", "System"); + + public static class Collections + { + public static class Generic + { + public static readonly TypeIdentity List = + new("List", "System.Collections.Generic"); + } + } + } + } + } + """; + + /// + /// Mirrors the compiler-synthesised extension containers the C# compiler emits for extension + /// blocks: nested public types inside an internal static class, and a public nested type inside a + /// public component type that genuinely exposes a framework type. + /// + const string NestedComponentSource = """ + namespace Fixture.Component + { + internal static class InternalExtensionContainer + { + public sealed class NestedExtensionBlock + { + public Purview.SourceGeneratorFramework.TypeIdentity Identity => default; + } + } + + public class PublicSurface + { + public sealed class NestedExtensionBlock + { + public Purview.SourceGeneratorFramework.TypeIdentity Identity => default; + } + } + } + """; + + /// + /// Mirrors a component's generated type library: a public static class in the component's own + /// namespace, stamped by the framework's type-library generator, with nested namespace classes + /// that expose framework type identities. + /// + const string GeneratedTypeLibrarySource = """ + using System.CodeDom.Compiler; + + namespace Fixture.Component + { + [GeneratedCode("TypeLibraryGenerator", "1.0.0-test")] + public static partial class TypeLibrary + { + public static class System + { + [GeneratedCode("TypeLibraryGenerator", "1.0.0-test")] + public static readonly Purview.SourceGeneratorFramework.TypeIdentity String = default; + } + } + } + """; + + /// + /// A framework shape whose public surface is not confined to the framework namespace, mirroring the + /// framework's own Microsoft.CodeAnalysis.*Extensions and System.StringExtensions + /// extension classes: those types are only recognizable as framework-owned by assembly identity. + /// + const string ForeignNamespaceFrameworkSource = """ + namespace Fixture.Framework + { + public sealed class TypeReference + { + public string Name { get; } + + public TypeReference(string name) => Name = name; + } + + public static class TypeReferenceExtensions + { + public static string Describe(this TypeReference reference) => reference.Name; + } + } + """; + + /// + /// A framework shape with a non-sealed class usable as a generic constraint. + /// + const string ConstraintFrameworkSource = """ + namespace Purview.SourceGeneratorFramework + { + public class TypeIdentity + { + public string Name { get; set; } + } + } + """; + + /// + /// A component whose public generic constraints reference framework types. + /// + const string ConstraintComponentSource = """ + namespace Fixture.Component + { + public sealed class Constrained + where T : Purview.SourceGeneratorFramework.TypeIdentity + { + } + + public static class ConstrainedMethods + { + public static void Use() + where T : Purview.SourceGeneratorFramework.TypeIdentity + { + } + } + } + """; + + /// + /// A component that grants another assembly access to its internals. + /// + const string ComponentWithInternalsGrantSource = """ + using System.Runtime.CompilerServices; + + [assembly: InternalsVisibleTo("Fixture.Component.Tests")] + + namespace Fixture.Component + { + public sealed class PublicType + { + } + } + """; + [Test] public async Task Apply_GivenOwnedPublicTypes_InternalizesThemAndKeepsComponentsPublic( CancellationToken cancellationToken @@ -200,7 +352,7 @@ CancellationToken cancellationToken // Arrange cancellationToken.ThrowIfCancellationRequested(); using TestWorkspace workspace = new(); - var frameworkPath = typeof(Purview.SourceGeneratorFramework.TypeIdentity).Assembly.Location; + var frameworkPath = typeof(TypeIdentity).Assembly.Location; var componentPath = workspace.Compile("Fixture.RealComponent", RealFrameworkComponentSource, frameworkPath); var outputPath = workspace.GetPath("merged", "Fixture.RealComponent.dll"); TestLogger logger = new(); @@ -234,6 +386,208 @@ CancellationToken cancellationToken await Assert.That(publicFrameworkTypes[0]).IsEqualTo("Purview.SourceGeneratorFramework.ComponentEntryPoint"); } + [Test] + public async Task Apply_GivenNestedFrameworkTypes_InternalizesThemThroughTheDeclaringChain( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.NestedFramework", NestedFrameworkSource); + List warnings = []; + + // Act + var report = FrameworkTypeInternalizer.Apply( + frameworkPath, + [workspace.GetPath("Fixture.NestedFramework")], + warnings.Add + ); + + // Assert + await Assert.That(report.PublicFrameworkTypesRemaining).IsEmpty(); + await Assert.That(report.PublicMembersExposingFrameworkTypes).IsEmpty(); + await Assert.That(warnings).IsEmpty(); + + using var framework = AssemblyDefinition.ReadAssembly(frameworkPath); + var typeLibrary = framework.MainModule.GetType("Purview.SourceGeneratorFramework.PurviewTypeLibrary"); + await Assert.That(typeLibrary).IsNotNull(); + await Assert.That(typeLibrary!.IsNotPublic).IsTrue(); + + var system = framework.MainModule.GetType("Purview.SourceGeneratorFramework.PurviewTypeLibrary/System"); + await Assert.That(system).IsNotNull(); + await Assert.That(system!.IsNestedAssembly).IsTrue(); + + var generic = framework.MainModule.GetType( + "Purview.SourceGeneratorFramework.PurviewTypeLibrary/System/Collections/Generic" + ); + await Assert.That(generic).IsNotNull(); + await Assert.That(generic!.IsNestedAssembly).IsTrue(); + } + + [Test] + public async Task Apply_GivenNestedTypeOnlyReachableThroughInternalType_DoesNotReportIt( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", FixtureFrameworkSource); + var componentPath = workspace.Compile("Fixture.Component", NestedComponentSource, frameworkPath); + List warnings = []; + + // Act + FrameworkTypeInternalizer.Apply( + componentPath, + [workspace.GetPath("Fixture.Component")], + warnings.Add, + ownedNamespaces: ["Purview.SourceGeneratorFramework"] + ); + + // Assert + await Assert + .That(warnings) + .DoesNotContain(static warning => warning.Contains("InternalExtensionContainer", StringComparison.Ordinal)); + await Assert + .That(warnings) + .Contains(static warning => warning.Contains("Fixture.Component.PublicSurface", StringComparison.Ordinal)); + } + + [Test] + public async Task Apply_GivenFrameworkGeneratedTypeLibrary_InternalizesItWithoutReportingIt( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", FixtureFrameworkSource); + var componentPath = workspace.Compile("Fixture.Component", GeneratedTypeLibrarySource, frameworkPath); + List warnings = []; + + // Act + var report = FrameworkTypeInternalizer.Apply( + componentPath, + [workspace.GetPath("Fixture.Component")], + warnings.Add, + ownedNamespaces: ["Purview.SourceGeneratorFramework"] + ); + + // Assert + await Assert.That(warnings).IsEmpty(); + await Assert.That(report.PublicMembersExposingFrameworkTypes).IsEmpty(); + + using var component = AssemblyDefinition.ReadAssembly(componentPath); + var typeLibrary = component.MainModule.GetType("Fixture.Component.TypeLibrary"); + await Assert.That(typeLibrary).IsNotNull(); + await Assert.That(typeLibrary!.IsNotPublic).IsTrue(); + + var system = component.MainModule.GetType("Fixture.Component.TypeLibrary/System"); + await Assert.That(system).IsNotNull(); + await Assert.That(system!.IsNestedAssembly).IsTrue(); + } + + [Test] + public async Task Apply_GivenFrameworkTypesOutsideTheFrameworkNamespace_InternalizesThemByName( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.ForeignFramework", ForeignNamespaceFrameworkSource); + List warnings = []; + + // Act + var report = FrameworkTypeInternalizer.Apply( + frameworkPath, + [workspace.GetPath("Fixture.ForeignFramework")], + warnings.Add, + ownedNamespaces: [], + ownedTypeFullNames: + [ + .. FrameworkTypeInternalizer.DefaultOwnedTypeFullNames, + .. FrameworkTypeInternalizer.CollectTypeFullNames(frameworkPath), + ] + ); + + // Assert + await Assert.That(report.PublicFrameworkTypesRemaining).IsEmpty(); + await Assert.That(warnings).IsEmpty(); + + using var framework = AssemblyDefinition.ReadAssembly(frameworkPath); + await Assert.That(framework.MainModule.GetType("Fixture.Framework.TypeReference")!.IsNotPublic).IsTrue(); + await Assert + .That(framework.MainModule.GetType("Fixture.Framework.TypeReferenceExtensions")!.IsNotPublic) + .IsTrue(); + } + + [Test] + public async Task Apply_GivenInternalsGrants_StripsThemFromTheMergedArtifact(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var componentPath = workspace.Compile("Fixture.Component", ComponentWithInternalsGrantSource); + + // Act + var report = FrameworkTypeInternalizer.Apply( + componentPath, + [workspace.GetPath("Fixture.Component")], + ownedNamespaces: ["Purview.SourceGeneratorFramework"] + ); + + // Assert + await Assert.That(report.StrippedInternalsGrantCount).IsEqualTo(1); + + using var component = AssemblyDefinition.ReadAssembly(componentPath); + await Assert + .That( + component.CustomAttributes.Any(static attribute => + attribute.AttributeType.FullName == "System.Runtime.CompilerServices.InternalsVisibleToAttribute" + ) + ) + .IsFalse(); + } + + [Test] + public async Task Apply_GivenGenericConstraintsExposingFrameworkTypes_ReportsThem( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", ConstraintFrameworkSource); + var componentPath = workspace.Compile("Fixture.Component", ConstraintComponentSource, frameworkPath); + List warnings = []; + + // Act + var report = FrameworkTypeInternalizer.Apply( + componentPath, + [workspace.GetPath("Fixture.Component")], + warnings.Add, + ownedNamespaces: ["Purview.SourceGeneratorFramework"] + ); + + // Assert + await Assert + .That(report.PublicMembersExposingFrameworkTypes) + .Contains("Fixture.Component.Constrained`1.generic parameter T"); + await Assert + .That(report.PublicMembersExposingFrameworkTypes) + .Contains("Fixture.Component.ConstrainedMethods.Use"); + await Assert + .That(warnings) + .Contains(static warning => + warning.Contains( + "'Fixture.Component.ConstrainedMethods' is public and exposes", + StringComparison.Ordinal + ) + ); + } + static bool IsFrameworkOwnedNamespace(string? @namespace) => @namespace is not null && ( diff --git a/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/MergeToolRunnerTests.cs b/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/MergeToolRunnerTests.cs index 86dab44..cc06de7 100644 --- a/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/MergeToolRunnerTests.cs +++ b/src/tests/SourceGeneratorFramework.MergeTool.UnitTests/MergeToolRunnerTests.cs @@ -75,6 +75,26 @@ public static class Consumer } """; + /// + /// A component that uses framework types only inside method bodies, so its public surface stays + /// free of framework types and the merge reports no findings for it. + /// + const string InternalUsageComponentSource = """ + namespace Fixture.Component + { + using Fixture.Framework; + + public static class Consumer + { + public static string Describe() + { + var options = new Options { Kind = OptionKind.Enabled }; + return new TypeReference(options.Kind.ToString()).Name; + } + } + } + """; + [Test] public async Task GivenDuplicateIsExternalInit_MergeProducesCanonicalWarningFreeAssembly( CancellationToken cancellationToken @@ -145,8 +165,158 @@ public async Task GivenNoDuplicateIsExternalInit_MergeKeepsExistingCleanPath(Can // Assert await Assert.That(exitCode).IsEqualTo(0); - await Assert.That(logger.Warnings).IsEmpty(); await Assert.That(File.Exists(outputPath)).IsTrue(); + + // ILRepack must stay quiet (this test covers the no-duplicate normalizer path)... + await Assert + .That(logger.Warnings) + .DoesNotContain(static warning => + warning.Contains( + "Method reference is used with definition return type / parameter", + StringComparison.Ordinal + ) + ); + + // ...while the framework identity seeding now makes the fixture framework assembly + // framework-owned, so the component's public member returning a framework type is reported even + // though the fixture framework does not live in the framework namespace. The report is grouped + // by the real declaring type. + await Assert + .That(logger.Warnings) + .Contains(static warning => + warning.Contains("'Fixture.Component.Consumer' is public and exposes", StringComparison.Ordinal) + ); + } + + [Test] + public async Task Run_GivenComponentUsingFrameworkTypesInternally_ReportsNoFindings( + CancellationToken cancellationToken + ) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", FrameworkSource); + var componentPath = workspace.Compile("Fixture.Component", InternalUsageComponentSource, frameworkPath); + var outputPath = workspace.GetPath("merged", "Fixture.Component.dll"); + TestLogger logger = new(); + + // Act + var exitCode = MergeToolRunner.Run( + [componentPath, frameworkPath, outputPath, TestWorkspace.NetStandardReferenceDirectory], + TextWriter.Null, + logger + ); + + // Assert + await Assert.That(exitCode).IsEqualTo(0); + await Assert.That(logger.Warnings).IsEmpty(); + } + + [Test] + public async Task Run_GivenWarningSeverityAndOrigin_WritesMsBuildWarning(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", FrameworkSource); + var componentPath = workspace.Compile("Fixture.Component", ComponentWithoutMarkerSource, frameworkPath); + var outputPath = workspace.GetPath("merged", "Fixture.Component.dll"); + var origin = workspace.GetPath("Fixture.Component", "Component.cs"); + using StringWriter error = new(); + + // Act + var exitCode = MergeToolRunner.Run( + [ + componentPath, + frameworkPath, + outputPath, + TestWorkspace.NetStandardReferenceDirectory, + "--public-surface-severity", + "warning", + "--origin", + origin, + ], + error + ); + + // Assert + await Assert.That(exitCode).IsEqualTo(0); + await Assert.That(error.ToString()).Contains($"{origin} : warning PSGFR41:"); + } + + [Test] + public async Task Run_GivenDefaultSeverity_WritesPlainTextWithoutCode(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", FrameworkSource); + var componentPath = workspace.Compile("Fixture.Component", ComponentWithoutMarkerSource, frameworkPath); + var outputPath = workspace.GetPath("merged", "Fixture.Component.dll"); + using StringWriter error = new(); + + // Act + var exitCode = MergeToolRunner.Run( + [componentPath, frameworkPath, outputPath, TestWorkspace.NetStandardReferenceDirectory], + error + ); + + // Assert + await Assert.That(exitCode).IsEqualTo(0); + await Assert.That(error.ToString()).Contains("is public and exposes"); + await Assert.That(error.ToString()).DoesNotContain("PSGFR41"); + } + + [Test] + public async Task Run_GivenUnknownOption_ReturnsUsageError(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using StringWriter error = new(); + + // Act + var exitCode = MergeToolRunner.Run(["--unknown"], error); + + // Assert + await Assert.That(exitCode).IsEqualTo(2); + await Assert.That(error.ToString()).Contains("Unknown option '--unknown'."); + } + + [Test] + public async Task Tag_GivenConfigurationValues_TracksThemInTheContentTag(CancellationToken cancellationToken) + { + // Arrange + cancellationToken.ThrowIfCancellationRequested(); + using TestWorkspace workspace = new(); + var frameworkPath = workspace.Compile("Fixture.Framework", FrameworkSource); + using StringWriter first = new(); + using StringWriter repeated = new(); + using StringWriter changed = new(); + + // Act + var firstExitCode = MergeToolRunner.Run( + ["--tag", frameworkPath, "--input-value", "namespaces=a"], + TextWriter.Null, + output: first + ); + var repeatedExitCode = MergeToolRunner.Run( + ["--tag", frameworkPath, "--input-value", "namespaces=a"], + TextWriter.Null, + output: repeated + ); + var changedExitCode = MergeToolRunner.Run( + ["--tag", frameworkPath, "--input-value", "namespaces=b"], + TextWriter.Null, + output: changed + ); + + // Assert + await Assert.That(firstExitCode).IsEqualTo(0); + await Assert.That(repeatedExitCode).IsEqualTo(0); + await Assert.That(changedExitCode).IsEqualTo(0); + await Assert.That(first.ToString()).IsEqualTo(repeated.ToString()); + await Assert.That(first.ToString()).IsNotEqualTo(changed.ToString()); } static async Task AssertMergedMetadataAsync(string outputPath)