If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Thunderbird 3.0b3

Status

Thunderbird
Mail Window Front End
--
blocker
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({regression})

Trunk
Thunderbird 3.0b3
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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.
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.
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.
Assignee: general → nobody
Component: JavaScript Engine → Mail Window Front End
Product: Core → Thunderbird
QA Contact: general → front-end
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...
(note this bug still belongs in Thunderbird anyhow, as the let is being redeclared by a var)
(Assignee)

Comment 5

9 years ago
Created attachment 371483 [details] [diff] [review]
The fix

Small fix for mail3PaneCommands.js - scope the case sections a bit and switch from var to let.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #371483 - Flags: review?(bienvenu)
(Assignee)

Comment 6

9 years ago
(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.
Why don't you just do:

let folder;

case ...:

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

This avoids any redeclaration.

Comment 8

9 years ago
Comment on attachment 371483 [details] [diff] [review]
The fix

either this or the other suggestion is fine with me.
Attachment #371483 - Flags: review?(bienvenu) → review+
(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
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...
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).
(Assignee)

Comment 12

9 years ago
(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.
filed bug 487264 on the var vs. let thing.
(Assignee)

Comment 14

9 years ago
Checked in: http://hg.mozilla.org/comm-central/rev/8b865f129cb3
Group: mozilla-confidential
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
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
(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. :-(
(Assignee)

Comment 17

9 years ago
(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.
Group: mozilla-confidential
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
> 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

9 years ago
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.
You need to log in before you can comment on or make changes to this bug.