Fix 14477, Nullable on struct with @disable this()

[jwhear]

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

Simple change to initialize Nullable's internal value to void, allowing structs with disabled default constructors to be stored. This is desirable because Nullable is the obvious tool for wrapping such structs such that they can be default initialized.

Adds unittest.

Fix 14477

[Hackerpilot]

✅

[JakobOvrum]

Doesn't this make Nullable intrinsically unusable in safe code?

[MetaLang]

LGTM.

[jwhear]

Doesn't this make Nullable intrinsically unusable in safe code?

My brain says yes, but DMD says no. I've marked the unittest as safe and it compiles and runs without error or warning.

[schuetzm]

It's un-@safe if T has indirections. To avoid regressions, we should test with static if() whether assignment works, and use the void initializer only if it doesn't.

[jwhear]

It's un-@safe if T has indirections.

@schuetzm Either I'm misunderstanding you or DMD isn't properly enforcing @safe. My latest commit adds indirections to the unittest and it still happily compiles.
That said, I'd be fine with adding the static if check--that was actually my initial implementation before I realized that all types would benefit from skipping the useless default-initialization.

[schuetzm]

@jwhear It's indeed a bug. Reported here: https://issues.dlang.org/show_bug.cgi?id=14496

[jwhear]

@schuetzm Thanks for filing. I'm wondering if Nullable can/ought to leverage @trusted in this situation to enable @safe code when working with such types.

T foo;  // won't compile
T baz = void; // unsafe in user code
Nullable!T bar; // the user can do this to be @safe

The rationale being that Nullable guarantees either a null state or a valid value T.

[schuetzm]

Currently it cannot be @trusted, because opAssign() just assigns to the value. It needs to check whether there's a valid object in there. If yes, it can assign, otherwise it needs to copy-emplace it. nullify() also just calls destroy(), even if there is no valid value. And the constructor assigns, but I thinks that's ok, because as the first assignment in a constructor, it will be treated as construction (didn't check though).

[jwhear]

Squashed to a single commit. Substantial changes, please re-review.

• Now using static ifs to retain previous behavior and guarantees for all existing code.
• nullify only destroys the value if it has been assigned/initialized.
• get has been changed from @safe to @trusted so that this won't break when https://issues.dlang.org/show_bug.cgi?id=14496 is fixed. My understanding is that this won't cause any breakage in existing safe code, is this correct?
• I've beefed up the unittest to include a struct with indirections.
• I've added a unittest marked with @safe to ensure that the old, default-initialized code path remains @safe.

[schuetzm]

• Changing get to @trusted doesn't make sense. Nothing in this method is unsafe, so it can stay @safe. It's construction that will become @system when the bug is fixed. Therefore, there need to be two constructors, a @trusted one for void initialization, and an unmarked one.
• opAssign is still wrong, because it can potentially call opAssign on an uninitialized object.

[MetaLang]

Is there a problem with using T t = T.init instead of T t = void to avoid all this unsafe behaviour?

struct Test(T)
{
    //Won't compile
    //T val1

    //Ok
    T val = T.init;
}

struct NoDefCon
{
    @disable this();
}

void main()
{
    Test!NoDefCon t;
}

[schuetzm]

Right, that's a good idea!

It still leaves the possibility that T's opAssign() and ~this() can't handle the init value, because it always expects to be constructed explicitly, but arguably, such a type is incorrect, so it's not our problem.

[monarchdodra]

T.init is the "correct" way to generically initialise something. It does not necessarilly guarantee opAssign (EDIT: or any other operation) will work, because the object may not yet be run-time initialized, but it remains safe.

The "mostest correct" alternative would be (IMO) to emplace on first assignment, and destroy when nullifying (which is what I'd expect ffrom null-ifying, BTW). But this is already good enough.

As for ~this, I'm 99% sure spec mandates it never chokes on T.init

LGTM (once changed to T.init)

[jwhear]

Now uses .init, reverted get to @safe

[monarchdodra]

Out of date comment. Mostly irrelevent now (see next comment).

[jwhear]

@monarchdodra @schuetzm Updated and dramatically simplified with .init

[monarchdodra]

I'm not really happy with this change. When a struct is marked as @disable this(), it is making an explicit request to be initialized before use. = init; is a low level initialization scheme that can by-pass that, but still assumes you will later do correct initialization (probably via emplace).

Currently, doing it this way is a blatant violation of default initializing something that has a disabled this. The compiler refused to compile the code, and I think it was correct in doing so. A workaround is to always initialize your nullable to a value, and make null afterwards.

If we really want this to work, that I'd really favor we go for an emplace/destroy scheme on every call to first-initialization/nullify. That would be guaranteed correct.

[jwhear]

@monarchdodra Sounds like you have a better handle on this than I do. Would you be willing to make a PR of your own to address this? If so, I'd be happy to close this one.

[monarchdodra]

I am totally overbooked, and don't even have a working environment from which to push from. Besides, I'm sure you can manage. If people who had a better handle always stepped in, we'd get nowhere fast.

[jwhear]

I'm closing this; when I'm able to put together another stab at it, I'll open another PR.