5117 – [CTFE] Member function call with rather complex this: side effects ignored

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
--------------------

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; }
}
--------------------

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.

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 

(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];

That did the trick, thanks!
http://www.dsource.org/projects/dmd/changeset/747