D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 16455 - Wrong code when calling a struct delegate
Summary: Wrong code when calling a struct delegate
Status: RESOLVED INVALID
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86_64 Windows
: P1 critical
Assignee: No Owner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-31 20:50 UTC by apham
Modified: 2016-09-01 18:54 UTC (History)
1 user (show)

See Also:


Attachments
Test case (1.80 KB, application/x-dsrc)
2016-08-31 20:50 UTC, apham
Details

Note You need to log in before you can comment on or make changes to this issue.
Description apham 2016-08-31 20:50:42 UTC
Created attachment 1612 [details]
Test case

module Test;

import std.stdio : writeln;

class Node(S)
{
    Node!S parent;
    Node!S first;
    Node!S next;
    S text;

    this()
    {
    }

    this(S aText)
    {
        text = aText;
    }

	NodeRange!S children()
	{
		return NodeRange!S(this);
	}
	
    Node!S push(Node!S aNode)
    {
		assert(aNode.next is null);
		
		aNode.parent = this;
        if (first is null)
            first = aNode;
        else
        {
            aNode.next = first;
            first = aNode;
        }
        return aNode;
    }
}

struct NodeRange(S)
{
private:
    Node!S parent;
    Node!S current;
    void delegate() doPopFront;

    void doMove()
    {
		writeln("doMove()");
        assert(current !is null);

        current = current.next;
    }

public:
    this(Node!S aParent)
    {
		writeln("aParent.text: ", aParent.text);
		
        parent = aParent;
        current = aParent.first;
        doPopFront = &doMove;
    }

    void popFront()
    {
	writeln("popFront()");
        assert(current !is null);

        // work if straight call
        //doMove(); 

        // access violation when calling delegate
		// or current is not set correctly
        doPopFront();  
    }
	
    @property 
	{
		bool empty()
    	{
        	return (current is null);
    	}

    	Node!S front()
    	{
        	return current;
    	}
	}
}

void main()
{
    Node!string tree = new Node!string();
    Node!string first = tree.push(new Node!string("first"));
	first.push(new Node!string("firstChild"));

    NodeRange!string r = tree.first.children();

    assert(!r.empty);
	writeln("r.front.text: ", r.front.text);
    assert(r.front.text == "firstChild");
    r.popFront();

	writeln("r.empty: ", r.empty);
    assert(r.empty);
}
Comment 1 ag0aep6g 2016-08-31 21:51:42 UTC
(In reply to apham from comment #0)
> struct NodeRange(S)
> {
[...]
>     void delegate() doPopFront;
> 
>     void doMove()
>     {
[...]
>     }
[...]
>     this(Node!S aParent)
>     {
[...]
>         doPopFront = &doMove;
>     }
[...]
> }

As far as I see, that code is invalid. &doMove refers to the current location of the struct. But structs can be copied and moved around (the compiler is even allowed to do it on its own). Whenever that happens, doPopFront is invalidated.

Closing as invalid. Please reopen if you disagree.
Comment 2 apham 2016-08-31 22:50:10 UTC
Based on this https://dlang.org/spec/function.html#closures and the code, the struct var is not moving anywhere and not out of scope, so it must work
Comment 3 apham 2016-09-01 02:52:20 UTC
need to move the setting "doPopFront = &doMove;" out of constructor to make it work. Can move it to empty() to complete the initialization
Comment 4 ag0aep6g 2016-09-01 18:54:57 UTC
(In reply to apham from comment #2)
> Based on this https://dlang.org/spec/function.html#closures and the code,
> the struct var is not moving anywhere and not out of scope, so it must work

The NodeRange struct may be moved during construction (constructed at one location, then moved to the target location). If this happens, it invalidates the internal pointer you set up in the constructor. Returning from `children` may mean another copy/move.

There are no hard rules here. The compiler has some leeway. From <https://dlang.org/spec/struct.html> (second paragraph):

> A struct is defined to not have an identity; that is, the implementation is free to make bit copies of the struct as convenient.

(In reply to apham from comment #3)
> need to move the setting "doPopFront = &doMove;" out of constructor to make
> it work. Can move it to empty() to complete the initialization

Note that any copy/move of the struct will still invalidate the pointer. And as far as I understand, the compiler may assume that the struct does not point to itself. So you may run into trouble, even when you have no explicit copy/move in your code.