1054357 - (fix-let) Update c-c to address non-backward compatible changes to JS let semantics

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

From post to m.d.platform 2014-08-13:

Hello all,

We are in the process of making JS 'let' semantics ES6-compliant in
SpiderMonkey. I hope to land bug 1001090 sometime this month or early next
month (I've been told there's a B2G uplift on Sept 1st), which is one of many
for ES6 'let'-compliance. It changes 'let' semantics in two non-backward
compatible ways:

  1. 'let' bindings may no longer redeclare existing bindings in function
     scope. (In ES6, this is also true for global code, but global 'let'
     changes are distinct from bug 1001090 and will be addressed by bug
     589199.)

     Currently, 'let' bindings at the function body-level are semantically
     equivalent to 'var' bindings. This means that the following JS are legal:

       function f(x) { let x = x || ""; }
       function g()  { let x = 42; let x = 43; }

     When bug 1001090 lands, these will be syntax errors.

  2. 'let' bindings may not be accessed in any fashion before initialization.
     Initialization means execution of the |let x| or |let x = rhs|
     forms. Before initialization, the 'let' bindings are in a "temporal dead
     zone" (TDZ). The TDZ is enforced dynamically, throwing a ReferenceError.

     When bug 1001090 lands, the following JS throw:

       function f() { x = 42; let x; }
       function g() { function inner() { x = 42; } inner(); let x; }
       function h() { switch (c) { case 0: let x; break; default: x = 42; break; } }

These changes wreak havoc on our existing chrome JS, largely due to item 1
above. It will be a rough landing, so I am raising awareness of these breaking
changes now. The changes will likely initially land under a Nightly-only
#ifdef.

I can fix (or can find someone to fix) code that is in mozilla-central. But
code that is outside of that tree will be problematic. Namely, Gaia and addons.
Can someone own updating Gaia JS to be ES6-compliant? It is unclear to me what
should be done about addons.

The takeaway for developers is to refrain from using the patterns I've listed
above starting now in any new code, before bug 1001090 lands. The patterns in
item 2 above are bad form and should never have been used, but those from item
1 have seen a fair bit of use.

Comment 1

[:aceman]

I like the changes. I always expected the semantics would be in this (new) way from the start on and always created code because other languages behave like this.
Let's see if it finds any bugs.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch for .js and .jsm files, v1

This is the patch for the .js and .jsm files. For the .xul and .xml file, I still have to write a awk script (of somebody else fixes this, awesome).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8490408 [details] [diff] [review]
patch for .js and .jsm files, v1

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

::: mail/test/mozmill/attachment/test-attachment.js
@@ +42,2 @@
>    wh.installInto(module);
>  

This 'wh' should be removed because window-helpers is already loaded.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8490408 [details] [diff] [review]
patch for .js and .jsm files, v1

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

::: mail/test/mozmill/attachment/test-attachment.js
@@ +37,5 @@
>    let wh = collector.getModule('window-helpers');
>    wh.installInto(module);
>    let composeHelper = collector.getModule('compose-helpers');
>    composeHelper.installInto(module);
> +  wh = collector.getModule('window-helpers');

Sorry, I meant this 'wh' line.

Comment 5

[:aceman]

Looks great. Is there a try run with this?

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: part 2, v1

Comment 7

[Mark Banner (:standard8)]

Pushed with that double "wh" dropped.

https://hg.mozilla.org/comm-central/rev/507a692b707c
https://hg.mozilla.org/comm-central/rev/e5a9b312e9da

Comment 9

[:aceman]

I think this one will still break some tests:
   402     this.waitingList.delete(windowType);
   403     let windowType = this.waitingForClose;

It is at https://hg.mozilla.org/comm-central/file/e5a9b312e9da/mail/test/mozmill/shared-modules/test-window-helpers.js#l402

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3

I thinks this patch is needed for mozmill tests.

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/f1e58fccab14

Comment 12

[:aceman]

--- a/mail/test/mozmill/shared-modules/test-window-helpers.js
+++ b/mail/test/mozmill/shared-modules/test-window-helpers.js
@@ -396,18 +396,18 @@ var WindowWatcher = {
     let didDisappear = (this.waitingList.get(this.waitingForClose) == null);
+    let windowType = this.waitingForClose;
     this.waitingList.delete(windowType);
-    let windowType = this.waitingForClose;
     this.waitingForClose = null;

Yes, this seems to change behaviour a bit. I wonder if it fixes any bugs or intermittent problems in mozmill tests.