Bug 15839 – [REG2.071-b1] this.outer is of wrong type
Status
RESOLVED
Resolution
FIXED
Severity
regression
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2016-03-27T10:30:00Z
Last change time
2016-10-01T11:46:52Z
Keywords
pull
Assigned to
nobody
Creator
doob
Comments
Comment #0 by doob — 2016-03-27T10:30:18Z
Compiling the code below will result in the following error:
main.d(15): Error: constructor main.GC.this (AnimatedProgress ap) is not callable using argument types (void*)
interface Runnable {}
class GC
{
this(AnimatedProgress ap) {}
}
class AnimatedProgress
{
void start()
{
auto r = new class Runnable {
void run()
{
GC gc = new GC(this.outer);
}
};
}
}
Comment #1 by k.hara.pg — 2016-03-27T12:24:25Z
This is an intentional bugfix introduced by fixing issue 14442.
Note that there was accepts-invalid bug. The spec page http://dlang.org/spec/class said:
> The property .outer used in a nested class gives the this pointer to its enclosing class. If the enclosing context is not a class, the .outer will give the pointer to it as a void* type.
It means `this.outer` in `run()` member function should represent the frame of 'start' function. Actually the generated code is following the spec, but front-end had incorrectly typed 'this.outer' like an AnimatedProgress class.
Comment #2 by jack — 2016-03-27T15:17:13Z
If this is the result of a breaking change, then it should be in the changelog.
Comment #3 by doob — 2016-03-27T20:33:56Z
(In reply to Kenji Hara from comment #1)
> > The property .outer used in a nested class gives the this pointer to its enclosing class. If the enclosing context is not a class, the .outer will give the pointer to it as a void* type.
>
> It means `this.outer` in `run()` member function should represent the frame
> of 'start' function. Actually the generated code is following the spec, but
> front-end had incorrectly typed 'this.outer' like an AnimatedProgress class.
So it's not representing the this reference of the outer class anymore? Is it possible to get that behavior somehow?
This is a huge regression for DWT.
Comment #4 by k.hara.pg — 2016-03-28T14:06:58Z
(In reply to Jacob Carlborg from comment #3)
> So it's not representing the this reference of the outer class anymore?
Until 2.070, the builtin .outer property sometimes returned correct class reference, sometimes invalid one. Test following code with 2.070 or earlier.
import core.stdc.stdio : printf;
interface Runnable {}
class GC
{
this(AnimatedProgress ap)
{
printf("GC.ctor, ap = %p\n", ap);
}
}
class AnimatedProgress
{
void start()
{
printf("start, this = %p\n", this);
version(ng) int a;
auto r = new class Runnable {
void run()
{
printf("run, this.outer = %p\n", this.outer);
GC gc = new GC(this.outer);
version(ng) int b = a;
}
};
r.run();
}
}
void main()
{
auto ap = new AnimatedProgress();
printf("main, ap = %p\n", ap);
ap.start();
}
Without -version=ng, the class reference (instance address) are same, so the code would work as the author's expected. But with -version=ng, the start() member function will become a closure, and this.outer will suddenly return a pointer to the closure environment - of course it's invalid as a class reference.
Note that, compiler cannot determine whether the start() makes a closure environment or not at the place where 'this.outer' is used. Because of that, the .outer property should have void* type, to be consistent with the lexical scope nesting.
> Is
> it possible to get that behavior somehow?
>
> This is a huge regression for DWT.
Theoretically the chain of 'outer' can reach to AnimatedProgress class instance.
Actually in my example code, following code can work in run() function with -version=ng case.
version(ng) printf("run, *cast(void**)this.outer = %p\n", *cast(void**)this.outer);
//GC gc = new GC(this.outer);
GC gc = new GC(cast(AnimatedProgress)*cast(void**)this.outer);
However... of course it's not stable. And as far as I know, there's no way to know the number of chains up to reach wanting enclosing scope.
Comment #5 by doob — 2016-03-28T19:05:32Z
(In reply to Kenji Hara from comment #4)
> However... of course it's not stable. And as far as I know, there's no way
> to know the number of chains up to reach wanting enclosing scope.
So the best approach would be to pass in the outer reference to the constructor when creating the anonymous class and use that instead of "this.outer"?
Comment #6 by code — 2016-03-29T22:33:31Z
(In reply to Kenji Hara from comment #4)
> Note that, compiler cannot determine whether the start() makes a closure
> environment or not at the place where 'this.outer' is used. Because of that,
> the .outer property should have void* type, to be consistent with the
> lexical scope nesting.
But this.outer always refers to the enclosing function even if no closure is required, right? A this.outer that is typed as void* and either refers to an enclosing function or an enclosing class would be highly confusing.
Comment #7 by doob — 2016-03-31T08:28:56Z
I tried modifying my code to explicitly pass in the outer this reference. That works until you have nested anonymous classes. For example:
interface A {}
interface B {}
class Foo
{
void foo()
{
auto a = new class(this) A {
Foo outer;
this(Foo outer)
{
this.outer = outer;
this.outer.bar();
auto b = new class(this) B {
A outer;
this(A outer)
{
this.outer = outer;
this.outer.outer.bar(); // line 20
}
};
}
};
}
void bar() {}
}
The above will of course fail to compile since there's no "bar" in "A":
main.d(20): Error: no property 'outer' for type 'main.A'
Any suggestions?
(In reply to Kenji Hara from comment #8)
> https://github.com/D-Programming-Language/dmd/pull/5613
>
> If a nested class has a parent class over some nested functions, '.outer'
> will properly point instance of the parent class.
So, this pull request will fix the issues I'm having?
Comment #10 by github-bugzilla — 2016-04-01T10:13:42Z
Comment #12 by github-bugzilla — 2016-05-09T01:53:48Z
Commits pushed to master at https://github.com/dlang/dmdhttps://github.com/dlang/dmd/commit/a117c87a8a9ffbccc78b2c28ae3643b5e5b02794
Fix invalid AST produced by issue 15839 fix
The fix for 15839 (accessing this.outer from a member function
inside a nested class) in bb5f550edd7 produces a ThisExp that
refers to the 'this' VarDecl in an outer scope, leading to the
latter being referenced from the nested scope without that being
added to its nestedRefs. Subsequently, the VarDecl is also not
present in the outer FuncDecls closureVars.
This is a weird construct, and outside the AST invariants that
would previously hold. This commit properly at least properly
registers the nested reference, but using a ThisExp in this
manner might not be the cleanest way in the first place.
https://github.com/dlang/dmd/commit/d178c8578e615c3e68702905b72c3e89bccbecd6
Merge pull request #5741 from klickverbot/fixup-15839-ast
Fix invalid AST produced by issue 15839 fix
Comment #13 by github-bugzilla — 2016-05-18T08:56:05Z