http://d.puremagic.com/issues/show_bug.cgi?id=5117

           Summary: [CTFE] Member function call with chained dots: side
                    effects ignored
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Keywords: wrong-code
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: rsinfu@gmail.com


--- Comment #0 from Shin Fujishiro <rsinfu@gmail.com> 2010-10-25 09:09:49 PDT ---
The interpretor neglects member functions mutating 'this' if a function call involves two or more chained dot expressions.  In the following repro, s.change() succeeds in mutating 'this' but r.s.change() does not:
--------------------
static int dummy = test();

int test()
{
    S s;
    s.change();
    assert(s.value == 1);       // (7) succeeds

    R r;
    r.s.change();
    assert(r.s.value == 1);     // (11) fails, value == 0

    return 0;
}

struct S
{
    int value;
    void change() { value = 1; }
}

struct R
{
    S s;
}
--------------------
% dmd -o- -c test.d
test.d(11): Error: assert(r.s.value == 1) failed
test.d(1): Error: cannot evaluate test() at compile time
test.d(1): Error: cannot evaluate test() at compile time
--------------------

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5117


Shin Fujishiro <rsinfu@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[CTFE] Member function call |[CTFE] Member function call
                   |with chained dots: side     |with rather complex this:
                   |effects ignored             |side effects ignored


--- Comment #1 from Shin Fujishiro <rsinfu@gmail.com> 2010-10-25 14:14:06 PDT ---
The problem lies in FuncDeclralation::interpret(), around line 223:
--------------------
    // Don't restore the value of 'this' upon function return
    if (needThis() && thisarg->op == TOKvar && istate)
    {
        VarDeclaration *thisvar = ((VarExp
*)(thisarg))->var->isVarDeclaration();
        for (size_t i = 0; i < istate->vars.dim; i++)
        {   VarDeclaration *v = (VarDeclaration *)istate->vars.data[i];
            if (v == thisvar)
            {   istate->vars.data[i] = NULL;
                break;
            }
        }
    }
--------------------

In the repro code in comment #1, thisarg is 'r.s' (TOKdotvar) and the local variable 'r' is not removed from the "restore list" istate->vars.  Then, the interpretor wrongly restores 'r' to init.

Just dealing with TOKdotvar fixes the specific reported problem, but it's not a general fix.  Still the interpretor should deal with references.  For example:
--------------------
enum dummy = test();

int test()
{
    S s;
    getRef(s).change();
    assert(s.value == 1);     // fails, value == 0
    return 0;
}
ref S getRef(ref S s) { return s; }

struct S
{
    int value;
    void change() { value = 1; }
}
--------------------

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5117


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
                 CC|                            |clugdbug@yahoo.com.au
           Severity|normal                      |critical


--- Comment #2 from Don <clugdbug@yahoo.com.au> 2010-10-29 01:49:57 PDT ---
PATCH:
interpret.c, line 224, FuncDeclaration::interpret().

    // Don't restore the value of 'this' upon function return
-    if (needThis() && thisarg->op == TOKvar && istate)
+    if (needThis() && istate)
    {
-        VarDeclaration *thisvar = ((VarExp
*)(thisarg))->var->isVarDeclaration();
+        VarDeclaration *thisvar = findParentVar(thisarg, istate->localThis);
        for (size_t i = 0; i < istate->vars.dim; i++)
        {   VarDeclaration *v = (VarDeclaration *)istate->vars.data[i];
            if (v == thisvar)
            {   istate->vars.data[i] = NULL;
                break;
            }

and also add
VarDeclaration * findParentVar(Expression *e, Expression *thisval);
to the top of the file.

This fixes both test cases.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5117


Walter Bright <bugzilla@digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla@digitalmars.com


--- Comment #3 from Walter Bright <bugzilla@digitalmars.com> 2010-11-07 17:13:14 PST ---
I applied the patch, and the first test case now works but the second still fails:

test.d(7): Error: assert(s.value == 1) failed
test.d(1): Error: cannot evaluate test() at compile time
test.d(1): Error: cannot evaluate test() at compile time

Perhaps your sources differ in other ways?

http://www.dsource.org/projects/dmd/changeset/742

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5117



--- Comment #4 from Don <clugdbug@yahoo.com.au> 2010-11-08 08:31:56 PST ---
(In reply to comment #3)
> I applied the patch, and the first test case now works but the second still fails:
> 
> test.d(7): Error: assert(s.value == 1) failed
> test.d(1): Error: cannot evaluate test() at compile time
> test.d(1): Error: cannot evaluate test() at compile time
> 
> Perhaps your sources differ in other ways?
> 
> http://www.dsource.org/projects/dmd/changeset/742

Not sure what's happened here. Maybe I got the test case wrong. Regardless, this change (line 228) fixes it.


    // Don't restore the value of 'this' upon function return
    if (needThis() && istate)
    {
        VarDeclaration *thisvar = findParentVar(thisarg, istate->localThis);
+        if (!thisvar) // it's a reference. Find which variable it refers to.
+            thisvar = findParentVar(thisarg->interpret(istate),
istate->localThis);
        for (size_t i = 0; i < istate->vars.dim; i++)
        {   VarDeclaration *v = (VarDeclaration *)istate->vars.data[i];

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5117


Walter Bright <bugzilla@digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


--- Comment #5 from Walter Bright <bugzilla@digitalmars.com> 2010-11-10 13:38:48 PST ---
That did the trick, thanks!

http://www.dsource.org/projects/dmd/changeset/747

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------