I believe there's a good case that struct invariants should not be called on struct destruction. Significantly, I believe that Phobos, in particular `moveEmplace`, is already written as if this is the case, even though it is not specified anywhere.

It is very common for structs' .init to violate invariants. Consider the humble struct S
{
    Object obj;
    invariant
    {
        assert(this.obj !is null);
    }
    @disable this();
    this(Object obj)
    in(obj !is null)
    {
        this.obj = obj;
    }
}

S is obviously intended to be constructed with a nonzero this.obj. However, even in @safe code we may take S.init, which violates the invariant.

Consequently, this code currently asserts out:

    S s = S.init;

Why is this a problem? ("Just don't use S.init!")

Well, for one it makes Nullable!S impossible. Nullable, if it is to be @nogc, *necessarily* has to construct an S.init struct member. This leads us into trouble:

    Nullable!s ns = Nullable!s();

^ Asserts out again - there is no way for Nullable to avoid invoking the destructor of its internal S field.

Now, currently this is not a problem because Nullable refuses to compile in this case. However, such a basic language feature should *really* work for every type, especially given that such basic types as SysTime already set up Nullable's reasonable fears of invalid behavior.

There is a simple tweak to make this work:

Simply require that struct constructors be defined for T.init, even if T.init should violate invariants. This can be implemented by not checking the invariant if `this is T.init`, or simply disabling the invariant in the struct destructor entirely.

I picked the second option, but more because I didn't know how to do the first.

https://github.com/dlang/dlang.org/pull/2410

https://github.com/dlang/dmd/pull/8462

About `moveEmplace`: If `moveEmplace` detects that the struct type it is operating on has a user-defined destructor, it manually overwrites its source with `T.init`. This suggests that `moveEmplace` already operates on this logic. Hence: it's necessary, it's unavoidable, and it has precedent.

Opine?

On Friday, 6 July 2018 at 10:44:09 UTC, FeepingCreature wrote:
> Consider the humble struct S
> {
>     Object obj;
>     invariant
>     {
>         assert(this.obj !is null);
>     }
>     @disable this();
>     this(Object obj)
>     in(obj !is null)
>     {
>         this.obj = obj;
>     }
> }

Oops - there should of course be a ~this() { } in there.

On Friday, 6 July 2018 at 10:44:09 UTC, FeepingCreature wrote:
> Why is this a problem? ("Just don't use S.init!")
>
> Well, for one it makes Nullable!S impossible. Nullable, if it is to be @nogc, *necessarily* has to construct an S.init struct member.

The rest looks sensible to me, but I have to say this is bollocks. This Nullable never has to construct an S.init:

struct Nullable(T) {
    ubyte[T.sizeof] _payload;
    bool _hasValue;

    @property @trusted
    ref T value() {
        assert(_hasValue);
        return *cast(T*)_payload.ptr;
    }

    @property @trusted
    void value(T val) {
        cleanup();
        _hasValue = true;
        _payload = *cast(typeof(_payload)*)&val;
    }

    @property
    void value(typeof(null)) {
        cleanup();
        _hasValue = false;
    }

    @trusted
    private void cleanup() {
        if (!_hasValue) return;
        destroy(*cast(T*)_payload.ptr);
        _hasValue = false;
    }

    this(T val) {
        value = val;
    }

    ~this() {
        cleanup();
    }

    this(typeof(null) val) {
        value = val;
    }

    void opAssign(T val) {
        value = val;
    }

    void opAssign(typeof(null) val) {
        value = val;
    }
}

struct S
{
    Object obj;
    invariant
    {
        assert(this.obj !is null);
    }
    @disable this();
    @safe
    this(Object obj)
    {
        this.obj = obj;
    }
    @safe ~this() {}
}

@safe unittest {
    Nullable!S a; // Look ma, no assert!
}

Not only do I think it's unnecessary for Nullable to declare a T field, I think it's a bug if it does. The destructor problem you point out is one of the key reasons for this.

--
  Simen

On Friday, 6 July 2018 at 12:10:58 UTC, Simen Kjærås wrote:
> The rest looks sensible to me, but I have to say this is bollocks. This Nullable never has to construct an S.init:
>
> struct Nullable(T) {
>     ubyte[T.sizeof] _payload;
>     bool _hasValue;
>

Come on, at least make it a union with a void[]. This way will fail with any struct that requires a certain alignment. Not to mention that you can stick a class in it and it'll be garbage collected, because ubyte[] must not store pointers to GC memory.

Which makes the point, really - this is an utterly blatant, unreliable, fragile hack.

On Friday, 6 July 2018 at 12:31:50 UTC, FeepingCreature wrote:
> On Friday, 6 July 2018 at 12:10:58 UTC, Simen Kjærås wrote:
>> The rest looks sensible to me, but I have to say this is bollocks. This Nullable never has to construct an S.init:
>>
>> struct Nullable(T) {
>>     ubyte[T.sizeof] _payload;
>>     bool _hasValue;
>>
>
> Come on, at least make it a union with a void[]. This way will fail with any struct that requires a certain alignment. Not to mention that you can stick a class in it and it'll be garbage collected, because ubyte[] must not store pointers to GC memory.
>
> Which makes the point, really - this is an utterly blatant, unreliable, fragile hack.

The union brings back the exact problems we're trying to avoid. It also makes it impossible to handle types where hasElaborateAssign is true and this() is @disabled at the same time. There's also the issue that at best it will do the wrong thing when you @disable this() and init, but you probably shouldn't do that anyway.

As for alignment, GC, and possibly other things, the code was not intended as a complete implementation of Nullable, only to show that an actual member of type T is not necessary. These issues are fixable, if perhaps nontrivial in some cases.

--
  Simen

On Friday, 6 July 2018 at 23:37:30 UTC, Simen Kjærås wrote:
> As for alignment, GC, and possibly other things, the code was not intended as a complete implementation of Nullable, only to show that an actual member of type T is not necessary. These issues are fixable, if perhaps nontrivial in some cases.
>
> --
>   Simen

Yeah, sorry - that was a snap answer; union is indeed not a solution, particularly since it doesn't even work for types with elaborate destructors.

That said, I agree that this can be implemented, however imo the fact that we have to retrace the work of the compiler in laying out the data is a warning sign - imo, it happens because what we actually want is for the compiler to *work a different way*, which is why we find ourselves painstakingly recreating its interna in userland, except with different destructor semantics. This points at a flaw in the language, imo - if we so urgently need a way to express different semantics, we shouldn't have to painstakingly hide the type behind the compiler's back; the compiler should have our back in this instead of fighting us.

On 7/7/18 11:06 PM, FeepingCreature wrote:
> On Friday, 6 July 2018 at 23:37:30 UTC, Simen Kjærås wrote:
>> As for alignment, GC, and possibly other things, the code was not intended as a complete implementation of Nullable, only to show that an actual member of type T is not necessary. These issues are fixable, if perhaps nontrivial in some cases.
>>
>> -- 
> 
> Yeah, sorry - that was a snap answer; union is indeed not a solution, particularly since it doesn't even work for types with elaborate destructors.
> 
> That said, I agree that this can be implemented, however imo the fact that we have to retrace the work of the compiler in laying out the data is a warning sign - imo, it happens because what we actually want is for the compiler to *work a different way*, which is why we find ourselves painstakingly recreating its interna in userland, except with different destructor semantics. This points at a flaw in the language, imo - if we so urgently need a way to express different semantics, we shouldn't have to painstakingly hide the type behind the compiler's back; the compiler should have our back in this instead of fighting us.

Hm... this reminds me of the recent discussion around __symbols, and how they are treated specially.

Could we reserve some subset of those symbols to say "treat these differently"?

Or have an attribute that marks a member in such a way? I'm reminded of of the future storage class __mutable as well.

-Steve

On Tuesday, 10 July 2018 at 00:01:28 UTC, Steven Schveighoffer wrote:
> On 7/7/18 11:06 PM, FeepingCreature wrote:
>> On Friday, 6 July 2018 at 23:37:30 UTC, Simen Kjærås wrote:
>>> As for alignment, GC, and possibly other things, the code was not intended as a complete implementation of Nullable, only to show that an actual member of type T is not necessary. These issues are fixable, if perhaps nontrivial in some cases.
>>>
>>> --
>> 
>> Yeah, sorry - that was a snap answer; union is indeed not a solution, particularly since it doesn't even work for types with elaborate destructors.
>> 
>> That said, I agree that this can be implemented, however imo the fact that we have to retrace the work of the compiler in laying out the data is a warning sign - imo, it happens because what we actually want is for the compiler to *work a different way*, which is why we find ourselves painstakingly recreating its interna in userland, except with different destructor semantics. This points at a flaw in the language, imo - if we so urgently need a way to express different semantics, we shouldn't have to painstakingly hide the type behind the compiler's back; the compiler should have our back in this instead of fighting us.
>
> Hm... this reminds me of the recent discussion around __symbols, and how they are treated specially.
>
> Could we reserve some subset of those symbols to say "treat these differently"?
>
> Or have an attribute that marks a member in such a way? I'm reminded of of the future storage class __mutable as well.
>
> -Steve

That would also solve the problem.

Note that I've recently realized that the format!"" weird-exception bug ( issue 19003 https://issues.dlang.org/show_bug.cgi?id=19003 ) is based on the same root assumption - Phobos blithely presumes that T.init is a valid instance of T that it can, say, call toString() on.

Do you think "Structs with nontrivial Domain Data Considered Harmful" would make a good article name?

Ping: Resurrecting this thread because @thewilsonator resurrected my spec pr.

https://github.com/dlang/dlang.org/pull/2410

On Sunday, 18 November 2018 at 13:05:28 UTC, FeepingCreature wrote:
> Ping: Resurrecting this thread because @thewilsonator resurrected my spec pr.
>
> https://github.com/dlang/dlang.org/pull/2410

Rebuking Simen's response then :)

On Friday, 6 July 2018 at 23:37:30 UTC, Simen Kjærås wrote:

> The union brings back the exact problems we're trying to avoid. It also makes it impossible to handle types where hasElaborateAssign is true and this() is @disabled at the same time. There's also the issue that at best it will do the wrong thing when you @disable this() and init, but you probably shouldn't do that anyway.

https://issues.dlang.org/show_bug.cgi?id=19321
https://github.com/dlang/dmd/pull/5830

Unions have been "fixed" for some time now. Use them, no need for hacks with ubyte.

struct Nullable(T) {
    private union U {
        T value = T.init;
    }
    private U _u;
    private @property ref _payload() inout { return _u.value; }
    private bool _hasValue;

    @property @trusted
    ref T value() {
        assert(_hasValue);
        return _payload;
    }

    @property @trusted
    void value(T val) {
        cleanup();
        _hasValue = true;
        _payload = val;
    }

    @property
    void value(typeof(null)) {
        cleanup();
        _hasValue = false;
    }

    @trusted
    private void cleanup() {
        if (!_hasValue) return;
        destroy(_payload);
        _hasValue = false;
    }

    this(T val) {
        value = val;
    }

    ~this() {
        cleanup();
    }

    this(typeof(null) val) {
        value = val;
    }

    void opAssign(T val) {
        value = val;
    }

    void opAssign(typeof(null) val) {
        value = val;
    }
}

struct S
{
    Object obj;
    invariant
    {
        assert(this.obj !is null);
    }
    @disable this();
    @safe
    this(Object obj)
    {
        this.obj = obj;
    }
    @safe void opAssign(S) { /* ... */ }
    @safe ~this() {}
}

@safe unittest {
    Nullable!S a; // Look ma, no assert!
}