Implement Environment.prototype.getVariable, .setVariable

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Blocks: 694526
Blocks: 723062
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: Other Branch → Trunk
(Assignee)

Comment 1

6 years ago
Created attachment 594361 [details] [diff] [review]
WIP 1

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
Blocks: 724862
No longer blocks: 723062
(Assignee)

Updated

6 years ago
Summary: Implement Environment.prototype.getVariable, .setVariable, .defineVariable, getVariableDescriptor → Implement Environment.prototype.getVariable, .setVariable
(Assignee)

Comment 2

6 years ago
Created attachment 595083 [details] [diff] [review]
v2
Attachment #594361 - Attachment is obsolete: true
Attachment #595083 - Flags: review?(jimb)

Comment 3

6 years ago
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

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

Comment 5

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

Comment 6

6 years ago
(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

6 years ago
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

6 years ago
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

6 years ago
Ooh! Do we test assignments to local functions' names?
(Assignee)

Comment 10

6 years ago
Created attachment 606767 [details] [diff] [review]
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)
Attachment #595083 - Attachment is obsolete: true
Attachment #606767 - Flags: review?(jimb)
Attachment #595083 - Flags: review?(jimb)

Comment 11

6 years ago
Comment on attachment 606767 [details] [diff] [review]
v3

Hurrah!
Attachment #606767 - Flags: review?(jimb) → review+

Comment 12

6 years ago
Let's land this!
Assignee: jorendorff → blackconnect
Component: JavaScript Engine → Java-Implemented Plugins
QA Contact: general → blackconnect

Updated

6 years ago
Assignee: blackconnect → jorendorff
Component: Java-Implemented Plugins → JavaScript Engine
QA Contact: blackconnect → general

Comment 13

6 years ago
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
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/63cd904be4a7
https://hg.mozilla.org/mozilla-central/rev/63cd904be4a7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.