Further changes to std.typecons.Unique

[JakobOvrum]

Follow-up to #3139.

• Documentation fixes and improvements
• Added examples
• A fix for destruction of held non-class values
• Deprecated empty in favour of cast(bool)
• Initialization is now a precondition for geting non-class values, to support nothrow

get is still a template with two overloads because it's the only way to make the documentation show them both, as far as I can tell.

Deprecation of empty

Currently there are two ways to check the validity of a Unique reference, empty and cast(bool). I think we should only have one, and that is cast(bool): empty is not only a bad name as it implies a range or container, but is highly likely to hijack T.empty in user code, which could conceivably cause hard to catch bugs ("why does it claim my unique container/range is never empty??").

Initialization now a precondition for geting non-class values

This is required to support nothrow. I plan on working on other attributes, particularly @safe, once #3031 is ready and merged.

---

What do you guys think?

edit:

BTW, what's up with deprecations not showing up in documentation?

[mihails-strasuns]

I am very opposed to leaving cast(bool) as the only way to check validity for the very reason it is a cast, inherently dangerous thing that needs to be avoided as much as possible. empty is a bad name but it can be simply replaced with something like initialized or valid.

[JakobOvrum]

cast(bool) has special recognition in a variety of expressions:

Unique!Object u;
assert(!u);
u = unique!Object();
assert(u);

if(u) {
    // Valid
} else {
    // Invalid
}

And so forth. If you want to avoid leaving the cast token in your source code, you can use !!u.

[mihails-strasuns]

!!u is an ugly and counter-intuitive workaround, it is not an acceptable solution without alternatives

[JakobOvrum]

In practice I assume most uses of the opCast primitive will be in if-statements, where its use is both elegant and intuitive. I don't think a tiny percentage of use cases warrants its own name in Unique, where names are extremely costly due to having a generic, user-supplied supertype. An alternative is declaring a free function; I could get behind that.

[HK47196]

nothrow is not worth the problems of debugging memory corruptions.

no but it's worth the problems of being @nogc, or else unique!T quickly becomes useless to a lot of users - exceptions aren't usable in @nogc.

[mihails-strasuns]

exceptions aren't usable in @nogc

Should I really explain why this statement is wrong? :)

[mihails-strasuns]

@JakobOvrum can you provide free function that check if its argument Unique is initialized? AFAIR D name resolution rules, it shouldn't take priority over member function : http://dpaste.dzfl.pl/5977af784165

[mihails-strasuns]

(it can have longish name like isValidUnique to make rish of clashes even smaller)

[JakobOvrum]

isValidUnique sounds good to me. Will add it to this PR.

Should I really explain why this statement is wrong? :)

It is true in practice for Phobos. Even the statically allocated exception thrown from onOutOfMemoryError will cause serious problems if onOutOfMemoryError is called again while the first exception is still in flight (such as in a scope(exit|failure) block, or a destructor). It is a hard problem to solve.

[mihails-strasuns]

All issues I know are about mutable pre-allocated exceptions. What is the issue with static immutable Exception reference chaining on itself? File/line will become immutable too, of course, but that is acceptable when you only need to use it as "soft abort".

[JakobOvrum]

The exception system in D is fundamentally broken in terms of non-mutable exceptions:

void main()
{
    static immutable ex = new Exception("");

    bool caught = false;
    try throw ex;
    catch(Exception e)
    {
        caught = true;
        e.next = null;
    }
    assert(caught);
}

Not only is the immutable exception caught as a mutable reference, but the exception infrastructure doesn't care about immutability either, it will happily mutate next in the chaining mechanism regardless of immutability. Statically allocated class instances are in global memory, so this code isn't even thread safe!

It has been discussed here and elsewhere in the past.

[JakobOvrum]

Added isValidUnique and relevant cross-references.

[JakobOvrum]

Does anyone know what the failures are about? They don't manifest on Windows...

Does it mean there's a memory leak somewhere?

[HK47196]

*** Error in `/.../.local/build/phobos/typecons': free(): invalid pointer: 0x00000000005b28f0 ***
======= Backtrace: =========
/usr/lib/libc.so.6(+0x7198e)[0x7ffff710c98e]
/usr/lib/libc.so.6(+0x76dee)[0x7ffff7111dee]
/usr/lib/libc.so.6(+0x775cb)[0x7ffff71125cb]
/.../.local/build/phobos/typecons[0x4129fa]


(gdb) info symbol 0x4129fa
std.typecons.Unique!(std.typecons.__unittestL247_2().I).Unique.~this() + 82 in section .text of /.../.local/build/phobos/typecons

(gdb) bt
#0  0x00007ffff70ce4b7 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff70cf88a in abort () from /usr/lib/libc.so.6
#2  0x00007ffff710c993 in __libc_message () from /usr/lib/libc.so.6
#3  0x00007ffff7111dee in malloc_printerr () from /usr/lib/libc.so.6
#4  0x00007ffff71125cb in _int_free () from /usr/lib/libc.so.6
#5  0x00000000004129fa in std.typecons.Unique!(std.typecons.__unittestL247_2().I).Unique.~this() (this=...) at std/typecons.d:125
#6  0x0000000000402dd1 in std.typecons.__unittestL247_2() () at std/typecons.d:257
#7  0x00000000004c2c56 in std.typecons.__modtest() ()
#8  0x00000000004faed0 in core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*) ()
#9  0x00000000004c7384 in object.ModuleInfo.opApply(scope int(object.ModuleInfo*) delegate).__lambda2(immutable(object.ModuleInfo*)) ()
#10 0x00000000004cf27f in rt.minfo.moduleinfos_apply(scope int(immutable(object.ModuleInfo*)) delegate).__foreachbody2(ref rt.sections_elf_shared.DSO) ()
#11 0x00000000004cf2f6 in rt.sections_elf_shared.DSO.opApply(scope int(ref rt.sections_elf_shared.DSO) delegate) ()
#12 0x00000000004cf215 in rt.minfo.moduleinfos_apply(scope int(immutable(object.ModuleInfo*)) delegate) ()
#13 0x00000000004c735d in object.ModuleInfo.opApply(scope int(object.ModuleInfo*) delegate) ()
#14 0x00000000004fad94 in runModuleUnitTests ()
#15 0x00000000004cba1f in rt.dmain2._d_run_main(int, char**, extern(C) int(char[][]) function*).runAll() ()
#16 0x00000000004cb9d2 in rt.dmain2._d_run_main(int, char**, extern(C) int(char[][]) function*).tryExec(scope void() delegate) ()
#17 0x00000000004cb94c in _d_run_main ()
#18 0x00000000004c3e30 in main ()
#19 0x00007ffff70bb800 in __libc_start_main () from /usr/lib/libc.so.6
#20 0x0000000000402ba9 in _start ()

If you comment out line 257 it doesn't error. I'm too busy atm to look into this any further, I hope that helps.

[MartinNowak]

No need to deprecate the function, we just added it.
You could add a deprecated alias for the previously existing isEmpty though, just in case someone used it.

[MartinNowak]

In practice I assume most uses of the opCast primitive will be in if-statements

In practice all uses will be in if statements, there is no use for a named function b/c Unique is a truth value itself.

if (uniq) {}
immutable val = uniq ? true : false;
immutable val2 = !!uniq;

[JakobOvrum]

immutable val2 = !!uniq;

See this comment. Ping @Dicebot

[mihails-strasuns]

Yeah I don't think I have anything to add there. !!expr idiom is common source of confusion/questions and is thus discouraged at least in some coding styles :)

[mrkline]

Sorry I'm late to the party. Ideally, I wanted to allow Unique to act just like a pointer in comparisons (just like unique_ptr in C++), allowing if (unique is null), etc. Unfortunately we can't do this as it would require alias _p this, and we agreed in the last PR that allowing the underlying pointer to escape Unique is undesirable. I think a named function is the next best bet.

I'm certainly with @Dicebot - there should be some function to indicate if the Unique is null. Much like many people (myself included) prefer if (somePointer == null) over if (!somePointer) and if (someString.empty) over if(someString == ""), I'm sure some would prefer the additional clarity of a named property or function. If empty is a poor name, renaming it is all well and good, but

• Why make this a freestanding function? It would only ever be used with a Unique and depends on the internals of Unique. If that isn't a mandate for it being a member, I don't know what is. Perhaps I'm not understanding your reasoning behind this - could you elaborate on why "names are extremely costly due to having a generic, user-supplied supertype"? Is it because it could overshadow some property of the same name on the type T it holds?
• isValidUnique sounds oddly verbose compared to isValid when you would only ever use it on a Unique. Is this for the same reason as the point above?

Now that I think about it, none of this would be an issue if D didn't use . for both members of a type and to access the given members on a pointer to a type (a la -> in C++). None of this is an issue with unique_ptr because accessing the member functions is done with -> and operations on the smart pointer itself are done with .. Oh well.

[JakobOvrum]

But here you'd better just replace the underlying file and all RefCounted!File aliases will use it b/c RefCounted already is a reference type.

Yes, which is enabled because of RefCounted!File. With just File there's no way to do that.

vs.

Your code is wrong. The correct code illustrates the issue:

auto ppi = refCounted!(int*);
ppi = null; // same as ppi.get = null;
ppi.nullify;

ppi = null and ppi.nullify do different things.

[MartinNowak]

ppi = null and ppi.nullify do different things.

You really think that's a blocker, given that ppi = null is a rare thing to do?
So far you've dismissed 2 proposals :). What other options do we have?

[MartinNowak]

Note that our design space is restricted to remain somewhat compatible with the existing RefCounted.

[MartinNowak]

Maybe we should simply go with isEmpty. Resetting can be done using object.destroy.

[mrkline]

Regardless of the rest of our discussion, I agree with @JakobOvrum's initial assessment that empty or isEmpty might not be the best name. Since it is essentially a smart pointer, I really think isNull would be a better call. And if we're going to do that, nullify is right around the corner. Hm...

Would it be a decent compromise to disable = null and comparisons to null? That way there would at least be less confusion between = null and nullify along with == null and isNull. If the Unique was a reference to another reference type, you can still use myUnique.get() = null.

[MartinNowak]

Would it be a decent compromise to disable = null and comparisons to null?

Doable, but it would require forwarding of opEquals and opAssign, not the most elegant solution b/c it's not possible to forward rvalues as rvalues.
We could use the isInitialized from RefCounted.

[mrkline]

That seems like a bit of a misnomer too, and something I was hoping we could move away from when we work RefCounted over. It's not as if an empty/null smart pointer is uninitialized (which to me implies it's in some invalid state). It just doesn't have a value currently.

I would wager that we're all tired of splitting hairs on this naming and convention issue, but

1. The oddities of these types are always brought up as major detriments to D (especially from C++ developers used to using smart pointers)
2. If we're going to make breaking changes to these types, we should make damn sure we do it properly so that this doesn't have to be revisited later (hopefully ever).

Because of these points I don't think we should introduce breaking changes to any of these types (Unique, RefCounted, and possibly Nullable) until we have a solid consensus. If that means reverting #3139 until this is all hashed out, so be it. Would you agree we don't want to spread the pain, as it were, across multiple releases?

[mrkline]

@JakobOvrum - I just did some experimenting which may put your concerns to rest - see the commit above I just made to my branch. Unlike Nullable, we already have a special case for classes. Creating a Unique!Object doesn't cause two layers of indirection because that's handled by the RefT alias. (We should give Nullable a similar treatment later.)

Even for non-class reference types (e.g. int*), someUnique = null is a compilation error, sidestepping the entire confusion Nullable currently has about = null vs. nullify. Observe:

// Unlike the current Nullable, assigning to null doesn't work,
// making confusion between the effects of "= null" and "nullify" moot.
auto uo = unique!Object();
// uo = null; // Error: forwards to get(), which returns an rvalue for classes
assert(uo);
uo.nullify();
assert(!uo);

auto up = unique!(int*);
assert(up);
int actual = 42;
up.get() = &actual;
// up = null; // Also donesn't work (no opAssign for typeof null)
assert(up);
up.nullify();
assert(!up);

With this brought to light, can we please

1. Adopt Nullable-like semantics for Unique and RefCounted, i.e. isNull and nullify.
2. Fix Nullable to behave like this so that = null becomes an error, sidestepping the whole issue we've been debating.

[MartinNowak]

Ah, b/c we already have an opAssign, that's also true for RefCounted. Sounds like a plan.

[mrkline]

To be completely transparent, there still might be some confusion with == null (which forwards to .get() == null) vs. isNull, and if we disable opEquals(typeof(null)), that means that assignment no longer forwards to get. I'm not sure if that's as big of a concern as the = null vs. nullify one, though.

[MartinNowak]

We can't deprecate the existing behavior of RefCounted without a very good reason, so maybe we should leave opEquals alone.

[mrkline]

Sounds fine to me - we can talk more about what to do with RefCounted in PRs related to it.

[MartinNowak]

Can we move on with this @JakobOvrum?

[MartinNowak]

I'll open a separate pull request.

[mrkline]

@MartinNowak Did you end up cutting another PR for this?

[mrkline]

And what's the plan for this with 2.068? I haven't been following the release schedule carefully, but if a new version is around the corner, perhaps we should defer this (and back out #3139) until we can get details hashed out and a plan for related changes to RefCounted.

[HK47196]

@mrkline I believe there's not going to be last minute PRs accepted anymore to streamline the release process/reduce regressions. So, that would mean this isn't going to be part of 2.068.

[mrkline]

@rsw0x: Right - my main point was that we should back out #3139 if we're not going to push this through for 2.068. Let's not change Unique twice and spread the pain over multiple releases.

[MartinNowak]

but if a new version is around the corner, perhaps we should defer this (and back out #3139) until we can get details hashed out and a plan for related changes to RefCounted.

Andrei thinks, it's important enough to push the whole Unique/RefCounted stuff into the next release.
I agree and am a bit unhappy that things have been laying around for 3-4 weeks.
I'll finish both pull requests over the weekend.

[mrkline]

Sorry for my general inactivity. My day job and life have been rather busy, but I should be back in the action to help with whatever you need.

So,

1. As per Andrei's suggestion, have you already reverted Improve std.typecons.Unique #3139 from whatever RC branch you have?
2. Seeing no comment from @JakobOvrum, are we going to go with the Nullable model? The appropriate changes should be ready to go in my PR against Jakob's work, whose tip is mrkline/phobos@2e7ec065772156a5af57898f5af7fbb56234fc9c

[MartinNowak]

but I should be back in the action to help with whatever you need.

It's fine, I'm working on all of the remaining issues. Currently trying to get the attributes for class destructors right, which is tricky b/c of polymorphism.
Also I intend to introduce a new RC type, b/c we can't really fix RefCounted w/o breaking code.

As per Andrei's suggestion, have you already reverted #3139 from whatever RC branch you have?

He meant 2.068, so the immediately next release.

are we going to go with the Nullable model?

Yes, I've based my work on your changes.

[MartinNowak]

Yikes, another attribute nightmare.
For any non-trivial code attributes make things at least twice as complicated :(.
MartinNowak@6434c8b

Here is my branch btw, https://github.com/MartinNowak/phobos/tree/smartRefs.

[mrkline]

He meant 2.068, so the immediately next release.

Sweet! I misread you originally as "we're putting it off for the release after 2.068 so we can take the time to finalize stuff".

Yes, I've based my work on your changes.

Great. Let me know when you open a PR on your branch. There's a few things to discuss besides the WIP destructor magic, such as:

• You can't make unique() @nogc. Even if emplace was @nogc (which it isn't), we're constructing arbitrary, user-defined objects that are free to GC up memory in their constructors.
• Similarly, you can't make the unit tests @safe before making std.algorithm.move and a few others @safe (and are they really?)

These are just things I noticed by trying to compile typecons.d - see https://github.com/mrkline/phobos/commit/76888a289c332e2baf438638c5de361976066f6b

[HK47196]

You can't make unique() @nogc. Even if emplace was @nogc (which it isn't), we're constructing arbitrary, user-defined objects that are free to GC up memory in their constructors.

this is true, but some(?) unit-tests should be @nogc because it's incredibly easy for a small change elsewhere/in the runtime to make a part of it not @nogc due to us being unable to enforce @nogc for the reason you listed. Being able to catch this early instead of in a regression is highly preferred IMO.

[mrkline]

Well, yes, but I don't know how unit tests would test Unique without... you know... calling the function (which is not @nogc) that creates a Unique.

[MartinNowak]

You can't make unique() @nogc. Even if emplace was @nogc (which it isn't), we're constructing arbitrary, user-defined objects that are free to GC up memory in their constructors.

Yes you can, you need to use attribute inference in such a way that @nogc is determined solely by the user object, that's what I meant with attribute nightmare.

Similarly, you can't make the unit tests @safe before making std.algorithm.move and a few others @safe (and are they really?)

Yes they are, and I already did that on my branch.

Same as #3259 (comment), closed in favor of a complete solution to smart refs.
WIP https://github.com/MartinNowak/phobos/tree/smartRefs.