Closed Bug 692984 Opened 13 years ago Closed 12 years ago

Implement Environment.prototype.getVariable, .setVariable

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
OS: Mac OS X → All
Hardware: x86 → All
Version: Other Branch → Trunk
Attached patch WIP 1 (obsolete) — Splinter Review
I'm only going to do getVariable and setVariable in this bug. It's mostly there, just need a few more tests.
Assignee: general → jorendorff
Summary: Implement Environment.prototype.getVariable, .setVariable, .defineVariable, getVariableDescriptor → Implement Environment.prototype.getVariable, .setVariable
Attached patch v2 (obsolete) — Splinter Review
Attachment #594361 - Attachment is obsolete: true
Attachment #595083 - Flags: review?(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.
Could we have a test for the environments pushed for 'catch' clauses?
Is it worthwhile to test the bindings created by generator expressions? I guess it's covered by the function argument stuff.
(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").
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 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?
Ooh! Do we test assignments to local functions' names?
Attached patch v3Splinter Review
(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)
Attachment #595083 - Attachment is obsolete: true
Attachment #606767 - Flags: review?(jimb)
Attachment #595083 - Flags: review?(jimb)
Comment on attachment 606767 [details] [diff] [review]
v3

Hurrah!
Attachment #606767 - Flags: review?(jimb) → review+
Let's land this!
Assignee: jorendorff → blackconnect
Component: JavaScript Engine → Java-Implemented Plugins
QA Contact: general → blackconnect
Assignee: blackconnect → jorendorff
Component: Java-Implemented Plugins → JavaScript Engine
QA Contact: blackconnect → general
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/jorendorff@mozilla.com-2d0d24329c9b
https://hg.mozilla.org/mozilla-central/rev/63cd904be4a7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.