Bug 11240 – assumeSafeAppend could implicitly break immutablity

I think assumeSafeAppend should reject array references which has non-mutable element type. Test case: import std.stdio; void main() { immutable(int[]) arr = [1,2,3]; immutable(int)[] a = arr[0..2]; writeln("a.capacity = ", a.capacity); // == 0 a = assumeSafeAppend(a[]); writeln("a.capacity = ", a.capacity); // != 0 a ~= 100; writeln(a); // [1, 2, 100] writeln(arr); // [1, 2, 100] <-- !! }

Is this valid though? assumeSafeAppend is an unsafe function that *requires* no one else have a view on the items after the end of the array. Just the same, you will overwrite the old items, without destroying them, nor assigning over them. I think this is just an unsafe function that was used wrong. The result is simply undefined behavior. I think it would be a needless restriction to not allow assumeSafeAppend on immutable (and const). This seems invalid to me.

(In reply to comment #1) > Is this valid though? > > assumeSafeAppend is an unsafe function that *requires* no one else have a view > on the items after the end of the array. > > Just the same, you will overwrite the old items, without destroying them, nor > assigning over them. > > I think this is just an unsafe function that was used wrong. The result is > simply undefined behavior. > > I think it would be a needless restriction to not allow assumeSafeAppend on > immutable (and const). > > This seems invalid to me. Because the unsafe-ness is hidden in assumeSafeAppend function template. If you pass immubtale(int)[] array reference to it in generic code, it could easily break type-system silently (And yes, I didn't noticed the risk until now). As the API design, it would be better to reject such a misuse. In other words, if you really wants to charge the capacity of immutable(int)[], enforcing explicit cast on caller side would be better. immutable(int)[] a = ...; //a = assubeSafeAppend(a); // compile error a = cast(typeof(a))aassubeSafeAppend(cast(int[])a); // ugly, but explicit

I concur with monarchdodra. I think that this would be too strong a restriction. assumeSafeAppend is already @system. You're already not supposed to be calling it on an array that isn't actually safe to append to. The constness of the array doesn't change that. Maybe more explicit warnings should be put in the documentation for assumeSafeAppend, but the documentation is already pretty clear that you shouldn't use it if it's not actually safe to append. I think that if assumeSafeAppend is misused, it's clearly a bug on the part of the caller and not assumeSafeAppend, even if that bug involves const or immutable. And given that the most likely type to use with assumeSafeAppend is probably string, restricting it to only work with mutable elements would be really reduce its usefulness.

I think that making it so that assumeSafeAppend didn't work with const or immutable would be akin to making free not work with const or immutable. Both function are inherently unsafe and must be used correctly and could cause serious issues if misused, and both are related to indicating which memory can be used. But both are also just fine if used correctly. The key thing is in making sure that it's clear in the documentation how the function is intented to be used and that using it differently is unsafe.

To that effect, the `@system` mark should probably be made explicit.

...and it's in: https://github.com/dlang/druntime/pull/2373 I'm going to close this until such time that a compelling argument arises. It's been rotting long enough.