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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

let is currently available in javascript;version=1.7 and 1.8.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Let let be versionless.

Comment 4

[Jason Orendorff [: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).

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Treat let as a context keyword in sloppy mode and make it versionless.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Treat let as a context keyword in sloppy mode and make it versionless.

Comment 7

[Jason Orendorff [: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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb8446a6d8d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bb8446a6d8d

Comment 10

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/let-statement-no-longer-requires-explicit-javascript-version-in-non-strict-mode/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44d5e0a25719

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44d5e0a25719

Comment 14

[Florian Scholz (Open Web Docs)]

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