dasMulli / data-builder-generator

Code generator to easily create data builder patterns for your model classes
MIT License
116 stars 11 forks source link

Nullable values #6

Open hhenrik08 opened 3 years ago

hhenrik08 commented 3 years ago

Hi there! I’m a big fan of your data builder generator!

I’m just having one small issue when working with nullable types. Since I like my classes to be immutable, I’m using constructors for all parameters, including those that may actually be null.

When using your generator with e.g. a nullable int, the generated code calls the constructor in the following way (excluding null checks):

var instance = new Module(_id ?? default, _header, _text);
return instance;

Unfortunately, the default seems to be the default value for int-types, so I never actually get nullwhen creating the class, as these are replaced by the value “0”.

Do you think that is something you could fix? I Would offer to look into it myself, but I doubt my experience will be sufficient to fix this.

Cheers! Henrik

dasMulli commented 3 years ago

@hhenrik08 did you define the parameter as int? or int (Consistently) in your class?

algrn-abirke commented 2 years ago

Hi @dasMulli,

sorry for picking up this topic again after such a long time. I might have a few insights on this, though.

There's a subtlety around nullable value types in C# that I only learned about a few hours ago. Apparently, the compiler behaves somewhat counter-intuitive (from my perspective this even looks like a bug in the language, but I might be missing something) when using the default literal.

This test shows what I mean. My expectation would be that the variables throughDefaultOperator and throughDefaultLiteral both contain null at the end of the test. The test will fail however, showing that throughDefaultLiteral actually contains 0.

[Fact]
public void CompareDefaultOperatorAndLiteral()
{
    int? nullableInt = null;

    int? throughDefaultLiteral = nullableInt ?? default;
    int? throughDefaultOperator = nullableInt ?? default(int?);

    Assert.Equal(null, throughDefaultOperator);
    Assert.Equal(null, throughDefaultLiteral);
}

How does this relate to this issue? As far as I can see, the data-builder-generator will generate slightly different Build() methods depending on whether a nullable value type member is included in a constructor or not. If it's not included in a constructor, there'll be a null check in the Build() method and possible null values are passed on, which is fine.

If a nullable value type is included in a constructor however, the Build() method will pass a null-coalescing expression to the constructor with a default literal on the right side which exposes the behaviour I described above.

var instance = new Address(_street, _streetNumber, _top, _staircase ?? default, _city, _postalCode, _country);

I am aware, this is not primarily a data-builder-generator issue, but I am wondering if you'd be open to hiding this behaviour from the user, as it makes the usage rather error-prone. I have drafted a PR to switch from default literals to explicit default operators in the constructor call of Build() methods and I'm curious to know what you think.

Thanks for this nice package and all your work.

Best regards André

For reference: there have been discussions on the behaviour of the default literal which led me to thinking this was regarded as a bug and has been fixed since language version 7.2. From what I can see today, though, I believe there are still issues around default literal usages. (see https://github.com/dotnet/csharplang/issues/970)

dasMulli commented 2 years ago

In this case ?? default on the Nullable type results in a call to .GetValueOrDefault() - https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLRII5wJZICaoC2EhwECyANHiANQA+AAgEwCMAsAFCMDMABCz4BhPgG8ufSQP6MALHwCyACgCUYiVM3YAdjAD8fKHwC8fPBDBQ4AGxgBuDZsk79fYCcN89B85Zv3HPgBfLiCgA

In this case any nullableInt ?? default expression is of type int (and 0 in case nullableInt.HasValue is false), which is then wrapped in another Nullable<int> when assigned,

You could open another issue on csharplang to discuss this. I agree that this seems counterintuitive.

algrn-abirke commented 2 years ago

Thanks for looking into this so quickly! I will try to follow up with the discussion over on csharplang.

What's your take on #10? Do you think it would be reasonable to change the way that data-builder-generator handles the constructor calls to make the behaviour more predictable?