Skip to content

Commit 0564c88

Browse files
committed
- Generated default ctor and deserialization now accounts for situations where a default ctor is infeasible.
- New analyzer warns if field initializers might be skipped on a [Wrapper]ValueObject.
1 parent b9e437e commit 0564c88

File tree

10 files changed

+216
-24
lines changed

10 files changed

+216
-24
lines changed
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
using System.Collections.Immutable;
2+
using Microsoft.CodeAnalysis;
3+
using Microsoft.CodeAnalysis.CSharp;
4+
using Microsoft.CodeAnalysis.CSharp.Syntax;
5+
using Microsoft.CodeAnalysis.Diagnostics;
6+
7+
namespace Architect.DomainModeling.Analyzer.Analyzers;
8+
9+
/// <summary>
10+
/// In [Wrapper]ValueObjects, this analyzer warns against the use of property/field initializers in classes without a default constructor (such as when a primary constructor is used).
11+
/// Without a default constructor, deserialization uses GetUninitializedObject(), which skips property/field initializers, likely to the developer's surprise.
12+
/// </summary>
13+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
14+
public sealed class ValueObjectFieldInitializerWithoutDefaultCtorAnalyzer : DiagnosticAnalyzer
15+
{
16+
[System.Diagnostics.CodeAnalysis.SuppressMessage("CodeQuality", "IDE0079:Remove unnecessary suppression", Justification = "False positive.")]
17+
[System.Diagnostics.CodeAnalysis.SuppressMessage("MicrosoftCodeAnalysisReleaseTracking", "RS2008:Enable analyzer release tracking", Justification = "Not yet implemented.")]
18+
private static readonly DiagnosticDescriptor DiagnosticDescriptor = new DiagnosticDescriptor(
19+
id: "ValueObjectFieldInitializerWithoutDefaultConstructor",
20+
title: "ValueObject has field initializers but no default constructor",
21+
messageFormat: "Field initializer on value object with no default constructor. Lack of a default constructor forces deserialization to use GetUninitializedObject(), which skips field initializers. Consider a calculated property (=> syntax) to avoid field initializers.",
22+
category: "Design",
23+
defaultSeverity: DiagnosticSeverity.Warning,
24+
isEnabledByDefault: true);
25+
26+
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [DiagnosticDescriptor];
27+
28+
public override void Initialize(AnalysisContext context)
29+
{
30+
context.EnableConcurrentExecution();
31+
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze);
32+
33+
context.RegisterSyntaxNodeAction(AnalyzeFieldDeclaration, SyntaxKind.FieldDeclaration);
34+
context.RegisterSyntaxNodeAction(AnalyzePropertyDeclaration, SyntaxKind.PropertyDeclaration);
35+
}
36+
37+
private static void AnalyzeFieldDeclaration(SyntaxNodeAnalysisContext context)
38+
{
39+
var field = (FieldDeclarationSyntax)context.Node;
40+
var semanticModel = context.SemanticModel;
41+
42+
if (!IsMemberOfRelevantType(field, semanticModel, context.CancellationToken))
43+
return;
44+
45+
// A field declaration can actually define multiple fields at once: private int One, Two = 1, 2;
46+
foreach (var fieldVariable in field.Declaration.Variables)
47+
{
48+
if (fieldVariable.Initializer?.Value is not { } initializer)
49+
continue;
50+
51+
// When using a primary constructor, then we use field initializers to assign its parameters
52+
// Such initializers are fine
53+
// It is only OTHER (non-parameterized) initalizers that are likely to cause confusion
54+
if (InitializerReferencesAnyConstructorParameter(initializer, semanticModel, context.CancellationToken))
55+
return;
56+
57+
context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptor, fieldVariable.Initializer.GetLocation()));
58+
}
59+
}
60+
61+
private static void AnalyzePropertyDeclaration(SyntaxNodeAnalysisContext context)
62+
{
63+
var property = (PropertyDeclarationSyntax)context.Node;
64+
var semanticModel = context.SemanticModel;
65+
66+
if (!IsMemberOfRelevantType(property, semanticModel, context.CancellationToken))
67+
return;
68+
69+
if (property.Initializer?.Value is not { } initializer)
70+
return;
71+
72+
// When using a primary constructor, then we use field initializers to assign its parameters
73+
// Such initializers are fine
74+
// It is only OTHER (non-parameterized) initalizers that are likely to cause confusion
75+
if (InitializerReferencesAnyConstructorParameter(initializer, semanticModel, context.CancellationToken))
76+
return;
77+
78+
context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptor, property.Initializer.GetLocation()));
79+
}
80+
81+
private static bool IsMemberOfRelevantType(MemberDeclarationSyntax member, SemanticModel semanticModel, CancellationToken cancellationToken)
82+
{
83+
if (member.Parent is not TypeDeclarationSyntax tds)
84+
return false;
85+
86+
if (semanticModel.GetDeclaredSymbol(tds, cancellationToken) is not { } type)
87+
return false;
88+
89+
// Only in reference types with the [Wrapper]ValueObjectAttributes
90+
if (type.IsValueType || !type.GetAttributes().Any(attr => attr.AttributeClass is
91+
{
92+
Name: "ValueObjectAttribute" or "WrapperValueObjectAttribute",
93+
ContainingNamespace: { Name: "DomainModeling", ContainingNamespace: { Name: "Architect", ContainingNamespace.IsGlobalNamespace: true, } },
94+
}))
95+
return false;
96+
97+
// Only without a default ctor
98+
if (type.InstanceConstructors.Any(ctor => ctor.Parameters.Length == 0))
99+
return false;
100+
101+
return true;
102+
}
103+
104+
private static bool InitializerReferencesAnyConstructorParameter(ExpressionSyntax initializer, SemanticModel semanticModel, CancellationToken cancellationToken)
105+
{
106+
// The initializer might reference constructor parameters
107+
// However, it might also DECLARE parameters and then reference them, which is irrelevant to us
108+
// To see the distinction, first observe which parameters are declared INSIDE the initializer
109+
var parametersDeclaredInsideInitializer = new HashSet<IParameterSymbol>(SymbolEqualityComparer.Default);
110+
foreach (var parameterSyntax in initializer.DescendantNodesAndSelf().OfType<ParameterSyntax>())
111+
if (semanticModel.GetDeclaredSymbol(parameterSyntax, cancellationToken) is { } parameterSymbol)
112+
parametersDeclaredInsideInitializer.Add(parameterSymbol);
113+
114+
foreach (var id in initializer.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>())
115+
{
116+
var symbol = semanticModel.GetSymbolInfo(id, cancellationToken).Symbol;
117+
if (symbol is IParameterSymbol parameterSymbol && !parametersDeclaredInsideInitializer.Contains(parameterSymbol))
118+
{
119+
// Parameter originates outside initializer
120+
// This initializer uses primary ctor params
121+
return true;
122+
}
123+
}
124+
return false;
125+
}
126+
}

DomainModeling.Generator/DomainEventGenerator.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ private static bool FilterSyntaxNode(SyntaxNode node, CancellationToken cancella
7474

7575
var existingComponents = DomainEventTypeComponents.None;
7676

77-
existingComponents |= DomainEventTypeComponents.DefaultConstructor.If(type.Constructors.Any(ctor =>
78-
!ctor.IsStatic && ctor.Parameters.Length == 0 /*&& ctor.DeclaringSyntaxReferences.Length > 0*/));
77+
existingComponents |= DomainEventTypeComponents.DefaultConstructor.If(type.InstanceConstructors.Any(ctor =>
78+
ctor.Parameters.Length == 0 /*&& ctor.DeclaringSyntaxReferences.Length > 0*/));
7979

8080
result.ExistingComponents = existingComponents;
8181

DomainModeling.Generator/EntityGenerator.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ private static bool FilterSyntaxNode(SyntaxNode node, CancellationToken cancella
7474

7575
var existingComponents = EntityTypeComponents.None;
7676

77-
existingComponents |= EntityTypeComponents.DefaultConstructor.If(type.Constructors.Any(ctor =>
78-
!ctor.IsStatic && ctor.Parameters.Length == 0 /*&& ctor.DeclaringSyntaxReferences.Length > 0*/));
77+
existingComponents |= EntityTypeComponents.DefaultConstructor.If(type.InstanceConstructors.Any(ctor =>
78+
ctor.Parameters.Length == 0 /*&& ctor.DeclaringSyntaxReferences.Length > 0*/));
7979

8080
result.ExistingComponents = existingComponents;
8181

DomainModeling.Generator/IdentityGenerator.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,8 @@ private static bool FilterSyntaxNode(SyntaxNode node, CancellationToken cancella
296296

297297
existingComponents |= IdTypeComponents.UnsettableValue.If(members.Any(member => member.Name == "Value" && member is not IFieldSymbol && member is not IPropertySymbol { SetMethod: not null }));
298298

299-
existingComponents |= IdTypeComponents.Constructor.If(type.Constructors.Any(ctor =>
300-
!ctor.IsStatic && ctor.Parameters.Length == 1 && ctor.Parameters[0].Type.Equals(underlyingType, SymbolEqualityComparer.Default)));
299+
existingComponents |= IdTypeComponents.Constructor.If(type.InstanceConstructors.Any(ctor =>
300+
ctor.Parameters.Length == 1 && ctor.Parameters[0].Type.Equals(underlyingType, SymbolEqualityComparer.Default)));
301301

302302
// Records override this, but our implementation is superior
303303
existingComponents |= IdTypeComponents.ToStringOverride.If(members.Any(member =>

DomainModeling.Generator/TypeSymbolExtensions.cs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System.Collections.Immutable;
22
using System.Runtime.CompilerServices;
33
using Microsoft.CodeAnalysis;
4+
using Microsoft.CodeAnalysis.CSharp.Syntax;
45

56
namespace Architect.DomainModeling.Generator;
67

@@ -503,6 +504,29 @@ public static bool IsOrImplementsInterface(this ITypeSymbol typeSymbol, Func<INa
503504
return false;
504505
}
505506

507+
/// <summary>
508+
/// Returns whether the <see cref="INamedTypeSymbol"/> has either no base type, or a base type that (implicitly or explicitly) exposes a non-private default constructor.
509+
/// </summary>
510+
public static bool BasePermitsDefaultConstruction(this INamedTypeSymbol typeSymbol)
511+
{
512+
if (typeSymbol.BaseType is not { } baseType)
513+
return true;
514+
515+
return baseType.InstanceConstructors.Any(ctor => ctor.Parameters.Length == 0 && ctor.DeclaredAccessibility != Accessibility.Private);
516+
}
517+
518+
/// <summary>
519+
/// Returns whether the <see cref="ITypeSymbol"/> declares a primary constructor.
520+
/// </summary>
521+
public static bool HasPrimaryConstructor(this ITypeSymbol typeSymbol)
522+
{
523+
foreach (var syntaxRef in typeSymbol.DeclaringSyntaxReferences)
524+
if (syntaxRef.GetSyntax() is TypeDeclarationSyntax { ParameterList: not null })
525+
return true;
526+
527+
return false;
528+
}
529+
506530
/// <summary>
507531
/// Returns whether the <see cref="ITypeSymbol"/> represents one of the 8 primitive integral types, such as <see cref="Int32"/> or <see cref="UInt64"/>.
508532
/// </summary>

DomainModeling.Generator/ValueObjectGenerator.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,10 @@ private static bool FilterSyntaxNode(SyntaxNode node, CancellationToken cancella
6262

6363
var existingComponents = ValueObjectTypeComponents.None;
6464

65-
existingComponents |= ValueObjectTypeComponents.DefaultConstructor.If(type.Constructors.Any(ctor =>
66-
!ctor.IsStatic && ctor.Parameters.Length == 0 /*&& ctor.DeclaringSyntaxReferences.Length > 0*/));
65+
existingComponents |= ValueObjectTypeComponents.DefaultConstructor.If(type.InstanceConstructors.Any(ctor =>
66+
ctor.Parameters.Length == 0 /*&& ctor.DeclaringSyntaxReferences.Length > 0*/) ||
67+
!type.BasePermitsDefaultConstruction() || // Base offers no visible default ctor
68+
type.HasPrimaryConstructor()); // Type has a primary ctor
6769

6870
// Records override this, but our implementation is superior
6971
existingComponents |= ValueObjectTypeComponents.ToStringOverride.If(members.Any(member =>

DomainModeling.Generator/WrapperValueObjectGenerator.cs

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ private static bool FilterSyntaxNode(SyntaxNode node, CancellationToken cancella
182182
result.IsComparable = type.AllInterfaces.Any(interf => interf.IsSystemType("IComparable", arity: 1) && interf.TypeArguments[0].Equals(type, SymbolEqualityComparer.Default)) &&
183183
underlyingType.IsComparable(seeThroughNullable: true);
184184
result.IsComparable |= underlyingType.GetAttribute(attr => attr.IsOrInheritsClass("IdentityValueObjectAttribute", "Architect", "DomainModeling", arity: 1, out _)) is not null;
185+
result.LacksDefaultConstructor = !type.IsValueType && (!type.BasePermitsDefaultConstruction() || type.HasPrimaryConstructor());
185186

186187
var members = type.GetMembers();
187188

@@ -191,14 +192,16 @@ private static bool FilterSyntaxNode(SyntaxNode node, CancellationToken cancella
191192

192193
existingComponents |= WrapperValueObjectTypeComponents.UnsettableValue.If(members.Any(member => member.Name == "Value" && member is not IFieldSymbol && member is not IPropertySymbol { SetMethod: not null }));
193194

194-
existingComponents |= WrapperValueObjectTypeComponents.Constructor.If(type.Constructors.Any(ctor =>
195-
!ctor.IsStatic && ctor.Parameters.Length == 1 && ctor.Parameters[0].Type.Equals(underlyingType, SymbolEqualityComparer.Default)));
195+
existingComponents |= WrapperValueObjectTypeComponents.Constructor.If(type.InstanceConstructors.Any(ctor =>
196+
ctor.Parameters.Length == 1 && ctor.Parameters[0].Type.Equals(underlyingType, SymbolEqualityComparer.Default)));
196197

197-
existingComponents |= WrapperValueObjectTypeComponents.NullableConstructor.If(underlyingType.IsValueType && type.Constructors.Any(ctor =>
198-
!ctor.IsStatic && ctor.Parameters.Length == 1 && ctor.Parameters[0].Type.IsNullableOf(underlyingType)));
198+
existingComponents |= WrapperValueObjectTypeComponents.NullableConstructor.If(underlyingType.IsValueType && type.InstanceConstructors.Any(ctor =>
199+
ctor.Parameters.Length == 1 && ctor.Parameters[0].Type.IsNullableOf(underlyingType)));
199200

200-
existingComponents |= WrapperValueObjectTypeComponents.DefaultConstructor.If(type.Constructors.Any(ctor =>
201-
!ctor.IsStatic && ctor.Parameters.Length == 0 && ctor.DeclaringSyntaxReferences.Length > 0));
201+
existingComponents |= WrapperValueObjectTypeComponents.DefaultConstructor.If(
202+
type.InstanceConstructors.Any(ctor => ctor.Parameters.Length == 0 && ctor.DeclaringSyntaxReferences.Length > 0) ||
203+
!type.BasePermitsDefaultConstruction() ||
204+
type.HasPrimaryConstructor());
202205

203206
// Records override this, but our implementation is superior
204207
existingComponents |= WrapperValueObjectTypeComponents.ToStringOverride.If(members.Any(member =>
@@ -609,7 +612,7 @@ public int CompareTo({typeName}{(generatable.IsClass ? "?" : "")} other)
609612
{(existingComponents.HasFlags(WrapperValueObjectTypeComponents.SerializeToUnderlying) ? "*/" : "")}
610613
611614
{(existingComponents.HasFlags(WrapperValueObjectTypeComponents.DeserializeFromUnderlying) ? "/*" : "")}
612-
{(existingComponents.HasFlags(WrapperValueObjectTypeComponents.UnsettableValue) ? $@"
615+
{(generatable.LacksDefaultConstructor || existingComponents.HasFlags(WrapperValueObjectTypeComponents.UnsettableValue) ? $@"
613616
[UnsafeAccessor(UnsafeAccessorKind.Field, Name = ""{valueFieldName}"")]
614617
private static extern ref {underlyingTypeFullyQualifiedName} GetValueFieldReference({typeName} instance);" : "")}
615618
@@ -619,11 +622,14 @@ public int CompareTo({typeName}{(generatable.IsClass ? "?" : "")} other)
619622
[MethodImpl(MethodImplOptions.AggressiveInlining)]
620623
static {typeName} IValueWrapper<{typeName}, {underlyingTypeFullyQualifiedName}>.Deserialize({underlyingTypeFullyQualifiedName} value)
621624
{{
622-
{(existingComponents.HasFlags(WrapperValueObjectTypeComponents.UnsettableValue) ? $@"
625+
{(generatable.LacksDefaultConstructor ? $@"
626+
// To instead get syntax that is safe at compile time, use a value type or ensure that a default constructor is available
627+
var result = ObjectInstantiator<{typeName}>.Instantiate(); GetValueFieldReference(result) = value; return result;" : "")}
628+
{(!generatable.LacksDefaultConstructor && existingComponents.HasFlags(WrapperValueObjectTypeComponents.UnsettableValue) ? $@"
623629
// To instead get syntax that is safe at compile time, make the Value property '{{ get; private init; }}' (or let the source generator implement it)
624630
var result = new {typeName}(); GetValueFieldReference(result) = value; return result;" : "")}
625631
#pragma warning disable CS0618 // Obsolete constructor is intended for us
626-
{(existingComponents.HasFlags(WrapperValueObjectTypeComponents.UnsettableValue) ? "//" : "")}return new {typeName}() {{ Value = value }};
632+
{(generatable.LacksDefaultConstructor || existingComponents.HasFlags(WrapperValueObjectTypeComponents.UnsettableValue) ? "//" : "")}return new {typeName}() {{ Value = value }};
627633
#pragma warning restore CS0618
628634
}}
629635
{(existingComponents.HasFlags(WrapperValueObjectTypeComponents.DeserializeFromUnderlying) ? "*/" : "")}
@@ -793,6 +799,7 @@ private sealed record Generatable
793799
public bool UnderlyingTypeIsString { get => this._bits.GetBit(10); set => this._bits.SetBit(10, value); }
794800
public bool IsToStringNullable { get => this._bits.GetBit(11); set => this._bits.SetBit(11, value); }
795801
public bool UnderlyingTypeIsInterface { get => this._bits.GetBit(12); set => this._bits.SetBit(12, value); }
802+
public bool LacksDefaultConstructor { get => this._bits.GetBit(13); set => this._bits.SetBit(13, value); }
796803
public string ValueFieldName { get; set; } = null!;
797804
public Accessibility Accessibility { get; set; }
798805
public WrapperValueObjectTypeComponents ExistingComponents { get; set; }
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
namespace Architect.DomainModeling.Tests.Analyzers;
2+
3+
#pragma warning disable IDE0079 // Remove unnecessary suppression -- False positive
4+
#pragma warning disable ValueObjectFieldInitializerWithoutDefaultConstructor // ValueObject has field initializers but no default constructor -- Testing presence of warning
5+
6+
// Unfortunately, we always get "unnecessary suppression" even when the warning is successfully suppressed
7+
// All we can do is manually outcomment the suppression temporarily to check that each statement in this file still warns
8+
9+
[ValueObject]
10+
public partial class ValueObjectImplicitConversionOnBinaryOperatorAnalyzerTestValueObject(int one)
11+
{
12+
public int Zero { get; private init; } // Fine, because no field initializer
13+
public int One { get; private init; } = one > 0 ? 1 : 0; // Fine, because field initializer uses primary ctor param
14+
public int Two { get; private init; } = 2; // Warning, because field initializer will not be run during deserialization, for lack of a default ctor (because of primary ctor)
15+
public int Three = 3; // Warning, because field initializer will not be run during deserialization, for lack of a default ctor (because of primary ctor)
16+
}
17+
18+
[WrapperValueObject<string>]
19+
public partial class ValueObjectImplicitConversionOnBinaryOperatorAnalyzerTestWrapperValueObject : Uri
20+
{
21+
public StringComparison StringComparisonAcceptable => StringComparison.Ordinal; // Fine, because no field initializer
22+
public StringComparison StringComparison { get; } = StringComparison.Ordinal; // Warning, because field initializer will not be run during deserialization, for lack of a default ctor (because base has none)
23+
24+
public ValueObjectImplicitConversionOnBinaryOperatorAnalyzerTestWrapperValueObject(string value)
25+
: base(value)
26+
{
27+
this.Value = value ?? throw new ArgumentNullException(nameof(value));
28+
}
29+
}

DomainModeling.Tests/WrapperValueObjectTests.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -858,8 +858,9 @@ public sealed partial class AlreadyPartial : WrapperValueObject<int>
858858
}
859859

860860
// Should compile in spite of already consisting of multiple partials, both with and without the attribute
861-
public partial class AlreadyPartial
861+
public partial class AlreadyPartial(int value)
862862
{
863+
public int Value { get; private init; } = value;
863864
}
864865

865866
// Should be recognized in spite of the attribute and the base class to be defined on different partials
@@ -887,15 +888,17 @@ public sealed partial class IntValue : WrapperValueObject<int>
887888
}
888889

889890
[WrapperValueObject<string>]
890-
public readonly partial record struct StringValue : IComparable<StringValue>
891+
public readonly partial struct StringValue(string value) : IComparable<StringValue>
891892
{
892893
private StringComparison StringComparison => StringComparison.OrdinalIgnoreCase;
893894

894895
public StringComparison GetStringComparison() => this.StringComparison;
896+
897+
public string Value { get; private init; } = value ?? throw new ArgumentNullException(nameof(value));
895898
}
896899

897900
[WrapperValueObject<decimal>]
898-
public readonly partial struct DecimalValue : IComparable<DecimalValue>
901+
public readonly partial record struct DecimalValue : IComparable<DecimalValue>
899902
{
900903
}
901904

0 commit comments

Comments
 (0)