Issue 13506 - std.array.array is not @safe in some cases
Summary: std.array.array is not @safe in some cases
Status: RESOLVED WORKSFORME
Alias: None
Product: D
Classification: Unclassified
Component: phobos (show other issues)
Version: D2
Hardware: x86 Windows
: P1 enhancement
Assignee: No Owner
URL:
Keywords: safe
Depends on:
Blocks:
 
Reported: 2014-09-20 12:54 UTC by bearophile_hugs
Modified: 2016-06-07 11:40 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description bearophile_hugs 2014-09-20 12:54:51 UTC
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 hsteoh 2014-09-21 04:00:44 UTC
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 hsteoh 2014-09-21 04:04:34 UTC
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 bearophile_hugs 2014-09-21 09:44:06 UTC
(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 bearophile_hugs 2014-09-21 09:46:34 UTC
(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 hsteoh 2014-09-21 22:55:27 UTC
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 bearophile_hugs 2014-09-21 23:26:59 UTC
(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?
Comment 7 Walter Bright 2016-06-07 11:40:29 UTC
This works with the current compiler.