611388 - const should be block-scoped and an initializer should be required

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

<@brendan> crowder: for Harmony, const and let will be block scoped, not hoisted (so not usable before the declaration), and const must have an initializer

Comment 1

[Tom S]

Trying this again after nearly a year.

Comment 2

[Tom S]

Attachment: wip

I did the super obvious first thing and got const working at the block level. At the moment we convert all lets to vars at the body scope, which makes it harder to change in such an easy way.

Comment 3

[Tom S]

This bug is far harder to do without first making let more like in ES6, which should also make this change automatically work for global let. Thus setting the correct dependency.

Comment 4

[Tom S]

I am not working on this right now, but it would be awesome if somebody fixed bug 589199, so that we can get this done quickly.

Comment 5

[Eric Faust [:efaust]]

Attachment: WIP take 2

Let's try this again. This looks like it might work, though there are loads more tests to clean up. Likely there's something hokey going on with the isConst() stuff in the bytecode emitter. Most of it looked safe to leave unified, but I would appreciate another set of eyes.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8497847 [details] [diff] [review]
WIP take 2

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

Looks solid!

Since we're getting lexical const, I suggest renaming PND_LET to PND_LEXICAL, and Binding::Kind to { ARGUMENT, MUTABLE, IMMUTABLE }. Similarly, ParseNode::isHoistedLetUse should be renamed isHoistedLexicalUse. Also should fix up the TDZ error message to refer to lexical binding instead of just 'let'.

One thing this patch doesn't handle is consts in 'for' heads, which are still treated as function-scoped (see Parser::forStatement). That said, those are not conformant for lets *either*, so I'm not sure what approach we should take for this patch. If it isn't too much undue burden, maybe we should give consts the historical non-compliant lexical scope in 'for' heads in the meantime until 'for' scopes are fixed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6850,1 @@
>          ok = pn->isArity(PN_BINARY)

Maybe assert that if PNK_CONST, !pn->isArity(PN_BINARY)? I'd like to kill let exprs and stmts though...

::: js/src/frontend/Parser.cpp
@@ +1355,5 @@
>                      // }
>                      //
>                      // The use of 'x' inside 'inner' needs to be marked.
> +                    if (bodyLevelHoistedUse &&
> +                        (outer_dn->isLet() || outer_dn->kind() == Definition::CONST))

Wouldn't lexical consts be both isLet() and isConst()? That is, the second disjunct is unneeded.

@@ +3585,5 @@
>  Parser<ParseHandler>::variables(ParseNodeKind kind, bool *psimple,
>                                  StaticBlockObject *blockObj, VarContext varContext)
>  {
>      /*
> +     * The three options here are:

Four :)

@@ +5454,2 @@
>        case TOK_VAR: {
>          Node pn = variables(tt == TOK_CONST ? PNK_CONST : PNK_VAR);

This ternary is unneeded now.

Comment 7

[Shu-yu Guo [:shu]]

Clearing NI since efaust is working on it.

Comment 8

[Eric Faust [:efaust]]

Attachment: Make const bindings lexically scoped, and make initializers mandatory

This still might need more help, but it passes everything, so we should take a real look at it this time.

On try one last time at: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea0243314872

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8500730 [details] [diff] [review]
Make const bindings lexically scoped, and make initializers mandatory

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

Looks good over all; mostly nits.

Since you're still in the middle of ironing out bugs on try, I'd like to see the final patch.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6845,5 @@
>          ok = EmitLexicalScope(cx, bce, pn);
>          break;
>  
>        case PNK_LET:
> +      case PNK_CONST:

Could we assert in the PNK_CONST case that !pn->isArity(PN_BINARY)?

::: js/src/frontend/ParseNode.cpp
@@ +316,5 @@
>  const char *
>  Definition::kindString(Kind kind)
>  {
>      static const char * const table[] = {
> +        "", js_var_str, js_const_str, js_const_str, js_let_str, "argument", js_function_str, "unknown"

Hardcore.

::: js/src/frontend/Parser.cpp
@@ +146,5 @@
>  
>      if (prevDef) {
>          ParseNode **pnup = &prevDef->dn_uses;
>          ParseNode *pnu;
> +        unsigned start = (kind == Definition::LET || kind == Definition::CONST) ? pn->pn_blockid : bodyid;

Nit: > 100 chars wide

@@ +1167,1 @@
>          this->binder = Parser<ParseHandler>::bindVarOrConst;

bindVarOrConst is now kind of misleading. Perhaps bindVarOrGlobalConst in the interim?

@@ +1913,4 @@
>                  JSAutoByteString name;
>                  if (!AtomToPrintableString(context, funName, &name) ||
>                      !report(ParseError, false, null(), JSMSG_REDECLARED_VAR,
>                              Definition::kindString(dn), name.ptr()))

Can these (and other) ad-hoc redecl reports be refactored to call reportRedeclaration?

@@ +2773,5 @@
>      return false;
>  }
>  
>  /*
>   * Define a let-variable in a block, let-expression, or comprehension scope. pc

Nit: s/let-variable/lexical binding

@@ +2821,5 @@
>      // AppendPackedBindings.
>      if (!pn->pn_cookie.set(parser->tokenStream, pc->staticLevel, index))
>          return false;
>  
> +    // Useful for redeclaration error messages.

This comment confuses me. Both dn and bindingKind are used in many places instead of just reporting redeclaration errors.

@@ +2839,5 @@
>  
>      if (blockObj) {
>          bool redeclared;
>          RootedId id(cx, NameToId(name));
> +        RootedShape shape(cx, StaticBlockObject::addVar(cx, blockObj, id, data->isConst, index, &redeclared));

Nit: > 100 chars wide.

@@ +2845,5 @@
> +            if (redeclared) {
> +                // The only way to be redeclared without a previous definition is if we're in a
> +                // comma separated list in a DontHoistVars block, so a let block of for header. In
> +                // that case, we must be redeclaring the same type of definition as we're trying to
> +                // make.

Can't normal let expressions have comma-separated list with duplicates too?

@@ +3742,5 @@
>  }
>  
>  template <>
>  ParseNode *
> +Parser<FullParseHandler>::lexicalDeclaration(bool isConst)

Suggestion: perhaps just replace the bool with ParseNodeKind, and assert that it's either PNK_LET or PNK_CONST; then there's no need to always comment /* isConst = */ when calling this.

@@ +4147,5 @@
>              return null();
>          break;
>  
>        case TOK_NAME:
>          // Handle the form |export a} in the same way as |export let a|, by

Is this } supposed to be a |?

@@ +4340,5 @@
> +    for (ParseNode *assign = pn1->pn_head; assign; assign = assign->pn_next) {
> +        MOZ_ASSERT(assign->isKind(PNK_ASSIGN) || assign->isKind(PNK_NAME));
> +        if (assign->isKind(PNK_NAME) && !assign->isAssigned())
> +            return false;
> +        // PNK_ASSIGN nodes are always assignments.

Change this part per our IRC discussion.

::: js/src/jit-test/tests/asm.js/testGlobals.js
@@ +1,5 @@
>  load(libdir + "asm.js");
>  load(libdir + "asserts.js");
>  
>  assertAsmTypeFail(USE_ASM + "var i; function f(){} return f");
> +// We don't need to check uninitialized const. It's no longer syntactically valid.

I don't think you need to put a comment here.

::: js/src/jit-test/tests/basic/testDestructuringVarInsideWith.js
@@ +1,1 @@
> +var b;

Why is this needed?

::: js/src/jsreflect.cpp
@@ -3187,5 @@
> -
> -      case PNK_NAME:
> -        if (pkind && (pn->pn_dflags & PND_CONST))
> -            *pkind = VARDECL_CONST;
> -        /* FALL THROUGH */

Why is it okay to remove this case?

::: js/src/tests/js1_7/block/regress-349507.js
@@ -19,5 @@
>    enterFunc ('test');
>    printBugNumber(BUGNUMBER);
>    printStatus (summary);
>  
> -  expect = /TypeError: redeclaration of const b/;

lol...

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +688,5 @@
>          assertLocalDecl("let " + pattSrcs[i].join(",") + ";", letDecl(pattPatts[i]));
>          assertBlockDecl("let " + pattSrcs[i].join(",") + ";", letDecl(pattPatts[i]));
>  
> +        // even though const has the same broken global behavior as let, we use
> +        // "const" as the kind throughout, so can just do this check.

I don't think this comment is necessary.

::: js/src/vm/ScopeObject.cpp
@@ +709,5 @@
>      return JSObject::addPropertyInternal<SequentialExecution>(cx, block, id,
>                                                                /* getter = */ nullptr,
>                                                                /* setter = */ nullptr,
>                                                                slot,
> +                                                              readonly | JSPROP_ENUMERATE | JSPROP_PERMANENT,

Nit: > 100 chars wide

@@ +789,5 @@
>                  JS_ASSERT(!redeclared);
>                  return false;
>              }
>  
> +            bool aliased = !!(propFlags > 1);

I suppose > 1 technically works here, but maybe you meant >> 1?

Comment 10

[Eric Faust [:efaust]]

(In reply to Shu-yu Guo [:shu] from comment #9)
> Comment on attachment 8500730 [details] [diff] [review]
> Make const bindings lexically scoped, and make initializers mandatory
> 
> Review of attachment 8500730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good over all; mostly nits.
> 
> Since you're still in the middle of ironing out bugs on try, I'd like to see
> the final patch.
> 
New patch to come.

> ::: js/src/frontend/ParseNode.cpp
> @@ +316,5 @@
> >  const char *
> >  Definition::kindString(Kind kind)
> >  {
> >      static const char * const table[] = {
> > +        "", js_var_str, js_const_str, js_const_str, js_let_str, "argument", js_function_str, "unknown"
> 
> Hardcore.

Inorite?

> @@ +1913,4 @@
> >                  JSAutoByteString name;
> >                  if (!AtomToPrintableString(context, funName, &name) ||
> >                      !report(ParseError, false, null(), JSMSG_REDECLARED_VAR,
> >                              Definition::kindString(dn), name.ptr()))
> 
> Can these (and other) ad-hoc redecl reports be refactored to call
> reportRedeclaration?

yyyes? Although it's hard to make everyone happy without making everyone do a lot more work than they wanted to.

> @@ +2845,5 @@
> > +            if (redeclared) {
> > +                // The only way to be redeclared without a previous definition is if we're in a
> > +                // comma separated list in a DontHoistVars block, so a let block of for header. In
> > +                // that case, we must be redeclaring the same type of definition as we're trying to
> > +                // make.
> 
> Can't normal let expressions have comma-separated list with duplicates too?

Is your contention that the comment misses a case? The logic does restrict to DontHoistVars cases, right? I think the code is right, even if the comment is wrong.

> ::: js/src/jit-test/tests/basic/testDestructuringVarInsideWith.js
> @@ +1,1 @@
> > +var b;
> 
> Why is this needed?

The assertion at the bottom throws a ReferenceError because the const is no longer hoisted. The test makes little sense. This just papers over it.Perhaps a better fix is to move the assertion within the block.
> 
> ::: js/src/jsreflect.cpp
> @@ -3187,5 @@
> > -
> > -      case PNK_NAME:
> > -        if (pkind && (pn->pn_dflags & PND_CONST))
> > -            *pkind = VARDECL_CONST;
> > -        /* FALL THROUGH */
> 
> Why is it okay to remove this case?
It's a fall-through, and we are doing the determination from the ParseNodeKind at the start. We don't need to do that set, right?

> @@ +789,5 @@
> >                  JS_ASSERT(!redeclared);
> >                  return false;
> >              }
> >  
> > +            bool aliased = !!(propFlags > 1);
> 
> I suppose > 1 technically works here, but maybe you meant >> 1?

yeah, for sure.

Comment 11

[Eric Faust [:efaust]]

Attachment: Part 0: Clean up How Const is handledd by Reflect.parse

There's no reason to do the set deep in the bowels of the path when we know what the answer is when we start.

Comment 12

[Eric Faust [:efaust]]

Attachment: Part 1: Make const block-scoped and require initializers.

With most of previous nits addressed. All talos issues should be cleaned up now. On try at: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=91925b7aab39

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8513131 [details] [diff] [review]
Part 0: Clean up How Const is handledd by Reflect.parse

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

Typo in commit message. Exercise left to the author.

::: js/src/jsreflect.cpp
@@ -2087,5 @@
>          RootedValue child(cx);
> -        /*
> -         * Unlike in |variableDeclaration|, this does not update |kind|; since let-heads do
> -         * not contain const declarations, declarators should never have PND_CONST set.
> -         */

I wonder why it was ever done this way. Some legacy const stuff I don't know about I guess?

Comment 14

[Eric Faust [:efaust]]

(In reply to Shu-yu Guo [:shu] from comment #13)
> I wonder why it was ever done this way. Some legacy const stuff I don't know
> about I guess?

It's been that way since dherman wrote the original copy of jsreflect.cpp. I guess there may have been other plans for the language that never happened, maybe? I expect dherman knew about the upcoming spec ideas when he wrote this. Maybe ES just went in a different direction.

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8513134 [details] [diff] [review]
Part 1: Make const block-scoped and require initializers.

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

r=me with checkFunctionDefinition fixed

Don't forget to bump the XDR bytecode version, since you touched js.msg

::: js/src/frontend/Parser.cpp
@@ +1999,5 @@
>           * Handle redeclaration and optimize cases where we can statically bind the
>           * function (thereby avoiding JSOP_DEFFUN and dynamic name lookup).
>           */
>          if (DefinitionNode dn = pc->decls().lookupFirst(funName)) {
> +            if (dn == Definition::GLOBALCONST) {

Existing bug; this should check for redecls for consts, lets, and global consts.

Please fix Parser<FullParseHandler>::checkFunctionDefinition as well. Also please add tests for redecl errors with consts and function statements.

Comment 16

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d6ee031d40
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77052db08766

Never say never, but it might even stick.

Comment 17

[Carsten Book [:Tomcat]]

Hi Erik, sorry had to backout this change because it seems this caused somehow in gaia-ui-test-functional-2 permanent failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3467254&repo=mozilla-inbound

Comment 18

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/db8ff9116376
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a7f16c817b

Relanded.

Try push here shows new push should be green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e045a8904613

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db8ff9116376
https://hg.mozilla.org/mozilla-central/rev/a9a7f16c817b

Comment 20

[ziyunfei]

Is this a bug or not:

js> const x = undefined;for (x of [1,2,3])print(x);
typein:2:25 SyntaxError: invalid for/in left-hand side:
typein:2:25 const x = undefined;for (x of [1,2,3])print(x);
typein:2:25 .........................^
js>
js> const x = undefined;
js> for (x of [1,2,3])print(x);     // Should this throw an error?
undefined
undefined
undefined

And I think the error message should be changed to "invalid for-in/for-of left-hand side"

Comment 21

[Eric Faust [:efaust]]

(In reply to ziyunfei from comment #20)
> Is this a bug or not:
> 
> js> const x = undefined;for (x of [1,2,3])print(x);
> typein:2:25 SyntaxError: invalid for/in left-hand side:
> typein:2:25 const x = undefined;for (x of [1,2,3])print(x);
> typein:2:25 .........................^
> js>
> js> const x = undefined;
> js> for (x of [1,2,3])print(x);     // Should this throw an error?
> undefined
> undefined
> undefined

"It is a Syntax Error if LeftHandSideExpression is a IdentifierReference that can be statically determined to always resolve to a declarative environment record binding and the resolved binding is an immutable binding." ES6 10/14/2014 13.6.4.1

Filed bug 1094995.

> And I think the error message should be changed to "invalid for-in/for-of
> left-hand side"

We can change that, it's not a big deal.

Comment 22

[Eric Faust [:efaust]]

Clear shu ni? reset by midair collision.

Comment 23

[Nicholas Nethercote [inactive]]

This appears to have broken the Lightbeam extension: https://github.com/mozilla/lightbeam/issues/622

Comment 24

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const
https://developer.mozilla.org/en-US/Firefox/Releases/36#JavaScript

A review would be really great.

Comment 25

[Florian Bender]

Release Note Request (optional, but appreciated)
[Why is this notable]: more stuff originally covered by Bug 1001090
[Suggested wording]: changed "const" semantics
[Links (documentation, blog post, etc)]: [This bug & Comment 24]

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Changed JavaScript 'const' semantics to match the ES6 specification" as wording. I am using the same wording as we have for the 'let' change (cf bug 1001090)