Fix issue 19037: use move/moveEmplace in Nullable

[FeepingCreature]

Makes Nullable usable with every type, aside the crazies who have pointers to their members. opAssign of the underlying type will no longer be called.

This enables Nullable on, for instance, structs with @disable this(); and a SysTime.

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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

Auto-close	Bugzilla	Severity	Description
✓	19037	enhancement	Nullable should use moveEmplace to support any type.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + phobos#6619"

[PetarKirov]

On a first glance the changes look good. Can you add one more test cases for payload types for which the use of move / moveEmplace is necessary?

[FeepingCreature]

Related question: should

Nullable!T nt = T(1);
nt = T(2);

call opAssign on the T in the Nullable? Or move() the T(2) over it, ie. destroy + memcpy?

[FeepingCreature]

Testcase added. I'll just stick with "always move" for now, for internal consistency.

[FeepingCreature]

ping

[wilzbach]

I like the idea! (but not the @trusted)

[wilzbach]

How can we be sure that this is always @safe?
Wouldn't it be better to make move/moveEmplace due the inference (and rather fix that than blasting things with @trusted here)?

[FeepingCreature]

Because move is @safe and moveEmplace is only unsafe because it calls memcpy, which we know to be safe because the target variable is only initialized to .init and no methods have been called on it yet, or .destroy has been called on it; semantically it's uninitialized memory.

"A method that can call one of two methods, one of which is @safe and the other of which we know to be @safe but cannot convince the compiler" is an archetypal usecase for @trusted.

edit: The interesting question would be if moveEmplace calls postblit.

edit: It does not. Which makes sense - if there's any spot where postblit should be called, it's when generating the opAssign call. What about struct destructors..?

edit: on the other hand, there's an actual problem in that we move the struct but then we don't call postblit, but then we destruct it in the opAssign function!

[FeepingCreature]

The assumptions used in this code are documented in dlang/dlang.org#2410 .

Note that DMD does not play ball yet - PR incoming.

[FeepingCreature]

Uh, as far as I can tell these aren't my failures.

[wilzbach]

CircleCi is unrelated -> #6626, but not sure why DAutoTest failed, but as it seems to be offline at the moment, it could be related to this.

[FeepingCreature]

Note that the current state of this PR does not make very much sense without dlang/dlang.org#2410 and dlang/dmd#8462 , which formalize the assumption gleamed from moveEmplace that T.init.~this() is always valid.

[FeepingCreature]

Hm, though given that T.init.~this is supposed to be valid anyways apparently, I guess this should go ahead regardless.

ping

edit: Rebased to rerun tests.

[FeepingCreature]

Re CircleCI, I really don't know what's going on there. std.uni does not even use Nullable.

And ci.dlang.io just times out.

[wilzbach]

Don't worry about this. CircleCi is currently broken as someone managed to change the -dip1000 semantics and break std.uni (see #6626).
We're working on a replacement for ci.dlang.io (dlang/ci#225 (comment)).
Sadly both issues are known.

[FeepingCreature]

@n8sh There's a potential problem with this PR.

As opposed to before, Nullable now actually depends on the assumption that calling a destructor on T.init is a valid operation. This may make problems if people use structs with invariants that check for null or 0 and an (implicit?) destructor.

I hate to delay my own PR, but wanna wait for CircleCI to come back so we can see? Or will it take too long?

[n8sh]

@FeepingCreature Merging anything but trivial changes without waiting for CircleCI would be imprudent. How do you mean this PR makes Nullable depend on the validity of calling a destructor on T.init any more than it already did? It seems like this PR only removes a place where T's destructor would formerly have been called.

[FeepingCreature]

Previously, in opAssign for instance since it'd be implemented as this.value_ = value, the value of value would still be valid (:smile:) at opAssign exit. With move/moveEmplace, it's reset to T.init and runs into the T.init.~this controversy.

[n8sh]

@FeepingCreature you could avoid resetting value to T.init by using std.algorithm.mutation : swap.

void opAssign()(T value)
{
    static if (!hasElaborateAssign!T && !hasElaborateDestructor!T)
    {
        _value = value;
    }
    else
    {
        import std.algorithm.mutation : swap;
        swap(value, _value);
    }
    _isNull = false;
}

That way if _value and value were both valid on function entry, they'll still both be valid on function exit.

[FeepingCreature]

True, but ... initially _value is not valid, since it has to be inited to T.init, so that'd still be equivalent to resetting value to T.init the first time it's assigned, and throw anyway?

Musical chairs with a crash...

[n8sh]

The small benefit to swap is that if you construct Nullable!T with this(inout T value) inout, later calls to opAssign will not cause anything to be reset to T.init unlike with move, but you're right that doesn't do anything to solve the problem of a default constructed Nullable!T. Musical chairs indeed.

The only way I know of to prevent a struct's destructor from being called is by passing it around inside a union. We could use moveEmplace/move in opAssign without worrying about the argument's destructor if the argument type were union { T value; }, but that would require callers to use different syntax. Maybe some kind of implicit conversion magic could avoid that ugliness.

[FeepingCreature]

Unions! That's it!

[FeepingCreature]

Explicit test for destructor call on reassignment added. t renamed to payload. Test syntax improved.

[FeepingCreature]

core.exception.AssertError@/var/lib/jenkins/dlang_projects/weka-io/mecca/src/mecca/reactor/io/signals.d(246): Assert: 3 < 5 Signal count incorrect

$ git clone --depth 1 https://github.com/weka-io/mecca.git
...
$ cd mecca
$ grep Nullable . -R
$

[FeepingCreature]

retrying with hope!

[wilzbach]

The Mecca failure is unrelated. If Buildkite passes you can safely assume that the failure is spurious. Buildkite runs the same tests as Jenkins and is supposed to replace it once the logs are public.

[FeepingCreature]

buildkite has been running for two hours now...? DAutoTest also seems to be stuck.

[wilzbach]

Nay, DAutoTest is just pretty busy and buildkite doesn't run with full resources yet as its experimental (it only uses two cheap VPS of mine for all build jobs).
Anyhow, could you please stop pinging us about the CI being in process?
That's normal and it doesn't block your PR from being merged (for example auto-tester will constantly invalidate its results whenever sth. new got merged and rebuild your PR on all hosts). Once this has the auto-merge label, it will have superior priority for the CIs.

Feel free to discuss CI problems in #ci on Slack

[FeepingCreature]

ping @n8sh ? also ping @andralex I guess is worth a try

[FeepingCreature]

Happy One Month Anniversary of this PR!

A merge tag would be a really good present...

edit: Blech. Just checked the docs and found that fields with destructors in unions are technically forbidden. 😢

Do I have to make a DIP/DMD PR to implement @manualScoped?

edit: Proposed semantics.

edit: Correction! The docs are wrong about this (see dlang/dmd#5830 ).

So this feature is actually 100% legal.

Amazing.

(Not the good kind of amazing.)

[CyberShadow]

I think this change is partially incorrect. The reason is that this moves Nullable and makes assertions about the language in a direction that is incompatible with supporting non-copyable objects.

Consider the opAssign declaration:

Nullable opAssign()(T value)

To support non-copyable objects, we'd just use move to move the value into the store payload. This is the way that many such constructs use. Instead, Nullable's implementation does something ugly and copies it into a weird union.

More importantly, there is a unit test which verifies that during an opAssign call, the destructor was never called on a default-initialized (.init) value.

Well, this is a problem. D does not have move semantics, so move can't actually tell the language to skip destruction of value; all it does is populate it with the default value, which will still have its destructor called.

So, the destructor will be called on value. The choice is between:

1. Letting move put .init in it, therefore the destructor will be called on the default value
2. Copying it, therefore the destructor will be called on the old copy of the value.

The unit test checks that the implementation does 2 and not 1. This is a problem if we want Nullable to support non-copyable types.

One conclusion we can draw from the above is that, because D lacks true move semantics, it should always be valid to call the destructor on the default value; otherwise, such objects will be incompatible with move and everything that uses it. So, I think the unit test is incorrect.

BTW, without move semantics, I don't see any way to support both non-copyable and init-less types :(

[FeepingCreature]

It's been six years, so I don't remember too well. I think I was trying to make Nullable act like Rebindable here? This is before I realized that Rebindable really needed to be its own thing, with RebindableNullable on top.

But... Nullable specifically can skip destruction, via the union hack, because it knows for a fact if its value field is populated or not. If it's not doing that, it probably should.

edit: Oh wait, you mean the parameter. Yeah.

[CyberShadow]

But... Nullable specifically can skip destruction, via the union hack, because it knows for a fact if its value field is populated or not. If it's not doing that, it probably should.

OK, but you can't do the same thing for opAssign's parameter. It's unavoidable in general.

[FeepingCreature]

Yeah I'd forgotten about the parameter. Fwiw, I think this is not the only place that assumes that T.init is a valid value.

[CyberShadow]

Some amendments to the above:

• .init and default construction are not the same thing, so this is about default constructor.
• Apparently things are even worse and move doesn't care if a type has @disable this(), it will still put .init there and let the language call the destructor on it.

[CyberShadow]

Fwiw, I think this is not the only place that assumes that T.init is a valid value.

The problem is actually the inverse - the code and tests try to avoid calling the destructor on T.init at great cost (making copies of the object). I think it's better to allow calling the destructor on T.init.

[FeepingCreature]

I think from a language design perspective this is some sort of "pick two out of three" thing between "getting away without a full borrow checker", "field mutation" and "move semantics". Pretty sure D has just design-overloaded itself in this matter.

[CyberShadow]

Well, here's the PR: #8874

[CyberShadow]

I am not a language designer but move semantics (i.e. invoked explicitly using some kind of new syntax) seem like a purely additive improvement.