487202 - Thunderbird trunk builds broken: mail3PaneWindowCommands.js - Error: redeclaration of let folder

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mark Banner (:standard8)]

The tracemonkey landing on trunk of bug 452498 seems to have broken Trunk builds of Thunderbird. The error is:

Error: redeclaration of let folder
Source File: chrome://messenger/content/mail3PaneWindowCommands.js
Line: 413, Column: 12
Source Code:
        var folder = GetSelectedMsgFolders()[0];

This is caused by the following (abbreviated) code:

switch (command)
{
  case "cmd_deleteFolder":
    var folders = GetSelectedMsgFolders();
    if (folders.length) {
      var folder = folders[0];
      if (folder.server.type == "nntp")
        return false; // Just disable the command for news.
      else
        return CanDeleteFolder(folder);
    }
    return false;
  case "button_archive":
    let folder = GetLoadedMsgFolder();
    return GetNumSelectedMessages() > 0 && folder &&
           !(IsSpecialFolder(folder,
                             Components.interfaces.nsMsgFolderFlags.Archive,
                             true));
  case "cmd_emptyTrash":
    var folder = GetSelectedMsgFolders()[0];
    return folder && folder.server.canEmptyTrashOnExit ?
                     IsMailFolderSelected() : false;
}

If I change the cmd_emptyTrash case to use folder1 rather than folder then it fixes the build.

From https://bugzilla.mozilla.org/show_bug.cgi?id=452498#c191 it is unclear if this was an intended change or not.

If it was an intended change then I'm quite happy to move to Thunderbird and fix up the code (which isn't pretty anyway), otherwise, please fix TM.

Comment 1

[Nochum Sossonko [:Natch]]

This is intended, see comment 248 in that bug, where only the catch redeclaration was a bug (bug 487209) while the other testcases (var shadowing let) were just fixed. IMHO it's pretty clear in that comment as well.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, this is intended. The comment in the bug is showing that let can shadow var if it's nested, but let and var at top level will cause a redeclaration error. Sorry if I don't get quite the right component.

Comment 3

[Nochum Sossonko [:Natch]]

Does that mean that a variable can't be declared with var then redeclared with let in the top level? var can be used to redeclare a var, while let can't? I thought the intention was that var can't redeclare (or shadow) a let variable not vice-versa...

Comment 4

[Nochum Sossonko [:Natch]]

(note this bug still belongs in Thunderbird anyhow, as the let is being redeclared by a var)

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix

Small fix for mail3PaneCommands.js - scope the case sections a bit and switch from var to let.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> Created an attachment (id=371483) [details]
> The fix
> 
> Small fix for mail3PaneCommands.js - scope the case sections a bit and switch
> from var to let.

Note we may have other broken areas, but hopefully our testers will find those.

Comment 7

[Nochum Sossonko [:Natch]]

Why don't you just do:

let folder;

case ...:

folder = ...
break;
case ...:
folder = ...

This avoids any redeclaration.

Comment 8

[David :Bienvenu]

Comment on attachment 371483 [details] [diff] [review]
The fix

either this or the other suggestion is fine with me.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Does that mean that a variable can't be declared with var then redeclared with
> let in the top level?

Yes.

> var can be used to redeclare a var, while let can't?

We (Ecma TC39) are future-proofing let to allow for optional type annotations and subtly different use-before-set (hoisting, let rec) semantics. All in all this means no redeclaration.

> I
> thought the intention was that var can't redeclare (or shadow) a let variable
> not vice-versa...

That's one intention. The vice versa would be { var x; { let x; } } which is alowed. The case you mean is not vice versa: { var x; let x; } and it's not allowed.

/be

Comment 10

[Nochum Sossonko [:Natch]]

IMHO Javascript is supposed to be a simple easy-to-use language, why such complications? I can understand not allowing a redeclaration of let with var, but var x;let x; should be allowed. Suppose I want to modularize my script, one of the common methods is:

var myScript; //does nothing if it's been previously declared
if (myScript) alert("Incompatible"); //myScript already in use...
else myScript = {};

now if I do that with let I may run in to a redeclaration error...

Comment 11

[Nochum Sossonko [:Natch]]

Basically what I'm saying is that there should be a fail-safe way to create a global object. If a redeclaration of let x with var x is considered an error, at least var x; let x; should still be fail-safe. The reason the |var myScript| is there is because it's considered an error (or at least a strict warning) to reference an undefined variable (IIRC).

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #7)
> Why don't you just do:
> 
> let folder;
> 
> case ...:
> 
> folder = ...
> break;
> case ...:
> folder = ...
> 
> This avoids any redeclaration.

It would also provide potential for someone to use the folder variable in the
expectation that it contained something. Whilst this would most likely be found
at the debugging stage I'd prefer to be explicit.

Comment 13

[Nochum Sossonko [:Natch]]

filed bug 487264 on the var vs. let thing.

Comment 14

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/8b865f129cb3

Comment 15

[Brendan Eich [:brendan]]

What is so simple about var and let being allowed to collide? How does that make anything clearer, never mind easier to implement or future-proof?

JS tolerating undefined property references or allowing implicit creation of global variables by assignment are bugs in the language, not ease-of-use virtues. Arguments for like bugs do not wash.

/be

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(I don't know why this is mozilla-confidential now: standard8?)

To be overly clear: I am not arguing against this change in the abstract, and I believe that the future-proofing is the right thing.  I am just nervous about making this change in the late stages of 3.5, after we've made extension compatibility promises post-b3.  I don't think very many people realized that upvar would have script compatibility impact until now. :-(

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> (I don't know why this is mozilla-confidential now: standard8?)

Sorry mike, must have been slip of the fingers on the touchpad.

Comment 18

[Brendan Eich [:brendan]]

Sorry, my comment 15 was in reply to comment 11. See bug 487264, not RESOLVED INVALID, or try a post-upvar2 js shell:

$ ./Darwin_DBG.OBJ/js
js> var x;
js> let x;
js> var x; let x
js> let x; var x 
js> 

No errors.

The issue shaver is talking about in comment 16 is that upvar2 made it an error for var to "shadow" let, e.g. { let x; { var x; } }. This was almost always not what was meant, since the var would be hoisted and any *use* of x in the inner block would refer to the let binding.

I still hope we can hang tough on this change. I'm sorry it broke Thunderbird, but at least that's in our VCS and easily fixed.

/be

Comment 19

[Brendan Eich [:brendan]]

> Sorry, my comment 15 was in reply to comment 11. See bug 487264, not RESOLVED

s/not RESOLVED/now RESOLVED/ of course.

The other way around, { var x; { let x; } } is fine still, as it should be. And let == var at top level. So we're really only talking about the pseudo-shadowing of a let by an inner block var of the same name. Which is probably a latent bug.

/be

Comment 20

[David :Bienvenu]

It was a very minor thing for Thunderbird, so no worries there. But we may not be a representative sample since our code is relatively let-free.