Closed Bug 932517 Opened 11 years ago Closed 9 years ago

Enable let without version=1.7+ in non-strict mode

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: cpeterson, Assigned: shu)

References

Details

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

Attachments

(1 file, 2 obsolete files)

This bug is forked from bug 855665. In bug 855665 comment 16, Rick Waldron points to TC39 minutes that conclude:

In non-strict code: let, with single token lookahead (where the single token is either an Identifier, "[", or "{" ), at the start of a statement is a let declaration. (Accepted breaking change)

https://github.com/rwaldron/tc39-notes/blob/master/es6/2012-11/nov-29.md#the-syntax-of-let
Blocks: es6:let
Microsoft says: "IE11 has shipped let/const support and block-scoped functions with (mostly) backwards compatible semantics. For example, `let let = 1;` works in IE11 today outside of strict mode."

https://mail.mozilla.org/pipermail/es-discuss/2014-January/035888.html
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
let is currently available in javascript;version=1.7 and 1.8.
Summary: Enable let without version=1.8 in non-strict mode → Enable let without version=1.7+ in non-strict mode
No longer blocks: es6:let
Attached patch Let let be versionless. (obsolete) — Splinter Review
Attachment #8675180 - Flags: review?(jorendorff)
Comment on attachment 8675180 [details] [diff] [review]
Let let be versionless.

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

+1!

...It's a little eerie that no tests care about versioned-ness of `let` (or probably any other feature, come to think of it).
Attachment #8675180 - Flags: review?(jorendorff) → review+
Depends on: 1167029
Keywords: site-compat
Keywords: site-compat
Attachment #8675180 - Attachment is obsolete: true
Attachment #8677811 - Attachment is obsolete: true
Attachment #8677811 - Flags: review?(jorendorff)
Comment on attachment 8678364 [details] [diff] [review]
Treat let as a context keyword in sloppy mode and make it versionless.

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

Woohoo!

r=me with a few minor comments and a few test requests.

::: js/src/frontend/Parser.cpp
@@ +5203,5 @@
>       */
>      bool isForDecl = false;
>  
> +    // True if a 'let' token at the head parsed as an identifier instead of
> +    // starting a lexical declaration?

Comment stilted.

@@ +5239,3 @@
>                  handler.disableSyntaxParser();
> +
> +                bool parseDecl;

At your preference, this might be a nice place for a short comment like

  // Check for the oddball backward-compatibility case `for (let.x in y)`
  // where `let` should be parsed as an identifier.

@@ +5253,5 @@
> +                    isForDecl = true;
> +                    blockObj = StaticBlockObject::create(context);
> +                    if (!blockObj)
> +                        return null();
> +                    // Initialize the enclosing scope manually for the call to

Blank line before comment.

@@ +5342,1 @@
>              headKind = PNK_FOROF;

This `&& !letIsIdentifier` is to ban `for (let.value of expr)`, right? Worth a comment, perhaps?

@@ +5598,5 @@
>                  JS_ALWAYS_FALSE(abortIfSyntaxParser());
>                  return null();
> +            } else if (tt == TOK_NAME && tokenStream.nextName() == context->names().let) {
> +                bool parseDecl = false;
> +                if (!peekShouldParseLetDeclaration(&parseDecl, TokenStream::Operand))

Unless I'm missing something, please abortIfSyntaxParser() unconditionally here rather than call peekShouldParseLetDeclaration()... the bizarro case where we treat `let` as an identifier is not worth the extra code, IMHO.

@@ +6677,5 @@
> +
> +    TokenKind tt;
> +    *parseDeclOut = false;
> +
> +    if (!tokenStream.peekToken(&tt))

Please add a test that `for (let/x;;) ///...` parses correctly (division, not a regexp)

@@ +6683,5 @@
> +
> +    switch (tt) {
> +      case TOK_NAME:
> +        // |let let| is disallowed per ES6 13.3.1.1, and for heads need to
> +        // parse things like |for (let in e)| as binding a name 'let'.

The rest of the comment after "and" seems unrelated to this case: in that case, tt would be TOK_IN and so we wouldn't get here.

@@ +6726,5 @@
> +    tokenStream.consumeKnownToken(TOK_NAME, modifier);
> +    if (!shouldParseLetDeclaration(parseDeclOut))
> +        return false;
> +
> +    // Unget the TOK_NAME of 'let' if not parsing a declaration.

Tricky contract :-P but OK.

::: js/src/frontend/Parser.h
@@ +796,5 @@
>  
> +    // Use when the current token is TOK_NAME and is known to be 'let'.
> +    bool shouldParseLetDeclaration(bool* parseDeclOut);
> +
> +    // Use when the lookahead token is TOK_NAME and might be 'let'. If a let

It looks like it's only called when the token is definitely `let`, not "might be".

::: js/src/jit-test/tests/parser/letContextualKeyword.js
@@ +8,5 @@
> +  }
> +  assertEq(log, "e");
> +}
> +
> +eval(`let x = 42; assertEq(x, 42);`);

Please also test `let` and `[...let] = []`, which I guess should work fine in sloppy mode.
Attachment #8678364 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/3bb8446a6d8d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1338277
No longer depends on: 1338277
Assignee: nobody → shu
You need to log in before you can comment on or make changes to this bug.