vkhorikov / CSharpFunctionalExtensions

Functional extensions for C#
MIT License
2.43k stars 303 forks source link

Improve null handling of `Maybe` #561

Closed JKamue closed 2 months ago

JKamue commented 2 months ago

I have noticed some unexptected behavior when using Maybe with nullable types.

Here is a short example to illustrate my thoughts (using seperate variables here on purpose to underline my issue)

public record Person(string Name); 

Person? personOrNull = null;

var maybePerson = Maybe.From(personOrNull);
var personResult = maybePerson
    .ToResult("No person was specified");
var nameOfPersonResult = personResult.Map(person => person.Name);

In this example maybePerson will be a Maybe<Person?> and nameOfPersonResult will be a Result<Person?>. Furthermore I will get a nullable warning in the Map statement for person.Name because Person is nullable there.

Personally I think this is very unexpected behavior since I am using Maybe.From already to ensure that Map only gets executed when personOrNull is not null.

Of course you can use ! and tell the compiler that null is not possible here but I would like to avoid "tricking" the compiler like that and making sure there really cannot be null in my code. A quick and dirty workaround I have been using is:

var nameOfPersonResult = Maybe.From(personOrNull)
    .ToResult(errorMessage)
    .EnsureNotNull(errorMessage)
    .Map(person => person.Name);

But this is not really a nice solution either because I think that a Maybe created from a nullable should always return a not nullable type.

Diving into the code I noticed that nullability has largely not been taken into account in the Maybe class yet. This is why in this pull request I am attempting to add this functionality to Maybe.

With the code in this pull request the following wont produce any nullability warnings:

var nameOfPersonResult = Maybe.From(personOrNull)
    .ToResult("No person was specified")
    .Map(person => person.Name);

I am very open to feedback and open to improve the code in this pull request. Please let me know what you think!

Uli-Armbruster commented 2 months ago

I experienced the same "smell" and I would appreciate accepting this PR

vkhorikov commented 2 months ago

Yeah, we have a ticket to reconcile Maybe with the nullable reference types in C#. Basically, put the nullability annotations where applicable. Thanks for taking a look into it.

I don't think we need both conditions in _value is null || HasNoValue, but this can be cleaned up later. Merged, thank you.

JKamue commented 2 months ago

I don't think we need both conditions in _value is null || HasNoValue, but this can be cleaned up later. Merged, thank you.

Yes it is redndant but necessary to avoid compiler warnings. The other option would be to use ! in a bunch of places but I generally dont like that approach. If you rely on ! and code is changed elsewhere you might suddenly really get an unexpected null value.

Would be interesting to know your thoughts on this, do you have another approach?

vkhorikov commented 2 months ago

I agree, but then if we do that, we can remove the check for HasNoValue.

JKamue commented 2 months ago

I am not sure about that. I tried removing HasNoValue and tests started failing.

I think it is because _value is assigneed default if the given value is null when creating a Maybe. https://github.com/vkhorikov/CSharpFunctionalExtensions/blob/298d9141a248fac980968856d1e6e61bc225ef76/CSharpFunctionalExtensions/Maybe/Maybe.cs#L81-L88

Since default of an int for example is 0 instead of null _value is null will actually be false making the HasNoValue a necessity

vkhorikov commented 2 months ago

Interesting. I'll look into this a bit more over the weekend.