Non-trivial instance construction with factory methods

[DeagleGross]

Idea:
Currently Dapper.AOT supports instance initialization via constructor using such syntax:

public class Foo
{
    public int Bar { get; set; }
    [DapperAot]
    public Foo(int bar) { Bar = bar; } 
}

Idea is to support initialization of instances via factory methods:

public class Foo
{
    public int Bar { get; set; }
    [DapperAot]
    public static Create(int bar) => new Foo { Bar = bar };
}

---

Details:
1: Only such methods can be considered as factory methods for type Foo:

• public
• static
• with response type Foo (so the same as the containing type)

2: To explicitly show intention to instantiate via the constructor, developer needs to mark constructor of type with ExplicitConstructorAttribute. To not complicate the attribute model, the same mechanism using the same attribute is used for factory methods instantiation.

3: Implementation is not starting to consider invisible (i.e. private) element members, if factory method is chosen to be used. Most of times immutable types with inaccessible members define a factory method (as in example below). To not complicate the flow depending on which construction mechanism is used, PR implementation does not consider hidden members in any of cases.

public class Foo
{
    private readonly int _bar;
    private Foo(int bar) { _bar = bar; }

    public static Foo Create(int bar) => new Foo(bar);
}

4: To not complicate the attribute model, PR implementation prioritizes constructor usage over factory method usage. So in example below instance initialization would be generated using constructor, not factory method. This is also covered by a separate diagnostic DAP041: Constructor overrides factory method:

public class Foo
{
    public int Bar { get; set; }
   
    [ExplicitConstructor]
    public Foo(int bar) { Bar = bar; } 

    [ExplicitConstructor]
    public static Create(int bar) => new Foo(bar);
}

Flow change rules:
0) If type does not have factory method, then current behaviour should not be broken \ changed.

1. If type has no constructor at all, and factory method is in place, then initialization is done via factory method.
2. If type has both constructor and factory method, then constructor is prioritized, and DAP041: Constructor overrides factory method diagnostic is reported (warning level)
3. The same ruling applies as for constructors in terms of ExplicitConstructor attribute:
   • a) single explicitly turned on factory method will be used. If there are multiple explicit factory methods found with ExplicitConstructor attribute, then diagnostic DAP039: Multiple explicit factory methods is reported.
   • b) single implicit factory method (which does not have ExplicitConstructor attribute) will be used. If multiple implicit factory methods found, then diagnostic DAP040: Ambiguous factory methods is reported.
   • c) both multiple explicit or implicit factory methods will not be used -> we dont know what method specifically to choose.

Closes #34

[mgravell]

Can we get a summary in the PR post about what the rules are? When it applies, what error conditions exist, etc.

If there are error conditions that result in diagnostics: check out the "verifiers" folder - we now have extensive unit tests for all diagnostic outputs and see the /docs folder which has explanatory notes about every diagnostic, usually with examples of good/bad usage.

[mgravell]

I haven't looked at the code (hard to until I can see the rules considered :p), but also note: I've moved all but the most essential code to an analyzer now. If the interceptor path can short-cut any additional work needed to check all conditions, that's fine - the analyzer can do the extra work at lower priority.

[mgravell]

Additional additional thought: I wonder if it would be clearer to extend [ExplicitConstructor] (a pre-existing Dapper attribute) to be allowed on methods, and have the system check for exactly one such static method, and warning if it is anywhere else (non-static, multiple, wrong return type, args, etc)

[DeagleGross]

Can we get a summary in the PR post about what the rules are? When it applies, what error conditions exist, etc.

If there are error conditions that result in diagnostics: check out the "verifiers" folder - we now have extensive unit tests for all diagnostic outputs and see the /docs folder which has explanatory notes about every diagnostic, usually with examples of good/bad usage.

Hey @mgravell , I added PR description: please, let me know what you think about assumptions I am making.

Additional additional thought: I wonder if it would be clearer to extend [ExplicitConstructor] (a pre-existing Dapper attribute) to be allowed on methods, and have the system check for exactly one such static method, and warning if it is anywhere else (non-static, multiple, wrong return type, args, etc)

I would propose renaming to [ExplicitConstruction], because it can be confusing to place "Constructor" near the method. Not sure if it would be a breaking change \ anything else, which you want to avoid.

[mgravell]

@DeagleGross the attribute is ancient - I can't rename it. Options:

• keep the name and change the allowable targets
• add a second attribute to Dapper
• leave Dapper alone and use some attribute in DapperAOT

Note that I did switch the other constructor stuff to use this old attribute rather than DapperAotAttribute.

Personally I'm not too concerned about the name being slightly weird - that might still be a better option that having more types.

[DeagleGross]

Hey @mgravell, I have done the following:

1. implemented diagnostics for factory methods (via analyzer as done for constructors) + added tests
2. decided to go with changing allowable targets for ExplicitConstructorAttribute - PR to Dapper

If that is fine, we need to release Dapper, update reference to latest in Dapper.AOT, test and merge current PR.
Let me know if I am doing everything correctly.

[DeagleGross]

@mgravell PR is ready, but we need to update the Dapper reference to make [ExplicitConstructor] work with methods.
DapperLib/Dapper#1982