1167029 - Remove SpiderMonkey support for let blocks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

+++ This bug was initially created as a clone of Bug #1023609 +++

ES6 does not spec let blocks. We should remove them.

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

Any plans for a compatibility period where they remain exposed to privileged code so as to prevent breakage in add-ons, even after they're no longer available for use on the Web?

Comment 2

[Chris Peterson [:cpeterson]]

That's a good suggestion, but it doesn't remove this feature's maintenance burden in the SpiderMonkey code. Firefox has been logging console warnings for let extensions usage since Firefox 36 (bug 1102131), so add-on developers have had plenty of warning. :)

Comment 3

[Philip Chee]

There is a password protected MXR containing the source code for all extensions submitted to addons.mozilla.org. The SOP is to grep through this MXR looking for usage of let blocks and let expressions to judge the impact of removing these. I suspect that the usage is very low given how these were never promoted or publicized widely.

Comment 4

[Chris Peterson [:cpeterson]]

Here is a query for let blocks (and expressions) in AMO extensions. There are some hits. This query includes many false positives because MXR's regular expression syntax is limited.

https://mxr.mozilla.org/addons/search?case=1&tree=addons&string=let+%28

Comment 5

[Shu-yu Guo [:shu]]

I'm going to rip the bandaid off as ES6 lexical behavior already broke addons and |let (| is ambiguous with ES6, where it treats 'let' in that case as an identifier.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Remove support for let blocks.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Remove support for let blocks.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8678365 [details] [diff] [review]
Remove support for let blocks.

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

r=me, and I think the conflicts actually won't be terribly bad if you want to land this right away instead of waiting for my stack. But thanks for the reviews.

I didn't look at all the tests; assuming it's all as routine as the few I spot-checked.

::: js/src/frontend/Parser.cpp
@@ -4097,5 @@
> -    uint32_t begin = pos().begin;
> -
> -    MUST_MATCH_TOKEN(TOK_LP, JSMSG_PAREN_BEFORE_LET);
> -
> -    Node vars = variables(yieldHandling, PNK_LET, NotInForInit, nullptr, blockObj, DontHoistVars);

We are so close to being able to remove DontHoistVars now.

@@ -4123,5 @@
> -    handler.setLexicalScopeBody(block, expr);
> -
> -    TokenPos letPos(begin, pos().end);
> -
> -    return handler.newLetBlock(vars, block, letPos);

With my stack, this is the only remaining newLetBlock call site, so the method can be deleted from both Handler classes, and PNK_LETBLOCK can be removed as well.

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> Comment on attachment 8678365 [details] [diff] [review]
> Remove support for let blocks.
> 
> Review of attachment 8678365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, and I think the conflicts actually won't be terribly bad if you want
> to land this right away instead of waiting for my stack. But thanks for the
> reviews.
> 
> I didn't look at all the tests; assuming it's all as routine as the few I
> spot-checked.
> 

The test changes are either:

 - Straight up removals for fuzztests that basically only test let blocks, or reftests explicitly testing let blocks.
 - Change |let (x = e) { s }| to |{ let x = e; s; }|

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/let-block-support-has-been-removed/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5b900fc494

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b5b900fc494

Comment 15

[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#let_blocks