369525 - this inside inner functions is not lexically scoped to parent's this pointer.

Status: RESOLVED WONTFIX

(Rhino Graveyard :: Core, defect)

Description

[Bob Jervis]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060926 Ubuntu/breezy-security Firefox/1.5.0.7
Build Identifier: Rhino 1_6R5

The value of 'this' is undefined for 'simple' function calls, as in foo().  'this' is well defined in cases such as:

object.method()
object[id]()
arbitraryValueExpression().method()

but is undefined for

func()

If func was a nested function definition inside a method that was invoked with a definite 'this' (due to the outer method being invoked by one of the well-defined mechanisms), the nested function will still not refer to the outer value of 'this'.

Reproducible: Always

Steps to Reproduce:
1. function outer() { function inner() { this.m = 3; } ; return inner; }
2. var b = new Object()
3. b.outer = outer
4. var m = 5
5. b.m = 4
6. var foo = b.outer()
7. foo()
Actual Results:  
The global m is 3, b.m is 4.

Expected Results:  
The b.m is 3, global m is 5.

I am preparing a patch for submission to this.  I have the fix working.

Comment 1

[Bob Jervis]

Attachment: Make this scoped to the caller's this.

Comment 2

[Bob Jervis]

Refer to bullet #3 on this slide from Brendan: http://developer.mozilla.org/es4/proposals/bug_fixes.html,

Comment 3

[Attila Szegedi]

Few remarks:

1. add yourself in the license boilerplate in the "Contributor(s)" section, alphabetically by your last name

2. this works in the shell (as launched by "java -jar js.jar" because it always uses interpreted mode. However, the bug needs to be fixed for bytecode-compiled functions as well (when scripts are compiled in a Context with optimizationLevel property set to 0 or 1), see o.m.javascript.optimizer.Codegen class, start from the compileToClassFile() method.

Comment 4

[Bob Jervis]

Attachment: Revised patch that includes code generation for jsc.

Comment 5

[Attila Szegedi]

Almost good - you must keep the NativeFunction.newObjectLiteral() method with
the old signature as well for backward compatibility. Developers can compile
scripts to .class files offline with "jsc" command line utility, and we strive
to preserve binary compatibility of the ScriptRuntime's public API, so scripts
compiled to .class files with earlier Rhino continue working with a later
version. Of course, you can have the old signature version delegate to new
signature version, and you can mark the old signature version deprecated.

Comment 6

[Bob Jervis]

Attachment: Cleaned up some bugs.

Turned out that I didn't fully understand compiled function initialization.  This fixes that so that the test suite does not change results (no tests are either broken or fixed by this).

Comment 7

[Attila Szegedi]

Bob,

unless I'm grossly misunderstanding something, you fixed a wrong problem. The fourth bullet at 

http://developer.mozilla.org/es4/proposals/bug_fixes.html

reads

"this in a nested function called by the function’s name from the outer function should bind to the outer this."

This should mean that when 'this' is referenced from an inner function that is *called* by the function name from the outer function should refer to the outer function's this.

In light of this, I have slightly modified your example. One modification is that the inner function is *called by name* from outer, not *returned* from it in step 1, and step 6 was modified accordingly.

1. function outer() { function inner() { this.m = 3; } ; inner(); }
2. var b = new Object()
3. b.outer = outer
4. var m = 5
5. b.m = 4
6. var foo = b.outer
7. foo()

Now, the expected result -- in my understanding -- would be exactly the opposite: foo() invoked at step 7 has the global scope as its this (it's not _undefined_, as you claim in your original bug submission), thus it is expected to modify global m. If invoked as b.outer() it should modify b's m. 

I'll admit that I can't state with authority what would be the correct behaviour of the example you specified originally be in its step 7, namely when the inner function is lifted out from the outer function and invoked in the global scope, although my opinion is that it'd need to assume the global scope as its "this" value for that particular invocation and thus act on the global scope, and not on the object bound to variable "b". That is, I believe the opposite should hold than you wrote.

Again (I'm really repeating myself here), my interpretation of the fourth point in that slide is that the inner function assumes outer function's 'this' value *only* when it is invoked by name from within the outer function's code. That's not what you did, AFAICT -- in your example, you don't invoke the inner function from the outer function, you just return it to the caller.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)

> Again (I'm really repeating myself here), my interpretation of the fourth point
> in that slide is that the inner function assumes outer function's 'this' value
> *only* when it is invoked by name from within the outer function's code. That's
> not what you did, AFAICT -- in your example, you don't invoke the inner
> function from the outer function, you just return it to the caller.

I sent private mail, but just for the record (that slide is ambiguous, isn't it?), the rule is a small generalization of what Attila wrote above:

When implementing ECMA-262 Edition 3 11.2.3 step 6 and 10.2.3 final bullet, instead of censoring Activation object references by replacing them as nominal |this| bindings with null, use the caller's |this|. So in addition to handling the case where the inner is called from the outer, you'll handle other calls to an inner function from within some other function (whose |this| will dynamically propagate).

/be