366550 - increment and decrement operators don't work for global vars with dynamic scopes

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Hannes Wallnoefer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.1.1) Gecko/20060601 Firefox/2.0.0.1 (Ubuntu-edgy)
Build Identifier: 

ScriptRuntime.nameIncrDecr() is currently not aware of dynamic scopes. When ++ or  -- is applied to a global variable in the dynamic scope, a ReferenceError is thrown. 

Reproducible: Always

Comment 1

[Hannes Wallnoefer]

Attachment: Fixes this bug by calling checkDynamicScope() when parent scope is null

Since ScriptRuntime.nameIncrDecr() now needs the current Context in order to know whether dynamic scopes are enabled, it now takes a Context parameter. The old signature is deprecated but still left in place for backwards compatibility.

(I also fixed two unrelated javadoc @link tags since Intellij pointed me to it. Sorry if this is confusing.)

Comment 2

[Attila Szegedi]

Can you explain why you need to test for

if (cx.useDynamicScope && scopeChain.getParentScope() == null)

instead of 

if (cx.useDynamicScope)

or even better

if (cx.hasFeature(Context.FEATURE_DYNAMIC_SCOPE))

?

Comment 3

[Attila Szegedi]

Also, it is not completely clear to me when should a dynamic scoping lookup be performed. Namely -- is dynamic scoping the property of the current context, or the property of the function? Functions have a "dynamic scoping" flag that is set to true if their *compiling* Context was set to have dynamic scoping. I think that in light of this, ScriptRuntime.nameIncrDecr() should maybe instead have a boolean flag that's set to true if the invoking function was compiled in a Context that had dynamic scopes.

Actually, I'm coming to believe that dynamic scope is more trouble than it is worth. For one thing, it's non-ECMA. Back in the day, I used to think it's a nice (even if non-ECMA compliant) feature, especially since I had a system using a shared scope with global functions that was set as the prototype of per--thread scopes. I believe (correct me if I'm wrong) that most people use dynamic scopes in this exact setting. I did however reach the conclusion that having such global shared prototype scopes is not such a good idea -- the performance impact of just defining those global functions in each of your scripts instead is negligable. Even if you use a shared prototype scope, you can do it without dynamic scopes feature -- you just need to remember to explicitly qualify access to global variables with "this" withing the body of these global functions. If I had it my way, I'd declare the dynamic scopes to be a deprecated feature, and just educate the users about how to live without it.

Comment 4

[Hannes Wallnoefer]

(In reply to comment #2)
> Can you explain why you need to test for
> 
> if (cx.useDynamicScope && scopeChain.getParentScope() == null)
> 
> instead of 
> 
> if (cx.useDynamicScope)

We're walking down the scope chain, and only the last, top level scope is replaced by the dynamic scope (which must have the static top level scope as its prototype). Calling ScriptRuntime.checkDynamicScope with anything else than the static top scope as second argument would be wrong.

> or even better
> 
> if (cx.hasFeature(Context.FEATURE_DYNAMIC_SCOPE))

cx.useDynamicScope is merely a "proxy" field for cx.hasFeature(Context.FEATURE_DYNAMIC_SCOPE). It is set in ScriptRuntime.doTopCall() and used in various places in ScriptRuntime.

Comment 5

[Hannes Wallnoefer]

(In reply to comment #3)
> Also, it is not completely clear to me when should a dynamic scoping lookup be
> performed. Namely -- is dynamic scoping the property of the current context, or
> the property of the function? Functions have a "dynamic scoping" flag that is
> set to true if their *compiling* Context was set to have dynamic scoping. 

I think the confusion comes from the fact that there are two ways to enable dynamic scopes: the old, deprecated way, and the current way. The old way worked via setting Context.setCompileFunctionsWithDynamicScope() and was indeed a compile-time feature. The problem is that it didn't work with nested functions, so it was replaced by the current solution, which checks whether to use dynamic scopes at various places in ScriptRuntime.

> Actually, I'm coming to believe that dynamic scope is more trouble than it is
> worth. For one thing, it's non-ECMA. Back in the day, I used to think it's a
> nice (even if non-ECMA compliant) feature, especially since I had a system
> using a shared scope with global functions that was set as the prototype of
> per--thread scopes. I believe (correct me if I'm wrong) that most people use
> dynamic scopes in this exact setting. I did however reach the conclusion that
> having such global shared prototype scopes is not such a good idea -- the
> performance impact of just defining those global functions in each of your
> scripts instead is negligable. Even if you use a shared prototype scope, you
> can do it without dynamic scopes feature -- you just need to remember to
> explicitly qualify access to global variables with "this" withing the body of
> these global functions. If I had it my way, I'd declare the dynamic scopes to
> be a deprecated feature, and just educate the users about how to live without
> it.

Please don't! Helma applications often grow to several hundred kB of Javascript code, and compiling everything 20 or 40 times instead of just once would simply cause them to stop working under load. 

We have been using dynamic scopes for years and never really had a big problem, except the nested function problem with the old approach, and, well, this bug now. You can blame me for not switching to the new approach earlier, Partly it had to do with the fact that I was confused myself, just see bug #347154 for some of that. 

Regarding ECMA, it's true that this setup is obviously not described in the spec, but saying that it is non ECMA-compliant is a bit far-fetched, too. I don't think ECMA specifies how the top level scope object should be implemented, and to the outside, the prototyped dynamic scope behaves exactly as defined in the spec. I' saying this to the best of my knowledge without having looked at the spec recently, but I could check if this is an issue.

Comment 6

[Attila Szegedi]

(In reply to comment #5)

> > these global functions. If I had it my way, I'd declare the dynamic scopes to

> > be a deprecated feature, and just educate the users about how to live without

> > it.

> 

> Please don't! Helma applications often grow to several hundred kB of Javascript

> code, and compiling everything 20 or 40 times instead of just once would simply

> cause them to stop working under load. 

> 


No worries -- I'm not interested in wrecking significant efforts of people who built their own systems atop of Rhino :-) As a matter of fact, you wouldn't need to *compile* everything repeatedly, just *execute* a once-compiled script in every new top-level scope. That'd just create new Function objects, but their code is already compiled (either into InterpreterData instance or into a Java class). It's still more overhead than not even creating new Function objects at all, but the complexity of your scope setup gets reduced in turn. Yes, it's a tradeoff, and what I'd want to do is teach the people about the existence of this tradeoff, so they can choose one way or the other.  

> Regarding ECMA, it's true that this setup is obviously not described in the

> spec, but saying that it is non ECMA-compliant is a bit far-fetched, too. I

> don't think ECMA specifies how the top level scope object should be

> implemented, and to the outside, the prototyped dynamic scope behaves exactly

> as defined in the spec. I' saying this to the best of my knowledge without

> having looked at the spec recently, but I could check if this is an issue.

> 


The ECMA spec is very explicit regarding how is a name to be resolved against a scope chain. With dynamic scopes, a function defined in the prototype object resolves names differently based on where it is invoked from. As I see it, people are using this mainly to avoid having to write a lot of "this." qualifiers in the functions in the prototype object. I wouldn't have problems with it if this behaviour were encapsulated within a special implementation of Scriptable - in that case you could claim that the object behaves like that on the "inside" while retaining spec-compliant behaviour on the "outside". What I have issues with is that the concept of dynamic scopes is being present in the ScriptRuntime.

Regardless, for the time being, I'll commit your patch -- it's a good solution for the problem within the constraints of the current implementation

Comment 7

[Attila Szegedi]

Patch committed to CVS HEAD.

Comment 8

[Norris Boyd]

Adding target milestone of 1.6R6 based on the date this bug was resolved FIXED.