Closed Bug 1054357 (fix-let) Opened 6 years ago Closed 6 years ago

Update c-c to address non-backward compatible changes to JS let semantics

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 35.0

People

(Reporter: rkent, Assigned: aryx)

References

Details

Attachments

(3 files)

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.
Blocks: cc-backlog
Component: Composition → Backend
Summary: Update c-c to address non-backward compatible changes to JS let semantics → (fix let) Update c-c to address non-backward compatible changes to JS let semantics
Alias: fix-let
Summary: (fix let) Update c-c to address non-backward compatible changes to JS let semantics → (fix-let) Update c-c to address non-backward compatible changes to JS let semantics
Summary: (fix-let) Update c-c to address non-backward compatible changes to JS let semantics → Update c-c to address non-backward compatible changes to JS let semantics
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.
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).
Attachment #8490408 - Flags: review?(standard8)
Depends on: 1067969
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 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.
Looks great. Is there a try run with this?
Attachment #8490597 - Flags: review?(standard8) → review+
Attachment #8490408 - Flags: review?(standard8) → review+
Pushed with that double "wh" dropped.

https://hg.mozilla.org/comm-central/rev/507a692b707c
https://hg.mozilla.org/comm-central/rev/e5a9b312e9da
Assignee: nobody → archaeopteryx
OS: Windows 8.1 → All
Hardware: x86_64 → All
Target Milestone: --- → Thunderbird 35.0
Duplicate of this bug: 1067969
Blocks: 1001090
No longer depends on: 1067969
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
Attached patch Part 3Splinter Review
I thinks this patch is needed for mozmill tests.
Attachment #8490607 - Flags: review?(standard8)
Attachment #8490607 - Flags: review?(standard8) → review+
--- 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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1069819
No longer blocks: cc-backlog
You need to log in before you can comment on or make changes to this bug.