Closed Bug 1167029 Opened 9 years ago Closed 9 years ago

Remove SpiderMonkey support for let blocks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: cpeterson, Assigned: shu)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

+++ 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?
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. :)
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.
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
Keywords: addon-compat
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.
Attached patch Remove support for let blocks. (obsolete) — Splinter Review
Attachment #8677806 - Flags: review?(jorendorff)
Blocks: 932517
Attachment #8678365 - Flags: review?(jorendorff)
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+
(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
Closed: 9 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.