I'm a bit confused right now, the following test case works the same in
dhtml and swf10 although it includes function closures.
foo.m()() returns `42`
foo2.m()() returns `42`
foo3.m()() returns `42`
(compiled with OL4.9 to dhtml, tested in IE,FF,Opera,Safari on Win7)
<canvas debug="true">
<node id="foo">
<attribute name="attr" value="42" />
<method name="m" >
return function inner () { return attr }
</method>
</node>
<class name="lzxFoo" >
<attribute name="attr" value="42" />
<method name="m" >
return function inner () { return attr }
</method>
</class>
<lzxFoo id="foo2" />
<script when="immediate"><![CDATA[
public class lzsFoo {
public var attr = 42;
public function m () {
return function inner () { return attr }
}
}
var foo3 = new lzsFoo();
]]></script>
</canvas>
In plain Javascript:
({
attr: 42,
m: function () {
with (this) { return function () { return attr } }
}
}).m()() returns `42` as well
On 3/11/2011 5:28 PM, P T Withington wrote:
Indeed.
All functions get an implicit `this` operand, which if you call the function
directly, will be bound to either null or the global object (the latter
'feature' is a security hole that will eventually go away).
It is unfortunate that ECMAScript does _not_ specify that a function closure
should close over `this` for these cases, which is why you are seeing the issue
you are seeing.
This issue only occasionally an issue for OL developers. Once the lesson is
learned most are able to understand and program around it. But it would be
very nice if we could handle this in the automatic way that swf does.
If you look at: LzRuntime#148, you will see my proposed solution to the
problem. With that definition in place, what we would do upon noticing that an
inner function was closing over an instance variable is re-write the code to
(using your test case);
<method name="closureOverUnshadowedValue2" args="">
return (function inner () {
return this.attrValue;
}).bind(this)
</method>
[This is essentially what swf is doing.]
André objected to polluting Function.prototype, which is the reason the
definition of bind is currently commented out. But I don't think this is a
strong objection. Perhaps it is time to uncomment it (and use it to solve at
least this problem).
If you don't think you can easily amend your change to address this, I think it
would be fine to file this as an improvement (since it is _not_ a regression
from the current state of affairs).
Here is another test case that will not work with our current level of analysis:
<method name="methodClosure">
return attrValue;
</method>
<method name="broken">
return this.methodClosure
</method>
Calling the return value of broken will not get you the closed-over value
attrValue. Solving this will be needed to eventually replace the use of
delegates with closures. Again, the re-write would take advantage of
Function/bind:
<method name="broken">
return this.methodClosure.bind(this);
</method>
This would be another improvement we should file, unless you feel it is easily
achievable with the information now available in the current change.
[One more comment in-line below.]
On 2011-03-11, at 10:55, Donald Anderson wrote:
Tucker,
I've written some test cases and found some that I can't pass - indeed, the
current version cannot pass it either. Here's a representative one:
<attribute name="attrValue" value="ATTRIBUTE VALUE" type="string" />
<!-- no shadowing, just grab the attrValue -->
<method name="closureOverUnshadowedValue2" args="">
return function inner () {
return attrValue;
}
</method>
The method should return "ATTRIBUTE VALUE", and it does in SWF10. But in the
current version of DHTML, it returns undefined, even though it's wrapped by with(this).
And my changes don't fix this either. FWIW, explicit setting of this. does not do the
right thing:
<method name="closureOverUnshadowedValue2" args="">
return function inner () {
return this.attrValue;
}
</method>
That returns undefined as well. But here's an approach that does work:
<method name="closureOverUnshadowedValue2" args="">
var _this = this;
return function inner () {
return _this.attrValue;
}
</method>
(Would wrapping the entire function in 'var _this = this; with (_this) {....}'
work? I don't know yet)
I do not believe that will work reliably because function closures are
_lexical_ (they should only close over lexically apparent bindings), and `with`
only amends they dynamic environment. It may accidentally work in some VM's
that (incorrectly) implement closures by snapshotting the dynamic environment
(e.g., swf5 did this), but it will not work in any modern VM, especially the
JIT-compiled VM's, which work hard to eliminate closures any time they can
prove they are not needed.
I'm inclined to defer this with another JIRA so we can push through the major
improvement, and we aren't breaking anything that isn't already broken.
- Don
--
Don Anderson
Java/C/C++, Berkeley DB, systems consultant
voice: 617-306-2057
email: [email protected]
www: http://www.ddanderson.com
blog: http://libdb.wordpress.com
