Fix issue 17440: Nullable!Class should redirect to Nullable!(Class,null)

[quickfur]

The current implementation of Nullable!T (the overload with a single type parameter) is clearly designed for types T with by-value semantics, as evidenced by the use of .destroy in .nullify, along with other implementation decisions. As a result, nullifying a Nullable!Class also triggers destruction of the class object, which is surprising behaviour. Furthermore, since class references already have a null state, Nullable!Class adds a second, distinct null state, which only adds to the confusion.

Since there's a second overload of Nullable that can be used for proxying a class reference's null semantics with the Nullable API (i.e., isNull and nullify), simply forward instantiations of Nullable!Class to Nullable!(Class, null) instead.

An alternative fix is to change .nullify so that it does not call .destroy on class references. However, this does not solve the problem of confusion caused by introducing two distinct null states in Nullable!Class.

As for why one would use Nullable on a type that already has a null state, to quote a comment from the bugzilla issue:

For those confused as to why you'd want to wrap a Nullable around something that already has nullable semantics, it's mostly about making APIs that explicitly declare their optional return values. See: http://www.oracle.com/technetwork/articles/java/java8-optional-2175753.html

[dlang-bot]

Thanks for your pull request, @quickfur!

Bugzilla references

Auto-close	Bugzilla	Description
✓	17440	Nullable.nullify() resets referenced object

[quickfur]

P.S. For ease of review, use this link with whitespace changes ignored: https://github.com/dlang/phobos/pull/6038/files?w=1

[JackStouffer]

Seems reasonable.

I think this is a great opportunity to document why the nullable class option exists as well, making sure to mention Optional and Maybe, and adding an example. That way if someone searches dlang Optional they get this.

[wilzbach]

I agree that calling .nullify for reference types is a bug.

[wilzbach]

I agree that calling .nullify for reference types is a bug.

Though for the record - people could depend on the old behavior of nullify calling the dtor :/
I argue that they shouldn't and the correct place to call the dtor is once the class gets properly destructed.

[MetaLang]

Calling destroy on class instances from .nullify is a huge mistake IMO, but we really can't change that now. Likewise, I'm all for this, but I think it needs to be deprecated for a few releases first (i.e., Nullable!Class prints a deprecation message).

[quickfur]

Hmph. So should we deprecate Nullable!Class first, or should we redirect it to Nullable!(Class, null)?

While cooking up a nice ddoc'd unittest example for using classes with Nullable, I realized that simply redirecting to Nullable!(Class, null) may not be as ideal a solution as I initially thought, because one use case for this, which is to provide a unified API for nullable value types and reference types, actually doesn't quite work out that nicely in practice:

void myGenericFunc(T)(Nullable!T value)
{
    if (value.isNull) { ... }
}
myGenericFunc(nullable(new Object)); // Compile error: T doesn't match !(Object, null)

In order to make it work, you'd have to make T variadic:

void myGenericFunc(T...)(Nullable!T value)
    if (T.length >= 1)
{ ... }

It works, but requires patching all code that references Nullable!T, which may not be so desirable.

[quickfur]

Not to mention, Nullable!(Class, null) doesn't quite work the way I thought it did. For example:

Nullable!(Object, null) nullObj;
assert(nullObj is null); // core.exception.AssertError@std/typecons.d(3208): Called `get' on null Nullable!(Object,nullValue).

:-(

Looking at the code and docs, it seems that both overloads of Nullable as well as NullableRef were written with value types in mind, and exhibit strange / unexpected behaviour or are just plain awkward when instantiated with a class type. Unfortunately, the original code did not include the proper sig constraints, so now we're stuck with users relying on Nullable!Class with questionable semantics.

[quickfur]

Blocking for the time being until we decide on an appropriate solution to this mess.

[MetaLang]

@quickfur yup, Nullable has many long-standing issues. It's almost better to replace it if we can't fix them without breaking code. We're fairly constrained in what can be done.

[quickfur]

Is there a list of issues somewhere that I can take a look at? Or is it just searching for nullable on bugzilla? I'm almost tempted to write a proper Maybe type or something similar, that has a proper design, so that we can start deprecating Nullable.

[MetaLang]

Searching Nullable on bugzilla should return at least a few defects. AFAIK there are one or two intended replacements but I don't remember there being any PRs (I could be wrong about that). The problem is that Nullable is deeply but not obviously flawed and problems don't pop up until you start trying to use it in 'serious' code. I may see if I can remember and write them down later when I have the time.

[wilzbach]

Is there a list of issues somewhere that I can take a look at?
AFAIK there are one or two intended replacements but I don't remember there being any PRs

I remember the following two PRs being related to Nullable / Option / Maybe as Nullable doesn't work well with ranges:

• Add std.range.Option #3915 (Option)
• Added std.range.orElse which provides a default value for empty ranges #5154 (orElse)

There's also at least this issue at Vibe.d:

• [RFC] Make Nullable @optional by default vibe-d/vibe.d#1827

[quickfur]

OK, looks like fixing this is going to be far more complicated than I anticipated. Maybe somebody else should take this up. :-P

[schveiguy]

This should be pretty simple -- nullify should set the value to null, and not destroy the class object. It's what happens for a pointer or array as well. It's just that destroy on class types will call the destructor.

people could depend on the old behavior of nullify calling the dtor

Meh, the GC will get to it eventually :)

[quickfur]

@schveiguy I was hoping to avoid that route, because it means Nullable!Class will have two distinct null states that are not equivalent. But since this is too hard to fix, I suppose I'll just have to settle with that. :-(

[quickfur]

Reboot: #6043

[schveiguy]

because it means Nullable!Class will have two distinct null states that are not equivalent.

I'll just respond to this point here: I think this is not only fine, but expected. Why else would you want to create such a type?

Typically, when I'm using Nullable, the null state means something different than just null. e.g. in vibe.d, it means a parameter was not provided to a route. I don't see why that would be different for a class (though it really doesn't apply to that case). You also may want to use the same API for dealing with the isNull state.

I think the biggest problem is that it's called Nullable. It's really a meta-type that adds an additional bool for whatever use you deem fit, not to be confused with null.

[quickfur]

Haha, well, it's not really for whatever use you deem fit; the code is such that you cannot access the underlying data when the bool is false. So it is some kind of null or not-present state. Given that, I have a hard time understanding the need to use Nullable (as currently implemented) with class references, since classes already have a built-in null state.

The reason stated in the bug was to explicitly document APIs that return an optional value; perhaps Nullable is not a good fit for that purpose, but it would make more sense for whatever wrapper type it is, that said wrapper type would simply map its null state to the built-in null state of classes, rather than tack on an additional, distinct null state.

[schveiguy]

Right. Part of the problem here is that things that actual nullable types support are not supportable with a wrapper struct. You just can't hook x is null.

IMO, the fact that !value doesn't translate to value.isNull is quite a problem in this regard.

I think it fits more with an optional type than a nullable type, which is unfortunate since the name of the type is Nullable.

In any case, I think deprecating Nullable!ClassType is too damaging at this point. Making Nullable!ClassType use the class reference's null value as the boolean might be possible, but undoubtedly we will break something somewhere :)

[quickfur]

Maybe it's time to write a proper Optional or Maybe type that addresses these issues.