181834 - wrong scope used for inner functions when compiling functions with dynamic scopes (interpreted only)

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Felix Meschberger]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021016

i fear, i found a scoping problem with rhino related to the 'compile functions
with dynamic scope' feature. given the code :

	function outer( /* String */ arg ) {
        var outer_d = 0;
	  function inner( /* int */ level ) {
          outer_d++;
          if (level > 0) {
            inner( level - 1 );
          } else {
            return outer_d;
          }
        }
        return inner(5);
      }
      var result = outer("arg");

should set the result variable to 6, but this is what happens (where o is the
optimization level and d is the dynamic function scope flag) :

	o >= 0 && d     : result == 6
	o >= 0 && !d    : result == 6
	o == -1 && d    : ReferenceError: "outer_d" is not defined.
	o == -1 && !d   : result == 6

so it seems, as long as the code is compiled to a class file, execution works
all right. but if the code is interpreted, something goes wrong with the scopes.

by inspecting the code (Interpreter.java), i found out, that inner functions are
compiled with the context's compileFunctionsWithDynamicScope flag. in the
dynamic case, this of course is the scope within which the outer function is
running instead of the variable object of the outer function, in which the inner
function is defined and obviously should be running.

i assume, inner functions should always be compiled with static scope regardless
of whether the context is set for dynamic scope compilations of functions or not.

if i am correct, then the Interpreter.generateNestedFunctions method should
probably be added the line

	            jsi.itsData.itsUseDynamicScope = false;

after creating the InterpreterData instance.

i tried it with the above sample code and it seems to fix the problem. though i
am not sure, whether it runs fine with more thorough regression tests.

what do you think of it ?

regards, felix

Reproducible: Always

Steps to Reproduce:
1. make Context interpreting (optimizationLevel = -1)
2. make Context "compile" functions with dynamic scope
3. run the above script and see the error noted using the shell tool.

Actual Results:  
error message : ReferenceError: "outer_d" is not defined.

Expected Results:  
the variable should be set to 6.

Comment 1

[Felix Meschberger]

cc-ing igor.

Comment 2

[Igor Bukanov]

A possible fix can be rather nontrivial as it has to address the issue of
functions defined by eval and new Function.

For example, int the above example should not give different results if 
function inner would be replaced by

inner = new Function("level", "outer_d++; ...");

so setting itsUseDynamicScope should depend on if the scope during compilation
time is suitable for dynamic behaviour... 

Comment 3

[Phil Schwartau]

I have a couple questions about the example:

1. The parameter |arg| to the outer function plays no role
   and can be omitted, right?

2. Shouldn't there be a |return| statement here?

         if (level > 0) {
            inner( level - 1 );   <<<---- should be |return inner(level -1);|  (?)

Comment 4

[Felix Meschberger]

(1) yes, the argument arg to the outer function plays no role and the argument
level to the inner function is only used to limit recursion depth.

(2) hmpf, yes, of course the line should read
        return inner(level-1);

Comment 5

[Phil Schwartau]

Felix: thanks. I've added your test to the JS testsuite:

      mozilla/js/tests/js1_5/Scope/regress-181834.js

Comment 6

[Igor Bukanov]

This is related to bug 154693 where the problem is exposed without activation of
dynamic scope. 

Comment 7

[Igor Bukanov]

Attachment: Do not use dynamic scope for nested functions and functions defined under with

The patch changes Interpreter to ignore dynamic scope flag for nested functions
and functions defined inside with statements as in the first case a dynamic
scope object should already present on the scope chain of a nested function and
in the second case there is no good way to use a dynamic scope. For example,
consider a script compiled with dynamic scope on:

var f;

with (some_object) {
  f = function() { ... }
}

here f contains a function with some_object as its scope object. But if a
dynamic scope object should be taken into account when resolving a particular
reference during execution of the function, how should it interfere with
some_object? The patch takes a simple approach of ignoring dynamic scope in
this case as anything else would add a lot of code which nobody may use...

Comment 8

[Igor Bukanov]

Attachment: Prev patch update due to unrelated changes in the Interpreter

Comment 9

[Phil Schwartau]

Question: is there any way for me to test this in the Rhino shell?

Would the testcase above, mozilla/js/tests/js1_5/Scope/regress-181834.js,
have to include code to turn on the 'compile functions with dynamic scope'
feature? Is that possible?

Comment 10

[Igor Bukanov]

Attachment: Patch update due to partial commit

I committed the patch part that check for dynamic scope flag at function
initialization, not during compilation phase. It removes difference between
ordinary functions and functions defined via Function constructor.

Comment 11

[Igor Bukanov]

In Rhino it is possible to check/switch to dynamic scope mode from JavaScript via:

function setDynamicScope(flag) {
	if (this.Packages) {
		var cx = this.Packages.org.mozilla.javascript.Context.getCurrentContext();
		cx.setCompileFunctionsWithDynamicScope(flag); 
	}
}


function getDynamicScope() {
	if (this.Packages) {
		var cx = this.Packages.org.mozilla.javascript.Context.getCurrentContext();
		return cx.hasCompileFunctionsWithDynamicScope(); 
	}
}

but it would not affect function statements in the current script and prior that
partial commit of the patch function expressions after switch to the dynamic
scope mode are not affected either. Thus one has to either use Function, eval or
call load on another script to see the difference.

Comment 12

[Igor Bukanov]

Attachment: Yet another patch update

With more commits patch became ratjer small and readable but still it does not
reduce its consiquences.

Comment 13

[Phil Schwartau]

Igor: thanks. I have added functionality from your Comment #11 
to the testcase above, to set and unset the 'compile functions
with dynamic scope' feature of Rhino. 

I use eval() to recompile the |outer| function each time
I set or unset this feature.

With the latest build of Rhino, the testcase fails in both interpreted
and compiled modes with the error that Felix reported above:

         ReferenceError: "outer_d" is not defined

Comment 14

[Igor Bukanov]

Note for the test cases: in Rhino eval and new Function always use the
interpreted mode, so the compilation mode under dynamic scope setup can not be
tested that way.

I commited the fix so the bug can be marked as fixed.

Comment 15

[Phil Schwartau]

Marking FIXED -

Comment 16

[Phil Schwartau]

Marking Verified Fixed.

The above testcase now passes in Rhino. As Igor indicated in Comment #14, 
however, this only exercises Rhino interpreted mode. To check Rhino
compiled mode, I tested interactively as follows:

 java org.mozilla.javascript.tools.shell.Main  -f file1.js -f file2.js

where "file1.js" sets the dynamic mode and "file2.js" contains the test.


We found that Rhino compiled mode passed this test both before and after
the patch was committed. As reported above, it was only interpreted mode
that was having a problem on this.

But that's fixed now -

Comment 17

[Felix Meschberger]

thanks very much too all.

Comment 18

[Igor Bukanov]

Targeting as resolved against 1.5R4