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

Maybe.Some(null) does not throw and has a different behavior than Maybe.None() #51

Closed
DeafLight opened this issue Mar 31, 2018 · 8 comments

Comments

@DeafLight
Copy link
Contributor

DeafLight commented Mar 31, 2018

I'm currently testing SuccincT, quite happily so far, but encountered a problem when trying to integrate it in a part of a project I'm working on.
Why is Maybe<string>.Some(null) allowed ?

var someNull = Maybe<string>.Some(null);

in this case, someValue.HasValue returns true and someNull.Value returns null, where I would have expected this to behave the way Maybe<string>.None() does (after all, it's seems to be the same thing semantically speaking), or at least to throw.

Of course this is not senseless, but in my mind it defeats the objective of Maybe, which is to provide a way of avoiding nulls. That means that if a legacy part of the project passes an unchecked value which turns to be null, Maybe will happily live with it, propagating it without complaining, meaning that I still need to rely on defensive code even in a Maybe enabled codebase.

Is there a reason I don't get behind this behavior ?

If not, which behavior would you recommend / prefer ?

@DeafLight DeafLight changed the title Maybe.Some(null) don't throw and have a different behavior than Maybe.None() Maybe.Some(null) does not throw and has a different behavior than Maybe.None() Mar 31, 2018
@DavidArno DavidArno added this to the V4.0 milestone Apr 3, 2018
@DavidArno DavidArno self-assigned this Apr 3, 2018
@DavidArno
Copy link
Owner

Thanks for reporting this.

I'll mark this as a bug for now, until I have time to properly look at the code to assess why it's like it is and whether it's by design or not and whether that design is sensible or not.

@DavidArno
Copy link
Owner

DavidArno commented Jun 21, 2018

This was definitely by design. null is a valid value for a string. However, I accept it's arguably flawed design, so I'd be happy to change it ... except what then happens with C# 8 when we have nullable reference types? I'd expect Maybe<string?> to definitely accept null then, and Maybe<string> to throw, but I don't know how that'll work. I need to ask around to see if there's a view amongst the language team as to how that'll be supported, if at all.

Quick update to this: these LDM notes are relevant to this topic.

@DeafLight
Copy link
Contributor Author

DeafLight commented Jun 21, 2018

I get your point, but does a Maybe<string?> make sense ? What I mean by that is that as far as I understand the objective of Maybe, Maybe<string?>.Some(null) does not mean anything (by definition, null is not "some"). So back to what I was mentioning before, I would go for either throwing in case of Maybe<string?>.Some(null), or maybe (because you're right, null is a valid value for string?), making Maybe<string?>.Some(null) behave the same way as Maybe<string?>.None().

What do you think ?

@DavidArno
Copy link
Owner

Well it turns out that the changes I'm making for v4 effectively force my hand here. I've never been happy with having Option<T> and Maybe<T>, so the latter is going and the former is becoming a struct. To make that play nicely with null:

  1. Assigning null to Some will throw,
  2. An implicit cast from null to an option will result in a none,
  3. Option<RefType>.None == null.

@DeafLight
Copy link
Contributor Author

Sounds nice!

@DavidArno
Copy link
Owner

@xlecoustillier, since this topic is of interest to you, I'd like your opinion on a related topic, please.

What should I do with unions? Taking an extreme case, if I create a Union<int, None> (not actually possible as I do not expose None as a type, but for the sake of argument, let's say I did), it seems reasonable that it should behave the same as Option<int> with regards to null. But then what if I created a Union<string, List<string>>. What is it's relationship to null now?

My first though was to create yet another breaking change and have unions throw exceptions is given a null. But since I'm turning them into structs, I could do Union<string, List<string>> u = default; and avoid that null detection. I would therefore have to either ignore that problem, as have runtime checking for null for every action performed on a union after creation. That would be slower than current behaviour.

So I'm tempted to just say that Option is a special case with regard to null. Unions do nothing special with them. Do you have an opinion on this choice? Thanks.

@DeafLight
Copy link
Contributor Author

DeafLight commented Jun 29, 2018

Interesting question.
From my point of view, default(Union<T1, T2>) does not really make sense, as Unions (and Eithers) should always hold exactly one value.
Of course, there is no way to avoid this behavior, even if we could consider this as an error in itself.
In an ideal world, a call to default(Union<T1, T2>) should throw or something, but I don't think this is possible.

So I would go for checking against null each time as you mentioned, or maybe have a property IsValid or something that would initialize to false in case of default (and to truein every other case, obviously) and check that instead.

Do you really think it would affect performance ? If yes, and if it's a problem, I personally can live with a default, as, again, it makes no sense and thus should not be used too much, even by mistake.

DavidArno added a commit that referenced this issue Nov 26, 2018
…ent in that issue, regarding how null and Option<> compare.
@DavidArno
Copy link
Owner

The following was implemented in v4:

Assigning null to Some will throw,
An implicit cast from null to an option will result in a none,
Option<RefType>.None == null.

Closing as implemented.

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

No branches or pull requests

2 participants