Non-trivial instance construction with factory methods

docs/rules/DAP036.md

@@ -1,4 +1,4 @@
-# DAP035
+# DAP036
 
 Your type has multiple constructors. Vanilla Dapper would be
 happy to pick one based on the exact columns, but Dapper.AOT wants

docs/rules/DAP039.md

@@ -0,0 +1,35 @@
+# DAP039
+
+_Terminology:_ **factory method** is a static method that returns an instance of a type.
+
+It looks like you have multiple factory methods on a type marked `[ExplicitConstructor]`; that's just confusing!
+Pick one, and remove the attribute from the others. This should *probably* be the one with the most parameters.
+
+Bad:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ [ExplicitConstructor]
+ public static MyType Create1(int a) => new MyType { A = a };
+
+ [ExplicitConstructor]
+ public static MyType Create2(int a) => new MyType { A = a };
+}
+```
+
+Good:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ [ExplicitConstructor]
+ public static MyType Create1(int a) => new MyType { A = a };
+
+ public static MyType Create2(int a) => new MyType { A = a };
+}
+```

docs/rules/DAP040.md

@@ -0,0 +1,34 @@
+# DAP040
+
+_Terminology:_ **factory method** is a static method that returns an instance of a type.
+
+Your type has multiple factory methods. Dapper.AOT expects a type to have a single constructor to use with all data.
+You might consider marking your preferred constructor with `[ExplicitConstructor]`.
+This should *probably* be the one with the most parameters.
+
+Bad:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ public static MyType Create1(int a) => new MyType { A = a };
+
+ public static MyType Create2(int a) => new MyType { A = a };
+}
+```
+
+Good:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ [ExplicitConstructor]
+ public static MyType Create1(int a) => new MyType { A = a };
+
+ public static MyType Create2(int a) => new MyType { A = a };
+}
+```

docs/rules/DAP041.md

@@ -0,0 +1,53 @@
+# DAP041
+
+_Terminology:_ **factory method** is a static method that returns an instance of a type.
+
+Your type has both at least one _constructor_ and at least one _factory method_ marked with `[ExplicitConstructor]`.
+Dapper.AOT simply **uses the constructor** in such a case. Recommended action is to remove the `[ExplicitConstructor]` attribute from one of the construction mechanisms.
+
+Bad:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ // constructor
+ [ExplicitConstructor] 
+ public MyType(int a) { A = a; }
+
+ // factory method
+ [ExplicitConstructor]
+ public static MyType Create(int a) => new MyType { A = a };
+}
+```
+
+Good:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ // constructor
+ [ExplicitConstructor] 
+ public MyType(int a) { A = a; }
+
+ public static MyType Create(int a) => new MyType { A = a };
+}
+```
+
+Also Good could be:
+
+``` csharp
+class MyType
+{
+ public int A { get; private set; }
+
+ public MyType(int a) { A = a; }
+
+ // factory method
+ [ExplicitConstructor]
+ public static MyType Create(int a) => new MyType { A = a };
+}
+``` 

src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs

@@ -47,6 +47,9 @@ public static readonly DiagnosticDescriptor
  ConstructorAmbiguous = LibraryError("DAP036", "Ambiguous constructors", "Type '{0}' has more than 1 constructor; mark one constructor with [ExplicitConstructor] or reduce constructors"),
  UserTypeNoSettableMembersFound = LibraryError("DAP037", "No settable members exist for user type", "Type '{0}' has no settable fields or properties"),
  ValueTypeSingleFirstOrDefaultUsage = LibraryWarning("DAP038", "Value-type single row 'OrDefault' usage", "Type '{0}' is a value-type; it will not be trivial to identify missing rows from {1}"),
+ FactoryMethodMultipleExplicit = LibraryError("DAP039", "Multiple explicit factory methods", "Only one factory method should be marked [ExplicitConstructor] for type '{0}'"),
+ FactoryMethodAmbiguous = LibraryError("DAP040", "Ambiguous factory methods", "Type '{0}' has more than 1 factory method; mark one factory method with [ExplicitConstructor] or reduce factory methods"),
+ ConstructorOverridesFactoryMethod = LibraryWarning("DAP041", "Constructor overrides factory method", "Type '{0}' has both constructor and factory method; Constructor will be used instead of a factory method"),
 
  // SQL parse specific
  GeneralSqlError = SqlWarning("DAP200", "SQL error", "SQL error: {0}"),

src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.cs

@@ -206,18 +206,37 @@ private void ValidateDapperMethod(in OperationAnalysisContext ctx, IOperation sq
  ctx.ReportDiagnostic(Diagnostic.Create(Diagnostics.ValueTypeSingleFirstOrDefaultUsage, location, resultMap.ElementType.Name, invoke.TargetMethod.Name));
  }
 
- // check for constructors on the materialized type
- DiagnosticDescriptor? ctorFault = ChooseConstructor(resultMap.ElementType, out var ctor) switch
+ // check for constructors and factoryMethods on the materialized type
+ var ctorFault = ChooseConstructor(resultMap.ElementType, out var ctor) switch
  {
  ConstructorResult.FailMultipleExplicit => Diagnostics.ConstructorMultipleExplicit,
  ConstructorResult.FailMultipleImplicit when aotEnabled => Diagnostics.ConstructorAmbiguous,
  _ => null,
  };
+ var factoryMethodFault = ChooseFactoryMethod(resultMap.ElementType, out var factoryMethod) switch
+ {
+ FactoryMethodResult.FailMultipleExplicit => Diagnostics.FactoryMethodMultipleExplicit,
+ FactoryMethodResult.FailMultipleImplicit when aotEnabled => Diagnostics.FactoryMethodAmbiguous,
+ _ => null,
+ };
+
+ // we cant use both ctor and factoryMethod, so reporting a warning that ctor is prioritized
+ if (ctor is not null && factoryMethod is not null)
+ {
+ var loc = factoryMethod?.Locations.FirstOrDefault() ?? resultMap.ElementType.Locations.FirstOrDefault();
+ ctx.ReportDiagnostic(Diagnostic.Create(Diagnostics.ConstructorOverridesFactoryMethod, loc, resultMap.ElementType.GetDisplayString()));
+ }
+
  if (ctorFault is not null)
  {
  var loc = ctor?.Locations.FirstOrDefault() ?? resultMap.ElementType.Locations.FirstOrDefault();
  ctx.ReportDiagnostic(Diagnostic.Create(ctorFault, loc, resultMap.ElementType.GetDisplayString()));
  }
+ else if (factoryMethodFault is not null)
+ {
+ var loc = factoryMethod?.Locations.FirstOrDefault() ?? resultMap.ElementType.Locations.FirstOrDefault();
+ ctx.ReportDiagnostic(Diagnostic.Create(factoryMethodFault, loc, resultMap.ElementType.GetDisplayString()));
+ }
  else if (resultMap.Members.IsDefaultOrEmpty && IsPublicOrAssemblyLocal(resultType, parseState, out _))
  {
  // there are so settable members + there is no constructor to use

src/Dapper.AOT.Analyzers/CodeAnalysis/DapperInterceptorGenerator.cs

@@ -654,7 +654,7 @@ private static void WriteRowFactory(in GenerateState context, CodeWriter sb, ITy
 
  var members = map.Members;
 
- if (members.IsDefaultOrEmpty && map.Constructor is null)
+ if (members.IsDefaultOrEmpty && map.Constructor is null && map.FactoryMethod is null)
  {
  // error is emitted, but we still generate default RowFactory to not emit more errors for this type
  WriteRowFactoryHeader();
@@ -664,8 +664,13 @@ private static void WriteRowFactory(in GenerateState context, CodeWriter sb, ITy
  }
 
  var hasInitOnlyMembers = members.Any(member => member.IsInitOnly);
- var hasGetOnlyMembers = members.Any(member => member.IsGettable && !member.IsSettable && !member.IsInitOnly);
- var useDeferredConstruction = map.Constructor is not null || hasInitOnlyMembers || hasGetOnlyMembers;
+ var hasGetOnlyMembers = members.Any(member => member is { IsGettable: true, IsSettable: false, IsInitOnly: false });
+ var useConstructorDeferred = map.Constructor is not null;
+ var useFactoryMethodDeferred = map.FactoryMethod is not null;
+
+ // Implementation detail: 
+ // constructor takes advantage over factory method.
+ var useDeferredConstruction = useConstructorDeferred || useFactoryMethodDeferred || hasInitOnlyMembers || hasGetOnlyMembers;
 
  WriteRowFactoryHeader();
 
@@ -720,7 +725,8 @@ void WriteReadMethod()
  sb.Append("public override ").Append(type).Append(" Read(global::System.Data.Common.DbDataReader reader, global::System.ReadOnlySpan<int> tokens, int columnOffset, object? state)").Indent().NewLine();
 
  int token = 0;
- var constructorArgumentsOrdered = new SortedList<int, string>();
+ var deferredMethodArgumentsOrdered = new SortedList<int, string>();
+
  if (useDeferredConstruction)
  {
  // don't create an instance now, but define the variables to create an instance later like 
@@ -736,12 +742,14 @@ void WriteReadMethod()
  if (Inspection.CouldBeNullable(member.CodeType)) sb.Append(CodeWriter.GetTypeName(member.CodeType.WithNullableAnnotation(NullableAnnotation.Annotated)));
  else sb.Append(CodeWriter.GetTypeName(member.CodeType));
  sb.Append(' ').Append(variableName).Append(" = default;").NewLine();
-
- // filling in the constructor arguments in first iteration through members
- // will be used afterwards to create the instance
- if (member.ConstructorParameterOrder is not null)
+
+ if (useConstructorDeferred && member.ConstructorParameterOrder is not null)
  {
- constructorArgumentsOrdered.Add(member.ConstructorParameterOrder.Value, variableName);
+ deferredMethodArgumentsOrdered.Add(member.ConstructorParameterOrder.Value, variableName);
+ }
+ else if (useFactoryMethodDeferred && member.FactoryMethodParameterOrder is not null)
+ {
+ deferredMethodArgumentsOrdered.Add(member.FactoryMethodParameterOrder.Value, variableName);
  }
 
  token++;
@@ -802,31 +810,53 @@ void WriteReadMethod()
 
  if (useDeferredConstruction)
  {
- // create instance using constructor. like
+ // create instance using constructor or factory method. like
  // ```
  // return new Type(member0, member1, member2, ...)
  // {
  // SettableMember1 = member3,
  // SettableMember2 = member4,
  // }
  // ```
-
- sb.Append("return new ").Append(type);
- if (map.Constructor is not null && constructorArgumentsOrdered.Count != 0)
+ // or in case of factory method:
+ // return Type.Create(member0, member1, member2, ...)
+ // ```
+
+ if (useConstructorDeferred)
  {
- // write `(member0, member1, member2, ...)` part of constructor
- sb.Append('(');
- foreach (var constructorArg in constructorArgumentsOrdered)
- {
- sb.Append(constructorArg.Value).Append(", ");
- }
- sb.RemoveLast(2); // remove last ', ' generated in the loop
+ // `return new Type(member0, member1, member2, ...);`
+ sb.Append("return new ").Append(type).Append('('); 
+ WriteDeferredMethodArgs();
  sb.Append(')');
+ WriteDeferredInitialization();
+ sb.Append(";").Outdent();
  }
-
- // if all members are constructor arguments, no need to set them again
- if (constructorArgumentsOrdered.Count != members.Length)
+ else if (useFactoryMethodDeferred)
  {
+ // `return Type.FactoryCreate(member0, member1, member2, ...);`
+ sb.Append("return ").Append(type)
+ .Append('.').Append(map.FactoryMethod!.Name).Append('(');
+ WriteDeferredMethodArgs();
+ sb.Append(')').Append(";").Outdent();
+ }
+ else
+ {
+ // left case is GetOnly or InitOnly - we can use only init syntax like:
+ // return new Type
+ // {
+ // Member1 = value1,
+ // Member2 = value2
+ // }
+ sb.Append("return new ").Append(type);
+ WriteDeferredInitialization();
+ sb.Append(";").Outdent();
+ }
+
+ void WriteDeferredInitialization()
+ { 
+ // if all members are constructor arguments, no need to set them again
+ if (deferredMethodArgumentsOrdered!.Count == members.Length) return;
+
  sb.Indent().NewLine();
  token = -1;
  foreach (var member in members)
@@ -837,8 +867,18 @@ void WriteReadMethod()
  }
  sb.Outdent(withScope: false).Append("}");
  }
-
- sb.Append(";").Outdent();
+
+ void WriteDeferredMethodArgs()
+ {
+ if (deferredMethodArgumentsOrdered!.Count == 0) return;
+
+ // write `member0, member1, member2, ...` part of method
+ foreach (var constructorArg in deferredMethodArgumentsOrdered!)
+ {
+ sb.Append(constructorArg.Value).Append(", ");
+ }
+ sb.RemoveLast(2); // remove last ', ' generated in the loop
+ }
  }
  else
  {