D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 5117 - [CTFE] Member function call with rather complex this: side effects ignored
Summary: [CTFE] Member function call with rather complex this: side effects ignored
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P2 critical
Assignee: No Owner
URL:
Keywords: patch, wrong-code
Depends on:
Blocks:
 
Reported: 2010-10-25 09:09 UTC by Shin Fujishiro
Modified: 2010-11-10 13:38 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 Shin Fujishiro 2010-10-25 09:09:49 UTC
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
--------------------
Comment 1 Shin Fujishiro 2010-10-25 14:14:06 UTC
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; }
}
--------------------
Comment 2 Don 2010-10-29 01:49:57 UTC

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.
Comment 3 Walter Bright 2010-11-07 17:13:14 UTC
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
Comment 4 Don 2010-11-08 08:31:56 UTC
(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];
Comment 5 Walter Bright 2010-11-10 13:38:48 UTC
That did the trick, thanks!

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