1233734 - It's possible to make identifiers unreachable in the REPL (TDZ)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Morgan Phillips [:mrrrgn]]

let a = Int32Array() // Throws an error (correct)
let a = 1 // Throws an error (a has already been declared)
a = 1     // ReferenceError: can't access lexical declaration `a' before initialization

A can never initialize, so we're locked out until we reload the page. This may be inconvenient for developers in the middle of debugging. One possible solution would involve nesting implicits block around each repl expression.

Comment 1

[Jason Orendorff [:jorendorff]]

This is a standard feature of the language -- one I argued against, for exactly this reason.

I guess we could, in the REPL only, automatically fix it for you:

    js> let a = Int32Array()
    typein:1:13 TypeError: calling a builtin typed array constructor without new is forbidden
    Warning: According to the standard, after the above exception,
    Warning:   the global binding `a` should be permanently uninitialized.
    Warning:   We have non-standard-ly initialized it to `undefined` for you.
    Warning:   This nicety only happens in the JS shell, not in the browser.

Sure would be nice if TC39 could avoid standardizing such nonsense.

Comment 2

[Morgan Phillips [:mrrrgn]]

After discussions with Shu in Vidyo, and shu, jorendorff and Waldo in irc it seems like Jason's way from above is a good start, at least for building a WIP. 

I'll approach this by 1.) differentiating between repl and non-repl invocations, 2.) initializing uninitialized bindings with undefined after a repl invocation.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: TDZ.wip

Here's a very raw implementation that works by adding a new JSAPI call "ForceLexicalInitialization" which takes flips all uninitialized values in a scope to undefined. The way I've done this is super sloppy, lacks checks (I should probably make sure no GC is happening), and doesn't print a warning message. That aside, does this seem like a fair strategy?

The hope is that embedders will also make use of the API call to make their own repls more friendly.

Comment 4

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8705851 [details] [diff] [review]
TDZ.wip

Not far from a real patch, will just hassle you with that.

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: repl-idents.diff

Here's a working implementation of what jorendorff suggested. I'm still struggling to understand enough to know if I'm making good choices here. I look forward to getting some feedback (and seeing how off the mark I am). :)

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8706042 [details] [diff] [review]
repl-idents.diff

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

Looks fine to me for the most part. I'd like to see a 2nd version with a non-public API.

::: js/src/jsapi-tests/testForceLexicalInitialization.cpp
@@ +29,5 @@
> +    ForceLexicalInitialization(cx, scope);
> +
> +    // Verify that "foopi" has been initialized to undefined
> +    const Value v2 = scope->getSlot(scope->lookup(cx, id)->slot());
> +    CHECK(!v2.isMagic(JS_UNINITIALIZED_LEXICAL));

This check is redundant given |v2.isUndefined()| below, but I suppose would give a better failure message.

::: js/src/jsapi.cpp
@@ +1276,5 @@
>      return lexical;
>  }
>  
> +extern JS_PUBLIC_API(bool)
> +JS::ForceLexicalInitialization(JSContext *cx, HandleObject obj)

What's the rationale for making this a public API? ISTM like something we don't want embedders to be able to do easily. If it had to be a public API, I would suggest making this function as narrow as possible -- to only force initialization of things inside the global lexical scope.

@@ +1282,5 @@
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, obj);
> +
> +    bool result = false;

Nit: Rename to "initializedAny" or something.

@@ +1283,5 @@
> +    CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, obj);
> +
> +    bool result = false;
> +    NativeObject *nobj = &obj->as<NativeObject>();

SM style now matches Gecko style in pointer types. * and & hug the type, e.g., |NativeObject* robj|.

@@ +1287,5 @@
> +    NativeObject *nobj = &obj->as<NativeObject>();
> +
> +    for (Shape::Range<NoGC> r(nobj->lastProperty()); !r.empty(); r.popFront()) {
> +        Shape *s = &r.front();
> +        if (s->hasSlot() && nobj->getSlot(s->slot()).isMagic(JS_UNINITIALIZED_LEXICAL)) {

isMagic here can crash for subtle reasons. A long time ago, the idea about magic values was that only a single kind of magic value can flow into any given location. Following that, isMagic asserts that if the value is indeed magic but *not* the magic the programmer is checking, we assert. Since then, that invariant doesn't hold everywhere. On scope objects, for instance, a slot may be either UNINITIALIZED_LEXICAL or OPTIMIZED_OUT if we bailed out of Ion and couldn't recover a scope's dead slot. We should probably just drop that invariant, but for now you'd write |v.isMagic() && v.whyMagic() == MAGIC|. Kinda dumb.

@@ +1288,5 @@
> +
> +    for (Shape::Range<NoGC> r(nobj->lastProperty()); !r.empty(); r.popFront()) {
> +        Shape *s = &r.front();
> +        if (s->hasSlot() && nobj->getSlot(s->slot()).isMagic(JS_UNINITIALIZED_LEXICAL)) {
> +            RootedValue v(cx, UndefinedValue());

No need to root.

::: js/src/jsapi.h
@@ +1533,5 @@
>  
> +namespace JS {
> +
> +/**
> + * Set all of the uninitialized lexicals on an object to undefined.

Since the bool return here isn't success/failure but "was anything initialized", please add a comment. The assumed convention for returning bool is success/failure.

::: js/src/shell/js.cpp
@@ +702,5 @@
> +        // setting uninitialized lexicals to undefined so that they may still
> +        // be used. This behavior is _only_ acceptable in the context of the repl.
> +        if (JS::ForceLexicalInitialization(cx, globalLexical)) {
> +            fputs("Warning: According to the standard, after the above exception,\n"
> +                  "Warning: the global binding should be permanently uninitialized.\n"

Nit: pluralize binding(s), could've been multiple.

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: repl-idents.diff

I moved the function to jsfriends instead of jsapi. At first I'd thought it may be nice for embedders to have this, but on second thought that's probably not such a good idea. Thanks!

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8710590 [details] [diff] [review]
repl-idents.diff

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

This is fine for the shell but isn't sufficient if we want similar behavior for the webconsole. I'm still undecided if it's okay to uninitialize dead bindings from other scripts. I guess it's fine, to go along with the maxim "make playing around as easy as possible".

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52d3e5be2dbc

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce2bb179f8e4

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/52d3e5be2dbc
https://hg.mozilla.org/mozilla-central/rev/ce2bb179f8e4