Bug 17440 – Nullable.nullify() resets referenced object

I am not sure if it is desired, but it was highly unintuitive and unexpected for me: If you use Nullable with a class type and call .nullify(), all members of the class are set to the uninitialized state. Example: class Test { int a; int[] b; char[2] c; this () { a = 5; b = [1,2,3,4]; c[] = 'B'; } } void main () { import std.typecons; import std.stdio; import core.memory; // Just to make sure it doesn't interfere in the test GC.disable(); Nullable!Test test = new Test; Test t2 = test; writefln("before nullify %s %s %s", t2.a, t2.b, t2.c); test.nullify; writefln("after nullify %s %s %s", t2.a, t2.b, t2.c); } Output is: before nullify 5 [1, 2, 3, 4] BB after nullify 0 [] ÿÿ My expectation would have been that test's reference would simply be set to null, leaving the referenced memory untouched.

From the docs: "The nullable struct defines a value paired with a null.". I think that the expectation is that nullable is used with value types, not reference types or variables of references.

(In reply to Marenz from comment #0) > I am not sure if it is desired, but it was highly unintuitive and unexpected > for me: > > If you use Nullable with a class type and call .nullify(), all members of > the class are set to the uninitialized state. Example: That is almost certainly not intended, and (as RazvanN wrote) is because the Nullable implementation makes no attempt to do anything different when used with a class. What happens is that it calls object.destroy on the class, which instead of setting the class reference to null, it clobbers the class data. However, I think your example is also incorrect, because it accesses the value of a nullified object. Is there any particular reason you need to use Nullify with a class type instead of simply using the class type directly and use "null" to indicate the unset state?

>Is there any particular reason you need to use Nullify with a class type instead of simply using the class type directly and use "null" to indicate the unset state? There is no pressing need to do this. I am having a special array class that wraps every member in a Nullable and of course it's templated. So I needed to add a special overload/case for class types to work around that. It's less a 'need' but more a 'this was very unexpected'.

Eveyone ok with closing this?

Closing? Well.. It is _very_ unexpected that Nullable would just follow a reference and kills everything. It's like you add a pointer to an array and after setting it to null your pointed object is also reset. While this is not a terrible problem to work around, it is quite inconsistent and surprising behavior and I think this should really be fixed. Do you really want to have such behavior in the std lib?

(In reply to Marenz from comment #5) > Closing? Well.. It is _very_ unexpected that Nullable would just follow a > reference and kills everything. It's like you add a pointer to an array and > after setting it to null your pointed object is also reset. > > While this is not a terrible problem to work around, it is quite > inconsistent and surprising behavior and I think this should really be fixed. > > Do you really want to have such behavior in the std lib? Nullable wasn't design for reference types. I guess that a check could be added to reject creating Nullable's from data structures that are not value types.

Yep, deprecating Nullable for reference types seems like a reasonable idea; feel free to reopen or refile as a new enhancement.

Reopening for reconsideration, because: When using Nullable as an analogue of Java Optional or Haskell Maybe, there is no reason why you shouldn't use a reference type. Destroying on nullify will invalidate *other* references to the object, which goes beyond the remit of Nullable. And finally, because I was asked to submit a bug report and this one already existed. See forum discussion: https://forum.dlang.org/thread/[email protected]

But by-reference objects in D *already* have nullable semantics, i.e.: ------- class C {} C c; assert(c is null); // c is null by default c = new C; assert(c !is null); c = null; // nullify assert(c is null); ------- Of course the syntax is slightly different (direct use of null vs. .isNull / .nullify). But there's really no benefit to using Nullable with reference types in D. OTOH, if you're using generic code that expect a single API for nullable objects, perhaps the solution is to write an overload of Nullable that simply maps .isNull to `is null` and .nullify to `= null` when the wrapped type is a class.

P.S. Actually, Nullable does have another overload that takes a null value to be used in lieu of a boolean flag. So conceivably, you could use Nullable!(T, null) instead of Nullable!T and you will have the requisite semantics.

I can only repeat what I said before. The principle of the least surprise should apply. No one expects their object to be destroyed when a reference to it is set to null, be it through .nullify(); or through = null; It's not difficult to fix and it saves lots of people debugging time, figuring out why the hell their object is gone. Note that already two persons have stumbled over this AND have reported it. God knows how many more this happened to who haven't given this valuable feedback.

I think you misunderstood my comment. I meant that one way to fix this bug is to change Nullable, or at least the overload that takes a single type parameter, so that it does not instantiate for reference types, or redirects the instantiation to Nullable!(T, null), so that when you call .nullify on it, it will just set the reference to null instead of attempting to destroy the object by calling .destroy on it. It really does not make sense to use Nullable!T, as currently implemented, for a class type T, since making that means there will be *two* null states, one with .nullify / .isNull, and the other with `= null` and `is null`, and the two null states will not be equivalent. That will only lead to more confusion. The fact that Nullable!T's dtor calls .destroy is further proof that the original intent was to use it with by-value types, not with class references. I'd say either the docs should recommend using Nullable!(T, null) for class types T, or else Nullable!T in that case should just internally redirect to Nullable!(T, null).

Perhaps it would be helpful to reiterate one of my comments from my original forum thread linked in comment 8... <snip> 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 </snip> If Phobos needs a new Maybe/Optional template - perhaps specialised for class types - then so be it, but at the moment Nullable is it.

Also - to emphasise what was said in comment 11 - let me just say that it took me *3 days* to find that the cause of a mysterious crash in my code was the destroy in nullify(). At the very least, the documentation for Nullable should contain a prominent warning that nullify() will invalidate your class references.

Rebooted: https://github.com/dlang/phobos/pull/6043 Due to design issues with the current implementation of Nullable, it's not possible to fix the problem of having two distinct null states without extensive changes and breaking backward-compatibility. So for now, we'll just make it so that .nullify doesn't call .destroy on class references.

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/5b04f455711a517cec273dad5eb4e4fda193ebff Fix issue 17440: do not call .destroy on class instances in .nullify. https://github.com/dlang/phobos/commit/0666fc6e21b594321fe638d30865b0eef472f7a5 Merge pull request #6043 from quickfur/issue17440b Fix issue 17440: do not call .destroy on class instances in Nullable.nullify merged-on-behalf-of: Steven Schveighoffer <[email protected]>