Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/DependencyModules.Runtime/Attributes/InterceptAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,16 @@ public class InterceptAttribute(params Type[] interceptors) : Attribute {
/// <see cref="DecoratorAttribute.Order"/>.
/// </summary>
public int Order { get; set; }

/// <summary>
/// Restricts the interception to a single module, matching the Realm property on the service
/// registration attributes and on <see cref="DecoratorAttribute.Realm"/>.
/// </summary>
/// <remarks>
/// Without a realm the interception belongs to every module that is not realm-only, which is how
/// services and decorators already behave. An <c>OnlyRealm</c> module previously received every
/// interception in the compilation regardless — including ones naming no realm at all — so a
/// module built to be isolated wrapped services it had never heard of.
/// </remarks>
public Type? Realm { get; set; }
}
Original file line number Diff line number Diff line change
@@ -1,2 +1,9 @@
; Unshipped analyzer release
; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

### New Rules

Rule ID | Category | Severity | Notes
--------|----------|----------|-------
DM0017 | DependencyModules | Error | A dependency module is declared inside another type.
DM0018 | DependencyModules | Info | A module with settable properties relies on generated equality.
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,18 @@ private ModuleEntryPointModel GetClassEntryPointModel(GeneratorSyntaxContext con
if (!typeDeclarationSyntax.Modifiers.Any(m => m.IsKind(SyntaxKind.PartialKeyword))) {
features |= ModuleEntryPointFeatures.NotPartial;
}

// A module nested inside another type cannot be completed where it was written: the
// generated half is emitted at namespace level, so it becomes a second, unrelated type and
// the nested declaration never implements IDependencyModule.
if (typeDeclarationSyntax.Parent is TypeDeclarationSyntax) {
features |= ModuleEntryPointFeatures.NestedInType;
}

return new ModuleEntryPointModel(
features,
context.Node.SyntaxTree?.FilePath ?? "",
LocationModel.From(context.Node),
((TypeDeclarationSyntax)context.Node).GetTypeDefinition(),
dependencyFlags.RegistrationType,
dependencyFlags.GenerateAttribute,
Expand Down Expand Up @@ -276,6 +284,7 @@ private ModuleEntryPointModel GetCompilationUnitSyntaxEntry(GeneratorSyntaxConte
return new ModuleEntryPointModel(
ModuleEntryPointFeatures.AutoGenerateModule,
context.Node.SyntaxTree?.FilePath ?? "",
LocationModel.From(context.Node),
TypeDefinition.Get("", "ApplicationModule"),
null,
true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,55 @@ public static class DependencyModuleDiagnostics {
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true);

/// <summary>
/// Raised for a module declared inside another type.
/// </summary>
/// <remarks>
/// The generated half is written at namespace level, so a nested declaration produces a second,
/// detached type of the same name while the nested one never implements
/// <c>IDependencyModule</c>. <c>AddModule&lt;Outer.Nested&gt;()</c> then fails to compile, but
/// <c>[assembly: Nested]</c> binds to the detached type's attribute and builds green, registering
/// nothing.
/// </remarks>
public static readonly DiagnosticDescriptor ModuleCannotBeNested = new(
id: "DM0017",
title: "Dependency module cannot be nested inside another type",
messageFormat:
"'{0}' is marked with [DependencyModule] but is declared inside another type. " +
"The generator completes a module at namespace level, so this would produce a second, " +
"unrelated type and register nothing. Move it out to the namespace.",
category: Category,
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true);

/// <summary>
/// Raised for a module carrying settable properties while relying on the generated
/// <c>Equals</c>/<c>GetHashCode</c>.
/// </summary>
/// <remarks>
/// Modules de-duplicate by type, which is what stops a module reached twice from registering
/// everything twice. A module with parameters is a different case: two instances carrying
/// different values are the same module by that rule, so the first one reached wins and the
/// other is silently discarded. Supplying an <c>Equals</c> that accounts for the properties
/// suppresses the generated one and makes the intent explicit either way.
/// </remarks>
public static readonly DiagnosticDescriptor ModuleWithPropertiesShouldImplementEquals = new(
id: "DM0018",
title: "Module with properties relies on generated equality",
messageFormat:
"'{0}' has settable properties but does not declare Equals, so the generated equality " +
"compares by type alone. Two instances carrying different values count as the same module " +
"and the first one reached wins. Declare Equals and GetHashCode on '{0}' to say which " +
"instances are the same.",
category: Category,
// Info, not Warning. Dedupe-by-type is correct for the common case — a parameterised module
// composed once — and this repo's own integration tests carry eleven such modules that are
// not doing anything wrong. Only a module reached twice with different values is actually
// bitten, and that is not decidable here. Promote it per project with
// dotnet_diagnostic.DM0018.severity = warning if you want it enforced.
defaultSeverity: DiagnosticSeverity.Info,
isEnabledByDefault: true);

/// <summary>
/// Raised when two conventions in one module register a type as the <i>same</i> service type.
/// </summary>
Expand Down Expand Up @@ -89,10 +138,11 @@ public static class DependencyModuleDiagnostics {
public static readonly DiagnosticDescriptor ConventionMatchedNothing = new(
id: "DM0005",
title: "Convention matched no types",
messageFormat:
"The convention registering '{0}' in '{1}' matched no types. " +
"Conventions match a type that declares the service type, or declares an interface " +
"extending it; call IncludeBaseClasses() to also match types that reach it through a base class.",
// The advice is a parameter rather than a fixed tail, because the causes need different
// fixes. Suggesting IncludeBaseClasses() unconditionally told a reader who had already
// called it — or who had named a class, which can never match whatever they call — to go
// looking for a typo in their own code.
messageFormat: "The convention registering '{0}' in '{1}' matched no types. {2}.",
category: Category,
defaultSeverity: DiagnosticSeverity.Warning,
isEnabledByDefault: true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,35 @@ private void ProcessEntryPoint(
context.ReportDiagnostic(
Diagnostic.Create(
DependencyModuleDiagnostics.ModuleMustBePartial,
Location.None,
entryPointModel.Location.ToLocationOrNone(),
entryPointModel.EntryPointType.Name));

return;
}

// Generating anyway would emit a same-named type at namespace level, which compiles and
// registers nothing — the failure this is here to replace.
if (entryPointModel.ModuleFeatures.HasFlag(ModuleEntryPointFeatures.NestedInType)) {
context.ReportDiagnostic(
Diagnostic.Create(
DependencyModuleDiagnostics.ModuleCannotBeNested,
entryPointModel.Location.ToLocationOrNone(),
entryPointModel.EntryPointType.Name));

return;
}

// Only when the generator is supplying equality. A module that declares its own Equals has
// already answered the question this asks about.
if (entryPointModel.PropertyInfoModels.Count > 0 &&
entryPointModel.ModuleFeatures.HasFlag(ModuleEntryPointFeatures.ShouldImplementEquals)) {
context.ReportDiagnostic(
Diagnostic.Create(
DependencyModuleDiagnostics.ModuleWithPropertiesShouldImplementEquals,
entryPointModel.Location.ToLocationOrNone(),
entryPointModel.EntryPointType.Name));
}

entryPointModel = EntryModelUtil.EnsureNamespace(entryPointModel,configurationModel);

var csharpFile = new CSharpFileDefinition(entryPointModel.EntryPointType.Namespace);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,14 @@ public record DecoratorModel(
IReadOnlyList<EnvironmentConditionModel>? Conditions = null,
ConstructorInfoModel? Constructor = null,
int InnerParameterIndex = -1,
bool TypeParametersMatchService = true) {
bool TypeParametersMatchService = true,

/// <summary>
/// Where the decorator was declared, so DM0007 and DM0013 can point at it rather than at the
/// project. Null for a decorator declared through [Decorate] on a module, which names two types
/// and has no declaration of its own to point at.
/// </summary>
LocationModel? Location = null) {

/// <summary>
/// Whether the decorator can be constructed by generated code.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,14 @@ public record InterceptorModel(
IReadOnlyList<InterceptedDeclarationModel> Declarations,
int Order,
InterceptionRefusal? Refusal = null,
IReadOnlyList<TypeParameterModel>? TypeParameters = null) {
IReadOnlyList<TypeParameterModel>? TypeParameters = null,
ITypeDefinition? Realm = null,

/// <summary>
/// Where the intercepted class was declared, so DM0008 and DM0015 can point at it rather than
/// at the project.
/// </summary>
LocationModel? Location = null) {

/// <summary>
/// Whether the intercepted service is an open generic, and so registers as an implementation type
Expand All @@ -368,8 +375,8 @@ public record InterceptorModel(
/// A model that generates nothing and explains why, so an unsupported shape produces a
/// diagnostic rather than a wrapper that does not compile.
/// </summary>
public static InterceptorModel Refused(string message) =>
Ignore with { Refusal = new InterceptionRefusal(message) };
public static InterceptorModel Refused(string message, LocationModel? location = null) =>
Ignore with { Refusal = new InterceptionRefusal(message), Location = location };

public bool IsIgnored => ReferenceEquals(this, Ignore);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace DependencyModules.Conventions.Models;
namespace DependencyModules.SourceGenerator.Impl.Models;

/// <summary>
/// Where a declaration sits in source, in a form that can live in an incremental model.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,22 @@ public enum ModuleEntryPointFeatures {
/// The module was declared without the partial modifier, so the generator cannot complete it.
/// </summary>
NotPartial = 16,

/// <summary>
/// The module was declared inside another type rather than directly in a namespace.
/// </summary>
/// <remarks>
/// Documented as always wrong in the README, the modules guide and the troubleshooting guide,
/// and until now reported by none of them: the generator emitted a second, detached type at
/// namespace level, the nested declaration never became a module, and the build stayed green.
/// </remarks>
NestedInType = 32,
}

public record ModuleEntryPointModel(
ModuleEntryPointFeatures ModuleFeatures,
string FileLocation,
LocationModel Location,
ITypeDefinition EntryPointType,
RegistrationType? RegistrationType,
bool? GenerateAttribute,
Expand All @@ -39,6 +50,12 @@ public bool Equals(ModuleEntryPointModel? x, ModuleEntryPointModel? y) {
if (x is null && y is null) return true;
if (x is null || y is null) return false;

// Location is deliberately absent. It is carried so diagnostics can point at the
// declaration, but it shifts whenever anything above the module is edited — including a
// comment — and this comparer is the incremental cache key. Including it regenerated every
// module on a keystroke that changed nothing (IncrementalGenerationTests covers exactly
// that). The cost is that a diagnostic replayed from cache can sit a line or two off until
// the next semantic edit, which is the cheaper of the two mistakes.
return x.FileLocation == y.FileLocation &&
x.EntryPointType.Equals(y.EntryPointType) &&
x.ModuleFeatures == y.ModuleFeatures &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,15 @@ public record ServiceModel(
/// attributes are declared on the class, so every registration it produces shares them and the
/// writer emits one guard around the lot.
/// </summary>
IReadOnlyList<EnvironmentConditionModel>? Conditions = null) {
IReadOnlyList<EnvironmentConditionModel>? Conditions = null,

/// <summary>
/// Where the implementation was declared, so a diagnostic about it can point at the class
/// rather than at the project. Deliberately absent from
/// <see cref="ServiceModelComparer"/> — it is not part of what makes two models the same
/// registration, and including it would miss the incremental cache on an edit above the class.
/// </summary>
LocationModel? Location = null) {
public static ServiceModel Ignore = new ServiceModel(
TypeDefinition.Get("", "Ignore"),
null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ public static class DecoratorModelUtility {
conditions,
constructor,
IndexOfInnerParameter(constructor, written),
TypeParametersMatchService(typeDeclarationSyntax, written));
TypeParametersMatchService(typeDeclarationSyntax, written),
LocationModel.From(typeDeclarationSyntax));
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,14 @@ public static InterceptorModel GetInterceptorModel(
var interceptorSymbols = new List<INamedTypeSymbol>();
var order = 0;
INamedTypeSymbol? explicitService = null;
INamedTypeSymbol? realm = null;

// Every [Intercept], not the first. The attribute is AllowMultiple, so stacking them is a
// supported way to write what one attribute can also express as a params list — and reading
// only the first dropped every interceptor after it, with nothing to say so.
foreach (var attribute in attributes) {
ReadAttribute(attribute, context, cancellationToken, interceptorSymbols, ref order, ref explicitService);
ReadAttribute(
attribute, context, cancellationToken, interceptorSymbols, ref order, ref explicitService, ref realm);
}

if (interceptorSymbols.Count == 0) {
Expand All @@ -70,11 +72,11 @@ public static InterceptorModel GetInterceptorModel(
var serviceSymbol = ResolveServiceInterface(implementationSymbol, explicitService, out var unsupported);

if (serviceSymbol == null) {
return Refuse(unsupported);
return Refuse(unsupported, context);
}

if (!InterceptedMemberReader.Read(serviceSymbol, out var members, out var declarations, out unsupported)) {
return Refuse(unsupported);
return Refuse(unsupported, context);
}

if (members.Count == 0) {
Expand All @@ -101,13 +103,15 @@ public static InterceptorModel GetInterceptorModel(
members,
declarations,
order,
TypeParameters: TypeParameterModels(implementationSymbol));
TypeParameters: TypeParameterModels(implementationSymbol),
Realm: realm?.GetTypeDefinition(),
Location: LocationModel.From(context.Node));
}

private static InterceptorModel Refuse(string? reason) =>
private static InterceptorModel Refuse(string? reason, SyntaxTransformContext context) =>
reason == null
? InterceptorModel.Ignore
: InterceptorModel.Refused(reason);
: InterceptorModel.Refused(reason, LocationModel.From(context.Node));

/// <summary>
/// The interfaces an interceptor implements, which decide the members it can be placed around.
Expand Down Expand Up @@ -167,7 +171,8 @@ private static void ReadAttribute(
CancellationToken cancellationToken,
List<INamedTypeSymbol> interceptors,
ref int order,
ref INamedTypeSymbol? explicitService) {
ref INamedTypeSymbol? explicitService,
ref INamedTypeSymbol? realm) {

if (attribute.ArgumentList == null) {
return;
Expand Down Expand Up @@ -199,6 +204,11 @@ private static void ReadAttribute(
explicitService = ResolveType(serviceTypeOf, context, cancellationToken);
}
break;
case "Realm":
if (argument.Expression is TypeOfExpressionSyntax realmTypeOf) {
realm = ResolveType(realmTypeOf, context, cancellationToken);
}
break;
}
}
}
Expand Down
Loading
Loading