From 8f96ddf8385ef80eaedb06569badc69d7f78c755 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 14 Aug 2026 12:21:22 -0400 Subject: [PATCH 1/5] Refuse open generic decoration and cross-wiring instead of failing silently Decoration and cross-wiring both replace a registration with a factory, and the container refuses a factory for an open generic service type. Neither path said so, and the two failed in different ways. A generic decorator is expanded against the closed constructions a compilation registers. An open generic registration closes nothing, so the expansion produced no decorations and the declaration was dropped in silence. A non-generic decorator named against an unbound service needed no expansion at all, so nothing caught it: it reached emission carrying IStore<> and produced Decorate>, which is CS7003 inside generated code. [CrossWireService] on a generic type was worse again, leaking the type parameter into typeof(ILedger) beside GetRequiredService>() for CS0246 and CS7003 together. DM0013 covers all three decorator shapes, whichever way the decorator was declared. DM0014 covers cross-wiring, dropping the whole registration rather than the cross-wired half, since keeping the implementation's own registration would leave the instance unreachable through any of its interfaces. A decorator naming a service the compilation does not register stays quiet. Naming a service someone else registers is what [Decorate] exists for, so reporting there would fire on the feature's primary use. HasUnboundServiceType was written to describe the CS7003 case and was never consulted; it is now the guard. DecoratorExpansion.Expand takes an out parameter carrying what it refused, which is a break on the generator extension points. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np5LZpJ2PPStWVcrNXfmD5 --- CHANGELOG.md | 56 +++++ .../Attributes/DecorateAttribute.cs | 7 +- .../AnalyzerReleases.Unshipped.md | 2 + .../DependencyModuleDiagnostics.cs | 60 +++++ .../Utilities/DecoratorExpansion.cs | 60 +++++ .../Conventions/ConventionGenerator.cs | 34 +++ .../ServiceSourceGenerator.cs | 54 +++++ .../GeneratorTests/DiagnosticsTests.cs | 207 ++++++++++++++++++ ...icApiTests.SourceGeneratorApi.verified.txt | 4 +- website/guide/decorators.md | 9 +- website/reference/diagnostics.md | 44 ++++ 11 files changed, 530 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b670754..9b38c24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,61 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- **Two diagnostics for open generics, where the generator previously failed silently or emitted + code that did not compile.** Both cases are the same underlying constraint: decoration and + cross-wiring replace a registration with a factory, and the container refuses a factory for an + open generic service type — ``Open generic service type 'IRepository`1[T]' requires registering an + open generic implementation type``. + + **`DM0013`** reports a decorator whose service is registered as an open generic. It covers all + three shapes of the mistake, which until now failed in two different ways. A *generic* decorator + is expanded against the closed constructions a compilation registers; an open generic registration + closes nothing, so the expansion produced no decorations and the declaration was dropped in + silence — a decorator sitting in the source, a green build, and nothing wrapping anything. A + *non-generic* decorator named against an unbound service needed no expansion at all, so nothing + caught it: it reached emission carrying `IStore<>` and produced `Decorate>`, which is + CS7003 inside generated code. Reported whichever way the decorator was declared, on the class or + on the module with `[Decorate]`. + + A decorator naming a service the compilation does **not** register stays quiet. Naming a service + someone else registers is what `[Decorate]` exists for, so reporting there would fire on the + feature's primary use. + + **`DM0014`** reports `[CrossWireService]` on a generic type. Cross-wiring shares one instance + across the implementation and every interface it declares, which is emitted as a factory per + interface. Registering each interface to the same open generic implementation type would compile + and is a different contract — one instance per service type, the opposite of what the attribute + promises — so it is refused rather than quietly substituted. The whole registration is dropped + rather than the cross-wired half, because keeping the implementation's own registration would + leave the instance unreachable through any of its interfaces. + +### Fixed + +- **Generated code that did not compile.** `[CrossWireService]` on a generic type leaked the type + parameter into the registration as `typeof(ILedger)`, with no `T` in scope, beside + `GetRequiredService>()` — CS0246 and CS7003, in a file the developer did not write. A + non-generic decorator named against an open generic service produced CS7003 and CS1503 the same + way. Both are now `DM0014` and `DM0013`. + +### Changed + +- **`DecorateAttribute`'s documentation said the opposite of the README.** Its `service` parameter + was documented as "may be an open generic", which reads as a service *registered* as one. It means + a generic service named unbound, expanded across the closed constructions the compilation + registers. The decorators guide also described the failure as an `InvalidOperationException` from + `DecoratorHelper`; that guard is unreachable from generated code, because the expansion drops the + decorator before anything is emitted, so the guide now points at `DM0013`. + +- **Breaking, for anyone building on the generator extension points:** + `DecoratorExpansion.Expand` takes an additional `out IReadOnlyList` parameter, + carrying the decorators it refused so the caller can report them. These are the extension points + the convention generator uses and are not a versioned API — see the + [generator guide](https://ipjohnson.github.io/DependencyModules/guide/extending). + ## [1.0.0-rc9230] - 2026-08-12 Everything since `1.0.0-rc9210`. Still a release candidate: convention registration and the NUnit @@ -522,5 +577,6 @@ The entries below were written for a 1.0.0 that was not cut. They describe the s Enable it with ``. - A tag-driven release workflow publishing to nuget.org and GitHub Packages. +[Unreleased]: https://github.com/ipjohnson/DependencyModules/compare/v1.0.0-rc9230...HEAD [1.0.0-rc9230]: https://github.com/ipjohnson/DependencyModules/releases/tag/v1.0.0-rc9230 [1.0.0-rc9210]: https://github.com/ipjohnson/DependencyModules/releases/tag/v1.0.0-rc9210 diff --git a/src/DependencyModules.Runtime/Attributes/DecorateAttribute.cs b/src/DependencyModules.Runtime/Attributes/DecorateAttribute.cs index 5f296f7..6d4313f 100644 --- a/src/DependencyModules.Runtime/Attributes/DecorateAttribute.cs +++ b/src/DependencyModules.Runtime/Attributes/DecorateAttribute.cs @@ -13,7 +13,12 @@ namespace DependencyModules.Runtime.Attributes; /// public partial class DataModule; /// /// -/// The service being decorated. May be an open generic. +/// +/// The service being decorated. A generic service is named unbound — typeof(IHandler<,>) +/// — and the decoration is expanded across the closed constructions the compilation registers. A +/// service registered as an open generic cannot be decorated at all, and is reported as +/// DM0013. +/// /// The decorator, which must implement . [AttributeUsage(AttributeTargets.Class, AllowMultiple = true, Inherited = false)] public class DecorateAttribute(Type service, Type decorator) : Attribute { diff --git a/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md b/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md index d60d403..2b7ae4a 100644 --- a/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md +++ b/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md @@ -17,3 +17,5 @@ DM0009 | DependencyModules | Error | A convention declaration could not be read. DM0010 | DependencyModules | Info | A service is registered by convention. DM0011 | DependencyModules | Info | A service is registered only when an environment condition holds. DM0012 | DependencyModules | Warning | An environment condition names nothing to test. +DM0013 | DependencyModules | Warning | A service registered as an open generic cannot be decorated. +DM0014 | DependencyModules | Warning | A generic type cannot be cross-wired. diff --git a/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs b/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs index fd5d73e..b72335c 100644 --- a/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs +++ b/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs @@ -213,4 +213,64 @@ public static class DependencyModuleDiagnostics { defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true); + /// + /// Raised for a decorator whose service is registered as an open generic. + /// + /// + /// Decoration replaces a registration with a factory, and the container refuses a factory for an + /// open generic service type — "requires registering an open generic implementation type". So + /// there is nothing to emit, and both shapes of the mistake failed badly until this existed. + /// + /// A generic decorator is expanded against the closed constructions a compilation + /// registers. An open generic registration closes nothing, so the expansion produced no + /// decorations and the declaration was dropped in silence — a build with a decorator in it that + /// never runs. + /// + /// A non-generic decorator named against an unbound service is worse: it needs no + /// expansion, so it reached emission carrying IHolder<> and produced + /// Decorate<IHolder<>> — CS7003 inside generated code, which is the one failure + /// mode this generator exists to avoid. + /// + /// Registering closed constructions is the way through, and the message says so. + /// + public static readonly DiagnosticDescriptor OpenGenericCannotBeDecorated = new( + id: "DM0013", + title: "Open generic registration cannot be decorated", + messageFormat: + "'{0}' is registered as an open generic, so '{1}' cannot decorate it. Decoration replaces a " + + "registration with a factory, and the container does not allow one for an open generic " + + "service type. Register closed constructions of '{0}' instead — a convention over the open " + + "generic registers one per implementation, and a generic decorator is then expanded across them.", + category: Category, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true); + + /// + /// Raised for [CrossWireService] on a generic type. + /// + /// + /// Cross-wiring means one instance shared across the implementation and every interface it + /// declares, which is emitted as s => s.GetRequiredService<T>() per interface — a + /// factory, and a factory is what an open generic registration cannot have. + /// + /// Registering each interface to the same open generic implementation type compiles, and is a + /// different contract: the container builds one instance per service type, which is the opposite + /// of what the attribute promises. Silently substituting that would be worse than refusing. + /// + /// Until this existed the generated code did not compile at all — the type parameter leaked into + /// the registration as typeof(ILedger<T>) (CS0246, no T in scope) beside + /// GetRequiredService<Ledger<>>() (CS7003). + /// + public static readonly DiagnosticDescriptor CrossWireCannotBeGeneric = new( + id: "DM0014", + title: "Generic type cannot be cross-wired", + messageFormat: + "'{0}' is generic, so [CrossWireService] cannot register it. Cross-wiring shares one instance " + + "across every service type, which needs a factory, and the container does not allow one for an " + + "open generic registration. Use [SingletonService], [ScopedService] or [TransientService] to " + + "register it, applying one per interface if it needs to answer to more than one.", + category: Category, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true); + } diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/DecoratorExpansion.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/DecoratorExpansion.cs index dca92a6..c9f1a75 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/DecoratorExpansion.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/DecoratorExpansion.cs @@ -22,13 +22,20 @@ namespace DependencyModules.SourceGenerator.Impl.Utilities; /// public static class DecoratorExpansion { + /// + /// Decorators that name a service the compilation registers as an open generic. Nothing can be + /// emitted for those — see — and the caller reports + /// DM0013 rather than letting them disappear. + /// public static IReadOnlyList Expand( IReadOnlyList decorators, IReadOnlyList registeredServiceTypes, + out IReadOnlyList refusedForOpenGenericRegistration, bool includeNonGeneric = true, Func? canClose = null) { var expanded = new List(decorators.Count); + List? refused = null; foreach (var decorator in decorators) { if (decorator.IsIgnored) { @@ -36,6 +43,17 @@ public static IReadOnlyList Expand( } if (!decorator.IsOpenGeneric) { + // An unbound service type has no legal emission at all: Decorate> is + // CS7003. A generic decorator reaches this state only when nothing closed it, and is + // handled below; a non-generic one never had an expansion step to catch it, so this + // is where it stops. Refused whatever is registered, because the emission is invalid + // on its own terms. + if (decorator.HasUnboundServiceType) { + (refused ??= new List()).Add(decorator); + + continue; + } + // A non-generic decorator names one service type and needs no expansion, so only // the pass that owns the declaration emits it. Anything that cannot be constructed // by generated code is dropped: generated code builds the decorator with a literal @@ -48,6 +66,8 @@ public static IReadOnlyList Expand( continue; } + var closedCount = 0; + foreach (var serviceType in registeredServiceTypes) { if (!ClosesTheSameGeneric(serviceType, decorator.ServiceType)) { continue; @@ -68,12 +88,52 @@ public static IReadOnlyList Expand( } expanded.Add(closed); + closedCount++; + } + + // Nothing closed it. Distinguishing the two reasons is the whole point: a compilation + // that registers the service as an open generic can never be decorated and is worth + // reporting, while a compilation that registers nothing at all is the ordinary + // cross-assembly case — [Decorate] exists to name a service someone else registers, so + // reporting that would fire on the feature's primary use. + if (closedCount == 0 && NamesAnOpenGenericRegistration(decorator, registeredServiceTypes)) { + (refused ??= new List()).Add(decorator); } } + refusedForOpenGenericRegistration = (IReadOnlyList?)refused ?? Array.Empty(); + return expanded; } + /// + /// Whether this compilation registers the decorated service as an open generic. + /// + /// + /// Matched on name, namespace and arity, with every type argument blank — the form + /// services.AddSingleton(typeof(IStore<>), typeof(Store<>)) produces. + /// + private static bool NamesAnOpenGenericRegistration( + DecoratorModel decorator, IReadOnlyList registeredServiceTypes) { + + if (decorator.ServiceType is not GenericTypeDefinition decorated) { + return false; + } + + foreach (var registered in registeredServiceTypes) { + if (registered is GenericTypeDefinition open && + open.TypeArguments.Count == decorated.TypeArguments.Count && + open.Name == decorated.Name && + open.Namespace == decorated.Namespace && + open.TypeArguments.All(argument => string.IsNullOrEmpty(argument.Name))) { + + return true; + } + } + + return false; + } + /// /// Whether a registered service type is a closed construction of the decorated open generic. /// diff --git a/src/DependencyModules.SourceGenerator/Conventions/ConventionGenerator.cs b/src/DependencyModules.SourceGenerator/Conventions/ConventionGenerator.cs index 94fb9f8..89177dc 100644 --- a/src/DependencyModules.SourceGenerator/Conventions/ConventionGenerator.cs +++ b/src/DependencyModules.SourceGenerator/Conventions/ConventionGenerator.cs @@ -355,10 +355,13 @@ private static void WriteDecorators( var expanded = DecoratorExpansion.Expand( decorators, registeredServiceTypes, + out var refusedForOpenGenericRegistration, canClose: (decoratorType, closedService) => DecoratorConstraintChecker.CanClose( moduleDecorators.Compilation, decoratorType, closedService)); + ReportOpenGenericDecoration(context, refusedForOpenGenericRegistration, logger); + if (expanded.Count == 0) { return; } @@ -428,6 +431,37 @@ private static IReadOnlyList CollectDecorators( return decorators; } + /// + /// Reports the decorators that were dropped because their service is registered as an open + /// generic. + /// + /// + /// The expansion cannot produce anything for these, and until this existed the two shapes failed + /// differently and both badly: a generic decorator vanished with a green build, and a non-generic + /// one reached emission carrying an unbound service type and produced CS7003 in generated code. + /// + private static void ReportOpenGenericDecoration( + SourceProductionContext context, + IReadOnlyList refused, + FileLogger logger) { + + foreach (var decorator in refused) { + var serviceName = decorator.ServiceType.Name; + var decoratorName = decorator.DecoratorType.Name; + + logger.Error( + $"'{decoratorName}' cannot decorate '{serviceName}' because it is registered as an " + + "open generic."); + + context.ReportDiagnostic( + Diagnostic.Create( + DependencyModuleDiagnostics.OpenGenericCannotBeDecorated, + Location.None, + serviceName, + decoratorName)); + } + } + /// /// Two decorators of one service sharing an order nest in an order nobody declared, so it is /// reported rather than resolved arbitrarily. diff --git a/src/DependencyModules.SourceGenerator/ServiceSourceGenerator.cs b/src/DependencyModules.SourceGenerator/ServiceSourceGenerator.cs index 8d39c5e..b356e56 100644 --- a/src/DependencyModules.SourceGenerator/ServiceSourceGenerator.cs +++ b/src/DependencyModules.SourceGenerator/ServiceSourceGenerator.cs @@ -40,6 +40,8 @@ protected override void GenerateSourceOutput(SourceProductionContext context, var serviceModels = ReportUnconstructableServices(context, inputData.Right, logger); + serviceModels = ReportCrossWiredGenerics(context, serviceModels, logger); + if (serviceModels.Length == 0) { return; } @@ -160,6 +162,58 @@ private static ImmutableArray ReportUnconstructableServices( return builder.ToImmutable(); } + /// + /// Reports cross-wired generic types and removes them from generation. + /// + /// + /// Cross-wiring shares one instance across every service type, which is emitted as a factory per + /// interface. An open generic registration cannot carry a factory, so the emission was invalid on + /// its face: the type parameter leaked into the registration as typeof(ILedger<T>) + /// and the factory read GetRequiredService<Ledger<>>(), giving CS0246 and + /// CS7003 in generated code. + /// + /// The whole model is dropped rather than the cross-wired registrations alone. Keeping the + /// implementation's own registration would honour half an attribute — the instance would no + /// longer be reachable through any of its interfaces, which is the entire reason the attribute + /// was written. + /// + private static ImmutableArray ReportCrossWiredGenerics( + SourceProductionContext context, ImmutableArray serviceModels, FileLogger logger) { + + if (!serviceModels.Any(IsCrossWiredGeneric)) { + return serviceModels; + } + + var builder = ImmutableArray.CreateBuilder(serviceModels.Length); + + foreach (var serviceModel in serviceModels) { + if (!IsCrossWiredGeneric(serviceModel)) { + builder.Add(serviceModel); + + continue; + } + + var typeName = serviceModel.ImplementationType.Name; + + logger.Error($"Skipping '{typeName}' because a generic type cannot be cross-wired."); + + context.ReportDiagnostic( + Diagnostic.Create( + DependencyModuleDiagnostics.CrossWireCannotBeGeneric, + Location.None, + typeName)); + } + + return builder.ToImmutable(); + } + + /// + /// A cross-wired registration on an implementation that is itself generic. + /// + private static bool IsCrossWiredGeneric(ServiceModel serviceModel) => + serviceModel.ImplementationType is GenericTypeDefinition { TypeArguments.Count: > 0 } && + serviceModel.Registrations.Any(registration => registration.CrossWire == true); + /// /// Reports what each conditional registration depends on, and refuses conditions that name /// nothing to test. diff --git a/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs b/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs index cd22f73..0b63b5e 100644 --- a/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs +++ b/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs @@ -158,6 +158,213 @@ public partial class TestModule; Assert.DoesNotContain(result.GeneratorDiagnostics, d => d.Id == "DM0002"); } + /// + /// A generic decorator declared on the class, over a service registered as an open generic. The + /// expansion has no closed construction to close over, so the declaration used to disappear with + /// a green build — a decorator in the source that never ran. + /// + [Fact] + public void GenericDecoratorOverOpenGenericRegistration_ReportsDM0013() { + var result = GeneratorTestHarness.Run( + OpenGenericStore( + """ + [Decorator] + public class LoggingStore(IStore inner) : IStore { + public string Read(T key) => inner.Read(key); + } + """)); + + var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0013"); + + Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); + Assert.Contains("IStore", diagnostic.GetMessage()); + Assert.Contains("LoggingStore", diagnostic.GetMessage()); + } + + /// + /// The same service, decorated from the module instead. Both declaration forms reach the same + /// expansion, so both have to report. + /// + [Fact] + public void ModuleDeclaredDecoratorOverOpenGenericRegistration_ReportsDM0013() { + var result = GeneratorTestHarness.Run( + OpenGenericStore( + """ + public class LoggingStore(IStore inner) : IStore { + public string Read(T key) => inner.Read(key); + } + """, + moduleAttributes: "[Decorate(typeof(IStore<>), typeof(LoggingStore<>))]")); + + Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0013"); + } + + /// + /// A non-generic decorator named against an unbound service type. This one needed no + /// expansion, so nothing caught it and it reached emission still carrying IStore<> — + /// which is CS7003 in generated code. + /// + [Fact] + public void NonGenericDecoratorOverOpenGenericRegistration_ReportsDM0013() { + var result = GeneratorTestHarness.Run( + OpenGenericStore( + """ + public class StringStoreDecorator(IStore inner) : IStore { + public string Read(string key) => inner.Read(key); + } + """, + moduleAttributes: "[Decorate(typeof(IStore<>), typeof(StringStoreDecorator))]")); + + Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0013"); + } + + /// + /// And the generated code compiles, which it did not before: GeneratedAssembly.Create + /// asserts the compilation is clean and emits. + /// + [Fact] + public void NonGenericDecoratorOverOpenGenericRegistration_StillCompiles() { + var generated = GeneratedAssembly.Create( + OpenGenericStore( + """ + public class StringStoreDecorator(IStore inner) : IStore { + public string Read(string key) => inner.Read(key); + } + """, + moduleAttributes: "[Decorate(typeof(IStore<>), typeof(StringStoreDecorator))]")); + + Assert.Contains(generated.Services, d => d.ServiceType == generated.Type("IStore`1")); + } + + /// + /// The case that must keep working: closed registrations, which a generic decorator is expanded + /// across. This is the shape a MediatR-style pipeline is built from. + /// + [Fact] + public void GenericDecoratorOverClosedRegistrations_DoesNotReportDM0013() { + var result = GeneratorTestHarness.Run( + """ + using DependencyModules.Runtime.Attributes; + + namespace TestNamespace; + + public interface IStore { string Read(T key); } + + [SingletonService] public class IntStore : IStore { public string Read(int key) => "int"; } + [SingletonService] public class StringStore : IStore { public string Read(string key) => "string"; } + + [Decorator] + public class LoggingStore(IStore inner) : IStore { + public string Read(T key) => inner.Read(key); + } + + [DependencyModule] + public partial class TestModule; + """); + + Assert.DoesNotContain(result.GeneratorDiagnostics, d => d.Id == "DM0013"); + } + + /// + /// A decorator naming a service this compilation does not register at all stays quiet. Naming a + /// service someone else registers is what [Decorate] is for, so reporting here would fire + /// on the feature's primary use. + /// + [Fact] + public void DecoratorForAServiceThisCompilationDoesNotRegister_DoesNotReportDM0013() { + var result = GeneratorTestHarness.Run( + """ + using DependencyModules.Runtime.Attributes; + + namespace TestNamespace; + + public interface IElsewhere { string Read(T key); } + + public class LoggingElsewhere(IElsewhere inner) : IElsewhere { + public string Read(T key) => inner.Read(key); + } + + [DependencyModule] + [Decorate(typeof(IElsewhere<>), typeof(LoggingElsewhere<>))] + public partial class TestModule; + """); + + Assert.DoesNotContain(result.GeneratorDiagnostics, d => d.Id == "DM0013"); + } + + /// + /// Cross-wiring shares one instance across every service type, which needs a factory — and an + /// open generic registration cannot have one. The emission was invalid on its face: the type + /// parameter leaked into typeof(ILedger<T>) beside + /// GetRequiredService<Ledger<>>(). + /// + [Fact] + public void CrossWiredGenericType_ReportsDM0014() { + var result = GeneratorTestHarness.Run(CrossWiredLedger("public class Ledger : ILedger, IAudit;")); + + var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0014"); + + Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); + Assert.Contains("Ledger", diagnostic.GetMessage()); + } + + [Fact] + public void CrossWiredGenericType_IsNotRegistered() { + var result = GeneratorTestHarness.Run(CrossWiredLedger("public class Ledger : ILedger, IAudit;")); + + Assert.DoesNotContain(result.GeneratedSources.Keys, key => key.Contains("Dependencies")); + } + + /// + /// Cross-wiring a non-generic type is untouched, and still shares one instance across both + /// interfaces. + /// + [Fact] + public void CrossWiredNonGenericType_StillRegisters() { + var generated = GeneratedAssembly.Create(CrossWiredLedger("public class Ledger : ILedger, IAudit;")); + + var provider = generated.BuildProvider(); + + // The point of cross-wiring: both interfaces answer with the one instance. + Assert.Same( + provider.GetService(generated.Type("ILedger`1").MakeGenericType(typeof(int))), + provider.GetService(generated.Type("IAudit`1").MakeGenericType(typeof(int)))); + } + + private static string OpenGenericStore(string body, string moduleAttributes = "") => + $$""" + using DependencyModules.Runtime.Attributes; + + namespace TestNamespace; + + public interface IStore { string Read(T key); } + + [SingletonService] + public class Store : IStore { public string Read(T key) => "store"; } + + {{body}} + + [DependencyModule] + {{moduleAttributes}} + public partial class TestModule; + """; + + private static string CrossWiredLedger(string implementation) => + $$""" + using DependencyModules.Runtime.Attributes; + + namespace TestNamespace; + + public interface ILedger; + public interface IAudit; + + [CrossWireService] + {{implementation}} + + [DependencyModule] + public partial class TestModule; + """; + private static string Module(string body) => $$""" using DependencyModules.Runtime.Attributes; diff --git a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt index 0ad69bc..88eefc2 100644 --- a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt +++ b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt @@ -1059,10 +1059,12 @@ namespace DependencyModules.SourceGenerator.Impl public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ConventionCannotBeRead; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ConventionMatchNotConstructable; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ConventionMatchedNothing; + public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor CrossWireCannotBeGeneric; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor EmptyEnvironmentCondition; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ExposedByConvention; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor GeneratorFailure; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ModuleMustBePartial; + public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor OpenGenericCannotBeDecorated; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor RegisteredConditionally; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ServiceCannotBeConstructed; } @@ -1565,7 +1567,7 @@ namespace DependencyModules.SourceGenerator.Impl.Utilities } public static class DecoratorExpansion { - public static System.Collections.Generic.IReadOnlyList Expand(System.Collections.Generic.IReadOnlyList decorators, System.Collections.Generic.IReadOnlyList registeredServiceTypes, bool includeNonGeneric = true, System.Func? canClose = null) { } + public static System.Collections.Generic.IReadOnlyList Expand(System.Collections.Generic.IReadOnlyList decorators, System.Collections.Generic.IReadOnlyList registeredServiceTypes, out System.Collections.Generic.IReadOnlyList refusedForOpenGenericRegistration, bool includeNonGeneric = true, System.Func? canClose = null) { } } public static class DecoratorModelUtility { diff --git a/website/guide/decorators.md b/website/guide/decorators.md index afb54af..2e56973 100644 --- a/website/guide/decorators.md +++ b/website/guide/decorators.md @@ -171,12 +171,11 @@ public class Repository : IRepository { } // registers IRepository<> its public class CachingRepository(IRepository inner) : IRepository { } ``` -``` -InvalidOperationException: 'IRepository`1' is registered as an open generic and cannot be -decorated by 'CachingRepository`1'. … -``` +This is [DM0013](/reference/diagnostics#dm0013) at build time, whichever way the decorator was +declared — on the class, or on the module with `[Decorate]`. -Register closed constructions instead. +Register closed constructions instead. A [convention](/guide/conventions) over the open generic +registers one per implementation, and an open generic decorator is then expanded across them. Note that this is about the **registration**, not the decorator. An open generic decorator over closed registrations — the example further up — works, and is the common case. diff --git a/website/reference/diagnostics.md b/website/reference/diagnostics.md index 5a78bb2..ce9310f 100644 --- a/website/reference/diagnostics.md +++ b/website/reference/diagnostics.md @@ -28,6 +28,8 @@ them if the IDE gets noisy; the rest are worth reading. | [DM0010](#dm0010) | Info | A service is registered by convention | | [DM0011](#dm0011) | Info | A service is registered only when a condition holds | | [DM0012](#dm0012) | Warning | An environment condition names nothing to test | +| [DM0013](#dm0013) | Warning | A service registered as an open generic cannot be decorated | +| [DM0014](#dm0014) | Warning | A generic type cannot be cross-wired | ## DM0001 {#dm0001} @@ -123,3 +125,45 @@ Informational, reported at the class. See [Environments](/guide/environments). `[IfEnvironment()]` and `[IfEnvironmentValue("")]` both compile. Written plain they mean the service never registers; written as the `IfNot` form they mean the attribute does nothing at all. + +## DM0013 {#dm0013} + +**A service registered as an open generic cannot be decorated.** + +Decoration replaces a registration with a factory, and the container does not allow one for an open +generic service type — `Open generic service type 'IRepository`1[T]' requires registering an open +generic implementation type`. + +```csharp +[SingletonService] +public class Repository : IRepository { } // registers IRepository<> itself + +[Decorator] +public class CachingRepository(IRepository inner) : IRepository { } // DM0013 +``` + +Reported whichever way the decorator was declared — on the class, or on the module with +`[Decorate]` — and whether or not the decorator is itself generic. + +Register closed constructions instead. A [convention](/guide/conventions) over the open generic +registers one per implementation, and an open generic decorator is expanded across them. See +[Decorators](/guide/decorators#one-limitation). + +## DM0014 {#dm0014} + +**A generic type cannot be cross-wired.** + +`[CrossWireService]` shares one instance across the implementation and every interface it declares, +which is emitted as a factory per interface — and an open generic registration cannot carry one. + +```csharp +[CrossWireService] +public class Ledger : ILedger, IAudit { } // DM0014 +``` + +Registering each interface to the same open generic implementation type would compile, and is a +different contract: the container builds one instance per service type, which is the opposite of what +the attribute promises. + +Use `[SingletonService]`, `[ScopedService]` or `[TransientService]` instead, applying one per +interface if the type needs to answer to more than one. From fb9a5a695aee24e06c4e0da6b2f97463ccd74cd2 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 14 Aug 2026 12:21:40 -0400 Subject: [PATCH 2/5] Stamp 1.0.0-rc9330 and write its changelog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Unreleased section holds one change — the open generic refusals — so it is stamped as rc9330 rather than left to grow. VersionSuffix is the fallback local and CI builds use. A release still takes its version from the tag, so cutting rc9330 means pushing v1.0.0-rc9330. Assembly and file versions carry no prerelease part and are unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np5LZpJ2PPStWVcrNXfmD5 --- CHANGELOG.md | 11 +++++++++-- Directory.Build.props | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b38c24..1070af0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,14 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [1.0.0-rc9330] - 2026-08-14 + +Everything since `1.0.0-rc9230`, and a small release: the generator refuses two things it used to +get wrong about open generics, rather than dropping a declaration in silence or emitting code that +does not compile. No registration that built correctly before builds differently now. + +Still a release candidate. `DecoratorExpansion.Expand` changed shape, but only on the generator +extension points, which are documented as unversioned. ### Added @@ -577,6 +584,6 @@ The entries below were written for a 1.0.0 that was not cut. They describe the s Enable it with ``. - A tag-driven release workflow publishing to nuget.org and GitHub Packages. -[Unreleased]: https://github.com/ipjohnson/DependencyModules/compare/v1.0.0-rc9230...HEAD +[1.0.0-rc9330]: https://github.com/ipjohnson/DependencyModules/releases/tag/v1.0.0-rc9330 [1.0.0-rc9230]: https://github.com/ipjohnson/DependencyModules/releases/tag/v1.0.0-rc9230 [1.0.0-rc9210]: https://github.com/ipjohnson/DependencyModules/releases/tag/v1.0.0-rc9210 diff --git a/Directory.Build.props b/Directory.Build.props index 2f99ec5..98a1848 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -11,7 +11,7 @@ The release candidate label. Assembly and file versions carry no prerelease part, so they stay put until the version they do carry changes. --> - rc9230 + rc9330 1.0.0.0 1.0.0.0 From d38e486f17dd28ea064f76935d13fefa3f5c02a7 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 14 Aug 2026 12:56:07 -0400 Subject: [PATCH 3/5] Recognise every attribute spelling, and stop Replace losing to the alphabet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four registrations that were silently not happening, and the diagnostics for two more that were refused without saying so. Attribute usages were compared as written, so a namespace-qualified spelling, a global:: prefix or a using alias missed and the class went unregistered with no diagnostic at all. Service attributes now resolve through the semantic model; module attributes match on the name the usage ends in, which covers every qualified form. A using alias of a module attribute is still not seen, because a predicate that must stay syntax-only cannot resolve one, and that case surfaces as CS0311 rather than silence. Replace and Try act on a registration that has to already be there, and registrations are emitted sorted by implementation type name. Named so the sort put them first they ran before their target existed, so Replace replaced nothing, added itself, and lost to the registration it meant to displace. They are emitted after the plain Add registrations now, the rule that already put conditional registrations last. [Mock] registered its double unkeyed even when the parameter carried [FromKeyedServices], leaving the keyed registration the consumer injects untouched: the test held a double it believed was wired in while the service under test kept the real implementation. DM0015 reports an interceptor absent from members whose shape it cannot serve. The sharpest form — an IInterceptor on an all-async service, where it never runs — was invisible to the generator too, which discarded the model before anything could report on it. DM0008 now says that one unsupported member costs the whole interface, which the guide had implied otherwise. CopyToOutputDirectory on a Compile item copies the source, and the metadata flowed to every referencing project: 50 files of generator internals in consumers' bin and publish output, this repository's own benchmarks included. The NuGet package was never affected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np5LZpJ2PPStWVcrNXfmD5 --- CHANGELOG.md | 111 +++++++++++++-- README.md | 8 +- integ-tests/Directory.Build.props | 16 +++ .../AnalyzerReleases.Unshipped.md | 1 + .../DependencyFileWriter.cs | 49 ++++++- .../DependencyModuleDiagnostics.cs | 38 +++++- .../Utilities/AttributeTypeMatcher.cs | 97 ++++++++++++++ .../Utilities/InterceptorModelUtility.cs | 10 +- .../Utilities/ServiceModelUtility.cs | 11 +- .../Utilities/SyntaxSelector.cs | 46 +++++-- .../DependencyModules.SourceGenerator.csproj | 4 +- .../InterceptorSourceGenerator.cs | 77 +++++++++++ .../Attributes/MockAttribute.cs | 27 +++- .../GeneratorTests/DiagnosticsTests.cs | 126 ++++++++++++++++++ .../GeneratorTests/GeneratedBehaviourTests.cs | 31 +++++ ...ests.RegistrationTypeVariants.verified.txt | 10 +- ...icApiTests.SourceGeneratorApi.verified.txt | 5 + .../TestParameterResolverTests.cs | 69 ++++++++++ website/guide/getting-started.md | 11 ++ website/guide/interception.md | 37 ++++- website/guide/modules.md | 14 ++ website/guide/troubleshooting.md | 20 +++ website/reference/attributes.md | 5 + website/reference/diagnostics.md | 58 +++++++- website/reference/msbuild.md | 32 ++++- 25 files changed, 863 insertions(+), 50 deletions(-) create mode 100644 src/DependencyModules.SourceGenerator.Impl/Utilities/AttributeTypeMatcher.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1070af0..847f867 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,20 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [1.0.0-rc9330] - 2026-08-14 -Everything since `1.0.0-rc9230`, and a small release: the generator refuses two things it used to -get wrong about open generics, rather than dropping a declaration in silence or emitting code that -does not compile. No registration that built correctly before builds differently now. +Everything since `1.0.0-rc9230`. The theme is registrations that were silently not happening: an +attribute the generator declined to recognise, a `Replace` that depended on how a class was named, a +mock that replaced the wrong slot, and three shapes it refused without saying so. Still a release candidate. `DecoratorExpansion.Expand` changed shape, but only on the generator extension points, which are documented as unversioned. +**Upgrade note:** three new warnings. A project building with `TreatWarningsAsErrors` may go red on +work that was previously green and quietly not doing anything — which is the point of them, but it is +a build break rather than a nudge. `NoWarn` takes them per-project; note that `.editorconfig` does not +(see below). + ### Added -- **Two diagnostics for open generics, where the generator previously failed silently or emitted - code that did not compile.** Both cases are the same underlying constraint: decoration and - cross-wiring replace a registration with a factory, and the container refuses a factory for an - open generic service type — ``Open generic service type 'IRepository`1[T]' requires registering an - open generic implementation type``. +- **Three diagnostics for shapes the generator used to refuse without saying so.** The first two share + a constraint: decoration and cross-wiring replace a registration with a factory, and the container + refuses a factory for an open generic service type — ``Open generic service type 'IRepository`1[T]' + requires registering an open generic implementation type``. The third is about an interceptor that + does not run. **`DM0013`** reports a decorator whose service is registered as an open generic. It covers all three shapes of the mistake, which until now failed in two different ways. A *generic* decorator @@ -44,8 +49,64 @@ extension points, which are documented as unversioned. rather than the cross-wired half, because keeping the implementation's own registration would leave the instance unreachable through any of its interfaces. + **`DM0015`** reports an interceptor that is quietly absent from some of the members it was applied + to. Three interfaces cover the member shapes and the generator picks per member, so an interceptor + implementing none of the one a member needs was simply left out of that member's chain. An + argument-rewriting interceptor stopped rewriting; read as an authorisation or audit gate, it was a + service that quietly was not gated. The sharpest form — an `IInterceptor` applied to a service whose + members are all async, where it never ran at all — was invisible even to the generator, which + discarded the model before anything could report on it. Reported once per interceptor and member + shape, so a wide interface produces one line rather than forty. + ### Fixed +- **An attribute the generator declined to recognise, depending on how it was spelled.** Attribute + usages were compared as written — the type's simple name, and that name with `Attribute` appended — + so every other legal spelling missed, and missing meant the registration was silently absent: no + diagnostic, a green build, and a failure at the first resolve. + + | Written as | Before | Now | + |---|---|---| + | `[SingletonService]` | registered | registered | + | `[DependencyModules.Runtime.Attributes.SingletonService]` | **skipped** | registered | + | `[global::DependencyModules.Runtime.Attributes.SingletonServiceAttribute]` | **skipped** | registered | + | `[DmAttrs.SingletonService]` (namespace alias) | **skipped** | registered | + | `[DmSingleton]` (type alias) | **skipped** | registered | + + Service attributes are now resolved through the semantic model rather than string-matched, which is + what makes an alias and a qualified name mean the same thing. Module attributes are matched on the + name the usage ends in, so every qualified form works there too; a `using` alias of a *module* + attribute is still not seen, because a predicate that must stay syntax-only cannot resolve one — and + that case fails as a `CS0311` at `AddModule()` rather than silently. + +- **`Using = Replace` and `Using = Try` decided by the alphabet.** Registrations within a module are + emitted sorted by implementation type name, and both act *on* a registration that has to already be + there. Named so that the sort put them first, they ran before their target existed: `Replace` + replaced nothing, added itself, and was then beaten by the very registration it meant to displace. + + ```csharp + [SingletonService(Using = RegistrationType.Replace)] public class AaaThing : IThing; + [SingletonService] public class ZzzThing : IThing; + // asked for AaaThing; got [AaaThing, ZzzThing], and ZzzThing won + ``` + + They are now emitted after the plain `Add` registrations in their group, the same rule that already + put conditional registrations last so the override pattern works. Renaming the class was the + previous workaround, and nothing said you needed it. + +- **`[Mock]` ignored `[FromKeyedServices]` on the same parameter.** The double was registered + unkeyed, leaving the keyed registration — the one a consumer injects — untouched. The service under + test kept the real implementation while the test held a double it believed was wired in: the + arrangement ran, the double recorded nothing, and the assertion failed somewhere else entirely. The + key on the parameter is now the key the double is registered under, and read back from. Identical + under NSubstitute, Moq and FakeItEasy. + +- **The generator copied 50 of its own source files into consuming projects' output.** + `CopyToOutputDirectory` on a `Compile` item copies the *source*, and the metadata flowed to every + project referencing the analyzer — 428KB of generator internals in `bin` and in `publish`. It + affected `ProjectReference` consumers and this repository's own `benchmarks/` and test output; the + NuGet package, which ships only `analyzers/dotnet/cs` and `build/`, was never affected. + - **Generated code that did not compile.** `[CrossWireService]` on a generic type leaked the type parameter into the registration as `typeof(ILedger)`, with no `T` in scope, beside `GetRequiredService>()` — CS0246 and CS7003, in a file the developer did not write. A @@ -54,6 +115,40 @@ extension points, which are documented as unversioned. ### Changed +- **`DM0008` now says what it costs.** One member the wrapper cannot override means *no* wrapper is + generated, so every other member on the interface goes uninterceped too — and the guide read as + though only the offending member did. A reader who fixed the named member and rebuilt would then + meet the next one. The message and the interception guide both say so now. + +- **Documentation corrections, each with a reproduction behind it.** The README's duplicate-module + example compared `module.someString` against a primary constructor parameter, which is captured + rather than a member — `CS1061`, and it was the only documented way to load a module more than + once. `ExcludeGeneratedCodeFromCoverage` was documented with a `DependencyModules_` prefix it does + not have. Getting started did not mention that a console app or class library needs + `Microsoft.Extensions.DependencyInjection` for `ServiceCollection` and `BuildServiceProvider`. + `[Decorator]`'s `Realm` property was undocumented. + +- **`DM####` diagnostics cannot be tuned through `.editorconfig`, and the reference said they could.** + They are reported by a source generator rather than an analyzer, and Roslyn's `.editorconfig` + severity mapping applies to analyzer diagnostics — so `dotnet_diagnostic.DM0005.severity = none` had + no effect. `NoWarn`, `WarningsAsErrors` and `#pragma warning disable` are applied at the compilation + level and do work; the reference now says that instead. + +- **Two traps are written down rather than left to be discovered.** + `DependencyModules_GenerateFactories` emits a factory per registration, which + `Microsoft.Extensions.DependencyInjection` cannot see inside — so it silently disables + `ValidateOnBuild` and `ValidateScopes` for the whole project, measured on the same captive + dependency with only that property differing. And an assembly declaring two modules that neither set + `OnlyRealm` puts the whole registration list in both, so loading both in one `AddModules` call + registers everything twice. + +- **`DependencyModules_*` properties are invisible over a `ProjectReference`.** They reach the + generator through `build/DependencyModules.SourceGenerator.targets`, which ships inside the NuGet + package, so a project referencing the analyzer as a project never imports it and every property + silently takes its default — `DependencyModules_LogOutputDirectory` included, producing no log and + no message. Troubleshooting now says so and gives the `CompilerVisibleProperty` block, and this + repository's own integration projects declare it. + - **`DecorateAttribute`'s documentation said the opposite of the README.** Its `service` parameter was documented as "may be an open generic", which reads as a service *registered* as one. It means a generic service named unbound, expanded across the closed constructions the compilation diff --git a/README.md b/README.md index 94a2e4c..9033ec0 100644 --- a/README.md +++ b/README.md @@ -174,11 +174,15 @@ public partial class CustomModule(string someString) : IServiceCollectionConfigu // custom logic } - public override bool Equals(object obj) + // Re-exposed as a member: a primary constructor parameter is captured, not a property, so + // module.someString would not compile. + private string Key => someString; + + public override bool Equals(object? obj) { if (obj is CustomModule module) { - return someString.Equals(module.someString); + return someString.Equals(module.Key); } return false; diff --git a/integ-tests/Directory.Build.props b/integ-tests/Directory.Build.props index c378683..2bd6a43 100644 --- a/integ-tests/Directory.Build.props +++ b/integ-tests/Directory.Build.props @@ -7,4 +7,20 @@ false + + + + + + + + + + diff --git a/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md b/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md index 2b7ae4a..e62eb69 100644 --- a/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md +++ b/src/DependencyModules.SourceGenerator.Impl/AnalyzerReleases.Unshipped.md @@ -19,3 +19,4 @@ DM0011 | DependencyModules | Info | A service is registered only when an environ DM0012 | DependencyModules | Warning | An environment condition names nothing to test. DM0013 | DependencyModules | Warning | A service registered as an open generic cannot be decorated. DM0014 | DependencyModules | Warning | A generic type cannot be cross-wired. +DM0015 | DependencyModules | Warning | An interceptor does not apply to every member it was applied to. diff --git a/src/DependencyModules.SourceGenerator.Impl/DependencyFileWriter.cs b/src/DependencyModules.SourceGenerator.Impl/DependencyFileWriter.cs index 4c0bec2..1bf3fb4 100644 --- a/src/DependencyModules.SourceGenerator.Impl/DependencyFileWriter.cs +++ b/src/DependencyModules.SourceGenerator.Impl/DependencyFileWriter.cs @@ -125,7 +125,7 @@ private string GenerateDependencyMethod(ModuleEntryPointModel entryPointModel, var stringBuilder = new StringBuilder(); - var sortedServiceModels = GetSortedServiceModels(serviceModels); + var sortedServiceModels = GetSortedServiceModels(serviceModels, configurationModel); var autoRegisterGenerators = entryPointModel.RegisterJsonSerializers ?? configurationModel.RegisterSourceGenerator; @@ -570,17 +570,34 @@ private static RegistrationType GetRegistrationType(ModuleEntryPointModel entryP /// because by the time it runs the service type is already registered. That is what Try /// means, and the override pattern wants the default Add. /// + /// + /// Try and Replace are ordered after plain Add within each group for the same + /// reason the conditional key exists: both act on a registration that has to already be + /// there. Ordered by name alone, whether they worked depended on how the two classes happened to + /// be named — a Replace emitted before its target replaced nothing, added itself, and was + /// then beaten by the very registration it meant to displace. Renaming the class fixed it, and + /// nothing said so. + /// /// - private List GetSortedServiceModels(IEnumerable serviceModels) { + private List GetSortedServiceModels( + IEnumerable serviceModels, DependencyModuleConfigurationModel configurationModel) { + var list = new List(serviceModels); list.Sort((x, y) => { var byCondition = IsConditional(x).CompareTo(IsConditional(y)); + if (byCondition != 0) { + return byCondition; + } + + var byStrategy = ActsOnExistingRegistration(x, configurationModel) + .CompareTo(ActsOnExistingRegistration(y, configurationModel)); + // Name is the tie-break rather than the only key, so the order stays total and the // output stays deterministic under List.Sort, which is not stable. - return byCondition != 0 - ? byCondition + return byStrategy != 0 + ? byStrategy : string.Compare(x.ImplementationType.Name, y.ImplementationType.Name, StringComparison.Ordinal); }); @@ -589,4 +606,28 @@ private List GetSortedServiceModels(IEnumerable serv private static bool IsConditional(ServiceModel serviceModel) => serviceModel.Conditions is { Count: > 0 }; + + /// + /// Whether any of a service's registrations only makes sense once its service type is registered. + /// + /// + /// TryEnumerable is deliberately not here. It skips only an identical service-and- + /// implementation pair, so several implementations of one service all register whatever order + /// they arrive in, and deferring it would change nothing. + /// + private static bool ActsOnExistingRegistration( + ServiceModel serviceModel, DependencyModuleConfigurationModel configurationModel) { + + foreach (var registration in serviceModel.Registrations) { + // Null means the registration took the project-wide default, which is what + // DependencyModules_RegistrationType sets. + var registrationType = registration.RegistrationType ?? configurationModel.RegistrationType; + + if (registrationType is RegistrationType.Try or RegistrationType.Replace) { + return true; + } + } + + return false; + } } \ No newline at end of file diff --git a/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs b/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs index b72335c..a003b2b 100644 --- a/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs +++ b/src/DependencyModules.SourceGenerator.Impl/DependencyModuleDiagnostics.cs @@ -131,10 +131,19 @@ public static class DependencyModuleDiagnostics { /// interface, and a few member shapes cannot be forwarded; saying so beats emitting a wrapper /// that does not compile. /// + /// + /// The message names the consequence as well as the cause. One member the wrapper cannot override + /// means no wrapper is generated, so every other member on the interface goes uninterceped + /// too — and the guide read as though only the offending member did. A reader who fixed the named + /// member and rebuilt would then meet the next one. + /// public static readonly DiagnosticDescriptor CannotIntercept = new( id: "DM0008", title: "Service cannot be intercepted", - messageFormat: "This service cannot be intercepted: {0}", + messageFormat: + "This service cannot be intercepted, so no wrapper was generated and none of its members are " + + "intercepted: {0}. Other members may be unsupported for the same reason. Write a decorator " + + "instead, or move the member to an interface that is not intercepted.", category: Category, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true); @@ -213,6 +222,33 @@ public static class DependencyModuleDiagnostics { defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true); + /// + /// Raised for an interceptor that cannot serve some of the members it was applied to. + /// + /// + /// Three interfaces cover the member shapes — IInterceptor for a direct return, + /// IAsyncInterceptor for a task, IAsyncEnumerableInterceptor for a stream — and the + /// generator picks per member. An interceptor that implements none of the one a member needs was + /// simply left out of that member's chain, with nothing said. + /// + /// That is the interceptor silently not running. An argument-rewriting interceptor stops + /// rewriting; read as an authorisation or audit gate, it is a service that quietly is not gated. + /// The sharpest form is an interceptor implementing only IInterceptor applied to a service + /// whose members are all async, where it never runs at all and the build is green. + /// + /// Reported once per interceptor and member shape rather than once per member, so a wide + /// interface produces one line rather than forty. + /// + public static readonly DiagnosticDescriptor InterceptorCannotServeMembers = new( + id: "DM0015", + title: "Interceptor does not apply to every member", + messageFormat: + "'{0}' does not implement '{1}', so it is not applied to {2} on '{3}': {4}. Those members run " + + "without it. Implement '{1}' on the interceptor, or apply it to a service that has no such member.", + category: Category, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true); + /// /// Raised for a decorator whose service is registered as an open generic. /// diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/AttributeTypeMatcher.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/AttributeTypeMatcher.cs new file mode 100644 index 0000000..14e50d9 --- /dev/null +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/AttributeTypeMatcher.cs @@ -0,0 +1,97 @@ +using CSharpAuthor; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace DependencyModules.SourceGenerator.Impl.Utilities; + +/// +/// Whether an attribute usage is a given attribute type, resolved rather than string-matched. +/// +/// +/// +/// Attribute usages were compared as written — attributeSyntax.Name.ToString() against the +/// type's simple name, and against that name with Attribute appended. Every other legal +/// spelling missed, and missing meant the registration was silently absent: no diagnostic, a green +/// build, and a failure at the first resolve. +/// +/// +/// [SingletonService] and [SingletonServiceAttribute] matched. +/// [DependencyModules.Runtime.Attributes.SingletonService], +/// [global::DependencyModules.Runtime.Attributes.SingletonServiceAttribute] and any +/// using alias did not. +/// +/// +/// This belongs in a transform, never in a predicate: it reads the semantic model, which is what +/// resolves an alias and a qualified name to the same symbol, and what a predicate visiting every +/// node in the compilation must not do. +/// +/// +public static class AttributeTypeMatcher { + + /// + /// Whether resolves to . + /// + /// + /// Falls back to comparing the written name when the symbol cannot be resolved, which happens + /// while a file is mid-edit and the attribute does not yet bind. Answering "no" there would make + /// registrations flicker out of the container between keystrokes. + /// + public static bool Matches( + SemanticModel semanticModel, + AttributeSyntax attributeSyntax, + ITypeDefinition attributeType, + CancellationToken cancellationToken) { + + var symbol = Resolve(semanticModel, attributeSyntax, cancellationToken); + + if (symbol == null) { + return MatchesAsWritten(attributeSyntax, attributeType); + } + + return symbol.Name == attributeType.Name && NamespaceOf(symbol) == attributeType.Namespace; + } + + /// + /// The attribute class an attribute usage names. + /// + /// + /// An attribute usage binds to a constructor, so the type is that constructor's containing type. + /// GetTypeInfo answers for the cases where the constructor could not be chosen — an + /// argument list that does not match any overload still names the attribute unambiguously. + /// + private static INamedTypeSymbol? Resolve( + SemanticModel semanticModel, AttributeSyntax attributeSyntax, CancellationToken cancellationToken) { + + var symbolInfo = semanticModel.GetSymbolInfo(attributeSyntax, cancellationToken); + + if (symbolInfo.Symbol?.ContainingType is { } containingType) { + return containingType; + } + + if (symbolInfo.CandidateSymbols.Length > 0 && + symbolInfo.CandidateSymbols[0].ContainingType is { } candidateType) { + return candidateType; + } + + return semanticModel.GetTypeInfo(attributeSyntax, cancellationToken).Type as INamedTypeSymbol; + } + + /// + /// The old comparison, kept only for the unresolvable case. + /// + private static bool MatchesAsWritten(AttributeSyntax attributeSyntax, ITypeDefinition attributeType) { + var written = attributeSyntax.Name.ToString(); + var lastDot = written.LastIndexOf('.'); + + if (lastDot >= 0) { + written = written.Substring(lastDot + 1); + } + + return written == attributeType.Name || written + "Attribute" == attributeType.Name; + } + + private static string NamespaceOf(INamedTypeSymbol symbol) => + symbol.ContainingNamespace is { IsGlobalNamespace: false } containing + ? containing.ToDisplayString() + : ""; +} diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs index f439694..0a5deb1 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs @@ -90,11 +90,11 @@ public static InterceptorModel GetInterceptorModel( } // Nothing here can be placed around anything, so the wrapper would forward every call - // untouched. Not generating one leaves the service registered as it already was. - if (!AnyMemberIsIntercepted(interceptors, members)) { - return InterceptorModel.Ignore; - } - + // untouched, and none is generated. The model is still returned rather than ignored: this is + // the sharpest form of an interceptor that does not run — an interceptor implementing only + // IInterceptor applied to a service whose members are all async never runs at all — and + // returning Ignore here is what kept DM0015 from ever seeing it. The generator drops it after + // reporting. return new InterceptorModel( serviceSymbol.GetTypeDefinition(), ToTypeDefinition(implementationSymbol), diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/ServiceModelUtility.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/ServiceModelUtility.cs index e0badb7..019b700 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/ServiceModelUtility.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/ServiceModelUtility.cs @@ -323,14 +323,17 @@ private static List GetRegistrations(SyntaxTransformCo foreach (var typeDefinition in _attributeTypes) { cancellationToken.ThrowIfCancellationRequested(); - if (attributeSyntax.Name.ToString() == typeDefinition.Name || - attributeSyntax.Name + "Attribute" == typeDefinition.Name) { + // Resolved, not compared as written: a namespace-qualified usage, a global:: prefix + // and a using alias all name the same attribute, and all of them used to be silently + // skipped — leaving the class unregistered with nothing to say so. + if (AttributeTypeMatcher.Matches( + context.SemanticModel, attributeSyntax, typeDefinition, cancellationToken)) { list.Add(GetServiceRegistration(context, attributeSyntax, classDefinition)); } } - if (attributeSyntax.Name.ToString() == _crossWireService.Name || - attributeSyntax.Name + "Attribute" == _crossWireService.Name) { + if (AttributeTypeMatcher.Matches( + context.SemanticModel, attributeSyntax, _crossWireService, cancellationToken)) { list.AddRange(GetCrossWiredService(context, attributeSyntax, classDefinition)); } } diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/SyntaxSelector.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/SyntaxSelector.cs index d370203..a01f5e9 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/SyntaxSelector.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/SyntaxSelector.cs @@ -16,12 +16,22 @@ protected BaseSyntaxSelector(params ITypeDefinition[] attributes) { _names = GetAttributeStrings(attributes); } + /// + /// The bare names an attribute may be written as, with and without the Attribute suffix. + /// + /// + /// Qualification is stripped from the usage rather than enumerated here — see + /// . Listing prefixes was the previous approach and it missed the + /// namespace-qualified form without the suffix, so + /// [DependencyModules.Runtime.Attributes.DependencyModule] — valid C# — was silently not a + /// module: no partial written, no diagnostic, and a CS0311 at the consumer's + /// AddModule<T>() naming neither the attribute nor the omission. + /// private List GetAttributeStrings(ITypeDefinition[] attributes) { var returnList = new List(); foreach (var attribute in attributes) { returnList.Add(attribute.Name); - returnList.Add(attribute.Namespace + "." + attribute.Name); if (attribute.Name.EndsWith(_attributeString)) { var simpleName = attribute.Name.Substring(0, attribute.Name.Length - _attributeString.Length); @@ -33,6 +43,31 @@ private List GetAttributeStrings(ITypeDefinition[] attributes) { return returnList; } + /// + /// An attribute usage reduced to the name it ends in, so every way of qualifying it compares + /// equal. + /// + /// + /// Covers Ns.Attr, global::Ns.Attr and alias::Ns.Attr. It does not cover a + /// using alias of the attribute type itself, which resolves only through the semantic + /// model and so cannot be seen from a predicate that must stay syntax-only. + /// + /// This does not widen what matches by namespace: the bare simple name was already accepted + /// regardless of which namespace it came from, so a same-named attribute from elsewhere was + /// always a candidate and is filtered downstream as it always was. + /// + private static string LastSegment(string attributeName) { + var lastDot = attributeName.LastIndexOf('.'); + + if (lastDot >= 0) { + return attributeName.Substring(lastDot + 1); + } + + var lastColon = attributeName.LastIndexOf(':'); + + return lastColon >= 0 ? attributeName.Substring(lastColon + 1) : attributeName; + } + protected abstract bool TestForTypes(SyntaxNode node, CancellationToken token); public bool Where(SyntaxNode node, CancellationToken token) { @@ -51,10 +86,7 @@ public bool Where(SyntaxNode node, CancellationToken token) { } var found = node.DescendantNodes() - .OfType().Any(a => { - var name = a.Name.ToString(); - return _names.Contains(name); - }); + .OfType().Any(a => _names.Contains(LastSegment(a.Name.ToString()))); return found; } @@ -72,9 +104,7 @@ private bool ProcessAttributeList(SyntaxList attributeLists var foundAttribute = false; foreach (var attributeListSyntax in attributeLists) { foreach (var attributeSyntax in attributeListSyntax.Attributes) { - var name = attributeSyntax.Name.ToString(); - - foundAttribute = _names.Contains(name); + foundAttribute = _names.Contains(LastSegment(attributeSyntax.Name.ToString())); if (foundAttribute) { break; diff --git a/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj b/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj index 12649c0..8436b4c 100644 --- a/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj +++ b/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj @@ -50,9 +50,11 @@ + Impl\%(RecursiveDir)/%(FileName)%(Extension) - PreserveNewest diff --git a/src/DependencyModules.SourceGenerator/InterceptorSourceGenerator.cs b/src/DependencyModules.SourceGenerator/InterceptorSourceGenerator.cs index 5eefc73..d894a98 100644 --- a/src/DependencyModules.SourceGenerator/InterceptorSourceGenerator.cs +++ b/src/DependencyModules.SourceGenerator/InterceptorSourceGenerator.cs @@ -113,9 +113,86 @@ private static IReadOnlyList ReportUnsupported( continue; } + ReportUnservedMembers(context, model, logger); + + // No member has an interceptor that can serve it, so a wrapper would forward every call + // untouched. Dropped after reporting rather than before, which is what makes the case + // visible at all. + if (!model.Members.Any(member => + model.Interceptors.Any(interceptor => interceptor.CanServe(member.Kind)))) { + + continue; + } + usable.Add(model); } return usable; } + + /// + /// Reports interceptors that are quietly absent from some of the members they were applied to. + /// + /// + /// The generator picks per member from the three interceptor interfaces, and an interceptor + /// implementing none of the one a member needs was simply left out of that member's chain. That + /// is an interceptor that does not run, which is a correctness question rather than a style one: + /// an argument-rewriting interceptor stops rewriting, and an authorisation gate stops gating. + /// + /// One diagnostic per interceptor and member shape, so a wide interface produces one line rather + /// than one per member. + /// + private static void ReportUnservedMembers( + SourceProductionContext context, InterceptorModel model, FileLogger logger) { + + foreach (var interceptor in model.Interceptors) { + foreach (var kind in new[] { + InterceptorKind.Sync, InterceptorKind.Async, InterceptorKind.Stream + }) { + + if (interceptor.CanServe(kind)) { + continue; + } + + var unserved = model.Members + .Where(member => member.Kind == kind) + .Select(member => member.Name) + .Distinct() + .OrderBy(name => name, StringComparer.Ordinal) + .ToArray(); + + if (unserved.Length == 0) { + continue; + } + + logger.Error( + $"'{interceptor.Type.Name}' does not implement {InterfaceFor(kind)}, so it is not " + + $"applied to {string.Join(", ", unserved)} on '{model.ServiceType.Name}'."); + + context.ReportDiagnostic( + Diagnostic.Create( + DependencyModuleDiagnostics.InterceptorCannotServeMembers, + Location.None, + interceptor.Type.Name, + InterfaceFor(kind), + DescriptionFor(kind), + model.ServiceType.Name, + string.Join(", ", unserved))); + } + } + } + + private static string InterfaceFor(InterceptorKind kind) => + kind switch { + InterceptorKind.Async => "IAsyncInterceptor", + InterceptorKind.Stream => "IAsyncEnumerableInterceptor", + _ => "IInterceptor" + }; + + private static string DescriptionFor(InterceptorKind kind) => + kind switch { + InterceptorKind.Async => "the members returning a task", + InterceptorKind.Stream => "the members returning an async stream", + _ => "the members returning a value directly" + }; } diff --git a/src/DependencyModules.Testing/Attributes/MockAttribute.cs b/src/DependencyModules.Testing/Attributes/MockAttribute.cs index 8ecf6e3..9f90e12 100644 --- a/src/DependencyModules.Testing/Attributes/MockAttribute.cs +++ b/src/DependencyModules.Testing/Attributes/MockAttribute.cs @@ -55,8 +55,17 @@ public void SetupServiceCollection( } var mockedValue = mockAttribute.ProvideMock(parameter.ParameterType); + var key = ServiceKeyOf(parameter); - serviceCollection.AddSingleton(parameter.ParameterType, _ => mockedValue); + // Registered under the parameter's key when it has one. Registering unkeyed regardless left + // the keyed registration — the one the consumer actually injects — untouched, so the service + // under test kept the real implementation while the test held a double it believed was wired + // in. The arrangement ran, the double recorded nothing, and the assertion failed elsewhere. + if (key == null) { + serviceCollection.AddSingleton(parameter.ParameterType, _ => mockedValue); + } else { + serviceCollection.AddKeyedSingleton(parameter.ParameterType, key, (_, _) => mockedValue); + } } /// @@ -77,6 +86,22 @@ public void SetupServiceCollection( /// public Task GetParameterValueAsync( ITestMethodContext testMethod, IServiceProvider serviceProvider, ParameterInfo parameter) { + var key = ServiceKeyOf(parameter); + + if (key != null && serviceProvider is IKeyedServiceProvider keyedServiceProvider) { + return Task.FromResult(keyedServiceProvider.GetKeyedService(parameter.ParameterType, key)); + } + return Task.FromResult(serviceProvider.GetService(parameter.ParameterType)); } + + /// + /// The key the parameter asks for, or null when it asks for the unkeyed service. + /// + /// + /// The same attribute the container path honours, read here so that a parameter carrying both + /// [Mock] and [FromKeyedServices] means one thing rather than two. + /// + private static object? ServiceKeyOf(ParameterInfo parameter) => + parameter.GetCustomAttribute()?.Key; } diff --git a/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs b/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs index 0b63b5e..d4947b8 100644 --- a/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs +++ b/tests/DependencyModules.Tests/GeneratorTests/DiagnosticsTests.cs @@ -331,6 +331,132 @@ public void CrossWiredNonGenericType_StillRegisters() { provider.GetService(generated.Type("IAudit`1").MakeGenericType(typeof(int)))); } + /// + /// An interceptor implementing only IInterceptor, applied to a service whose members are + /// all async. It never runs, and the build used to be green — the model was ignored before + /// anything could report on it. + /// + [Fact] + public void InterceptorThatServesNoMember_ReportsDM0015() { + var result = GeneratorTestHarness.Run( + Intercepted( + """ + public interface IAsyncOnly { + Task GetAsync(string key); + } + + [SingletonService] + [Intercept(typeof(SyncOnlyInterceptor))] + public class AsyncOnly : IAsyncOnly { + public Task GetAsync(string key) => Task.FromResult(key); + } + """)); + + var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0015"); + + Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); + Assert.Contains("SyncOnlyInterceptor", diagnostic.GetMessage()); + Assert.Contains("IAsyncInterceptor", diagnostic.GetMessage()); + Assert.Contains("GetAsync", diagnostic.GetMessage()); + } + + /// + /// The partial case: the interceptor serves the sync members and is quietly absent from the async + /// one, which is how an argument-rewriting interceptor stops rewriting halfway through a service. + /// + [Fact] + public void InterceptorThatServesSomeMembers_ReportsDM0015ForTheRest() { + var result = GeneratorTestHarness.Run( + Intercepted( + """ + public interface IMixed { + int Count(string key); + Task CountAsync(string key); + } + + [SingletonService] + [Intercept(typeof(SyncOnlyInterceptor))] + public class Mixed : IMixed { + public int Count(string key) => key.Length; + public Task CountAsync(string key) => Task.FromResult(key.Length); + } + """)); + + var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0015"); + + Assert.Contains("CountAsync", diagnostic.GetMessage()); + Assert.DoesNotContain("'Count'", diagnostic.GetMessage()); + } + + /// + /// An interceptor covering every shape the service uses says nothing. + /// + [Fact] + public void InterceptorThatServesEveryMember_DoesNotReportDM0015() { + var result = GeneratorTestHarness.Run( + Intercepted( + """ + public interface ISyncOnly { + int Count(string key); + } + + [SingletonService] + [Intercept(typeof(SyncOnlyInterceptor))] + public class SyncOnly : ISyncOnly { + public int Count(string key) => key.Length; + } + """)); + + Assert.DoesNotContain(result.GeneratorDiagnostics, d => d.Id == "DM0015"); + } + + /// + /// DM0008 drops the whole wrapper, not only the member it names, and the message has to say so — + /// the guide read as though the other members were still intercepted. + /// + [Fact] + public void UnsupportedMember_ReportsThatNoMemberIsIntercepted() { + var result = GeneratorTestHarness.Run( + Intercepted( + """ + public interface IAwkward { + bool TryGet(string key, out string value); + int Fine(string key); + } + + [SingletonService] + [Intercept(typeof(SyncOnlyInterceptor))] + public class Awkward : IAwkward { + public bool TryGet(string key, out string value) { value = key; return true; } + public int Fine(string key) => key.Length; + } + """)); + + var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0008"); + + Assert.Contains("none of its members are intercepted", diagnostic.GetMessage()); + Assert.Contains("TryGet", diagnostic.GetMessage()); + } + + private static string Intercepted(string body) => + $$""" + using System.Threading.Tasks; + using DependencyModules.Runtime.Attributes; + using DependencyModules.Runtime.Interception; + + namespace TestNamespace; + + [SingletonService] + public class SyncOnlyInterceptor : IInterceptor { + public TResult Intercept(InvocationContext context) => context.Proceed(); + } + + {{body}} + + [DependencyModule] + public partial class TestModule; + """; + private static string OpenGenericStore(string body, string moduleAttributes = "") => $$""" using DependencyModules.Runtime.Attributes; diff --git a/tests/DependencyModules.Tests/GeneratorTests/GeneratedBehaviourTests.cs b/tests/DependencyModules.Tests/GeneratorTests/GeneratedBehaviourTests.cs index 5dc2a35..0ea87a3 100644 --- a/tests/DependencyModules.Tests/GeneratorTests/GeneratedBehaviourTests.cs +++ b/tests/DependencyModules.Tests/GeneratorTests/GeneratedBehaviourTests.cs @@ -151,6 +151,37 @@ public void ReplaceRegistration_LeavesASingleRegistration() { Assert.Single(generated.Descriptors("IThing")); } + /// + /// Registrations within a module are emitted sorted by implementation type name, and both + /// Replace and Try act on a registration that has to already be there. Named so that the + /// alphabet puts them first, they used to run before their target existed: Replace replaced + /// nothing and added itself, then the registration it meant to displace was added after it and + /// won. Renaming the class fixed it, and nothing said so. + /// + [Fact] + public void ReplaceRegistration_WinsEvenWhenItsTypeNameSortsFirst() { + var generated = GeneratedAssembly.Create(Module( + """ + [SingletonService(Using = RegistrationType.Replace)] public class AaaThing : IThing; + [SingletonService] public class ZzzThing : IThing; + """)); + + Assert.Single(generated.Descriptors("IThing")); + Assert.Equal(generated.Type("AaaThing"), generated.ResolveRequired("IThing").GetType()); + } + + [Fact] + public void TryRegistration_DeclinesEvenWhenItsTypeNameSortsFirst() { + var generated = GeneratedAssembly.Create(Module( + """ + [SingletonService(Using = RegistrationType.Try)] public class AaaThing : IThing; + [SingletonService] public class ZzzThing : IThing; + """)); + + Assert.Single(generated.Descriptors("IThing")); + Assert.Equal(generated.Type("ZzzThing"), generated.ResolveRequired("IThing").GetType()); + } + [Fact] public void ConstructorDependencies_AreInjectedFromTheContainer() { var generated = GeneratedAssembly.Create(Module( diff --git a/tests/DependencyModules.Tests/Snapshots/ModuleGenerationSnapshotTests.RegistrationTypeVariants.verified.txt b/tests/DependencyModules.Tests/Snapshots/ModuleGenerationSnapshotTests.RegistrationTypeVariants.verified.txt index 108f641..8d135a3 100644 --- a/tests/DependencyModules.Tests/Snapshots/ModuleGenerationSnapshotTests.RegistrationTypeVariants.verified.txt +++ b/tests/DependencyModules.Tests/Snapshots/ModuleGenerationSnapshotTests.RegistrationTypeVariants.verified.txt @@ -13,16 +13,16 @@ namespace TestNamespace private static void ModuleDependencies(global::Microsoft.Extensions.DependencyInjection.IServiceCollection services) { - services.Replace(new global::Microsoft.Extensions.DependencyInjection.ServiceDescriptor( - typeof(global::TestNamespace.IReplace), - typeof(global::TestNamespace.ReplaceThing), - ServiceLifetime.Singleton - )); services.TryAddEnumerable(new global::Microsoft.Extensions.DependencyInjection.ServiceDescriptor( typeof(global::TestNamespace.ITryEnumerable), typeof(global::TestNamespace.TryEnumerableThing), ServiceLifetime.Singleton )); + services.Replace(new global::Microsoft.Extensions.DependencyInjection.ServiceDescriptor( + typeof(global::TestNamespace.IReplace), + typeof(global::TestNamespace.ReplaceThing), + ServiceLifetime.Singleton + )); services.TryAddSingleton( typeof(global::TestNamespace.ITry), typeof(global::TestNamespace.TryThing) diff --git a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt index 88eefc2..40aa776 100644 --- a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt +++ b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt @@ -1063,6 +1063,7 @@ namespace DependencyModules.SourceGenerator.Impl public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor EmptyEnvironmentCondition; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ExposedByConvention; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor GeneratorFailure; + public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor InterceptorCannotServeMembers; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor ModuleMustBePartial; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor OpenGenericCannotBeDecorated; public static readonly Microsoft.CodeAnalysis.DiagnosticDescriptor RegisteredConditionally; @@ -1534,6 +1535,10 @@ namespace DependencyModules.SourceGenerator.Impl.Utilities public static System.Collections.Generic.IReadOnlyList GetAttributeModels(DependencyModules.SourceGenerator.Impl.Utilities.SyntaxTransformContext context, Microsoft.CodeAnalysis.SyntaxNode node, System.Threading.CancellationToken cancellationToken, System.Func? filter = null) { } public static System.Collections.Generic.IEnumerable GetAttributes(DependencyModules.SourceGenerator.Impl.Utilities.SyntaxTransformContext context, Microsoft.CodeAnalysis.SyntaxList attributeListSyntax, System.Threading.CancellationToken cancellationToken, System.Func? filter = null) { } } + public static class AttributeTypeMatcher + { + public static bool Matches(Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.AttributeSyntax attributeSyntax, CSharpAuthor.ITypeDefinition attributeType, System.Threading.CancellationToken cancellationToken) { } + } public abstract class BaseAttributeWriter where T : DependencyModules.SourceGenerator.Impl.Models.IClassModel { diff --git a/tests/DependencyModules.Tests/TestingTests/TestParameterResolverTests.cs b/tests/DependencyModules.Tests/TestingTests/TestParameterResolverTests.cs index d5b96ab..a3df09e 100644 --- a/tests/DependencyModules.Tests/TestingTests/TestParameterResolverTests.cs +++ b/tests/DependencyModules.Tests/TestingTests/TestParameterResolverTests.cs @@ -133,6 +133,61 @@ public async Task ResolvingBeforeSetupThrows() { Assert.Contains(nameof(TestParameterResolver.SetupServiceCollection), exception.Message); } + /// + /// A [Mock] on a keyed parameter replaces the keyed registration. It used to register the + /// double unkeyed, leaving the keyed registration — the one a consumer injects — untouched, so + /// the service under test kept the real implementation while the test held a double it believed + /// was wired in. + /// + [Fact] + public async Task KeyedMockReplacesTheKeyedRegistration() { + var (resolver, provider) = Build( + nameof(Samples.KeyedMock), + services => services.AddKeyedSingleton("primary")); + + var arguments = await resolver.ResolveArgumentsAsync(provider, []); + + Assert.IsType(Assert.Single(arguments)); + Assert.IsType(provider.GetRequiredKeyedService("primary")); + } + + /// + /// And it does not spill into the unkeyed slot, where nothing asked for it. + /// + [Fact] + public void KeyedMockRegistersNothingUnkeyed() { + var (_, provider) = Build( + nameof(Samples.KeyedMock), + services => services.AddKeyedSingleton("primary")); + + Assert.Null(provider.GetService()); + } + + /// + /// A key the mock did not name is left alone, so mocking one keyed implementation leaves its + /// siblings real. + /// + [Fact] + public void KeyedMockLeavesOtherKeysAlone() { + var (_, provider) = Build( + nameof(Samples.KeyedMock), + services => { + services.AddKeyedSingleton("primary"); + services.AddKeyedSingleton("secondary"); + }); + + Assert.IsType(provider.GetRequiredKeyedService("secondary")); + } + + /// Control: an unkeyed mock still replaces the unkeyed registration. + [Fact] + public async Task UnkeyedMockReplacesTheUnkeyedRegistration() { + var arguments = await Resolve( + nameof(Samples.UnkeyedMock), services => services.AddSingleton()); + + Assert.IsType(Assert.Single(arguments)); + } + [Fact] public void SetupIsOfferedEveryParameter() { var services = new ServiceCollection(); @@ -175,9 +230,14 @@ private class StubContext(MethodInfo method) : ITestMethodContext { /// Signatures only — never invoked. Static so nothing needs constructing; the members are public /// within this private class so one set of binding flags finds them all. /// + [StubMockSupport] private static class Samples { public static void OneService(IThing thing) { } + public static void KeyedMock([Mock] [FromKeyedServices("primary")] IThing thing) { } + + public static void UnkeyedMock([Mock] IThing thing) { } + public static void WantsTheProvider(IServiceProvider provider) { } public static void DataThenService(int number, IThing thing) { } @@ -193,6 +253,15 @@ public static void UnregisteredWithInjectedValue([InjectValues("supplied")] Need public static void TwoRegisteringAttributes([RegistersOther] IThing first, [RegistersOther] IThing second) { } } + /// + /// Stands in for a mocking package, so the real [Mock] can be driven without one. What the + /// double actually is does not matter here; where it gets registered does. + /// + [AttributeUsage(AttributeTargets.Class)] + private class StubMockSupportAttribute : Attribute, IMockSupportAttribute { + public object ProvideMock(Type type) => new Other(); + } + /// /// Stands in for [Mock]: registers a replacement during setup, then lets ordinary container /// resolution hand it back. diff --git a/website/guide/getting-started.md b/website/guide/getting-started.md index fcccc7c..ab9eabd 100644 --- a/website/guide/getting-started.md +++ b/website/guide/getting-started.md @@ -48,6 +48,17 @@ dotnet add package DependencyModules.SourceGenerator Requires .NET 8.0 or later, and ships both `net8.0` and `net10.0` assemblies so a project on either LTS release gets one built against its own framework. +::: tip A console app or class library needs one more +`DependencyModules.Runtime` depends on `Microsoft.Extensions.DependencyInjection.Abstractions`, which +is the right dependency for a library — but `ServiceCollection` and `BuildServiceProvider()` live in +the implementation package. A project using the Web or Worker SDK already has it through its framework +reference. Anything else needs: + +```shell +dotnet add package Microsoft.Extensions.DependencyInjection +``` +::: + Those two are everything the library itself needs — [conventions](/guide/conventions) included. The optional packages are for [testing](/guide/testing), and this guide will tell you when you want them: diff --git a/website/guide/interception.md b/website/guide/interception.md index 49b14d7..2515645 100644 --- a/website/guide/interception.md +++ b/website/guide/interception.md @@ -135,7 +135,7 @@ dependencies of its own — as `TimingInterceptor` does with its `ILogger`. ## What cannot be intercepted The generator has to emit a real override, so some shapes are impossible. These are reported as -[DM0008](/reference/diagnostics#dm0008) and left unwrapped rather than failing the build: +[DM0008](/reference/diagnostics#dm0008) rather than failing the build: - `ref`, `in` and `out` parameters, and `ref struct` parameters - by-reference returns @@ -143,4 +143,37 @@ The generator has to emit a real override, so some shapes are impossible. These - static members - generic implementations, which register as an open generic -Write a [decorator](/guide/decorators) for those. +::: warning One such member disables interception for the whole interface +There is no partial wrapper. A single `out` parameter anywhere on the interface means no wrapper is +generated at all, so every other member goes uninterceped too, and `GetRequiredService()` +returns the plain implementation. The diagnostic names the member it found first; fixing it may +uncover another. + +Move the member to an interface that is not intercepted, or write a +[decorator](/guide/decorators) for the service instead. +::: + +## When an interceptor covers only some members + +Separate from the above, and quieter. Each interceptor is placed only around the members whose shape +it can serve — `IInterceptor` for a direct return, `IAsyncInterceptor` for a task, +`IAsyncEnumerableInterceptor` for a stream — and it is simply absent from the rest: + +```csharp +public class AuditInterceptor : IInterceptor { … } // sync only + +[SingletonService] +[Intercept(typeof(AuditInterceptor))] +public class Orders : IOrders { + public int Count(string customer) { … } // audited + public Task CountAsync(string customer) { … } // not audited +} +``` + +That is [DM0015](/reference/diagnostics#dm0015). It is worth taking seriously rather than silencing: +an interceptor that rewrites arguments stops rewriting them, and one that authorises or audits stops +doing that — on the async members, which are usually the ones doing the work. Implement the missing +interface, or apply the interceptor to a service with no such member. + +One type may implement any combination of the three, which is how a single interceptor covers a mixed +interface. diff --git a/website/guide/modules.md b/website/guide/modules.md index d6a179f..3451320 100644 --- a/website/guide/modules.md +++ b/website/guide/modules.md @@ -124,6 +124,20 @@ public partial class DiagnosticsModule; Convention registrations always name their declaring module as their realm, which is why two modules running conventions over the same interface do not leak into each other. +::: warning Two modules in one assembly, loaded together, register everything twice +"Joins every module in its compilation" is literal. An assembly declaring two modules that neither set +`OnlyRealm` puts the *whole* registration list in both — decorators included — so loading both in one +call runs it twice: + +```csharp +services.AddModules(new AppModule(), new DataModule()); // every service registered twice +``` + +Declaring two modules is fine; loading both is what doubles up. If they are meant to be composed +together, give one a realm, or have one compose the other with its +[generated attribute](#composing-modules) instead of naming both at the call site. +::: + ## Parameters A module can take values from whoever loads it — a connection string, a base URL. Declare them as diff --git a/website/guide/troubleshooting.md b/website/guide/troubleshooting.md index e5da103..5d786bf 100644 --- a/website/guide/troubleshooting.md +++ b/website/guide/troubleshooting.md @@ -45,6 +45,26 @@ the reason**. ``` +::: warning No log appeared? +Every `DependencyModules_*` property reaches the generator through +`build/DependencyModules.SourceGenerator.targets`, which ships **inside the NuGet package**. A project +that references the analyzer as a `ProjectReference` — building this library from source, or vendoring +it — never imports that file, so the property is invisible and silently takes its default. + +Declare them yourself in that project, or in a `Directory.Build.props` above it: + +```xml + + + + + + + + +``` +::: + ## 3. Check for DM diagnostics The generator reports what it can detect at build time, and a good deal of what goes wrong here is diff --git a/website/reference/attributes.md b/website/reference/attributes.md index 127cbcf..7b6f39b 100644 --- a/website/reference/attributes.md +++ b/website/reference/attributes.md @@ -59,6 +59,11 @@ A `[Decorator]` is never a convention candidate — it is not a service. | Property | | |---|---| | `Order` | nesting; lower sits closer to the implementation | +| `Service` | the decorated interface, when it cannot be inferred | +| `Realm` | restrict the decorator to one module, matching `Realm` on the service attributes | + +An unrestricted decorator belongs to every module that is not `OnlyRealm`, exactly as an unrestricted +service registration does — so a decorator with no `Realm` is not picked up by an `OnlyRealm` module. ### `[Intercept(params Type[])]` diff --git a/website/reference/diagnostics.md b/website/reference/diagnostics.md index ce9310f..7448622 100644 --- a/website/reference/diagnostics.md +++ b/website/reference/diagnostics.md @@ -4,15 +4,22 @@ The generator reports what it can work out at build time as `DM####` codes, so a shows up in the IDE rather than as a resolution failure at startup. This page says what each one means and what to do about it. -They behave like any other analyzer diagnostic, so each can be tuned or silenced through -`.editorconfig`: - -```ini -dotnet_diagnostic.DM0010.severity = none +These are reported by a source generator rather than by an analyzer, which decides how they are +tuned. Roslyn applies `.editorconfig` severity mapping to *analyzer* diagnostics, and a generator's +reach the compilation with the severity already fixed — so `dotnet_diagnostic.DM0005.severity = none` +has no effect. Use the compilation-level properties instead, which are applied later and do work: + +```xml + + $(NoWarn);DM0005 + $(WarningsAsErrors);DM0013 + ``` -`DM0010` and `DM0011` are informational and exist to make registration visible at the class. Silence -them if the IDE gets noisy; the rest are worth reading. +`#pragma warning disable DM0005` works too, for silencing one site rather than a project. + +`DM0010` and `DM0011` are informational and exist to make registration visible at the class, which +means they appear in the IDE and never in `dotnet build` at any verbosity. The rest are worth reading. | Code | Severity | Meaning | |---|---|---| @@ -30,6 +37,7 @@ them if the IDE gets noisy; the rest are worth reading. | [DM0012](#dm0012) | Warning | An environment condition names nothing to test | | [DM0013](#dm0013) | Warning | A service registered as an open generic cannot be decorated | | [DM0014](#dm0014) | Warning | A generic type cannot be cross-wired | +| [DM0015](#dm0015) | Warning | An interceptor does not apply to every member | ## DM0001 {#dm0001} @@ -167,3 +175,39 @@ the attribute promises. Use `[SingletonService]`, `[ScopedService]` or `[TransientService]` instead, applying one per interface if the type needs to answer to more than one. + +## DM0015 {#dm0015} + +**An interceptor does not apply to every member it was applied to.** + +Three interfaces cover the member shapes, and the generator picks per member: + +| Interface | Members | +|---|---| +| `IInterceptor` | returning a value directly, or `void` | +| `IAsyncInterceptor` | returning `Task`, `Task`, `ValueTask`, `ValueTask` | +| `IAsyncEnumerableInterceptor` | returning `IAsyncEnumerable` | + +An interceptor that implements none of the one a member needs is left out of that member's chain, and +those calls run without it: + +```csharp +public class AuditInterceptor : IInterceptor { … } // sync only + +[SingletonService] +[Intercept(typeof(AuditInterceptor))] +public class Orders : IOrders { + public int Count(string customer) { … } // audited + public Task CountAsync(string customer) { … } // DM0015 — not audited +} +``` + +This matters more than it first reads. An interceptor that rewrites arguments stops rewriting them; +one that authorises or audits stops doing that, on exactly the members most likely to be the +interesting ones. In the sharpest case — an `IInterceptor` applied to a service whose members are all +async — it never runs at all. + +Implement the missing interface on the interceptor, or apply it to a service with no such member. + +Reported once per interceptor and member shape, so a wide interface produces one line rather than +one per member. See [Interception](/guide/interception). diff --git a/website/reference/msbuild.md b/website/reference/msbuild.md index fe2bde2..797f886 100644 --- a/website/reference/msbuild.md +++ b/website/reference/msbuild.md @@ -6,11 +6,11 @@ when the packages are installed from NuGet. | Property | Default | | |---|---|---| -| `DependencyModules_GenerateFactories` | `false` | emit a `new` expression instead of `typeof(T)`, so the container does not construct by reflection | +| `DependencyModules_GenerateFactories` | `false` | emit a `new` expression instead of `typeof(T)`, so the container does not construct by reflection — [see the trade-off](#generatefactories-and-container-validation) | | `DependencyModules_RegistrationType` | `Add` | the default registration strategy for the project | | `DependencyModules_AutoGenerateModule` | `true` | generate `ApplicationModule` for a top-level `Program.cs` | | `DependencyModules_RegisterGenerator` | `false` | register discovered `JsonSerializerContext` types | -| `DependencyModules_ExcludeGeneratedCodeFromCoverage` | `true` | apply `[ExcludeFromCodeCoverage]` to generated members | +| `ExcludeGeneratedCodeFromCoverage` | `true` | apply `[ExcludeFromCodeCoverage]` to generated members — note this one carries no `DependencyModules_` prefix | | `DependencyModules_LogOutputDirectory` | *(none)* | write a generator log here — see [Troubleshooting](/guide/troubleshooting) | ```xml @@ -29,3 +29,31 @@ Not a DependencyModules property, but the one you will reach for most: true ``` + +## `GenerateFactories` and container validation {#generatefactories-and-container-validation} + +Worth knowing before turning this on project-wide. + +What it emits is a **factory** per registration: + +```csharp +services.AddSingleton( + typeof(OrderService), + provider => new OrderService(provider.GetRequiredService())); +``` + +`Microsoft.Extensions.DependencyInjection` cannot see inside a factory, so every registration in the +project becomes opaque to its own graph validation. Measured on the same captive dependency — a +singleton taking a scoped service — with only this property differing: + +| | `BuildServiceProvider(ValidateScopes + ValidateOnBuild)` | +|---|---| +| unset | throws — `Cannot consume scoped service 'IUnitOfWork' from singleton 'OrderService'` | +| `true` | builds cleanly | + +A missing registration goes the same way: the `GetRequiredService` call inside a factory is not +checked at build either, so it throws on first resolve instead. + +The property exists for startup cost and for Native AOT, which is exactly the setting a team turns on +late and everywhere. If you rely on `ValidateScopes` and `ValidateOnBuild` in development — and the +standard advice is to — keep this off there and turn it on for the published build. From 2dfcad7823808b5f760ff1e376c4902e8eb6d459 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 14 Aug 2026 14:14:27 -0400 Subject: [PATCH 4/5] Intercept generic services instead of refusing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A generic implementation registers as an open generic and was refused outright, because decoration cannot touch one and interception inherited the constraint without needing it. Decoration rewrites a registration into a factory, which an open generic service type cannot carry. Interception generates a type, and an open generic implementation type is exactly what the container does accept. The wrapper is now generic over the same parameters, and takes the implementation by its own type rather than the service — asking for the service would resolve the wrapper, which is registered as that service, and recurse. Its nested state classes close the self reference over the outer parameters, which is CS0305 written bare. DecoratorHelper.InterceptOpenGeneric swaps the registration and registers the implementation alongside it, carrying lifetime and service key across, and is idempotent so two modules carrying the same registration do not double-wrap. A constrained type parameter is still refused. The wrapper would have to repeat the constraint to reference the implementation, and there is no way to emit one; the message says that rather than describing the old factory reason. Verified under Native AOT, where the limit belongs to the container rather than to this: a published binary closes an open generic over reference types only, and throws for a value type argument. A plain [SingletonService] on a generic class behaves identically, so interception adds no restriction. Both the AOT guide and the interception guide now say so. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np5LZpJ2PPStWVcrNXfmD5 --- CHANGELOG.md | 29 ++++++- .../Helpers/DecoratorHelper.cs | 70 +++++++++++++++ .../InterceptorFileWriter.cs | 72 ++++++++++++++-- .../InterceptorRegistrationWriter.cs | 86 ++++++++++++++----- .../Models/InterceptorModel.cs | 21 ++++- .../Utilities/InterceptorModelUtility.cs | 72 ++++++++++++++-- .../InterceptorGenerationTests.cs | 84 ++++++++++++++---- .../PublicApiTests.RuntimeApi.verified.txt | 1 + ...icApiTests.SourceGeneratorApi.verified.txt | 4 +- website/guide/aot.md | 20 ++++- website/guide/interception.md | 34 +++++++- website/reference/diagnostics.md | 6 +- 12 files changed, 434 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 847f867..167cce3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Everything since `1.0.0-rc9230`. The theme is registrations that were silently not happening: an attribute the generator declined to recognise, a `Replace` that depended on how a class was named, a -mock that replaced the wrong slot, and three shapes it refused without saying so. +mock that replaced the wrong slot, and three shapes it refused without saying so. One of those +refusals turned out to be unnecessary, and generic services can now be intercepted. Still a release candidate. `DecoratorExpansion.Expand` changed shape, but only on the generator extension points, which are documented as unversioned. @@ -58,6 +59,32 @@ a build break rather than a nudge. `NoWarn` takes them per-project; note that `. discarded the model before anything could report on it. Reported once per interceptor and member shape, so a wide interface produces one line rather than forty. +- **A generic service can be intercepted.** A generic implementation registers as an open generic, and + it was refused outright — because *decoration* cannot touch one, and interception inherited that + constraint without needing it. Decoration rewrites a registration into a factory, which an open + generic service type cannot carry; interception generates a type, and an open generic implementation + type is what the container does accept. + + ```csharp + [SingletonService] + [Intercept(typeof(TracingInterceptor))] + public class Repository : IRepository { … } + ``` + + The wrapper is generic over the same parameters — `Repository_Intercepted : IRepository` — and + takes `Repository` by its own type rather than the service, which would resolve back to the + wrapper and recurse. `DecoratorHelper.InterceptOpenGeneric` swaps the registration and registers the + implementation alongside it, carrying the lifetime and the service key across. + + A **constrained** type parameter is still refused, and now says why: the wrapper would have to repeat + the constraint and there is no way to emit one. + + Worth knowing before relying on it under Native AOT, and true of open generic registrations + generally rather than of interception: a published binary closes them over reference types only. + Measured on `osx-arm64`, `IRepository` resolves and `IRepository` throws + `Unable to create a generic service … because 'System.Int32' is a ValueType`. A plain + `[SingletonService]` on a generic class behaves identically, which the AOT guide now says. + ### Fixed - **An attribute the generator declined to recognise, depending on how it was spelled.** Attribute diff --git a/src/DependencyModules.Runtime/Helpers/DecoratorHelper.cs b/src/DependencyModules.Runtime/Helpers/DecoratorHelper.cs index 6f45824..7cfb4ed 100644 --- a/src/DependencyModules.Runtime/Helpers/DecoratorHelper.cs +++ b/src/DependencyModules.Runtime/Helpers/DecoratorHelper.cs @@ -26,6 +26,76 @@ namespace DependencyModules.Runtime.Helpers; /// public static class DecoratorHelper { + /// + /// Swaps an open generic registration for a generated wrapper that implements the same + /// open generic service. + /// + /// The collection to rewrite. + /// The open generic service, such as IRepository<>. + /// + /// The open generic implementation currently registered for it, such as Repository<>. + /// + /// + /// The generated wrapper, such as Repository_Intercepted<>. It implements the service + /// and takes the implementation as a constructor parameter. + /// + /// + /// + /// cannot + /// serve this: it rewrites a registration into a factory, and the container rejects a factory for + /// an open generic service type — "requires registering an open generic implementation type". An + /// open generic implementation type is exactly what it does accept, and a generated wrapper is + /// one. + /// + /// + /// The implementation is additionally registered under its own concrete type, which is how the + /// wrapper receives it without asking for the service it is itself registered as — that would + /// resolve to the wrapper and recurse. Lifetime is carried across to both, so wrapping does not + /// change how long anything lives. + /// + /// + /// Idempotent: a second pass finds the wrapper in the slot rather than the implementation and + /// leaves it alone, which is what keeps two modules carrying the same registration from + /// double-wrapping. + /// + /// + public static void InterceptOpenGeneric( + IServiceCollection services, + Type serviceType, + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] + Type implementationType, + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] + Type wrapperType) { + + // Snapshotted: the loop appends the implementation's own registration, and re-reading Count + // would walk into what it just added. + var count = services.Count; + + for (var i = 0; i < count; i++) { + var descriptor = services[i]; + + if (descriptor.ServiceType != serviceType || ImplementationOf(descriptor) != implementationType) { + continue; + } + + services.Add(new ServiceDescriptor(implementationType, implementationType, descriptor.Lifetime)); + + services[i] = descriptor.IsKeyedService + ? new ServiceDescriptor(serviceType, descriptor.ServiceKey, wrapperType, descriptor.Lifetime) + : new ServiceDescriptor(serviceType, wrapperType, descriptor.Lifetime); + } + } + + /// + /// The implementation type of a descriptor, keyed or not. + /// + /// + /// A keyed descriptor throws from ImplementationType rather than returning null, so the two + /// cannot be read through one property. + /// + private static Type? ImplementationOf(ServiceDescriptor descriptor) => + descriptor.IsKeyedService ? descriptor.KeyedImplementationType : descriptor.ImplementationType; + /// /// Wraps every registration of using . /// diff --git a/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs b/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs index a546701..bc4d1dc 100644 --- a/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs +++ b/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs @@ -33,6 +33,17 @@ public string Write(InterceptorModel model, string wrapperName, string namespace var wrapper = csharpFile.AddClass(wrapperName); wrapper.Modifiers |= ComponentModifier.Internal; + + // A generic service is wrapped by a generic type closed over the same parameters, which is + // what lets the container register it as an open generic implementation. Only constraint-free + // parameters reach here; a constrained one is refused upstream, because the wrapper would have + // to repeat the constraint and there is no way to emit one. + if (model.IsOpenGeneric) { + foreach (var typeParameter in model.TypeParameters!) { + wrapper.AddGenericParameter(typeParameter); + } + } + wrapper.AddBaseType(model.ServiceType); wrapper.AddAttribute(TypeDefinition.Get("System.Diagnostics.CodeAnalysis", "ExcludeFromCodeCoverage")); @@ -62,8 +73,51 @@ public string Write(InterceptorModel model, string wrapperName, string namespace return output.Output(); } + /// + /// What the wrapper holds and is handed as the instance it wraps. + /// + /// + /// The service interface for an ordinary wrapper, which the container hands over because + /// decoration captured the original registration first. + /// + /// For an open generic there is no factory to capture anything, so the wrapper is the + /// registration for the service — and asking for the service would resolve the wrapper itself and + /// recurse. It takes the implementation by its own concrete type instead, which + /// DecoratorHelper.InterceptOpenGeneric registers alongside it. + /// + private static ITypeDefinition InnerType(InterceptorModel model) => + model.IsOpenGeneric ? Closed(model.ImplementationType, model.TypeParameters!) : model.ServiceType; + + /// + /// A type closed over the wrapper's own type parameters — Repository becomes + /// Repository<T>. + /// + private static ITypeDefinition Closed(ITypeDefinition type, IReadOnlyList typeParameters) { + var arguments = new ITypeDefinition[typeParameters.Count]; + + for (var i = 0; i < arguments.Length; i++) { + arguments[i] = TypeDefinition.Get("", typeParameters[i]); + } + + return new GenericTypeDefinition( + TypeDefinitionEnum.ClassDefinition, type.Namespace, type.Name, arguments); + } + + /// + /// How a nested state class names the wrapper that owns it. + /// + /// + /// A nested type inherits its outer type's parameters but still has to write them: inside + /// Repository_Intercepted<T> the name is Repository_Intercepted<T>, and + /// the bare name is CS0305. + /// + private static ITypeDefinition SelfType(InterceptorModel model, string wrapperName) => + model.IsOpenGeneric + ? Closed(TypeDefinition.Get("", wrapperName), model.TypeParameters!) + : TypeDefinition.Get("", wrapperName); + private static void WriteFields(ClassDefinition wrapper, InterceptorModel model) { - var inner = wrapper.AddField(model.ServiceType, InnerField); + var inner = wrapper.AddField(InnerType(model), InnerField); inner.Modifiers |= ComponentModifier.Private | ComponentModifier.Readonly; for (var index = 0; index < model.Interceptors.Count; index++) { @@ -93,7 +147,7 @@ private static void WriteFields(ClassDefinition wrapper, InterceptorModel model) private static void WriteConstructor(ClassDefinition wrapper, InterceptorModel model) { var constructor = wrapper.AddConstructor(); - constructor.AddParameter(model.ServiceType, "inner"); + constructor.AddParameter(InnerType(model), "inner"); constructor.AddIndentedStatement($"{InnerField} = inner"); for (var index = 0; index < model.Interceptors.Count; index++) { @@ -333,8 +387,10 @@ private static void WriteState( state.WhereStatement = Constraints(member); - WriteStateFields(state, member, wrapperName); - WriteStateConstructor(state, member, index, wrapperName); + var selfType = SelfType(model, wrapperName); + + WriteStateFields(state, member, selfType); + WriteStateConstructor(state, member, index, selfType); WriteCallerAndCount(state, member, index); WriteArgumentsIndexer(state, member); WriteNameAt(state, member); @@ -342,9 +398,9 @@ private static void WriteState( } private static void WriteStateFields( - ClassDefinition state, InterceptedMemberModel member, string wrapperName) { + ClassDefinition state, InterceptedMemberModel member, ITypeDefinition selfType) { - var self = state.AddField(TypeDefinition.Get("", wrapperName), "_self"); + var self = state.AddField(selfType, "_self"); self.Modifiers |= ComponentModifier.Private | ComponentModifier.Readonly; for (var index = 0; index < member.Parameters.Count; index++) { @@ -358,11 +414,11 @@ private static void WriteStateFields( /// non-nullable reference is definitely assigned and the wrapper needs no nullable suppression. /// private static void WriteStateConstructor( - ClassDefinition state, InterceptedMemberModel member, int index, string wrapperName) { + ClassDefinition state, InterceptedMemberModel member, int index, ITypeDefinition selfType) { var constructor = state.AddConstructor(); - constructor.AddParameter(TypeDefinition.Get("", wrapperName), "self"); + constructor.AddParameter(selfType, "self"); constructor.AddIndentedStatement("_self = self"); for (var argument = 0; argument < member.Parameters.Count; argument++) { diff --git a/src/DependencyModules.SourceGenerator.Impl/InterceptorRegistrationWriter.cs b/src/DependencyModules.SourceGenerator.Impl/InterceptorRegistrationWriter.cs index 9f1b4b2..0e86219 100644 --- a/src/DependencyModules.SourceGenerator.Impl/InterceptorRegistrationWriter.cs +++ b/src/DependencyModules.SourceGenerator.Impl/InterceptorRegistrationWriter.cs @@ -34,6 +34,26 @@ public string Write( return EntryModelUtil.ApplyRecordDeclaration(outputContext.Output(), entryPointModel); } + /// + /// A type written as its unbound generic form — IVault<> rather than + /// IVault<T>. + /// + /// + /// The only form a typeof can carry outside the type's own declaration. Writing the + /// parameter names instead is CS0246, because no T is in scope at the registration. + /// Blank-named arguments are how this codebase represents unbound throughout. + /// + private static ITypeDefinition Unbound(ITypeDefinition type, int arity) { + var arguments = new ITypeDefinition[arity]; + + for (var i = 0; i < arity; i++) { + arguments[i] = TypeDefinition.Get("", ""); + } + + return new GenericTypeDefinition( + TypeDefinitionEnum.ClassDefinition, type.Namespace, type.Name, arguments); + } + private static void WriteInterceptor( ModuleEntryPointModel entryPointModel, ClassDefinition classDefinition, @@ -80,30 +100,52 @@ private static void WriteInterceptor( var wrapperName = $"{model.ImplementationType.Name.Replace(".", "_")}_Intercepted"; var wrapperType = TypeDefinition.Get(model.ImplementationType.Namespace, wrapperName); - // The wrapper is generated right here, so its constructor is known exactly: the intercepted - // instance, then one parameter per interceptor. Emitting the `new` rather than handing the - // type to ActivatorUtilities is what keeps interception working in a published Native AOT - // application — the same reason decorators are emitted closed. - var arguments = new List { CodeOutputComponent.Get("inner") }; + method.NewLine(); - for (var i = 0; i < model.Interceptors.Count; i++) { - arguments.Add( - new InvokeGenericDefinition( - "provider", "GetRequiredService", new[] { model.Interceptors[i].Type })); - } + if (model.IsOpenGeneric) { + // An open generic service cannot be decorated: decoration rewrites the registration into + // a factory, and the container refuses a factory for one. It does accept an open generic + // implementation type, and the wrapper is one — so the registration is swapped for the + // wrapper and the implementation is registered under its own type for the wrapper to take. + // + // Nothing is closed here. The container closes the wrapper per requested construction, and + // every type it names exists in the assembly, so this survives publishing as the closed + // path does. + method.AddIndentedStatement( + new StaticInvokeStatement( + KnownTypes.DependencyModules.Helpers.DecoratorHelper, + "InterceptOpenGeneric", + new List { + CodeOutputComponent.Get(services.Name), + TypeOf(Unbound(model.ServiceType, model.TypeParameters!.Count)), + TypeOf(Unbound(model.ImplementationType, model.TypeParameters!.Count)), + TypeOf(Unbound(wrapperType, model.TypeParameters!.Count)) + })); + } else { + // The wrapper is generated right here, so its constructor is known exactly: the + // intercepted instance, then one parameter per interceptor. Emitting the `new` rather than + // handing the type to ActivatorUtilities is what keeps interception working in a published + // Native AOT application — the same reason decorators are emitted closed. + var arguments = new List { CodeOutputComponent.Get("inner") }; + + for (var i = 0; i < model.Interceptors.Count; i++) { + arguments.Add( + new InvokeGenericDefinition( + "provider", "GetRequiredService", new[] { model.Interceptors[i].Type })); + } - method.NewLine(); - method.AddIndentedStatement( - SyntaxHelpers.InvokeGeneric( - KnownTypes.DependencyModules.Helpers.DecoratorHelper, - "Decorate", - new[] { model.ServiceType }, - CodeOutputComponent.Get(services.Name), - TypeOf(wrapperType), - new WrapStatement( - CodeOutputComponent.Get(" => "), - CodeOutputComponent.Get("(provider, inner)"), - New(wrapperType, arguments.ToArray())))); + method.AddIndentedStatement( + SyntaxHelpers.InvokeGeneric( + KnownTypes.DependencyModules.Helpers.DecoratorHelper, + "Decorate", + new[] { model.ServiceType }, + CodeOutputComponent.Get(services.Name), + TypeOf(wrapperType), + new WrapStatement( + CodeOutputComponent.Get(" => "), + CodeOutputComponent.Get("(provider, inner)"), + New(wrapperType, arguments.ToArray())))); + } // A field initializer registers the method, matching how decorator registrations are hooked // up. DynamicDependency keeps the trimmer from removing a method only referenced this way. diff --git a/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs b/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs index 04805df..fd34bbd 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs @@ -282,6 +282,14 @@ public override int GetHashCode() { /// after the position, so the order is part of the model. /// /// What the wrapper declares, pointing back into the members. +/// +/// The implementation's type parameter names, empty for a non-generic service. The wrapper repeats +/// them verbatim, so Repository<T> becomes Repository_Intercepted<T> +/// implementing IRepository<T> and holding a Repository<T>. +/// +/// Names rather than models: only constraint-free parameters reach here, since the writer cannot +/// emit a constraint and one that is dropped produces code that does not compile. +/// public record InterceptorModel( ITypeDefinition ServiceType, ITypeDefinition ImplementationType, @@ -289,7 +297,14 @@ public record InterceptorModel( IReadOnlyList Members, IReadOnlyList Declarations, int Order, - InterceptionRefusal? Refusal = null) { + InterceptionRefusal? Refusal = null, + IReadOnlyList? TypeParameters = null) { + + /// + /// Whether the intercepted service is an open generic, and so registers as an implementation type + /// rather than through a factory. + /// + public bool IsOpenGeneric => TypeParameters is { Count: > 0 }; /// /// Sentinel for a node carrying the attribute that produced no usable model and nothing to say @@ -333,7 +348,8 @@ public bool Equals(InterceptorModel? x, InterceptorModel? y) { Equals(x.Refusal, y.Refusal) && ModelEquality.ListEquals(x.Interceptors, y.Interceptors) && ModelEquality.ListEquals(x.Members, y.Members) && - ModelEquality.ListEquals(x.Declarations, y.Declarations); + ModelEquality.ListEquals(x.Declarations, y.Declarations) && + ModelEquality.ListEquals(x.TypeParameters, y.TypeParameters); } public int GetHashCode(InterceptorModel obj) { @@ -346,6 +362,7 @@ public int GetHashCode(InterceptorModel obj) { hash = hash * 31 + ModelEquality.ListHashCode(obj.Interceptors); hash = hash * 31 + ModelEquality.ListHashCode(obj.Members); hash = hash * 31 + ModelEquality.ListHashCode(obj.Declarations); + hash = hash * 31 + ModelEquality.ListHashCode(obj.TypeParameters); return hash; } diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs index 0a5deb1..6f99ff5 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs @@ -42,15 +42,26 @@ public static InterceptorModel GetInterceptorModel( return InterceptorModel.Ignore; } - // A generic implementation registers as an open generic, and decorating one of those is not - // supported: DecoratorHelper rewrites the registration into a factory, which the container - // rejects for an open generic service type. Refusing here turns what would be an - // ArgumentException when the provider is built into a message naming the declaration. + // A generic implementation registers as an open generic. Decoration cannot touch one — it + // rewrites the registration into a factory, and the container refuses a factory for an open + // generic service type — but interception does not need one: the wrapper is a generated type, + // and an open generic implementation type is exactly what the container does accept. It is + // registered as the service, and takes the implementation by its own type so that resolving + // it does not come back round to the wrapper. + // + // Constraints are the one thing that shape cannot carry. The wrapper has to repeat the + // implementation's constraints to reference it, and the writer has no way to emit them, so a + // constrained implementation is refused rather than emitted as code that does not compile. if (implementationSymbol.IsGenericType) { - return InterceptorModel.Refused( - $"'{implementationSymbol.Name}' is generic, so it registers as an open generic, and " + - "decorating an open generic registration is not supported. Register a closed " + - $"construction instead, such as a class deriving from '{implementationSymbol.Name}<...>'"); + var constrained = ConstrainedTypeParameter(implementationSymbol); + + if (constrained != null) { + return InterceptorModel.Refused( + $"'{implementationSymbol.Name}' is generic and its type parameter " + + $"'{constrained}' is constrained. The generated wrapper would have to repeat the " + + "constraint and cannot, so it was not generated. Intercept a closed construction " + + $"instead, such as a class deriving from '{implementationSymbol.Name}<...>'"); + } } var interceptorSymbols = new List(); @@ -101,7 +112,8 @@ public static InterceptorModel GetInterceptorModel( interceptors, members, declarations, - order); + order, + TypeParameters: TypeParameterNames(implementationSymbol)); } private static InterceptorModel Refuse(string? reason) => @@ -268,6 +280,48 @@ private static ImmutableArray DeclaredInterfaces(INamedTypeSym return ImmutableArray.Empty; } + /// + /// The first type parameter carrying a constraint, or null when none does. + /// + /// + /// Every kind counts, class and new() included: the wrapper closes the + /// implementation over its own parameters, and any constraint the implementation declares has to + /// hold there too. + /// + private static string? ConstrainedTypeParameter(INamedTypeSymbol symbol) { + foreach (var parameter in symbol.TypeParameters) { + if (parameter.HasReferenceTypeConstraint || + parameter.HasValueTypeConstraint || + parameter.HasNotNullConstraint || + parameter.HasUnmanagedTypeConstraint || + parameter.HasConstructorConstraint || + parameter.ConstraintTypes.Length > 0) { + + return parameter.Name; + } + } + + return null; + } + + /// + /// The implementation's type parameter names, which the wrapper repeats verbatim so that its own + /// parameters line up with the ones the service and the implementation are closed over. + /// + private static IReadOnlyList TypeParameterNames(INamedTypeSymbol symbol) { + if (symbol.TypeParameters.Length == 0) { + return Array.Empty(); + } + + var names = new string[symbol.TypeParameters.Length]; + + for (var i = 0; i < names.Length; i++) { + names[i] = symbol.TypeParameters[i].Name; + } + + return names; + } + private static ITypeDefinition ToTypeDefinition(INamedTypeSymbol symbol) { var namespaceName = symbol.ContainingNamespace.IsGlobalNamespace ? "" diff --git a/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs b/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs index 902180d..854bc42 100644 --- a/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs +++ b/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs @@ -579,34 +579,84 @@ public partial class TestModule; } /// - /// A generic implementation registers as an open generic, and decorating one of those rewrites - /// the registration into a factory, which the container rejects for an open generic service - /// type. Refused here so the failure names the declaration rather than surfacing as an - /// ArgumentException when the provider is built. + /// A generic implementation registers as an open generic. Decoration cannot touch one — it + /// rewrites the registration into a factory, and the container refuses a factory for an open + /// generic service type — but interception does not need one: the wrapper is a generated type, + /// and an open generic implementation type is what the container does accept. /// [Fact] - public void GenericImplementation_ReportsDM0008() { - var result = GeneratorTestHarness.Run( - $$""" - {{Preamble}} + public void GenericImplementation_IsIntercepted() { + var result = GeneratorTestHarness.Run(GenericRepo("public class Repo : IRepo { public void Run() { } }")); - {{Tracing("Tracing", "tracing")}} + Assert.DoesNotContain(result.GeneratorDiagnostics, d => d.Id == "DM0008"); + Assert.Contains(result.GeneratedSources.Keys, key => key.Contains("Repo_Intercepted")); + } - public interface IRepo { void Run(); } + /// + /// The wrapper is generic over the same parameters, so the container can register it as an open + /// generic implementation type and close it per construction. + /// + [Fact] + public void GenericImplementation_EmitsAGenericWrapper() { + var result = GeneratorTestHarness.Run(GenericRepo("public class Repo : IRepo { public void Run() { } }")); - [SingletonService] - [Intercept(typeof(TracingInterceptor))] - public class Repo : IRepo { public void Run() { } } + var wrapper = Assert.Single(result.GeneratedSources, pair => pair.Key.Contains("Repo_Intercepted")).Value; - [DependencyModule] - public partial class TestModule; - """); + Assert.Contains("class Repo_Intercepted", wrapper); + + // It takes the implementation by its own type. Asking for the service would resolve the + // wrapper itself, which is registered as that service, and recurse. + Assert.Contains("Repo inner", wrapper); + } + + /// + /// Registered by swapping the open generic registration rather than through the decorator + /// factory, which an open generic service type cannot carry. Every type is written unbound: no + /// `T` is in scope at the registration. + /// + [Fact] + public void GenericImplementation_RegistersAsAnOpenGenericImplementation() { + var result = GeneratorTestHarness.Run(GenericRepo("public class Repo : IRepo { public void Run() { } }")); + + var registration = Assert.Single(result.GeneratedSources, pair => pair.Key.Contains("Interceptors")).Value; + + Assert.Contains("InterceptOpenGeneric", registration); + Assert.Contains("typeof(global::TestNamespace.IRepo<>)", registration); + Assert.Contains("typeof(global::TestNamespace.Repo_Intercepted<>)", registration); + } + + /// + /// A constrained type parameter is the one shape this cannot carry: the wrapper would have to + /// repeat the constraint to reference the implementation, and there is no way to emit one. + /// Refused rather than emitted as code that does not compile. + /// + [Fact] + public void ConstrainedGenericImplementation_ReportsDM0008() { + var result = GeneratorTestHarness.Run( + GenericRepo("public class Repo : IRepo where T : class { public void Run() { } }")); var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0008"); - Assert.Contains("open generic", diagnostic.GetMessage()); + Assert.Contains("constrained", diagnostic.GetMessage()); + Assert.DoesNotContain(result.GeneratedSources.Keys, key => key.Contains("Repo_Intercepted")); } + private static string GenericRepo(string implementation) => + $$""" + {{Preamble}} + + {{Tracing("Tracing", "tracing")}} + + public interface IRepo { void Run(); } + + [SingletonService] + [Intercept(typeof(TracingInterceptor))] + {{implementation}} + + [DependencyModule] + public partial class TestModule; + """; + /// /// A closed construction of a generic service, which is the answer to the refusal above. The /// interface is reached through the base rather than declared, and a service registration finds diff --git a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.RuntimeApi.verified.txt b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.RuntimeApi.verified.txt index 8bb382a..6f3b991 100644 --- a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.RuntimeApi.verified.txt +++ b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.RuntimeApi.verified.txt @@ -189,6 +189,7 @@ namespace DependencyModules.Runtime.Helpers public static void Decorate(Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Type serviceType, System.Func decoratorFactory) { } public static void Decorate(Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Type decoratorIdentity, System.Func decoratorFactory) where TService : class { } + public static void InterceptOpenGeneric(Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Type serviceType, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type implementationType, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type wrapperType) { } } public sealed class DecoratorRegistration { diff --git a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt index 40aa776..2849910 100644 --- a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt +++ b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt @@ -1353,15 +1353,17 @@ namespace DependencyModules.SourceGenerator.Impl.Models public class InterceptorModel : System.IEquatable { public static readonly DependencyModules.SourceGenerator.Impl.Models.InterceptorModel Ignore; - public InterceptorModel(CSharpAuthor.ITypeDefinition ServiceType, CSharpAuthor.ITypeDefinition ImplementationType, System.Collections.Generic.IReadOnlyList Interceptors, System.Collections.Generic.IReadOnlyList Members, System.Collections.Generic.IReadOnlyList Declarations, int Order, DependencyModules.SourceGenerator.Impl.Models.InterceptionRefusal? Refusal = null) { } + public InterceptorModel(CSharpAuthor.ITypeDefinition ServiceType, CSharpAuthor.ITypeDefinition ImplementationType, System.Collections.Generic.IReadOnlyList Interceptors, System.Collections.Generic.IReadOnlyList Members, System.Collections.Generic.IReadOnlyList Declarations, int Order, DependencyModules.SourceGenerator.Impl.Models.InterceptionRefusal? Refusal = null, System.Collections.Generic.IReadOnlyList? TypeParameters = null) { } public System.Collections.Generic.IReadOnlyList Declarations { get; init; } public CSharpAuthor.ITypeDefinition ImplementationType { get; init; } public System.Collections.Generic.IReadOnlyList Interceptors { get; init; } public bool IsIgnored { get; } + public bool IsOpenGeneric { get; } public System.Collections.Generic.IReadOnlyList Members { get; init; } public int Order { get; init; } public DependencyModules.SourceGenerator.Impl.Models.InterceptionRefusal? Refusal { get; init; } public CSharpAuthor.ITypeDefinition ServiceType { get; init; } + public System.Collections.Generic.IReadOnlyList? TypeParameters { get; init; } public static DependencyModules.SourceGenerator.Impl.Models.InterceptorModel Refused(string message) { } } public class InterceptorModelComparer : System.Collections.Generic.IEqualityComparer diff --git a/website/guide/aot.md b/website/guide/aot.md index 882afe3..6a6d801 100644 --- a/website/guide/aot.md +++ b/website/guide/aot.md @@ -55,8 +55,24 @@ a compile-time decision, and belongs to `#if`. See [what conditions cost](/guide/environments#what-conditions-cost). **Open generic registration is the least AOT-friendly part of the container itself**, independent of -this library — the container has to construct a closed type at run time. If you are targeting Native -AOT aggressively, prefer closed registrations. +this library — the container has to construct a closed type at run time, and Native AOT only has code +for the instantiations the compiler could see. + +In practice the line falls between reference and value type arguments. Measured on a published +`osx-arm64` binary, with `[SingletonService]` on `Bin : IBin`: + +``` +GetRequiredService>() works — reference types share one instantiation +GetRequiredService>() InvalidOperationException: Unable to create a generic service + for type 'IBin`1[System.Int32]' because 'System.Int32' is a + ValueType. Native code to support creating generic services + might not be available with native AOT. +``` + +This is the container, not the generator: an [intercepted](/guide/interception) open generic behaves +exactly the same way, because it is registered the same way. If you are targeting Native AOT, register +closed constructions — a [convention](/guide/conventions) over the open generic does that for you, +emitting one registration per implementation. **Runtime assembly discovery is not supported**, because there would be nothing to resolve at build time. See [Scanning a package](/guide/scanning). diff --git a/website/guide/interception.md b/website/guide/interception.md index 2515645..4ea56cb 100644 --- a/website/guide/interception.md +++ b/website/guide/interception.md @@ -118,7 +118,7 @@ public async IAsyncEnumerable InterceptStream(StreamInvocationCont |---|---| | `Proceed()` / `ProceedAsync()` | run the rest of the pipeline — more than once to retry, or not at all to skip the implementation | | `Caller.ServiceType`, `Caller.MemberName` | what is being called | -| `Arguments` | by index or by name, and **writable** — a write replaces what the implementation receives | +| `Arguments` | by index, and **writable** — a write replaces what the implementation receives. `NameAt(index)` gives the declared parameter name | Arguments cost nothing until you read one. @@ -141,7 +141,9 @@ The generator has to emit a real override, so some shapes are impossible. These - by-reference returns - `init`-only setters - static members -- generic implementations, which register as an open generic +- a generic implementation whose type parameters are **constrained** — the wrapper would have to + repeat the constraint, and there is no way to emit one. An unconstrained generic implementation is + intercepted; see below. ::: warning One such member disables interception for the whole interface There is no partial wrapper. A single `out` parameter anywhere on the interface means no wrapper is @@ -153,6 +155,34 @@ Move the member to an interface that is not intercepted, or write a [decorator](/guide/decorators) for the service instead. ::: +## Intercepting a generic service + +A generic implementation registers as an open generic, and a decorator cannot touch one — decoration +rewrites a registration into a factory, and the container refuses a factory for an open generic +service type. Interception does not need a factory: the wrapper is a generated type, and an open +generic implementation type is what the container does accept. + +```csharp +[SingletonService] +[Intercept(typeof(TracingInterceptor))] +public class Repository : IRepository { … } +``` + +The wrapper is generic over the same parameters — `Repository_Intercepted : IRepository` — and +takes `Repository` by its own type rather than the service, which would resolve back to the wrapper +and recurse. The container closes it per construction, so `IRepository` and +`IRepository` each get their own. + +::: warning Native AOT closes this over reference types only +An open generic registration is the container's least AOT-friendly shape, intercepted or not: a +published binary can construct `IRepository` and throws for `IRepository`. That is not +specific to interception — a plain `[SingletonService]` on a generic class behaves identically. See +[Trimming and AOT](/guide/aot#what-it-does-not-cover). +::: + +A **constrained** type parameter is refused, because the wrapper cannot repeat the constraint. Give +the service a closed construction to intercept instead. + ## When an interceptor covers only some members Separate from the above, and quieter. Each interceptor is placed only around the members whose shape diff --git a/website/reference/diagnostics.md b/website/reference/diagnostics.md index 7448622..415496d 100644 --- a/website/reference/diagnostics.md +++ b/website/reference/diagnostics.md @@ -98,7 +98,11 @@ Their nesting would be ambiguous. See [Decorators](/guide/decorators#ordering). **A service marked for interception cannot be wrapped.** The member uses `ref`, `in`, `out` or a `ref struct` parameter, returns by reference, has an -`init`-only setter, is static, or the implementation is generic. See +`init`-only setter, or is static — or the implementation is generic with a **constrained** type +parameter, which the generated wrapper would have to repeat and cannot. + +One such member costs the whole interface: no wrapper is generated, so every other member goes +uninterceped too. The message names the first offender it found. See [Interception](/guide/interception#what-cannot-be-intercepted). ## DM0009 {#dm0009} From 72d51fd55be7c3cad1ba7265f5f11f4357ba3814 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 14 Aug 2026 16:31:31 -0400 Subject: [PATCH 5/5] Repeat a generic service's constraints instead of refusing them Interception refused a constrained generic implementation because the wrapper has to repeat the constraints to reference what it wraps, and there was no way to emit one. There is now: CSharpAuthor 1.1.1010 adds AddConstraint, which takes the parts and writes them in the order C# requires. Repository where T : class, IEntity, new() is wrapped by Repository_Intercepted : IRepository where T : class, IEntity, new(). struct and unmanaged already guarantee a default constructor and Roslyn reports one for them, so new() is dropped rather than repeated - writing it out is CS0451. The method path moves onto the same model. Constraints were read into a rendered string for generic methods and, once classes needed them too, would have been read a second way for classes - two copies of the same precedence rules, including that unmanaged has to be tested before struct because it implies it. TypeParameterReader reads a parameter once for both, and the writer hands the parts to the library rather than formatting a clause itself. Verified by compiling and running the result: a service constrained to class, IMarker, new() resolves as its wrapper and is intercepted, and one constrained to struct does too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np5LZpJ2PPStWVcrNXfmD5 --- CHANGELOG.md | 13 +++- ...endencyModules.SourceGenerator.Impl.csproj | 2 +- .../InterceptorFileWriter.cs | 75 ++++++++++++++----- .../Models/InterceptorModel.cs | 64 +++++++++++++--- .../Utilities/InterceptedMemberReader.cs | 47 ++---------- .../Utilities/InterceptorModelUtility.cs | 58 +++----------- .../Utilities/TypeParameterReader.cs | 45 +++++++++++ .../DependencyModules.SourceGenerator.csproj | 2 +- .../InterceptorGenerationTests.cs | 63 +++++++++++++--- ...icApiTests.SourceGeneratorApi.verified.txt | 54 ++++++++++--- website/guide/interception.md | 9 +-- website/reference/diagnostics.md | 3 +- 12 files changed, 290 insertions(+), 145 deletions(-) create mode 100644 src/DependencyModules.SourceGenerator.Impl/Utilities/TypeParameterReader.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 167cce3..742e695 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,8 +76,11 @@ a build break rather than a nudge. `NoWarn` takes them per-project; note that `. wrapper and recurse. `DecoratorHelper.InterceptOpenGeneric` swaps the registration and registers the implementation alongside it, carrying the lifetime and the service key across. - A **constrained** type parameter is still refused, and now says why: the wrapper would have to repeat - the constraint and there is no way to emit one. + Constraints come along with the parameters: `Repository where T : class, IEntity, new()` is + wrapped by `Repository_Intercepted : IRepository where T : class, IEntity, new()`, without + which the wrapper could not reference what it wraps. `struct` and `unmanaged` already guarantee a + default constructor and Roslyn reports one for them, so `new()` is dropped rather than repeated — + writing it out is CS0451. Worth knowing before relying on it under Native AOT, and true of open generic registrations generally rather than of interception: a published binary closes them over reference types only. @@ -142,6 +145,12 @@ a build break rather than a nudge. `NoWarn` takes them per-project; note that `. ### Changed +- **`CSharpAuthor` 1.1.1010**, for `AddConstraint`. A `where` clause was previously assembled as a + string and assigned to `WhereStatement`, which put C#'s ordering rules — one primary constraint + first, `new()` last — in this generator. Two places needed them once a class could carry constraints + as well as a method, and two copies of a subtle rule drift. `TypeParameterReader` reads a parameter + once for both, and the library puts the parts in order. + - **`DM0008` now says what it costs.** One member the wrapper cannot override means *no* wrapper is generated, so every other member on the interface goes uninterceped too — and the guide read as though only the offending member did. A reader who fixed the named member and rebuilt would then diff --git a/src/DependencyModules.SourceGenerator.Impl/DependencyModules.SourceGenerator.Impl.csproj b/src/DependencyModules.SourceGenerator.Impl/DependencyModules.SourceGenerator.Impl.csproj index b59cd20..34e7c57 100644 --- a/src/DependencyModules.SourceGenerator.Impl/DependencyModules.SourceGenerator.Impl.csproj +++ b/src/DependencyModules.SourceGenerator.Impl/DependencyModules.SourceGenerator.Impl.csproj @@ -65,7 +65,7 @@ all runtime; build; native; contentfiles; analyzers; buildtransitive - + all build diff --git a/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs b/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs index bc4d1dc..e0de6f5 100644 --- a/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs +++ b/src/DependencyModules.SourceGenerator.Impl/InterceptorFileWriter.cs @@ -40,7 +40,9 @@ public string Write(InterceptorModel model, string wrapperName, string namespace // to repeat the constraint and there is no way to emit one. if (model.IsOpenGeneric) { foreach (var typeParameter in model.TypeParameters!) { - wrapper.AddGenericParameter(typeParameter); + wrapper.AddGenericParameter(typeParameter.Name); + + WriteConstraint(wrapper.AddConstraint(typeParameter.Name), typeParameter); } } @@ -92,11 +94,13 @@ private static ITypeDefinition InnerType(InterceptorModel model) => /// A type closed over the wrapper's own type parameters — Repository becomes /// Repository<T>. /// - private static ITypeDefinition Closed(ITypeDefinition type, IReadOnlyList typeParameters) { + private static ITypeDefinition Closed( + ITypeDefinition type, IReadOnlyList typeParameters) { + var arguments = new ITypeDefinition[typeParameters.Count]; for (var i = 0; i < arguments.Length; i++) { - arguments[i] = TypeDefinition.Get("", typeParameters[i]); + arguments[i] = TypeDefinition.Get("", typeParameters[i].Name); } return new GenericTypeDefinition( @@ -116,6 +120,53 @@ private static ITypeDefinition SelfType(InterceptorModel model, string wrapperNa ? Closed(TypeDefinition.Get("", wrapperName), model.TypeParameters!) : TypeDefinition.Get("", wrapperName); + /// + /// The constraints a member declares, which both the forwarding member and its state class have + /// to repeat or the call they forward will not satisfy them. + /// + private static void WriteConstraints( + InterceptedMemberModel member, Func addConstraint) { + + foreach (var typeParameter in member.TypeParameters) { + WriteConstraint(addConstraint(typeParameter.Name), typeParameter); + } + } + + /// + /// Repeats one type parameter's constraints. + /// + /// + /// The parts go in as the symbol reported them and come out in the order C# requires, which is + /// ConstraintDefinition's job rather than this writer's. + /// + private static void WriteConstraint(ConstraintDefinition constraint, TypeParameterModel typeParameter) { + switch (typeParameter.Primary) { + case "class": + constraint.Class(); + break; + case "class?": + constraint.Class(nullable: true); + break; + case "struct": + constraint.Struct(); + break; + case "unmanaged": + constraint.Unmanaged(); + break; + case "notnull": + constraint.NotNull(); + break; + } + + foreach (var constraintType in typeParameter.ConstraintTypes) { + constraint.Implements(constraintType); + } + + if (typeParameter.DefaultConstructor) { + constraint.DefaultConstructor(); + } + } + private static void WriteFields(ClassDefinition wrapper, InterceptorModel model) { var inner = wrapper.AddField(InnerType(model), InnerField); inner.Modifiers |= ComponentModifier.Private | ComponentModifier.Readonly; @@ -300,7 +351,7 @@ private static void WriteForwardingMethod( method.AddGenericParameter(new TypeParameterDefinition(typeParameter.Name)); } - method.WhereStatement = Constraints(member); + WriteConstraints(member, method.AddConstraint); var arguments = new List { "this" }; @@ -385,7 +436,7 @@ private static void WriteState( state.AddGenericParameter(typeParameter.Name); } - state.WhereStatement = Constraints(member); + WriteConstraints(member, state.AddConstraint); var selfType = SelfType(model, wrapperName); @@ -670,20 +721,6 @@ private static string ClosedStateName(InterceptedMemberModel member, int index) ">"; } - /// - /// The constraints the member declares, which both the forwarding member and the state class - /// have to repeat or the call they forward will not satisfy them. - /// - private static IOutputComponent? Constraints(InterceptedMemberModel member) { - var clauses = member.TypeParameters - .Where(parameter => parameter.Constraints.Length > 0) - .Select(parameter => $"where {parameter.Name} : {parameter.Constraints}") - .ToList(); - - return clauses.Count == 0 - ? null - : new CodeOutputComponent(" " + string.Join(" ", clauses)) { Indented = false }; - } private static string InterceptorField(int index) => $"_dmInterceptor{index}"; diff --git a/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs b/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs index fd34bbd..08e8669 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Models/InterceptorModel.cs @@ -92,7 +92,54 @@ public record InterceptedParameterModel( /// /// The constraints without the where T : prefix, such as class, new(), or empty. /// -public record InterceptedTypeParameterModel(string Name, string Constraints); +/// +/// A type parameter of the intercepted class, with its constraints held as parts rather than +/// rendered. +/// +/// +/// The wrapper is declared over the same parameters and has to repeat their constraints, or it cannot +/// reference the implementation it wraps. Parts rather than a string because the writer decides how a +/// type name is written and the reader does not know: rendering here would bake one output mode into +/// the model, and CSharpAuthor's AddConstraint takes the pieces and puts them in the +/// order C# requires. +/// +/// The parameter name, repeated verbatim on the wrapper. +/// +/// The primary constraint keyword — class, struct, unmanaged, notnull — +/// or null when there is none. At most one is legal. +/// +/// Base class and interface constraints, in declaration order. +/// Whether new() was declared. +public record TypeParameterModel( + string Name, + string? Primary, + IReadOnlyList ConstraintTypes, + bool DefaultConstructor) { + + /// + /// Structural equality over the constraint types, which the compiler-generated version compares + /// by reference — two identical models built on consecutive runs would never match, and the + /// incremental cache would miss on every keystroke. + /// + public virtual bool Equals(TypeParameterModel? other) => + other is not null && + Name == other.Name && + Primary == other.Primary && + DefaultConstructor == other.DefaultConstructor && + ModelEquality.ListEquals(ConstraintTypes, other.ConstraintTypes); + + public override int GetHashCode() { + unchecked { + var hash = Name.GetHashCode(); + + hash = hash * 31 + (Primary?.GetHashCode() ?? 0); + hash = hash * 31 + DefaultConstructor.GetHashCode(); + hash = hash * 31 + ModelEquality.ListHashCode(ConstraintTypes); + + return hash; + } + } +} /// /// An interceptor named by the attribute, with the interfaces it implements. @@ -147,7 +194,7 @@ public record InterceptedMemberModel( ITypeDefinition? ReturnType, ITypeDefinition ResultType, IReadOnlyList Parameters, - IReadOnlyList TypeParameters, + IReadOnlyList TypeParameters, ReturnShape ReturnShape) { /// @@ -283,12 +330,11 @@ public override int GetHashCode() { /// /// What the wrapper declares, pointing back into the members. /// -/// The implementation's type parameter names, empty for a non-generic service. The wrapper repeats -/// them verbatim, so Repository<T> becomes Repository_Intercepted<T> -/// implementing IRepository<T> and holding a Repository<T>. -/// -/// Names rather than models: only constraint-free parameters reach here, since the writer cannot -/// emit a constraint and one that is dropped produces code that does not compile. +/// The implementation's type parameters, empty for a non-generic service. The wrapper repeats them +/// and their constraints, so Repository<T> where T : class becomes +/// Repository_Intercepted<T> : IRepository<T> where T : class holding a +/// Repository<T>. A constraint that were dropped would leave the wrapper unable to +/// reference what it wraps. /// public record InterceptorModel( ITypeDefinition ServiceType, @@ -298,7 +344,7 @@ public record InterceptorModel( IReadOnlyList Declarations, int Order, InterceptionRefusal? Refusal = null, - IReadOnlyList? TypeParameters = null) { + IReadOnlyList? TypeParameters = null) { /// /// Whether the intercepted service is an open generic, and so registers as an implementation type diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptedMemberReader.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptedMemberReader.cs index 37238a3..7c96ef3 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptedMemberReader.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptedMemberReader.cs @@ -158,7 +158,7 @@ private static bool ReadProperty( type, type, indices, - Array.Empty(), + Array.Empty(), ReturnShape.Value)); } @@ -178,7 +178,7 @@ private static bool ReadProperty( null, KnownTypes.DependencyModules.Interception.NoResult, arguments, - Array.Empty(), + Array.Empty(), ReturnShape.Void)); } @@ -234,7 +234,7 @@ private static InterceptedMemberModel EventAccessor( null, KnownTypes.DependencyModules.Interception.NoResult, new InterceptedParameterModel[] { new("value", "value", handlerType, null) }, - Array.Empty(), + Array.Empty(), ReturnShape.Void); /// @@ -337,53 +337,20 @@ private static IEnumerable EnumerateMembers(INamedTypeSymbol serviceTyp /// The member's type parameters and their constraints. The state class repeats both, or the call /// it forwards will not satisfy the constraints the interface declared. /// - private static IReadOnlyList ReadTypeParameters(IMethodSymbol method) { + private static IReadOnlyList ReadTypeParameters(IMethodSymbol method) { if (method.TypeParameters.Length == 0) { - return Array.Empty(); + return Array.Empty(); } - var typeParameters = new List(); + var typeParameters = new List(); foreach (var parameter in method.TypeParameters) { - typeParameters.Add(new InterceptedTypeParameterModel(parameter.Name, RenderConstraints(parameter))); + typeParameters.Add(TypeParameterReader.Read(parameter)); } return typeParameters; } - private static string RenderConstraints(ITypeParameterSymbol parameter) { - var constraints = new List(); - - // Unmanaged implies a value type constraint, so it has to be tested first or the narrower - // constraint would be rendered as the wider one. - if (parameter.HasUnmanagedTypeConstraint) { - constraints.Add("unmanaged"); - } else if (parameter.HasValueTypeConstraint) { - constraints.Add("struct"); - } else if (parameter.HasReferenceTypeConstraint) { - constraints.Add( - parameter.ReferenceTypeConstraintNullableAnnotation == NullableAnnotation.Annotated - ? "class?" - : "class"); - } else if (parameter.HasNotNullConstraint) { - constraints.Add("notnull"); - } - - foreach (var constraintType in parameter.ConstraintTypes) { - var builder = new StringBuilder(); - - constraintType.GetTypeDefinition().WriteTypeName(builder, TypeOutputMode.Global); - - constraints.Add(builder.ToString()); - } - - if (parameter.HasConstructorConstraint) { - constraints.Add("new()"); - } - - return string.Join(", ", constraints); - } - private static string EscapeIdentifier(string name) => Microsoft.CodeAnalysis.CSharp.SyntaxFacts.GetKeywordKind(name) == Microsoft.CodeAnalysis.CSharp.SyntaxKind.None ? name diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs index 6f99ff5..65b3f23 100644 --- a/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/InterceptorModelUtility.cs @@ -49,20 +49,8 @@ public static InterceptorModel GetInterceptorModel( // registered as the service, and takes the implementation by its own type so that resolving // it does not come back round to the wrapper. // - // Constraints are the one thing that shape cannot carry. The wrapper has to repeat the - // implementation's constraints to reference it, and the writer has no way to emit them, so a - // constrained implementation is refused rather than emitted as code that does not compile. - if (implementationSymbol.IsGenericType) { - var constrained = ConstrainedTypeParameter(implementationSymbol); - - if (constrained != null) { - return InterceptorModel.Refused( - $"'{implementationSymbol.Name}' is generic and its type parameter " + - $"'{constrained}' is constrained. The generated wrapper would have to repeat the " + - "constraint and cannot, so it was not generated. Intercept a closed construction " + - $"instead, such as a class deriving from '{implementationSymbol.Name}<...>'"); - } - } + // Constraints come along with the parameters. The wrapper is declared over the same ones and + // repeats their constraints, without which it could not reference what it wraps. var interceptorSymbols = new List(); var order = 0; @@ -113,7 +101,7 @@ public static InterceptorModel GetInterceptorModel( members, declarations, order, - TypeParameters: TypeParameterNames(implementationSymbol)); + TypeParameters: TypeParameterModels(implementationSymbol)); } private static InterceptorModel Refuse(string? reason) => @@ -281,45 +269,21 @@ private static ImmutableArray DeclaredInterfaces(INamedTypeSym } /// - /// The first type parameter carrying a constraint, or null when none does. - /// - /// - /// Every kind counts, class and new() included: the wrapper closes the - /// implementation over its own parameters, and any constraint the implementation declares has to - /// hold there too. - /// - private static string? ConstrainedTypeParameter(INamedTypeSymbol symbol) { - foreach (var parameter in symbol.TypeParameters) { - if (parameter.HasReferenceTypeConstraint || - parameter.HasValueTypeConstraint || - parameter.HasNotNullConstraint || - parameter.HasUnmanagedTypeConstraint || - parameter.HasConstructorConstraint || - parameter.ConstraintTypes.Length > 0) { - - return parameter.Name; - } - } - - return null; - } - - /// - /// The implementation's type parameter names, which the wrapper repeats verbatim so that its own - /// parameters line up with the ones the service and the implementation are closed over. + /// The implementation's type parameters and their constraints, which the wrapper repeats so its + /// own parameters line up with the ones the service and the implementation are closed over. /// - private static IReadOnlyList TypeParameterNames(INamedTypeSymbol symbol) { + private static IReadOnlyList TypeParameterModels(INamedTypeSymbol symbol) { if (symbol.TypeParameters.Length == 0) { - return Array.Empty(); + return Array.Empty(); } - var names = new string[symbol.TypeParameters.Length]; + var models = new TypeParameterModel[symbol.TypeParameters.Length]; - for (var i = 0; i < names.Length; i++) { - names[i] = symbol.TypeParameters[i].Name; + for (var i = 0; i < models.Length; i++) { + models[i] = TypeParameterReader.Read(symbol.TypeParameters[i]); } - return names; + return models; } private static ITypeDefinition ToTypeDefinition(INamedTypeSymbol symbol) { diff --git a/src/DependencyModules.SourceGenerator.Impl/Utilities/TypeParameterReader.cs b/src/DependencyModules.SourceGenerator.Impl/Utilities/TypeParameterReader.cs new file mode 100644 index 0000000..8d9d0fa --- /dev/null +++ b/src/DependencyModules.SourceGenerator.Impl/Utilities/TypeParameterReader.cs @@ -0,0 +1,45 @@ +using CSharpAuthor; +using DependencyModules.SourceGenerator.Impl.Models; +using Microsoft.CodeAnalysis; + +namespace DependencyModules.SourceGenerator.Impl.Utilities; + +/// +/// Reads a type parameter's constraints into parts. +/// +/// +/// One reader for both places a wrapper repeats constraints — the class it is declared as, and each +/// generic method it forwards. The rules are subtle enough that two copies would drift: only one +/// primary constraint is legal, unmanaged has to be tested before struct because it +/// implies it, and Roslyn reports a constructor constraint for a struct-constrained parameter +/// even though repeating new() alongside it is CS0451. +/// +public static class TypeParameterReader { + + public static TypeParameterModel Read(ITypeParameterSymbol parameter) { + string? primary = null; + + if (parameter.HasUnmanagedTypeConstraint) { + primary = "unmanaged"; + } else if (parameter.HasValueTypeConstraint) { + primary = "struct"; + } else if (parameter.HasReferenceTypeConstraint) { + primary = parameter.ReferenceTypeConstraintNullableAnnotation == NullableAnnotation.Annotated + ? "class?" + : "class"; + } else if (parameter.HasNotNullConstraint) { + primary = "notnull"; + } + + var constraintTypes = new ITypeDefinition[parameter.ConstraintTypes.Length]; + + for (var i = 0; i < constraintTypes.Length; i++) { + constraintTypes[i] = parameter.ConstraintTypes[i].GetTypeDefinition(); + } + + var defaultConstructor = parameter.HasConstructorConstraint && + primary is not ("struct" or "unmanaged"); + + return new TypeParameterModel(parameter.Name, primary, constraintTypes, defaultConstructor); + } +} diff --git a/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj b/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj index 8436b4c..3805258 100644 --- a/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj +++ b/src/DependencyModules.SourceGenerator/DependencyModules.SourceGenerator.csproj @@ -39,7 +39,7 @@ all runtime; build; native; contentfiles; analyzers; buildtransitive - + all build diff --git a/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs b/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs index 854bc42..a2169fc 100644 --- a/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs +++ b/tests/DependencyModules.Tests/GeneratorTests/InterceptorGenerationTests.cs @@ -626,22 +626,65 @@ public void GenericImplementation_RegistersAsAnOpenGenericImplementation() { } /// - /// A constrained type parameter is the one shape this cannot carry: the wrapper would have to - /// repeat the constraint to reference the implementation, and there is no way to emit one. - /// Refused rather than emitted as code that does not compile. + /// The wrapper repeats the implementation's constraints, without which it could not reference + /// what it wraps. /// [Fact] - public void ConstrainedGenericImplementation_ReportsDM0008() { + public void ConstrainedGenericImplementation_RepeatsTheConstraints() { var result = GeneratorTestHarness.Run( - GenericRepo("public class Repo : IRepo where T : class { public void Run() { } }")); + GenericRepo( + "public class Repo : IRepo where T : class, IMarker, new() { public void Run() { } }", + supporting: "public interface IMarker;")); - var diagnostic = Assert.Single(result.GeneratorDiagnostics, d => d.Id == "DM0008"); + Assert.DoesNotContain(result.GeneratorDiagnostics, d => d.Id == "DM0008"); + + var wrapper = Assert.Single(result.GeneratedSources, pair => pair.Key.Contains("Repo_Intercepted")).Value; + + Assert.Contains("where T : class, global::TestNamespace.IMarker, new()", wrapper); + } + + /// + /// struct already guarantees a default constructor, and repeating new() alongside it is CS0451. + /// Roslyn reports the constructor constraint for a struct-constrained parameter anyway, so the + /// reader has to drop it rather than pass it through. + /// + [Fact] + public void StructConstrainedGeneric_DoesNotRepeatTheDefaultConstructor() { + var result = GeneratorTestHarness.Run( + GenericRepo("public class Repo : IRepo where T : struct { public void Run() { } }")); + + var wrapper = Assert.Single(result.GeneratedSources, pair => pair.Key.Contains("Repo_Intercepted")).Value; - Assert.Contains("constrained", diagnostic.GetMessage()); - Assert.DoesNotContain(result.GeneratedSources.Keys, key => key.Contains("Repo_Intercepted")); + Assert.Contains("where T : struct", wrapper); + Assert.DoesNotContain("new()", wrapper); } - private static string GenericRepo(string implementation) => + /// + /// And the constrained wrapper is not merely well-formed text: it compiles, loads and runs. + /// + [Fact] + public void ConstrainedGenericImplementation_ResolvesAndIntercepts() { + var generated = GeneratedAssembly.Create( + GenericRepo( + "public class Repo : IRepo where T : class, IMarker, new() { public void Run() { } }", + supporting: """ + public interface IMarker; + + public class Marked : IMarker; + """)); + + var closed = generated.Type("IRepo`1").MakeGenericType(generated.Type("Marked")); + var resolved = generated.BuildProvider().GetService(closed); + + Assert.NotNull(resolved); + Assert.StartsWith("Repo_Intercepted", resolved!.GetType().Name); + } + + /// + /// The attributes land on whatever declares first, so anything + /// the implementation needs alongside it goes in . + /// + private static string GenericRepo(string implementation, string supporting = "") => $$""" {{Preamble}} @@ -649,6 +692,8 @@ private static string GenericRepo(string implementation) => public interface IRepo { void Run(); } + {{supporting}} + [SingletonService] [Intercept(typeof(TracingInterceptor))] {{implementation}} diff --git a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt index 2849910..5052420 100644 --- a/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt +++ b/tests/DependencyModules.Tests/Snapshots/PublicApiTests.SourceGeneratorApi.verified.txt @@ -14,7 +14,9 @@ namespace CSharpAuthor { public AttributeDefinition(CSharpAuthor.ITypeDefinition attributeType) { } public System.Collections.Generic.IList? Arguments { get; set; } + public string? Target { get; set; } protected override void WriteComponentOutput(CSharpAuthor.IOutputContext outputContext) { } + public void WriteInline(CSharpAuthor.IOutputContext outputContext) { } } public abstract class BaseBlockDefinition : CSharpAuthor.BaseOutputComponent { @@ -113,6 +115,7 @@ namespace CSharpAuthor public class ClassDefinition : CSharpAuthor.BaseOutputComponent, CSharpAuthor.IConstructContainer, CSharpAuthor.INamedComponent { public ClassDefinition(string name) { } + public System.Collections.Generic.IReadOnlyList Constraints { get; } public System.Collections.Generic.IReadOnlyList Constructors { get; } public int FieldCount { get; } public System.Collections.Generic.IReadOnlyList Fields { get; } @@ -120,11 +123,14 @@ namespace CSharpAuthor public System.Collections.Generic.IReadOnlyList Methods { get; } public string Name { get; } public System.Collections.Generic.IReadOnlyList Properties { get; } + public bool TerminateWithSemicolon { get; set; } public CSharpAuthor.ClassKeyword TypeKeyword { get; set; } public CSharpAuthor.IOutputComponent? WhereStatement { get; set; } public CSharpAuthor.ClassDefinition AddBaseType(CSharpAuthor.ITypeDefinition typeDefinition) { } + public CSharpAuthor.ClassDefinition AddBaseType(CSharpAuthor.ITypeDefinition typeDefinition, params CSharpAuthor.IOutputComponent[] arguments) { } public CSharpAuthor.ClassDefinition AddClass(string name) { } public void AddComponent(CSharpAuthor.IOutputComponent outputComponent) { } + public CSharpAuthor.ConstraintDefinition AddConstraint(string typeParameter) { } public CSharpAuthor.ConstructorDefinition AddConstructor(CSharpAuthor.IOutputComponent? baseComponent = null) { } public CSharpAuthor.EnumDefinition AddEnum(string name) { } public CSharpAuthor.EventDefinition AddEvent(CSharpAuthor.ITypeDefinition handlerType, string name) { } @@ -186,10 +192,25 @@ namespace CSharpAuthor NoAccessibility = 2048, Sealed = 4096, } + public class ConstraintDefinition + { + public ConstraintDefinition(string typeParameter) { } + public bool IsEmpty { get; } + public string TypeParameter { get; } + public CSharpAuthor.ConstraintDefinition Class(bool nullable = false) { } + public CSharpAuthor.ConstraintDefinition Default() { } + public CSharpAuthor.ConstraintDefinition DefaultConstructor() { } + public CSharpAuthor.ConstraintDefinition Implements(CSharpAuthor.ITypeDefinition type) { } + public CSharpAuthor.ConstraintDefinition NotNull() { } + public CSharpAuthor.ConstraintDefinition Struct() { } + public CSharpAuthor.ConstraintDefinition Unmanaged() { } + public void WriteOutput(CSharpAuthor.IOutputContext outputContext) { } + } public class ConstructorDefinition : CSharpAuthor.MethodDefinition { public ConstructorDefinition(string name, CSharpAuthor.IOutputComponent? base = null) { } public CSharpAuthor.IOutputComponent? Base { get; } + public bool IsPrimary { get; set; } protected override void WriteAccessModifier(CSharpAuthor.IOutputContext outputContext) { } protected override void WriteEndOfMethodSignature(CSharpAuthor.IOutputContext outputContext) { } protected override void WriteReturnType(CSharpAuthor.IOutputContext outputContext) { } @@ -214,12 +235,14 @@ namespace CSharpAuthor public CSharpAuthor.EnumDefinition AddFlags() { } public CSharpAuthor.EnumValueDefinition AddValue(string enumValueName) { } public CSharpAuthor.EnumValueDefinition AddValue(string enumValueName, object value) { } + protected override void WriteComment(CSharpAuthor.IOutputContext outputContext) { } protected override void WriteComponentOutput(CSharpAuthor.IOutputContext outputContext) { } } public class EnumValueDefinition : CSharpAuthor.BaseOutputComponent { public EnumValueDefinition(string enumValueName) { } public object? Value { get; set; } + protected override void WriteComment(CSharpAuthor.IOutputContext outputContext) { } protected override void WriteComponentOutput(CSharpAuthor.IOutputContext outputContext) { } } public class EventDefinition : CSharpAuthor.BaseOutputComponent, CSharpAuthor.INamedComponent @@ -445,6 +468,7 @@ namespace CSharpAuthor protected readonly System.Collections.Generic.List ParameterList; protected int VariableCount; public MethodDefinition(string name) { } + public System.Collections.Generic.IReadOnlyList Constraints { get; } public System.Collections.Generic.List GenericParameters { get; } public CSharpAuthor.ITypeDefinition? InterfaceImplementation { get; set; } public string Name { get; } @@ -452,6 +476,7 @@ namespace CSharpAuthor public string? ReturnComment { get; set; } public CSharpAuthor.ITypeDefinition? ReturnType { get; } public CSharpAuthor.IOutputComponent? WhereStatement { get; set; } + public CSharpAuthor.ConstraintDefinition AddConstraint(string typeParameter) { } public void AddGenericParameter(CSharpAuthor.ITypeDefinition typeDefinition) { } public CSharpAuthor.MethodDefinition AddParameter(CSharpAuthor.ParameterDefinition parameterDefinition) { } public CSharpAuthor.ParameterDefinition AddParameter(CSharpAuthor.ITypeDefinition typeDefinition, string name) { } @@ -471,6 +496,7 @@ namespace CSharpAuthor { public NamespaceDefinition(string ns = "") { } public bool FileScopedNamespace { get; set; } + public string Namespace { get; } public CSharpAuthor.ClassDefinition AddClass(string name) { } public void AddComponent(CSharpAuthor.IOutputComponent component) { } public CSharpAuthor.EnumDefinition AddEnum(string name) { } @@ -1311,7 +1337,7 @@ namespace DependencyModules.SourceGenerator.Impl.Models } public class InterceptedMemberModel : System.IEquatable { - public InterceptedMemberModel(string Name, string Identifier, DependencyModules.SourceGenerator.Impl.Models.AccessorForm Form, CSharpAuthor.ITypeDefinition? ReturnType, CSharpAuthor.ITypeDefinition ResultType, System.Collections.Generic.IReadOnlyList Parameters, System.Collections.Generic.IReadOnlyList TypeParameters, DependencyModules.SourceGenerator.Impl.Models.ReturnShape ReturnShape) { } + public InterceptedMemberModel(string Name, string Identifier, DependencyModules.SourceGenerator.Impl.Models.AccessorForm Form, CSharpAuthor.ITypeDefinition? ReturnType, CSharpAuthor.ITypeDefinition ResultType, System.Collections.Generic.IReadOnlyList Parameters, System.Collections.Generic.IReadOnlyList TypeParameters, DependencyModules.SourceGenerator.Impl.Models.ReturnShape ReturnShape) { } public DependencyModules.SourceGenerator.Impl.Models.AccessorForm Form { get; init; } public string Identifier { get; init; } public DependencyModules.SourceGenerator.Impl.Models.InterceptorKind Kind { get; } @@ -1320,7 +1346,7 @@ namespace DependencyModules.SourceGenerator.Impl.Models public CSharpAuthor.ITypeDefinition ResultType { get; init; } public DependencyModules.SourceGenerator.Impl.Models.ReturnShape ReturnShape { get; init; } public CSharpAuthor.ITypeDefinition? ReturnType { get; init; } - public System.Collections.Generic.IReadOnlyList TypeParameters { get; init; } + public System.Collections.Generic.IReadOnlyList TypeParameters { get; init; } public virtual bool Equals(DependencyModules.SourceGenerator.Impl.Models.InterceptedMemberModel? other) { } public override int GetHashCode() { } } @@ -1333,12 +1359,6 @@ namespace DependencyModules.SourceGenerator.Impl.Models public string Name { get; init; } public CSharpAuthor.ITypeDefinition Type { get; init; } } - public class InterceptedTypeParameterModel : System.IEquatable - { - public InterceptedTypeParameterModel(string Name, string Constraints) { } - public string Constraints { get; init; } - public string Name { get; init; } - } public class InterceptionRefusal : System.IEquatable { public InterceptionRefusal(string Message) { } @@ -1353,7 +1373,7 @@ namespace DependencyModules.SourceGenerator.Impl.Models public class InterceptorModel : System.IEquatable { public static readonly DependencyModules.SourceGenerator.Impl.Models.InterceptorModel Ignore; - public InterceptorModel(CSharpAuthor.ITypeDefinition ServiceType, CSharpAuthor.ITypeDefinition ImplementationType, System.Collections.Generic.IReadOnlyList Interceptors, System.Collections.Generic.IReadOnlyList Members, System.Collections.Generic.IReadOnlyList Declarations, int Order, DependencyModules.SourceGenerator.Impl.Models.InterceptionRefusal? Refusal = null, System.Collections.Generic.IReadOnlyList? TypeParameters = null) { } + public InterceptorModel(CSharpAuthor.ITypeDefinition ServiceType, CSharpAuthor.ITypeDefinition ImplementationType, System.Collections.Generic.IReadOnlyList Interceptors, System.Collections.Generic.IReadOnlyList Members, System.Collections.Generic.IReadOnlyList Declarations, int Order, DependencyModules.SourceGenerator.Impl.Models.InterceptionRefusal? Refusal = null, System.Collections.Generic.IReadOnlyList? TypeParameters = null) { } public System.Collections.Generic.IReadOnlyList Declarations { get; init; } public CSharpAuthor.ITypeDefinition ImplementationType { get; init; } public System.Collections.Generic.IReadOnlyList Interceptors { get; init; } @@ -1363,7 +1383,7 @@ namespace DependencyModules.SourceGenerator.Impl.Models public int Order { get; init; } public DependencyModules.SourceGenerator.Impl.Models.InterceptionRefusal? Refusal { get; init; } public CSharpAuthor.ITypeDefinition ServiceType { get; init; } - public System.Collections.Generic.IReadOnlyList? TypeParameters { get; init; } + public System.Collections.Generic.IReadOnlyList? TypeParameters { get; init; } public static DependencyModules.SourceGenerator.Impl.Models.InterceptorModel Refused(string message) { } } public class InterceptorModelComparer : System.Collections.Generic.IEqualityComparer @@ -1523,6 +1543,16 @@ namespace DependencyModules.SourceGenerator.Impl.Models public DependencyModules.SourceGenerator.Impl.Models.RegistrationType? RegistrationType { get; init; } public CSharpAuthor.ITypeDefinition ServiceType { get; init; } } + public class TypeParameterModel : System.IEquatable + { + public TypeParameterModel(string Name, string? Primary, System.Collections.Generic.IReadOnlyList ConstraintTypes, bool DefaultConstructor) { } + public System.Collections.Generic.IReadOnlyList ConstraintTypes { get; init; } + public bool DefaultConstructor { get; init; } + public string Name { get; init; } + public string? Primary { get; init; } + public virtual bool Equals(DependencyModules.SourceGenerator.Impl.Models.TypeParameterModel? other) { } + public override int GetHashCode() { } + } } namespace DependencyModules.SourceGenerator.Impl.Utilities { @@ -1691,6 +1721,10 @@ namespace DependencyModules.SourceGenerator.Impl.Utilities public static DependencyModules.SourceGenerator.Impl.Utilities.SyntaxTransformContext op_Implicit(Microsoft.CodeAnalysis.GeneratorAttributeSyntaxContext context) { } public static DependencyModules.SourceGenerator.Impl.Utilities.SyntaxTransformContext op_Implicit(Microsoft.CodeAnalysis.GeneratorSyntaxContext context) { } } + public static class TypeParameterReader + { + public static DependencyModules.SourceGenerator.Impl.Models.TypeParameterModel Read(Microsoft.CodeAnalysis.ITypeParameterSymbol parameter) { } + } public static class TypeSyntaxExtensions { public static string GetFullName(this Microsoft.CodeAnalysis.INamespaceSymbol? namespaceSymbol) { } diff --git a/website/guide/interception.md b/website/guide/interception.md index 4ea56cb..1d5fe96 100644 --- a/website/guide/interception.md +++ b/website/guide/interception.md @@ -141,9 +141,7 @@ The generator has to emit a real override, so some shapes are impossible. These - by-reference returns - `init`-only setters - static members -- a generic implementation whose type parameters are **constrained** — the wrapper would have to - repeat the constraint, and there is no way to emit one. An unconstrained generic implementation is - intercepted; see below. +- a generic *method* whose shape the wrapper cannot forward, by the same rules as above ::: warning One such member disables interception for the whole interface There is no partial wrapper. A single `out` parameter anywhere on the interface means no wrapper is @@ -180,8 +178,9 @@ specific to interception — a plain `[SingletonService]` on a generic class beh [Trimming and AOT](/guide/aot#what-it-does-not-cover). ::: -A **constrained** type parameter is refused, because the wrapper cannot repeat the constraint. Give -the service a closed construction to intercept instead. +Constraints come along with the parameters. `Repository where T : class, IEntity, new()` is wrapped +by `Repository_Intercepted : IRepository where T : class, IEntity, new()`, because without them +the wrapper could not reference what it wraps. ## When an interceptor covers only some members diff --git a/website/reference/diagnostics.md b/website/reference/diagnostics.md index 415496d..10c056a 100644 --- a/website/reference/diagnostics.md +++ b/website/reference/diagnostics.md @@ -98,8 +98,7 @@ Their nesting would be ambiguous. See [Decorators](/guide/decorators#ordering). **A service marked for interception cannot be wrapped.** The member uses `ref`, `in`, `out` or a `ref struct` parameter, returns by reference, has an -`init`-only setter, or is static — or the implementation is generic with a **constrained** type -parameter, which the generated wrapper would have to repeat and cannot. +`init`-only setter, or is static. One such member costs the whole interface: no wrapper is generated, so every other member goes uninterceped too. The message names the first offender it found. See