Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable values #6

Open
hhenrik08 opened this issue Aug 29, 2021 · 4 comments
Open

Nullable values #6

hhenrik08 opened this issue Aug 29, 2021 · 4 comments

Comments

@hhenrik08
Copy link

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

@dasMulli
Copy link
Owner

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

@algrn-abirke
Copy link

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 dotnet/csharplang#970)

@dasMulli
Copy link
Owner

dasMulli commented Aug 31, 2022

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants