From c1431b133bb26d3a777bdea0815c7212d1ff09d7 Mon Sep 17 00:00:00 2001 From: Mathis Beer Date: Wed, 27 Jun 2018 15:22:33 +0200 Subject: [PATCH 1/4] Use move/moveEmplace in Nullable Fixes issue 19037. --- std/typecons.d | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/std/typecons.d b/std/typecons.d index 87fde67e730..053248ca320 100644 --- a/std/typecons.d +++ b/std/typecons.d @@ -2569,24 +2569,7 @@ Practically `Nullable!T` stores a `T` and a `bool`. */ struct Nullable(T) { - // simple case: type is freely constructable - static if (__traits(compiles, { T _value; })) - { - private T _value; - } - // type is not constructable, but also has no way to notice - // that we're assigning to an uninitialized variable. - else static if (!hasElaborateAssign!T) - { - private T _value = T.init; - } - else - { - static assert(false, - "Cannot construct " ~ typeof(this).stringof ~ - ": type has no default constructor and overloaded assignment." - ); - } + private T _value = T.init; private bool _isNull = true; @@ -2802,7 +2785,19 @@ Params: */ void opAssign()(T value) { - _value = value; + import std.algorithm.mutation : moveEmplace, move; + + if (_isNull) + { + // trusted since _value is known to be T.init here. + // the lifetime of value shall be managed by this Nullable; + // hence moveEmplace resets it to T.init. + () @trusted { moveEmplace(value, _value); }(); + } + else + { + move(value, _value); + } _isNull = false; } From 11b4a157352ca488dbe1201ece355a90978689bd Mon Sep 17 00:00:00 2001 From: Mathis Beer Date: Fri, 29 Jun 2018 09:10:49 +0200 Subject: [PATCH 2/4] Add testcase for issue 19037 unittest fix --- std/typecons.d | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/std/typecons.d b/std/typecons.d index 053248ca320..055f7fc8562 100644 --- a/std/typecons.d +++ b/std/typecons.d @@ -3269,6 +3269,80 @@ auto nullable(T)(T t) assert(foo.approxEqual(bar)); } +// bugzilla issue 19037 +@safe unittest +{ + import std.datetime : SysTime; + + struct Test + { + bool b; + + nothrow invariant { assert(b == true); } + + SysTime _st; + + static bool destroyed; + + @disable this(); + this(bool b) { this.b = b; } + ~this() @safe { destroyed = true; } + + // mustn't call opAssign on Test.init in Nullable!Test, because the invariant + // will be called before opAssign on the Test.init that is in Nullable + // and Test.init violates its invariant. + void opAssign(Test rhs) @safe { assert(false); } + } + + { + Nullable!Test nt; + + nt = Test(true); + + // destroy value + Test.destroyed = false; + + nt.nullify; + + assert(Test.destroyed); + + Test.destroyed = false; + } + // don't run destructor on T.init in Nullable on scope exit! + assert(!Test.destroyed); +} +// check that the contained type's destructor is called on assignment +@system unittest +{ + struct S + { + // can't be static, since we need a specific value's pointer + bool* destroyedRef; + + ~this() + { + if (this.destroyedRef) + { + *this.destroyedRef = true; + } + } + } + + Nullable!S ns; + + bool destroyed; + + ns = S(&destroyed); + + // reset from rvalue destruction in Nullable's opAssign + destroyed = false; + + // overwrite Nullable + ns = S(null); + + // the original S should be destroyed. + assert(destroyed == true); +} /** Just like `Nullable!T`, except that the null state is defined as a From 4af38e5c735fb9bae9ba0a7b3603e60cbfa8c293 Mon Sep 17 00:00:00 2001 From: Mathis Beer Date: Wed, 11 Jul 2018 10:25:50 +0200 Subject: [PATCH 3/4] Exploit unions to avoid destructor call to wrapped type in Nullable --- std/typecons.d | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/std/typecons.d b/std/typecons.d index 055f7fc8562..a0ddc55d637 100644 --- a/std/typecons.d +++ b/std/typecons.d @@ -2569,7 +2569,12 @@ Practically `Nullable!T` stores a `T` and a `bool`. */ struct Nullable(T) { - private T _value = T.init; + private union DontCallDestructorT + { + T payload; + } + + private DontCallDestructorT _value = DontCallDestructorT.init; private bool _isNull = true; @@ -2581,7 +2586,7 @@ Params: */ this(inout T value) inout { - _value = value; + _value.payload = value; _isNull = false; } @@ -2596,14 +2601,14 @@ Params: return rhs._isNull; if (rhs._isNull) return false; - return _value == rhs._value; + return _value.payload == rhs._value.payload; } /// Ditto bool opEquals(U)(auto ref const(U) rhs) const if (is(typeof(this.get == rhs))) { - return _isNull ? false : rhs == _value; + return _isNull ? false : rhs == _value.payload; } /// @@ -2689,7 +2694,7 @@ Params: if (isNull) put(writer, "Nullable.null"); else - formatValue(writer, _value, fmt); + formatValue(writer, _value.payload, fmt); } //@@@DEPRECATED_2.086@@@ @@ -2702,7 +2707,7 @@ Params: } else { - sink.formatValue(_value, fmt); + sink.formatValue(_value.payload, fmt); } } @@ -2717,7 +2722,7 @@ Params: } else { - sink.formatValue(_value, fmt); + sink.formatValue(_value.payload, fmt); } } @@ -2760,9 +2765,9 @@ Forces `this` to the null state. void nullify()() { static if (is(T == class) || is(T == interface)) - _value = null; + _value.payload = null; else - .destroy(_value); + .destroy(_value.payload); _isNull = true; } @@ -2787,16 +2792,18 @@ Params: { import std.algorithm.mutation : moveEmplace, move; + // the lifetime of the value in copy shall be managed by + // this Nullable, so we must avoid calling its destructor. + auto copy = DontCallDestructorT(value); + if (_isNull) { - // trusted since _value is known to be T.init here. - // the lifetime of value shall be managed by this Nullable; - // hence moveEmplace resets it to T.init. - () @trusted { moveEmplace(value, _value); }(); + // trusted since payload is known to be T.init here. + () @trusted { moveEmplace(copy.payload, _value.payload); }(); } else { - move(value, _value); + move(copy.payload, _value.payload); } _isNull = false; } @@ -2837,13 +2844,13 @@ Returns: { enum message = "Called `get' on null Nullable!" ~ T.stringof ~ "."; assert(!isNull, message); - return _value; + return _value.payload; } /// ditto @property get(U)(inout(U) fallback) inout @safe pure nothrow { - return isNull ? fallback : _value; + return isNull ? fallback : _value.payload; } /// @@ -3475,7 +3482,9 @@ Params: */ void opAssign()(T value) { - _value = value; + import std.algorithm.mutation : swap; + + swap(value, _value); } /** From b0972bdda36be44c31036c818b00fc6bec9b2972 Mon Sep 17 00:00:00 2001 From: Mathis Beer Date: Wed, 11 Jul 2018 12:36:18 +0200 Subject: [PATCH 4/4] Nullable: with implicit destruction disabled, generate and test an explicit destructor that only runs if !isNull --- std/typecons.d | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/std/typecons.d b/std/typecons.d index a0ddc55d637..4cc77ac8a30 100644 --- a/std/typecons.d +++ b/std/typecons.d @@ -2590,6 +2590,17 @@ Params: _isNull = false; } + static if (is(T == struct) && hasElaborateDestructor!T) + { + ~this() + { + if (!_isNull) + { + destroy(_value.payload); + } + } + } + /** If they are both null, then they are equal. If one is null and the other is not, then they are not equal. If they are both non-null, then they are @@ -3350,6 +3361,28 @@ auto nullable(T)(T t) // the original S should be destroyed. assert(destroyed == true); } +// check that the contained type's destructor is still called when required +@system unittest +{ + bool destructorCalled = false; + + struct S + { + bool* destroyed; + ~this() { *this.destroyed = true; } + } + + { + Nullable!S ns; + } + assert(!destructorCalled); + { + Nullable!S ns = Nullable!S(S(&destructorCalled)); + + destructorCalled = false; // reset after S was destroyed in the NS constructor + } + assert(destructorCalled); +} /** Just like `Nullable!T`, except that the null state is defined as a