[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #17 from github-bugzi...@puremagic.com ---
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 

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

github-bugzi...@puremagic.com changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #16 from hst...@quickfur.ath.cx ---
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.

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

Steven Schveighoffer  changed:

   What|Removed |Added

 CC||schvei...@yahoo.com

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

hst...@quickfur.ath.cx changed:

   What|Removed |Added

   Keywords||pull

--- Comment #15 from hst...@quickfur.ath.cx ---
https://github.com/dlang/phobos/pull/6038

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #14 from Chris Paulson-Ellis  ---
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.

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #13 from Chris Paulson-Ellis  ---
Perhaps it would be helpful to reiterate one of my comments from my original
forum thread linked in comment 8...


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


If Phobos needs a new Maybe/Optional template - perhaps specialised for class
types - then so be it, but at the moment Nullable is it.

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #12 from hst...@quickfur.ath.cx ---
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).

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #11 from Marenz  ---
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.

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #10 from hst...@quickfur.ath.cx ---
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.

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

hst...@quickfur.ath.cx changed:

   What|Removed |Added

 CC||hst...@quickfur.ath.cx

--- Comment #9 from hst...@quickfur.ath.cx ---
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.

--

[Issue 17440] Nullable.nullify() resets referenced object

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

Chris Paulson-Ellis  changed:

   What|Removed |Added

 Status|RESOLVED|REOPENED
 CC||ch...@edesix.com
 Resolution|INVALID |---

--- Comment #8 from Chris Paulson-Ellis  ---
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/rglbcxolihbogsvvn...@forum.dlang.org

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #7 from Vladimir Panteleev  ---
Yep, deprecating Nullable for reference types seems like a reasonable idea;
feel free to reopen or refile as a new enhancement.

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #6 from RazvanN  ---
(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.

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #5 from Marenz  ---
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?

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

RazvanN  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |INVALID

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #4 from RazvanN  ---
Eveyone ok with closing this?

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #3 from Marenz  ---
>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'.

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

--- Comment #2 from Vladimir Panteleev  ---
(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?

--

[Issue 17440] Nullable.nullify() resets referenced object

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=17440

RazvanN  changed:

   What|Removed |Added

 CC||razvan.nitu1...@gmail.com

--- Comment #1 from RazvanN  ---
>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.

--