Closed DeagleGross closed 10 months ago
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.
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.
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)
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.
@DeagleGross the attribute is ancient - I can't rename it. Options:
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.
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.
@mgravell PR is ready, but we need to update the Dapper reference to make [ExplicitConstructor]
work with methods.
https://github.com/DapperLib/Dapper/pull/1982
Idea: Currently Dapper.AOT supports instance initialization via constructor using such syntax:
Idea is to support initialization of instances via factory methods:
Details: 1: Only such methods can be considered as
factory
methods for typeFoo
: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.
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
: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 ofExplicitConstructor
attribute:ExplicitConstructor
attribute, then diagnosticDAP039: Multiple explicit factory methods
is reported.ExplicitConstructor
attribute) will be used. If multiple implicit factory methods found, then diagnosticDAP040: Ambiguous factory methods
is reported.Closes #34