You're right, with does amend the lexical environment (dynamically!) in ES3, so
in the _current_ compiler, where method bodies with free references get
with(this) and the inner fuction, not being a method does not get with(this),
things will work. My second example, where you return (what you might thinks
was) a method closure will not (because of the inner with(this), which will be
incorrectly bound.
But we _still_ want to eliminate the use of with(this), so let's not worry
about this case where it happens to work out. with(this) is banned in ES5
strict not just for security reasons, but because it is not analyzable (without
enormous overhead) so not optimizable by JIT engines. So we want to eliminate
our use of it.
I should also point out that Function.prototype.bind is part of ES5, so we
should only conditionally define it.
[And we need to start thinking about how we are going to take advantage of ES5:
new target platform that is conditionally loaded by browser sniffing? or just
trying to re-engineer our code to be neutral with respect to ES3 or ES5 by
adding runtime expedients that provide the ES5 features we need in ES3
platforms?]
On 2011-03-11, at 11:39, André Bargull wrote:
> 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
>>>
>>>
>>>
>>>
>>>
>>
>>
