k94ll13nn3 / AutoConstructor

C# source generator that generates a constructor from readonly fields/properties in a class or struct
MIT License
40 stars 4 forks source link

Parameterless constructor for serialization use when using nested types #119

Closed DomasM closed 1 month ago

DomasM commented 1 month ago

Recently I have noticed that to define dtos life is quite simple if there is no inheritance, record with primary constructor can be used to express oneself very succinctly

public record Something(int X, string Y);

This works both to ensure that Something instances are initialized properly by requiring x and y when creating them, and works well with json serializers. However, once inheritance is added to the mix, things get more complicated as this does not work as expected:

public record Something(int id, int X, string Y): ThingBase(id);
public abstract record ThingBase(int Id);

'id' collides with another property.

Thus, my idea to declare an explicit constructor by hand with the required properties, and to let code generator generate empty ones, with all the boilerplate required:

[SerializerConstructor]
public partial class Test
{
    public int Id { get; init; }
    public string Name {get; init;}
    public int OptionalPropA {get; init;}
    public int OptionalPropB {get; init;}

    public Test (int id, string name){
        Id = id;
        Name = name;
    }
}
partial class Test
    {
        #pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
        [global::System.ObsoleteAttribute("For serialization only",true)]
        public Test()
        {
        }
    }

This generated constructor will be used by serializer, while a manually defined constructor is used to ensure proper object creation in code. This could be further combined with existing AutoContstructor functionality to generate both constructors:

[SerializerConstructor, AutoConstructor]
public partial class Test
{
   [field:AutoConstructorInject]  public int Id { get; init; }
   [field:AutoConstructorInject] public string Name {get; init;}
    public int OptionalPropA {get; init;}
    public int OptionalPropB {get; init;}
}

Would you be interested including such feature in this library, given that I could do PR and the new feature would be fully backward compatible, as [AutoContstructor] attribute would work the same way as now. I played around a little to see how feasible is to add such functionality to the libary, instead of starting my own nano-generator, and it seems quite easy. I haven't touched analyzers yet, and added only some basic tests, but as complexity of [SerializerConstructor] is smaller than that of [AutoContstructor], there would be fewer things to cover. https://github.com/k94ll13nn3/AutoConstructor/compare/main...DomasM:AutoConstructor:feat/add-serialization-only-empty-constructor

k94ll13nn3 commented 1 month ago

Hello, I think that this could be a nice feature to have, but I also think that having an attribute that is only doing a parameterless constructor is kind of outside the scope of this package.

I would be ok with this being on option on AutoConstructor to be able to also generate a parameterless constructor (and so maybe expend the rules to allow AutoConstructor on a class without fields if the parameter is set), but as a standalone attribute this feature would not make sense in this package.

DomasM commented 1 month ago

Okay, then I assume we add a new property to AutoConstructorAttribute which defaults to old behavior but can be overridden, similar to constructor accessibility . Let's call this option bool 'AddEmpty'. Considering that a type might or not might have anything to inject via ctor and option might or might not be present, such different outcomes are possible:

Anything to inject? AddEmpty Generation result Comment
No False Analyzer error Same as now
No True Only empty ctor Analyzer needs to take AddEmpty into account and not report an error
Yes False Ctor with arguments Same as now
Yes True Two ctors: empty and with arguments Impossible to generate just empty ctor. Both ctors will have the same accesibility. Guess this is OK.

A few more questions to decide on before continuing with PR:

  1. Is 'AddEmpty' a good name for a new property
  2. Should we output both ctors in the same file? This might make writing tests a bit harder, but there is less of IDE spamming with multiple partials and easier for users to understand what exactly has been generated.
  3. Do we need to support any customization for empty constructor generated - I think it's better toleave it simple for now and only add customization later on if requried.
k94ll13nn3 commented 1 month ago

Yes your cases are what I expect to be generated.

A few more questions to decide on before continuing with PR:

  1. Is 'AddEmpty' a good name for a new property

I why thinking more GenerateParameterlessConstructor.

  1. Should we output both ctors in the same file? This might make writing tests a bit harder, but there is less of IDE spamming with multiple partials and easier for users to understand what exactly has been generated.

Yes, that is one one the reason that I wanted this to be an option on the existing attribute, to have only one generated file. It also simplify the generation.

  1. Do we need to support any customization for empty constructor generated - I think it's better toleave it simple for now and > only add customization later on if requried.

Only the accessibility like you said in your table for now.

One thing to note is that since #110, there is more checks on this and base calls, and since now a parameterless constructor could be generated in the class, the checks should be updated to reflect this.

There is also the question of inheritance because a parameterless constructor on a child class won't compile if the parent class has a non empty constructor.

DomasM commented 1 month ago

Hi, thank you for the feedback.

Regarding this and base calls - as I understand:

So there is little work to do here. We could add an analyzer to detect situations when the base type does not have parameterless constructor, but this will be caught by the compiler anyway.

GenerateParameterlessConstructor - won't that be a bit too clunky/repetitive:

[AutoConstructor(GenerateParameterlessConstructor:true)]
class SomeType{
...

we repeat Constructor here twice in five words, and all the words in are really long in new property name.

k94ll13nn3 commented 1 month ago

Hi, thank you for the feedback.

Regarding this and base calls - as I understand:

  • parameterless constructor never needs to call base constructor explicitly because there are no arguments to pass anyway

If there is a constructor on the base class with parameter, the parameterless constructor will have to call it otherwise it will generate a compilation error like you said, it's just that I try to avoid when possible to generate code that will not compile.

  • and never needs to call this(...), as type can't have two parameterless constructors

Yes, there is no need to call this, I was referring to the code in this package that detects if there is already a parameterless constuctor on the class to call, because the code that checks if there is already a parameterless constructor on the class won't see the generated one (here).

So there is little work to do here. We could add an analyzer to detect situations when the base type does not have parameterless constructor, but this will be caught by the compiler anyway.

GenerateParameterlessConstructor - won't that be a bit too clunky/repetitive:

[AutoConstructor(GenerateParameterlessConstructor:true)]
class SomeType{
...

we repeat Constructor here twice in five words, and all the words in are really long in new property name.

Yes maybe it is a bit too long, it could be only AddParameterless.

DomasM commented 1 month ago

"If there is a constructor on the base class with parameter, the parameterless constructor will have to call it"

I don't think it's possible to do that, as there are nowhere to take the values for the arguments. It might be possible to pass default values for the argument type, but that would make very little sense. So I think if we are generating parameterless ctor, base type must have parameterless one, whether implicit or explicit.

On Thu, May 16, 2024, 10:03 Kévin Gallienne @.***> wrote:

Hi, thank you for the feedback.

Regarding this and base calls - as I understand:

  • parameterless constructor never needs to call base constructor explicitly because there are no arguments to pass anyway

If there is a constructor on the base class with parameter, the parameterless constructor will have to call it otherwise it will generate a compilation error like you said, it's just that I try to avoid when possible to generate code that will not compile.

  • and never needs to call this(...), as type can't have two parameterless constructors

Yes, there is no need to call this, I was referring to the code in this package that detects if there is already a parameterless constuctor on the class to call, because the code that checks if there is already a parameterless constructor on the class won't see the generated one (here https://github.com/k94ll13nn3/AutoConstructor/blob/1b6f5d91f74ff2b54e81ed0b1379f3f4c29a6c80/src/AutoConstructor.Generator/AutoConstructorGenerator.cs#L152 ).

So there is little work to do here. We could add an analyzer to detect situations when the base type does not have parameterless constructor, but this will be caught by the compiler anyway.

GenerateParameterlessConstructor - won't that be a bit too clunky/repetitive:

[AutoConstructor(GenerateParameterlessConstructor:true)]class SomeType{ ...

we repeat Constructor here twice in five words, and all the words in are really long in new property name.

Yes maybe it is a bit too long, it could be only AddParameterless.

— Reply to this email directly, view it on GitHub https://github.com/k94ll13nn3/AutoConstructor/issues/119#issuecomment-2114217144, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADG3FPWUUBDHZYSIOK3KDHLZCRK47AVCNFSM6AAAAABHWDM532VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJUGIYTOMJUGQ . You are receiving this because you authored the thread.Message ID: @.***>

k94ll13nn3 commented 1 month ago

Yes I agree, I was not saying that we have to generate a call to the base, we can't, I was just saying that since we can't do it, it will generate a compilation error.