Remove SpiderMonkey support for let blocks

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: shu)

Tracking

(Blocks 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

ES6 does not spec let blocks. We should remove them.
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?
(Reporter)

Comment 2

4 years ago
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

4 years ago
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.
(Reporter)

Comment 4

4 years ago
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
No longer blocks: es6:let
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Attachment #8677806 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Blocks: 932517
(Assignee)

Comment 7

4 years ago
Attachment #8678365 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Attachment #8677806 - Attachment is obsolete: true
Attachment #8677806 - Flags: review?(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.
Attachment #8678365 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 9

4 years ago
(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; }|
https://hg.mozilla.org/mozilla-central/rev/a6f01cc9c740
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee: nobody → shu
You need to log in before you can comment on or make changes to this bug.