692984 - Implement Environment.prototype.getVariable, .setVariable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

I'm only going to do getVariable and setVariable in this bug. It's mostly there, just need a few more tests.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 3

[Jim Blandy :jimb]

I would expect bug 714857 to be visible via Debugger.Environment as well. We shouldn't hold this feature until that's fixed, but it would be nice to have a bug filed for that.

Comment 4

[Jim Blandy :jimb]

Could we have a test for the environments pushed for 'catch' clauses?

Comment 5

[Jim Blandy :jimb]

Is it worthwhile to test the bindings created by generator expressions? I guess it's covered by the function argument stuff.

Comment 6

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #5)
> Is it worthwhile to test the bindings created by generator expressions? I
> guess it's covered by the function argument stuff.

Testing this might be impeded by bug 709367 ("Bogus ReferenceError with generator expression, closure").

Comment 7

[Jim Blandy :jimb]

I would like to see more aggressive testing of Debugger.Environment instances referring to environments whose dynamic scopes we've left. As I understand it, this is exactly the kind of area where we need to watch out for the compiler backsliding. Each type of scope object should be tested, for example.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 595083 [details] [diff] [review]
v2

Review of attachment 595083 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! I'm interested to hear about function statements and extensibility before I r+.

::: js/src/jit-test/tests/debug/Environment-setVariable-07.js
@@ +3,5 @@
> +var g = newGlobal('new-compartment');
> +function test(code) {
> +    g.eval("function f() { " + code + " }");
> +    var dbg = new Debugger(g);
> +    var val = Math.random();

Could we seed this generator, or just use a global, or run several passes on a series of explicit values? It'd sure be nice to keep the tests deterministic.

::: js/src/jit-test/tests/debug/Environment-setVariable-08.js
@@ +1,1 @@
> +// setVariable throws if the scope is lexical and no binding exists.

I think it's fine that this test will fail if the setVariable ever succeeds; we can revise the test if that ever happens. But I think the comment should mention the other correct behavior.

::: js/src/jit-test/tests/debug/Environment-setVariable-11.js
@@ +9,5 @@
> +g.eval("function f(obj) { with (obj) { d(); } }");
> +
> +g.eval("var obj = {x: 1}");
> +g.f(g.obj);
> +assertEq(g.eval("obj.x"), 7);

You couldn't just say, "assertEq(g.obj.x, 7)"?

::: js/src/jit-test/tests/debug/Environment-setVariable-13.js
@@ +19,5 @@
> +var dbg = Debugger(g);
> +dbg.onDebuggerStatement = function (frame) {
> +    var x = 0;
> +    for (var env = frame.older.environment; env; env = env.parent) {
> +        print(uneval(env.names()));

Should these 'print' calls stay?

::: js/src/vm/Debugger.cpp
@@ -3737,5 @@
>  static JSBool
>  DebuggerEnv_find(JSContext *cx, uintN argc, Value *vp)
>  {
>      REQUIRE_ARGC("Debugger.Environment.find", 1);
> -    THIS_DEBUGENV_OWNER(cx, argc, vp, "get type", args, envobj, env, dbg);

Ugh. Thanks.

@@ +3766,5 @@
>  
> +static JSBool
> +DebuggerEnv_getVariable(JSContext *cx, uintN argc, Value *vp)
> +{
> +    REQUIRE_ARGC("Debugger.Environment.getVariable", 1);

You don't want ".prototype" in there, too?

@@ +3794,5 @@
> +
> +static JSBool
> +DebuggerEnv_setVariable(JSContext *cx, uintN argc, Value *vp)
> +{
> +    REQUIRE_ARGC("Debugger.Environment.setVariable", 1);

Similarly, ".prototype".

It seems to me this might as well just require two arguments. One doesn't want to set a variable to 'undefined' so frequently that we need an API wrinkle for it.

@@ +3814,5 @@
> +        /* This can trigger setters. */
> +        ErrorCopier ec(ac, dbg->toJSObject());
> +
> +        /*
> +         * Scope objects always answer true to isExtensible(), even if they

The compiler has already figured out and marked the scopes that it expects might acquire new bindings, but that only gets reflected at runtime as the bindings' EXTENSIBLE_PARENTS flag; the scope itself isn't marked. It's a pity we can't use that information here.

@@ +3826,5 @@
> +            /* 'let' and 'catch' scopes are never extensible. */
> +            JS_ASSERT(env->isClonedBlock());
> +            extensible = false;
> +        } else if (env->isCall()) {
> +            /* Only non-strict direct eval can make a call extensible. */

Does this catch function statements, as well?

@@ +3834,5 @@
> +                         funobj->toFunction()->script()->usesEval;
> +        } else {
> +            extensible = true;
> +        }
> +        if (!extensible && env->nativeLookup(cx, id) == NULL)

Could we do the nativeLookup test first, and then only determine extensibility when we see it's necessary?

Comment 9

[Jim Blandy :jimb]

Ooh! Do we test assignments to local functions' names?

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v3

(in response to comment 3)
> I would expect bug 714857 to be visible via Debugger.Environment as
> well. We shouldn't hold this feature until that's fixed, but it would be
> nice to have a bug filed for that.

I left a comment in that bug about this. I think that should be
sufficient.

(comment 4)
> Could we have a test for the environments pushed for 'catch' clauses?

I added a comprehensive test, Environment-variables.js, that includes a
test for catch clauses.

(comment 5)
> Is it worthwhile to test the bindings created by generator expressions?
> I guess it's covered by the function argument stuff.

I wrote a test for this and immediately ran into:

(comment 6)
> (In reply to Jim Blandy :jimb from comment #5)
> > Is it worthwhile to test the bindings created by generator expressions? I
> > guess it's covered by the function argument stuff.
> 
> Testing this might be impeded by bug 709367 ("Bogus ReferenceError with
> generator expression, closure").

Yep. I left a comment in that bug. I will eventually take that bug, fix
it, and uncomment the test. Generator expressions and array
comprehensions both currently have ridiculous semantics which are *not*
proposed for ES6, so I think this will actually get fixed fairly soon
(this year, as opposed to "oh, sometime").

(comment 7)
> I would like to see more aggressive testing of Debugger.Environment
> instances referring to environments whose dynamic scopes we've left. As
> I understand it, this is exactly the kind of area where we need to watch
> out for the compiler backsliding. Each type of scope object should be
> tested, for example.

Take a look at Environment-variables.js and see what you'd like me to add.

> ::: js/src/jit-test/tests/debug/Environment-setVariable-07.js
> Could we seed this generator, or just use a global, or run several
> passes on a series of explicit values? It'd sure be nice to keep the
> tests deterministic.

Done, using explicit values. No idea why I wrote it that way.

> ::: js/src/jit-test/tests/debug/Environment-setVariable-08.js
> > +// setVariable throws if the scope is lexical and no binding exists.
> 
> I think it's fine that this test will fail if the setVariable ever
> succeeds; we can revise the test if that ever happens. But I think the
> comment should mention the other correct behavior.

We discussed this on IRC and decided the behavior of env.setVariable is
more predictable if it never creates bindings. So I changed it to throw
if the variable doesn't already exist.

(But see Environment-setVariable-12.js for a funny case.)

> > +assertEq(g.eval("obj.x"), 7);
> You couldn't just say, "assertEq(g.obj.x, 7)"?

Fixed.

> ::: js/src/jit-test/tests/debug/Environment-setVariable-13.js
> Should these 'print' calls stay?

Removed.

> @@ +3766,5 @@
> >  
> > +static JSBool
> > +DebuggerEnv_getVariable(JSContext *cx, uintN argc, Value *vp)
> > +{
> > +    REQUIRE_ARGC("Debugger.Environment.getVariable", 1);
> 
> You don't want ".prototype" in there, too?

No, none of the other REQUIRE_ARGC calls have it. It's just informal. If
you'd prefer them to be precise, feel free to take a pass through and
change them all, rs=me.

(about DebuggerEnv_setVariable)
> It seems to me this might as well just require two arguments. One
> doesn't want to set a variable to 'undefined' so frequently that we need
> an API wrinkle for it.

Done.

> The compiler has already figured out and marked the scopes that it
> expects might acquire new bindings, but that only gets reflected at
> runtime as the bindings' EXTENSIBLE_PARENTS flag; the scope itself isn't
> marked. It's a pity we can't use that information here.

Agreed. But now this code is gone, which is even better.

(about the code in setVariable to test whether a scope is extensible)
> Does this catch function statements, as well?

Added a test in Environment-variables.js.

Not only that, but the test actually passes. Now that setVariable never
creates a binding, the abovementioned code is gone.

> Ooh! Do we test assignments to local functions' names?

Added in Environment-variables.js.

Also added a test for assigning to the name of a FunctionExpression. The
binding is read-only, so basically we should throw.
(Environment-setVariable-11.js)

Also added a test for assigning to a with-block in a way that should
create a new binding, though from the Environment's perspective it's not
a new variable, exactly. (Environment-setVariable-12.js)

Comment 11

[Jim Blandy :jimb]

Comment on attachment 606767 [details] [diff] [review]
v3

Hurrah!

Comment 12

[Jim Blandy :jimb]

Let's land this!

Comment 13

[Mozilla RelEng Bot]

Try run for 2d0d24329c9b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2d0d24329c9b
Results (out of 35 total builds):
    success: 30
    warnings: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2d0d24329c9b

Comment 14

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63cd904be4a7

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/63cd904be4a7