std.typecons: make Nullable payload void initialized

[ljmf00]

Signed-off-by: Luís Ferreira contact@lsferreira.net

[dlang-bot]

Thanks for your pull request and interest in making D better, @ljmf00! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8219"

[thewilsonator]

Why?

[ljmf00]

Why?

Because this payload only gets valid when assigned and the assignment only occurs when it is not null. Therefore, it doesn't necessarily need to be initialized.

It still needs to be initialized in order to be @safe in edge cases where a reference to the payload is returned and assigned afterwards as get might return a reference.

This can be beneficial and have a real-world impact on beefy structs or static arrays.

[ljmf00]

Why?

Because this payload only gets valid when assigned and the assignment only occurs when it is not null. Therefore, it doesn't necessarily need to be initialized.

It still needs to be initialized in order to be @safe in edge cases where a reference to the payload is returned and assigned afterwards as get might return a reference.

This can be beneficial and have a real-world impact on beefy structs or static arrays.

This can be fully void initialized if the assignment of void initialized memory doesn't override it, although I'm not totally sure about that, and doubt it.

[ljmf00]

I can come up with an idea to fully void initialize this safely and discuss

[schveiguy]

This will not achieve the desired effect. Because there is at least one non-void member, the entire struct will get an initializer value, and the entire struct will be initialized to set the value of the non-void member _isNull. I believe the void initialized member will be set to all 0.

Not only that, but the _isNull condition assumes that the payload is T.init, so if the =void initializer actually did what you wanted, it could be consequentially bad, as I think its destructor would be called on assignment.

[ljmf00]

This will not achieve the desired effect. Because there is at least one non-void member, the entire struct will get an initializer value, and the entire struct will be initialized to set the value of the non-void member _isNull. I believe the void initialized member will be set to all 0.

Can you prove me with a specification point? I think that behaviour is implementation-defined, although didn't find anything in there. If there is no point talking about that, I assume the default behaviour of the void initialization.

Not only that, but the _isNull condition assumes that the payload is T.init, so if the =void initializer actually did what you wanted, it could be consequentially bad, as I think its destructor would be called on assignment.

What do you mean here? The destructor is never called as it is wrapped in a union to specifically have that behaviour.

// 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 this is not what you mean, can you elaborate on that with an example, please?

[schveiguy]

The spec does not say how types with partial void initializers are initialized. True, one could implement a compiler that only initializes the non void parts. But that's not what happens now. But at least Andrei agrees that the current implementation is substandard (while also acknowledging the "spec" of TypeInfo requires the current implementation): https://issues.dlang.org/show_bug.cgi?id=21277

What do you mean here?

I actually misread this note:

        if (_isNull)
        {
            // trusted since payload is known to be T.init here.
            () @trusted { moveEmplace(copy.payload, _value.payload); }();
        }

I didn't realize moveEmplace would not call the destructor. So you are right on that point. Perhaps change the note.

The code inside the assign operator is confusing for sure (why make a copy to avoid destructing the value, when the value's destructor will already run?). I have the feeling that this can be done in a simpler way. But this PR is not about that.

[ljmf00]

The spec does not say how types with partial void initializers are initialized. True, one could implement a compiler that only initializes the non void parts. But that's not what happens now. But at least Andrei agrees that the current implementation is substandard (while also acknowledging the "spec" of TypeInfo requires the current implementation): issues.dlang.org/show_bug.cgi?id=21277

Arguing that this shouldn't be a thing because currently it is implemented in a way it is not described in the specification is not correct.

I actually misread this note:

        if (_isNull)
        {
            // trusted since payload is known to be T.init here.
            () @trusted { moveEmplace(copy.payload, _value.payload); }();
        }

I didn't realize moveEmplace would not call the destructor. So you are right on that point. Perhaps change the note.

I changed.

The code inside the assign operator is confusing for sure (why make a copy to avoid destructing the value, when the value's destructor will already run?). I have the feeling that this can be done in a simpler way. But this PR is not about that.

This is to efficiently move memory to _value.payload reference as it is known to be uninitialized, and therefore no need to call the destructor although I'm not aware of how this can be done in a better way.

[pbackus]

The code inside the assign operator is confusing for sure (why make a copy to avoid destructing the value, when the value's destructor will already run?). I have the feeling that this can be done in a simpler way. But this PR is not about that.

This is a workaround introduced in PR #6619 to deal with types whose destructor cannot be called on T.init, because T.init violates the type's invariant contract. See issue 19065 for further discussion.

[ljmf00]

Commit message changed to comply with the contributions guidelines.

[ljmf00]

The code inside the assign operator is confusing for sure (why make a copy to avoid destructing the value, when the value's destructor will already run?). I have the feeling that this can be done in a simpler way. But this PR is not about that.

This is a workaround introduced in PR #6619 to deal with types whose destructor cannot be called on T.init, because T.init violates the type's invariant contract. See issue 19065 for further discussion.

Thanks for clarifying this! Anyway, this enters a bit out of the scope of this PR.