Bug 13506 – std.array.array is not @safe in some cases
Status
RESOLVED
Resolution
WORKSFORME
Severity
enhancement
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
x86
OS
Windows
Creation time
2014-09-20T12:54:00Z
Last change time
2016-06-07T11:40:29Z
Keywords
safe
Assigned to
nobody
Creator
bearophile_hugs
Comments
Comment #0 by bearophile_hugs — 2014-09-20T12:54:51Z
void main() @safe {
import std.algorithm: cartesianProduct;
import std.array: array;
const r = cartesianProduct([1], [1]).array;
}
dmd 2.067alpha gives:
test.d(4,41): Error: safe function 'D main' cannot call system function 'std.array.array!(Result).array'
Comment #1 by hsteoh — 2014-09-21T04:00:44Z
This bug appears unrelated to cartesianProduct; it's ultimately caused by a particular overload of Phobos internal function std.conv.emplaceImpl. It just so happens that other similar range-based calls to std.array.array end up in other, @safe instantiation paths, so the problem isn't (yet) exposed elsewhere (though you can probably find the same problem showing up elsewhere if you knew where to look).
The following Phobos patch fixes this problem, but I'm not ready to submit that as a PR yet because I'm not 100% confident that it's the correct fix (sticking @trusted on that call may be a bit too blunt of an instrument; probably some other static checks should be added to make sure that it is in fact @trust-worthy).
------snip------
diff --git a/std/array.d b/std/array.d
index b9012e4..8ad8cc3 100644
--- a/std/array.d
+++ b/std/array.d
@@ -2567,7 +2567,9 @@ if (isDynamicArray!A)
else
auto ref uitem() @trusted nothrow @property { return cast(Unqual!T)item; }
- emplaceRef!(Unqual!T)(bigData[len], uitem);
+ () @trusted {
+ emplaceRef!(Unqual!T)(bigData[len], uitem);
+ }();
//We do this at the end, in case of exceptions
_data.arr = bigData;
------snip------
Comment #2 by hsteoh — 2014-09-21T04:04:34Z
Oh, and just to be clear, this particular instantiation of emplaceRef is un-@safe because it eventually takes the address of an item and calls memcpy on it. It's not caused by something in cartesianProduct range being non-@safe.
Comment #3 by bearophile_hugs — 2014-09-21T09:44:06Z
(In reply to hsteoh from comment #1)
> This bug appears unrelated to cartesianProduct; it's ultimately caused by a
> particular overload of Phobos internal function std.conv.emplaceImpl.
Thank you. I change the issue title.
Comment #4 by bearophile_hugs — 2014-09-21T09:46:34Z
(In reply to hsteoh from comment #1)
> The following Phobos patch fixes this problem, but I'm not ready to submit
> that as a PR yet because I'm not 100% confident that it's the correct fix
Currently creating a pull request seems the best way to receive comments and to have a chance to fix your idea. Here in Bugzilla it's less likely to receive comments on the code.
Comment #5 by hsteoh — 2014-09-21T22:55:27Z
Actually, it looks like it's probably the *wrong* fix, because if appender is used with a type with overloaded assignment operator, and the assign operator was un-@safe, then this patch will cause unsafe code to be called from @trusted code, so it will break @safety in a subtle but fairly horrible way.
Comment #6 by bearophile_hugs — 2014-09-21T23:26:59Z
(In reply to hsteoh from comment #5)
> Actually, it looks like it's probably the *wrong* fix, because if appender
> is used with a type with overloaded assignment operator, and the assign
> operator was un-@safe, then this patch will cause unsafe code to be called
> from @trusted code, so it will break @safety in a subtle but fairly horrible
> way.
Yours was a first try, don't worry. Can you verify at compile time the safety of the assignment, and use a safe or a not safe nested lambda accordingly with a static if?