1246215 - After an error in a let-declaration initializer, the variable is doomed forever

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

STR:

1. Try this in the console:

let bob = invalidName;
ReferenceError: invalidName is not defined
bob
ReferenceError: can't access lexical declaration `bob' before initialization
let bob = 3;
SyntaxError: redeclaration of let bob
bob
ReferenceError: can't access lexical declaration `bob' before initialization

ER:

I would like to be able to redeclare `bob` successfully with `let bob = 3` since the first attempt contained a ReferenceError.

AR:

At the moment, I am forbidden from ever using the name `bob` due to the ReferenceError in the first declaration which seems to have marked the name as used, but still left the actual variable undefined.

Comment 1

[Jeff Walden [:Waldo]]

This is, approximately, a side effect of how |let| is specified to work.  Within a page, <script>let z = (() => { throw 42; })();</script> will result in a permanently unusable |z| declaration -- declaring the variable, but throwing before its initialization, and then redeclarations always being an error mean you're stuck.  You're seeing basically the same behavior here.

We did (dubiously, IMO) implement a very bogus hack to quasi-ignore the specification in the JS shell, to initialize bindings that throw before evaluation to |undefined|.  See bug 1233734.  But my testing suggests that |let bob = invalidName| in the console, adds a lexical declaration to the *page-visible* global lexical environment.  Unlike the shell, the web is not our personal playbox; we can't decide to willfully violate spec requirements whose effect we don't like.

My vote would be to figure out some sort of way to better explain the error (and indicate that it's a fair cop but the spec's to blame), then WONTFIX this.  The spec is concededly dumb here, and it constrains what we can do.

Comment 2

[Jeff Walden [:Waldo]]

CCing the usual suspects.

Comment 3

[Morgan Phillips [:mrrrgn]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> STR:
> 
> 1. Try this in the console:
> 
> let bob = invalidName;
> ReferenceError: invalidName is not defined
> bob
> ReferenceError: can't access lexical declaration `bob' before initialization
> let bob = 3;
> SyntaxError: redeclaration of let bob
> bob
> ReferenceError: can't access lexical declaration `bob' before initialization
> 
> ER:
> 
> I would like to be able to redeclare `bob` successfully with `let bob = 3`
> since the first attempt contained a ReferenceError.
> 
> AR:
> 
> At the moment, I am forbidden from ever using the name `bob` due to the
> ReferenceError in the first declaration which seems to have marked the name
> as used, but still left the actual variable undefined.

I'm glad you filed this. I recently added this to the JS shell in bug 1233734. I'd been hesitant to attack the Web Console right after, but if we want this feature I'm pumped to implement it.

Comment 4

[Shu-yu Guo [:shu]]

Unlike for the shell, there're some additional things to consider for this to go into the web console.

Do we want to: 

1) Allow the console to redeclare any uninitialized (and thus inaccessible) bindings?
2) Disallow the console from introducing any uninitialized (and thus inaccessible) bindings?

The distinction makes a difference when considering uninitialized bindings introduced by scripts. For example, suppose I have a page with

<script>
let G =(() => { throw 42; })();
</script>

A) Should the console be able to initialize |G|?

B) Should future scripts be able to initialize |G| without explicit initializing of |G| from within the webconsole? (Morgan's current patch for the shell would allow such a thing.)

My opinion is that what we actually want is 1) above, yes to A), and no to B).

Comment 5

[Morgan Phillips [:mrrrgn]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> Unlike for the shell, there're some additional things to consider for this
> to go into the web console.
> 
> Do we want to: 
> 
> 1) Allow the console to redeclare any uninitialized (and thus inaccessible)
> bindings?
> 2) Disallow the console from introducing any uninitialized (and thus
> inaccessible) bindings?
> 
> The distinction makes a difference when considering uninitialized bindings
> introduced by scripts. For example, suppose I have a page with
> 
> <script>
> let G =(() => { throw 42; })();
> </script>
> 
> A) Should the console be able to initialize |G|?
> 
> B) Should future scripts be able to initialize |G| without explicit
> initializing of |G| from within the webconsole? (Morgan's current patch for
> the shell would allow such a thing.)
> 
> My opinion is that what we actually want is 1) above, yes to A), and no to
> B).

Currently the api call that forces initialization works on everything in the global scope. Definitely an awful idea here. I had been thinking that it wouldn't be too difficult to just add a method that forces initialization of individual bindings. We could track those from the web console and call the "force initialization" method catch as catch can.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Overall, I certainly agree we should tread carefully when making the console behave differently from page scripts in any way, so it's important consider our choices carefully here, to avoid developer surprise and confusion when using the console.

(In reply to Shu-yu Guo [:shu] from comment #4)
> Unlike for the shell, there're some additional things to consider for this
> to go into the web console.
> 
> Do we want to: 
> 
> 1) Allow the console to redeclare any uninitialized (and thus inaccessible)
> bindings?
> 
> 2) Disallow the console from introducing any uninitialized (and thus
> inaccessible) bindings?
> 
> The distinction makes a difference when considering uninitialized bindings
> introduced by scripts. For example, suppose I have a page with
> 
> <script>
> let G =(() => { throw 42; })();
> </script>
> 
> A) Should the console be able to initialize |G|?
> 
> B) Should future scripts be able to initialize |G| without explicit
> initializing of |G| from within the webconsole? (Morgan's current patch for
> the shell would allow such a thing.)
> 
> My opinion is that what we actually want is 1) above, yes to A), and no to
> B).

Yes, I think your opinion makes sense.  This allows declaration errors from either the console or a bad page script to be explored in the console.  However, a correctly initialized binding should remain an error if redeclared (I guess that's assumed).  This approach appears to make no change to the behavior of page scripts alone, which I assume is most important.

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: api.diff

Okay, I have an approach and I'm not far from a patch for review. Here I've added a new static method to the Debugger object which will try to initialize any uninitialized binding on the global with a name matching your argument (the argument is a string).

We can easily detect let/const declarations with Reflect.parse, using that info plus a try/catch we will be able to initialize bindings for the user.

We can avoid initializing bindings not created by the user by looking at the type of error we catch. That is to say, if we catch an error with a message like: "SyntaxError: redeclaration of" we won't try initializing it.

Comment 8

[Morgan Phillips [:mrrrgn]]

Loling at myself a bit for being so rash. I need to find a way to call a Debugger method that effects the users's session without exposing the Debugger object to the user. Thinking through some strategies.

Comment 9

[Morgan Phillips [:mrrrgn]]

Moved "forceLexicalInitializationByName" to Debugger.Object https://www.youtube.com/watch?v=HpkecYVRt_E

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: redec.diff

Sorry to flag two of you for this, but since I touched some SpiderMonkey stuff I figured I should bug shu.

Comment 11

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8720611 [details] [diff] [review]
redec.diff

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

I forgot to add documentation for this. On that now, but will leave this patch up. I'll post my doc paragraph in a comment.

::: devtools/server/actors/webconsole.js
@@ +1279,5 @@
>      else {
>        result = dbgWindow.executeInGlobalWithBindings(aString, bindings, evalOptions);
>      }
>  
> +    // Attempt to initialize any declarations found in the evaluated string

Hmm, I can improve this comment:

"// Attempt to initialize any declarations found in the evaluated string
 // if there was an error since they may now be stuck in an "initializing"
 // state"

::: devtools/shared/webconsole/client.js
@@ +1,2 @@
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

This was annoying the beejebus out of me.

::: js/src/vm/Debugger.cpp
@@ +4572,5 @@
>      args.rval().setBoolean(result);
>      return true;
>  }
>  
> +

*sigh*

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8720611 [details] [diff] [review]
redec.diff

:bgrins is a better reviewer for the DevTools parts.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8720611 [details] [diff] [review]
redec.diff

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

::: devtools/server/actors/webconsole.js
@@ +1287,5 @@
> +      for (let line of ast.body) {
> +        if (line.type == "VariableDeclaration" &&
> +          (line.kind == "let" || line.kind == "const")) {
> +          for (let decl of line.declarations)
> +            dbgWindow.forceLexicalInitializationByName(decl.id.name);

Will this re-initialize any 'lets' that happen in the case of any error?  i.e. in `let foo = 1; let bar = 2; throw 'error';` then foo and bar get re-initialized.  That's my reading of this at least, and I wonder if the debugger can return more info to be clear which exact declaration should be reset.

Comment 14

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8720611 [details] [diff] [review]
> redec.diff
> 
> Review of attachment 8720611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/webconsole.js
> @@ +1287,5 @@
> > +      for (let line of ast.body) {
> > +        if (line.type == "VariableDeclaration" &&
> > +          (line.kind == "let" || line.kind == "const")) {
> > +          for (let decl of line.declarations)
> > +            dbgWindow.forceLexicalInitializationByName(decl.id.name);
> 
> Will this re-initialize any 'lets' that happen in the case of any error? 
> i.e. in `let foo = 1; let bar = 2; throw 'error';` then foo and bar get
> re-initialized.  That's my reading of this at least, and I wonder if the
> debugger can return more info to be clear which exact declaration should be
> reset.

'forceLexicalInitializationByName' resets values _only_ in the case where a binding has the 'uninitialized' property. So it [sh|w]ouldn't be a problem.

Comment 15

[Morgan Phillips [:mrrrgn]]

^^ To clarify, any binding that's already been set won't be in an uninitialized state. That state is unique to a binding being 'lost' due to an error happening during its initial declaration.

Comment 16

[Morgan Phillips [:mrrrgn]]

>> let foo = 1; let bar = 2; throw 'error';
error
>> foo
1
>> bar
2

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8720611 [details] [diff] [review]
redec.diff

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

Seems generally fine - can you add a test case for this in devtools/shared/webconsole/test/test_jsterm.html?

::: devtools/server/actors/webconsole.js
@@ +1275,3 @@
>      let result;
>      if (frame) {
>        result = frame.evalWithBindings(aString, bindings, evalOptions);

I'm thinking that we only want to run the new code if we aren't paused at a frame.  As an example:

data:text/html,<script>let bar = 1; function foo() { debugger; } new foo()</script>

`let bar = nothing;` will then throw "ReferenceError: nothing is not defined".  It sounds like this won't actually uninitialize bar in the global because of Comment 15, but on the other hand there's probably no value in running the new code in this case anyway since new declarations won't bind to the top-level when calling frame.evalWithBindings.

So in short, I think the new code should probably only live inside of the 'else' condition below.

Comment 18

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Comment on attachment 8720611 [details] [diff] [review]
> redec.diff
> 
> Review of attachment 8720611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems generally fine - can you add a test case for this in
> devtools/shared/webconsole/test/test_jsterm.html?
> 
> ::: devtools/server/actors/webconsole.js
> @@ +1275,3 @@
> >      let result;
> >      if (frame) {
> >        result = frame.evalWithBindings(aString, bindings, evalOptions);
> 
> I'm thinking that we only want to run the new code if we aren't paused at a
> frame.  As an example:
> 
> data:text/html,<script>let bar = 1; function foo() { debugger; } new
> foo()</script>
> 
> `let bar = nothing;` will then throw "ReferenceError: nothing is not
> defined".  It sounds like this won't actually uninitialize bar in the global
> because of Comment 15, but on the other hand there's probably no value in
> running the new code in this case anyway since new declarations won't bind
> to the top-level when calling frame.evalWithBindings.
> 
> So in short, I think the new code should probably only live inside of the
> 'else' condition below.

Oh, that makes sense. Thanks ! I'll throw up a patch with the test case and the docs.

Comment 19

[Morgan Phillips [:mrrrgn]]

* the docs I mentioned in comment 11

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 8720611 [details] [diff] [review]
redec.diff

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

C++ parts look good to me. r=me with documentation.

::: js/src/vm/Debugger.cpp
@@ +7840,5 @@
> +// Lookup a binding on the referent's global scope and change it to undefined
> +// if it is an uninitialized lexical. The method's return value is true _only_
> +// when an uninitialized lexical has been altered, otherwise it is false.
> +bool
> +DebuggerObject_forceLexicalInitializationByName(JSContext *cx, unsigned argc, Value* vp)

Needs documentation in doc/Debugger/Debugger.Object.md

Comment 21

[Morgan Phillips [:mrrrgn]]

Attachment: redec2.diff

Landing this patch (contains requested changes) if this try run looks alright: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b50cf9fde7

Comment 22

[Morgan Phillips [:mrrrgn]]

Attachment: redec2.diff

I made a small change to the import style for `Parser` I don't believe this calls for re-review, but posting the patch for good record keeping. In light of the change, will land if this try run looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=980bcac1a28e

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6104903f75c5

Comment 24

[Morgan Phillips [:mrrrgn]]

Attachment: redec2.diff

Comment 25

[Tom S [:evilpie]]

Comment on attachment 8723188 [details] [diff] [review]
redec2.diff

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

::: js/src/vm/Debugger.cpp
@@ +8054,5 @@
> +                             "string", InformalValueTypeName(args[0]));
> +        return false;
> +    }
> +
> +    PropertyName* name = args[0].toString()->asAtom().asPropertyName();

Sorry, just saw this patch go by on m-i. Nothing here ensures that the strings is already an atom or a propertyname.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6104903f75c5

Comment 27

[Morgan Phillips [:mrrrgn]]

Attachment: atoms.diff

LookupByName seems to already handle the non-property case. As expected, the test I added triggers an assertion on the current build (because the string passed in is stored as a rope).

Comment 28

[Morgan Phillips [:mrrrgn]]

(In reply to Morgan Phillips [:mrrrgn] from comment #27)
> Created attachment 8723931 [details] [diff] [review]
> atoms.diff
> 
> LookupByName seems to already handle the non-property case. As expected, the
> test I added triggers an assertion on the current build (because the string
> passed in is stored as a rope).

Hmm, 30 seconds after I posted this I realized that I was quite wrong. /me is not the smartest cookie. :(

js> gw.forceLexicalInitializationByName("1")
Assertion failure: !isIndex(&dumm

Comment 29

[Morgan Phillips [:mrrrgn]]

Attachment: atoms.diff

Comment 30

[Tom S [:evilpie]]

Comment on attachment 8723938 [details] [diff] [review]
atoms.diff

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

great.

::: js/src/vm/Debugger.cpp
@@ +8055,5 @@
>          return false;
>      }
>  
> +    RootedId id(cx);
> +    if (!ValueToIdentifier(cx, args[0], &id))

TIL, nice thing that function :)

@@ +8060,5 @@
> +        return false;
> +
> +    RootedObject pobj(cx);
> +    RootedShape shape(cx);
> +

Remove space.

Comment 31

[Pulsebot]

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

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6437b3b18d7

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In testing with Nightly 48, it seems like the STR in comment 0 still fails like it did before changes landed here.  Are more changes needed, or did it work for a while and now there is some regression?  I'm not quite sure.

Comment 34

[Jason Orendorff [:jorendorff]]

What mrrrgn fixed is this part:

    >> let x = Marth.PI;
    ReferenceError: Marth.PI is not defined
    >> x = Math.PI
    ReferenceError: can't access x before it is initialized

Now the second line runs just fine.

It is reasonable to want to just hit the up-arrow and redeclare the binding, but we haven't gone that far yet. I will clone this bug for the remaining work.

Comment 35

[Jason Orendorff [:jorendorff]]

Actually that bug is already on file - see bug 1257088.

Changing the summary here to reflect what was actually fixed.