Closed Bug 611388 Opened 14 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
relnote-firefox --- 36+

People

(Reporter: crowderbt, Assigned: efaust)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(2 files, 3 obsolete files)

<@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
Blocks: 383902
Blocks: 443726
No longer blocks: 383902
Depends on: 383902
Blocks: es6
Assignee: general → evilpies
Assignee: evilpies → general
Trying this again after nearly a year.
QA Contact: evilpies
Attached patch wip (obsolete) — Splinter Review
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.
Assignee: general → evilpies
Status: NEW → ASSIGNED
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.
Depends on: 589199
Blocks: 867617
Blocks: 880083
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.
Assignee: evilpies → nobody
Status: ASSIGNED → NEW
QA Contact: evilpies
Blocks: es6:let
Flags: needinfo?(shu)
Summary: Make Spidermonkey's const implementation be Harmony compatible → const should be block-scoped
Summary: const should be block-scoped → const should be block-scoped and an initializer should be required
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Attached patch WIP take 2 (obsolete) — Splinter Review
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.
Attachment #8497847 - Flags: feedback?(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.
Attachment #8497847 - Flags: feedback?(shu) → feedback+
Clearing NI since efaust is working on it.
Flags: needinfo?(shu)
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
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8500730 - Flags: review?(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?
Attachment #8500730 - Flags: review?(shu)
(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.
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.
Attachment #728656 - Attachment is obsolete: true
Attachment #8497847 - Attachment is obsolete: true
Attachment #8500730 - Attachment is obsolete: true
Attachment #8513131 - Flags: review?(shu)
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
Attachment #8513134 - Flags: review?(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?
Attachment #8513131 - Flags: review?(shu) → review+
(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 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.
Attachment #8513134 - Flags: review?(shu) → review+
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
https://hg.mozilla.org/mozilla-central/rev/db8ff9116376
https://hg.mozilla.org/mozilla-central/rev/a9a7f16c817b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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"
Flags: needinfo?(shu)
Keywords: site-compat
Depends on: 1094995
(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.
Clear shu ni? reset by midair collision.
Flags: needinfo?(shu)
Depends on: 1095036
Depends on: 1095439
Depends on: 1095299
This appears to have broken the Lightbeam extension: https://github.com/mozilla/lightbeam/issues/622
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]
relnote-firefox: --- → ?
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)
Depends on: 1147456
You need to log in before you can comment on or make changes to this bug.