std.container.slist: Fix a @safe cannot call @system issue

[carblue]

make -j6 -f posix.mak unittest
results in:
...

std/container/slist.d(280): Error: @safe function std.container.slist.SList!int.SList.__unittest_L278_C11 cannot call @system constructor std.container.slist.SList!int.SList.__ctor!int.this
std/container/slist.d(282): Error: @safe function std.container.slist.SList!int.SList.__unittest_L278_C11 cannot call @system constructor std.container.slist.SList!int.SList.__ctor!int.this
std/algorithm/mutation.d(171): Error: template instance `std.container.slist.SList!int` error instantiating

sources used (on Linux x86_64):
dmd : Latest commit 17bc56c
druntime: Latest commit f2711a3
phobos : Latest commit 6947d80

[dlang-bot]

Thanks for your pull request and interest in making D better, @carblue! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[carblue]

@wilzbach
Thanks for reviewing: Please don't merge this !
I'm about to update this (and it will be -dip1000 compilable then) by:

user@host:~/git/phobos$ git diff std/container/slist.d
diff --git a/std/container/slist.d b/std/container/slist.d
index 04c4d73..4fc2feb 100644
--- a/std/container/slist.d
+++ b/std/container/slist.d
@@ -140,7 +140,8 @@ Constructor taking a number of nodes
      */
     this(U)(U[] values...) if (isImplicitlyConvertible!(U, T))
     {
-        insertFront(values);
+        U[] new_values = values.dup;
+        insertFront(new_values);
     }
 
 /**
@@ -354,7 +355,7 @@ Returns: The number of elements inserted
 
 Complexity: $(BIGOH m), where $(D m) is the length of $(D stuff)
      */
-    size_t insertFront(Stuff)(scope Stuff stuff)
+    size_t insertFront(Stuff)(Stuff stuff)
     if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, T))
     {
         initialize();

The decoupling is required, though ugly dup-ing, otherwise I'll run into this:

std/container/slist.d(366): Error: scope variable item may not be returned
std/container/slist.d(155): Error: template instance `std.container.slist.SList!string.SList.insertFront!(Range)` error instantiating
std/container/slist.d(251):        instantiated from here: __ctor!(Range)
std/container/slist.d(802):        instantiated from here: SList!string

[n8sh]

This is painful and it's inane that we have to do this. Would manually inlining the insertFront loop remove the need to copy the array for DIP1000?

[carblue]

What exactly is meant by manually inlining the insertFront loop?
I tried foreach lowered to for (auto item=stuff.front...
That didn't change the error: Error: scope variable item may not be returned

[JackStouffer]

Removing trivial label due to discussion.

[carblue]

Thanks for reviewing. For some SList(T). struct Node. T _payload;
the copying is avoided now, for the remaining, there is the error
std/container/slist.d(366): Error: scope variable item may not be returned
which is a real issue, though the error message is somewhat misleading: Expired stack data may end up in auto newNode = new Node(null, item);
Thus I currently think, copying can't be avoided in general !?

[JackStouffer]

Ok this same thing came up in #6281 and I'm at the point where I believe this should be considered a fatal flaw of DIP1000.

How is anyone supposed to implement DIP1000 if any time they need to assign a scope variable to a result struct/class they need to copy it?

Am I missing an obvious solution here?

[Geod24]

Welcome to the club. Yes DIP1000 is about as useful as head const. Or probably worse.

[n8sh]

@carblue Thanks for your work on this.

What exactly is meant by manually inlining the insertFront loop?

Make this change only:

diff --git a/std/container/slist.d b/std/container/slist.d
index 8c2f2fc6d..9c3bc0e7b 100644
--- a/std/container/slist.d
+++ b/std/container/slist.d
@@ -140,7 +140,15 @@ Constructor taking a number of nodes
      */
     this(U)(U[] values...) if (isImplicitlyConvertible!(U, T))
     {
-        insertFront(values);
+        initialize();
+        Node * n, newRoot;
+        foreach (item; values)
+        {
+            auto newNode = new Node(null, item);
+            (newRoot ? n._next : newRoot) = newNode;
+            n = newNode;
+        }
+        _first = newRoot;
     }
 
 /**

On my machine this removes all DIP1000 problems from slist.d without making a copy of the values array. (Separately you'll need to suppress the complaints regarding RBTree.)

[carblue]

Credit and thanks to @n8sh ! My blockhead: What else than what You're describing is 'manually inlining'! Great idea.
@JackStouffer Sorry for not being responsive so far, I just had a twist in my mind about yes/no copy requirement, where in this case the data are from unittest (temporary/volatile/non-escaping), thus in principle it should be possible to avoid copying (if the compiler is smart enough), and that's what seems to be true here as long as the data are "protected" by scope (lifetime control).

[wilzbach]

Maybe use version(DIP1000) as proposed in #6281?
version(DIP1000) isn't defined yet, but

• it could be once all Phobos code is passing
• it allows easy grepping for such tests
• in theory we could already add it to the individual .test target by doing a replacement like s/-dip1000/-dip1000 -version(DIP1000)

[wilzbach]

@carblue now that we have https://github.com/dlang/phobos/blob/master/dip1000.mak, could you set the respective modules to -dip1000? Thanks!

[wilzbach]

DStyle: Node*

[carblue]

@wilzbach Yep, I'll update dip1000.mak when something got merged and thus ready for -dip1000; the only one that magically got -dip1000 again in the meantime (without depends on) is:
aa[std.container.dlist]=-dip1000
I'll push that

Next to come: (?)
aa[std.container.slist]=-dip1000 -version=DIP1000

and std.exception will be an easy fix with a workaround acc. to https://issues.dlang.org/show_bug.cgi?id=18637

[wilzbach]

@carblue Oh sorry. I was thinking about updating dip1000.mak directly in this PR, s.t. it's easy for everyone to see that this does fix the DIP1000 problems + it's immediately locked to -dip1000, so there's no time for regressions to creep in.

[carblue]

@wilzbach My fault. That's better, combining them. It looks like I get blockheads when looking at a screen for more than 12h.

[wilzbach]

I also added -dip1000 to std.algorithm.mutation as it was depending on std.container.slist

[WalterBright]

Since this is congruent with the D vision, I added the Vision label and this is a priority issue. https://wiki.dlang.org/Vision/2018H1

[wilzbach]

FYI: there's an easy way to set this label to all -dip1000 PRs:

(screenshot of https://github.com/dlang/phobos/pulls?q=is%3Aopen+is%3Apr+project%3Adlang%2Fphobos%2F6)

[wilzbach]

I feel like we should dive into why dip1000 requires the duplication first as this might severely hurt the usability and adoption of dip1000.
For this I extracted a slist.d to a reduced file for local experimentation:

struct SList(T)
{
    import std.range.primitives : isInputRange, isForwardRange, ElementType;
    import std.traits : isImplicitlyConvertible;

    private struct Node
    {
        Node * _next;
        T _payload;
    }
    private struct NodeWithoutPayload
    {
        Node* _next;
    }
    static assert(NodeWithoutPayload._next.offsetof == Node._next.offsetof);

    private Node * _root;

    private void initialize() @trusted nothrow pure
    {
        if (_root) return;
        _root = cast (Node*) new NodeWithoutPayload();
    }

    private ref inout(Node*) _first() @property @safe nothrow pure inout
    {
        assert(_root);
        return _root._next;
    }

    this(U)(U[] values...) if (isImplicitlyConvertible!(U, T))
    {
        insertFront(values);
    }

    this(Stuff)(Stuff stuff)
    if (isInputRange!Stuff
            && isImplicitlyConvertible!(ElementType!Stuff, T)
            && !is(Stuff == T[]))
    {
        insertFront(stuff);
    }

    @property bool empty() const
    {
        return _root is null || _first is null;
    }

    @property ref T front()
    {
        assert(!empty, "SList.front: List is empty");
        return _first._payload;
    }

    @safe unittest
    {
        auto s = SList!int(1, 2, 3);
        s.front = 42;
        assert(s == SList!int(42, 2, 3));
    }

    size_t insertFront(Stuff)(Stuff stuff)
    if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, T))
    {
        initialize();
        size_t result;
        Node * n, newRoot;
        foreach (item; stuff)
        {
            auto newNode = new Node(null, item);
            (newRoot ? n._next : newRoot) = newNode;
            n = newNode;
            ++result;
        }
        if (!n) return 0;
        // Last node points to the old root
        n._next = _first;
        _first = newRoot;
        return result;
    }

    /// ditto
    size_t insertFront(Stuff)(Stuff stuff)
    if (isImplicitlyConvertible!(Stuff, T))
    {
        initialize();
        auto newRoot = new Node(_first, stuff);
        _first = newRoot;
        return 1;
    }
}

///
@safe unittest
{
    auto s = SList!int(1, 2, 3);
}

Interesting

• this only fails with ~master, it works with 2.079.0: https://run.dlang.io/is/unzxRA
  Note that the nightlies are currently broken.
• It works if I add scope to insertFront to the reduced examples, but not to the full std.container.slist, which suggests that something weird is going on with the "scope variable item may not be copied into allocated memory" error

[wilzbach]

Here's a dustmited version of the error:

struct SList(T)
{
    struct Node
    {
        Node* _next;
        T _payload;
    }

    struct Range
    {
        Node _head;

        @property front()
        {
            return _head._payload;
        }
        enum empty = true;
        void popFront(){}
    }

    this(Stuff)(Stuff stuff)
    {
        insertFront(stuff);
    }

    @property dup()
    {
        SList(this[]);
    }

    Range opSlice()
    {
        return Range();
    }

    size_t insertFront(Stuff)(scope Stuff stuff)
    {
        foreach (item; stuff)
            new Node(null, item);

        return 0;
    }
}

void main()
{
    SList!string s;
}

[wilzbach]

edit: sorry, it's not not that simple.

[wilzbach]

Reduced it even further:

struct SList(T)
{
    struct Range
    {
        T payload;
    }

    void insertFront(Stuff)(scope Stuff stuff)
    {
        new Range(stuff.payload);
    }
}

void main()
{
    SList!string s;
    typeof(s).Range r;
    s.insertFront(r);
}

> dmddev -c -dip1000 foo.d
foo.d(10): Error: scope variable stuff may not be copied into allocated memory
foo.d(18): Error: template instance `foo.SList!string.SList.insertFront!(Range)` error instantiating

[JackStouffer]

Odd, I get a completely different error with both 2.079 and nightly

onlineapp.d(8): Error: parameter stuff is return but function does not return any indirections
onlineapp.d(18): Error: template instance `onlineapp.SList!string.SList.insertFront!(Range)` error instantiating

[wilzbach]

Odd, I get a completely different error with both 2.079 and nightly

Yeah, I get the same error for 2.079, but note that the nightlies are currently broken.
That's the last log:
http://nightlies.dlang.org/dmd-master-2018-03-02/build.html (looks like Martin's GPG key has expired)
You only get the "may not be copied into allocated memory" from master. It was introduced in dlang/dmd#8054

[WalterBright]

something weird is going on with the "scope variable item may not be copied into allocated memory" error

In the example you gave, it is indeed copying a scope variable into allocated memory:

void insertFront(Stuff)(scope Stuff stuff)
{
    new Range(stuff.payload);
}

Scope means the pointer does not escape the scope. Copying it into allocated memory is escaping it.

[JackStouffer]

@WalterBright This PR is not a viable solution, as all it does is copy and paste the function contents of insertFront into the ctor. We cannot allow DIP1000 adoption to degrade code quality in Phobos.

I'm going to revert this. @carblue Please feel free to open a new PR addressing this issue.

[JackStouffer]

I would note that @wilzbach's approval was on an earlier version of this PR, and not this code.

[carblue]

Yes, the problem of DIP1000 seemingly requiring code duplication (to get slist.d to compile with current master) still isn't solved. I'm about to open a bugzilla issue for that, using @wilzbach 's analysis.
I would prefer that to be solved first, then further fixing this PR to not require code duplication, and only then continue with rbtree.d.