data-builder-generator icon indicating copy to clipboard operation
data-builder-generator copied to clipboard

Nullable values

Open hhenrik08 opened this issue 2 years ago • 4 comments

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 null when 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

hhenrik08 avatar Aug 29 '21 10:08 hhenrik08

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

dasMulli avatar Sep 13 '21 21:09 dasMulli

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)

algrn-abirke avatar Aug 30 '22 13:08 algrn-abirke

In this case ?? default on the Nullable<T> 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.

dasMulli avatar Aug 31 '22 07:08 dasMulli

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?

algrn-abirke avatar Aug 31 '22 09:08 algrn-abirke