Issue 3882: Use cast(void) on calls to assumeSafeAppend

[nordlow]

This should make dlang/dmd#3342 compile Phobos without warnings.

[andralex]

Wait, why is assumeSafeAppend pure and why does it return something interesting?

[nordlow]

This function is a bit of an ugly duckling:
http://dlang.org/phobos/object.html#.assumeSafeAppend

Eventhough DMD (my Issue 3882 pull request for DMD at least warns about it being non-void returning strictly pure) infers it to be strictly pure it does modify memory allocations. However I don't understand how it can be inferred to be pure because it calls

    extern (C) void _d_arrayshrinkfit(TypeInfo ti, void[] arr);

Is this perhaps a bug in DMD?

See line 641 in object.di.

I'm not sure what to do with this.

A question for Walter perhaps?

Anyway it would to be nice to get this issue merged for both Phobos and DMD.

[monarchdodra]

Wait, why is assumeSafeAppend pure

It is pure one the grounds that it is a GC operation, that never actually manipulates elements. This is recent. It's PR: dlang/druntime#553

why does it return something interesting?

It returns it's argument so that you write things such as:

int b = a[0 .. 5].assumeSafeAppend();

(for example).

That said, it is a weekly pure function, so it is my understanding that the error should not be triggered.

[andralex]

That said, it is a weekly pure function, so it is my understanding that the error should not be triggered.

That would save the day. @nordlow ?

[nordlow]

Hmm. according to my tests cases only calls to non-void-returning strictly pure functions are warned about in my DMD pull.

Is perhaps assumeSafeAppend accidentally strictly pure instead of just weakly pure?

This code

@safe pure nothrow void strictVoid(T)(T x) { }
@safe pure nothrow bool strictBool(T)(T x) { return false; }

@safe pure nothrow void nonstrictVoid(T)(ref T x) { x += 1; }
@safe pure nothrow bool nonstrictBool(T)(ref T x) { x += 1; return false; }

void main(string args[])
{
    int x = 3;
    strictVoid(x);
    nonstrictVoid(x);
    strictBool(x);
    nonstrictBool(x);
}

generates only this single line

t_warn_unused_return.d(16,15): Warning: Call to strictly pure function t_warn_unused_return.strictBool!int.strictBool discards return value of type bool, prepend a cast(void) if intentional

when compiled using dlang/dmd#3342

[monarchdodra]

non-void-returning strictly pure

I don't know how you test for strict purity, but assumeSafeAppend takes a parameter that has non-immutable indirection, so it's not strictly pure, even if the argument is passed by value.

Unless in these use cases, we are calling assumeSafeAppend on an immutable array? That can't be right... can it?

[nordlow]

I added this Phobos pull request because my DMD pull errrors as

https://d.puremagic.com/test-results/pull.ghtml?projectid=1&runid=931351

[schveiguy]

Unless in these use cases, we are calling assumeSafeAppend on an immutable array? That can't be right... can it?

Yes, you can call assumeSafeAppend on a tail-immutable array. The result is un safe, but legal, as long as you don't violate immutability. For example, if you have a string, you can append to the string. If you want to re-use the buffer, and know there are no references to the buffer you are removing, you can legally do it.

For example, this is legal:

string s = "hello";
s ~= " world";
writeln(s);
s.length = 5;
s.assumeSafeAppend();
s ~= " joe";

Simply because there is never any other reference to " world".

Note that assumeSafeAppend in this case, probably should not be marked as pure, since it will be misinterpreted by the compiler as not having an effect on the parameter. I'm not sure how one could fix this, there is no specific "weak pure" attribute. BTW, I wasn't aware that your pull made it pure, I thought the pull just made it nothrow. I think we should back out the pure attribute.

[andralex]

I think we should back out the pure attribute.

Agreed.

[monarchdodra]

Agreed.

I really don't see how it is any less pure than, say appending to an array. The fact that it is mis-interpreted as strongly pure rather than weakly pure, shouldn't mean we ditch its purity entirely. The medicine seems harsher than the illness here, IMO.

[nordlow]

What do you mean by backing out the pure attribute? Neither assumeSafeAppend nor _d_arrayshrinkfit are explicitly tagged pure.

[monarchdodra]

assumeSafeAppend is infered pure, because _d_arrayshrinkfit is explicitly forward declared as extern pure in object_.d/object.di

[nordlow]

Ok. Who removes the pure keyword from _d_arrayshrinkfit? Me?

Strange...I don't see any explicit pure in my sources...

object.di declares it as

private
{
    extern (C) void _d_arrayshrinkfit(TypeInfo ti, void[] arr);
}

and object_.d declares it as

private
{
    extern (C) void _d_arrayshrinkfit(const TypeInfo ti, void[] arr);
}

Have I missed something?

[nordlow]

Oops. I was using a too old copy of druntime. Sorry.

Shall someone revert this or do we need time to reflect more on this matter?

[monarchdodra]

or do we need time to reflect more on this matter?

assumeSafeAppend is functionally pure. If we can find a way to tell the compiler that it is weakly pure, then there is nothing to revert or reflect on.

For example, adding a hidden dummy pointer should fix it?

auto ref inout(T[]) assumeSafeAppend(T)(auto ref inout(T[]) arr, void* dummy = null);

This fixes the issue, and we preserve the weak purity.

[schveiguy]

@monarchdodra @nordlow I think we should back out the pure attribute for now. It was a very very recent change, so virtually no code depends on it. If we can re-introduce it in a way that makes it weak-pure for all types, then we can debate that.

Note that assumeSafeAppend uses a cast to call _d_arrayShrinkFit with a void *, so we must tread very carefully -- it is casting away immutability.

[nordlow]

Shall I remove the pure attribute and remove my cast(void)s on calls to assumeSafeAppend in my pull request?

[andralex]

@nordlow that would be great. Thanks!

[nordlow]

I guess that means that this pull request is not needed right?

I only need to fix druntime right?

[andralex]

Yah. This work shouldn't go to waste though. Please also submit a bugzilla issue so we figure how we go about the larger problem. Thanks!

[nordlow]

Done: https://github.com/D-Programming-Language/druntime/pull/747/files

[monarchdodra]

I guess that means that this pull request is not needed right?

Yeah, this pull request is definitely not needed. assumeSafeAppend is definitely not strongly pure, and anything that caters to that is wrong.

I only need to fix druntime right?

Yes, please remove the pure attribute. I'll pull you myself.

[quickfur]

Based on last comment, should this PR be closed?