Open Bug 1069841 Opened 10 years ago Updated 2 years ago

"Error: ReferenceError: can't access let declaration `tags' before initialization" despite let declaration earlier

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows 8.1
defect

Tracking

()

People

(Reporter: aryx, Unassigned)

Details

Attachments

(1 file)

Attached file file with issue
With the file attached (part of Thunderbird), I get the following error:

Error: ReferenceError: can't access let declaration `tags' before initialization
Source File: chrome://messenger-newsblog/content/feed-parser.js
Line: 133

But |tags| is defined on the top level of the method:
    let tags = this.childrenByTagNameNS(channel, nsURI, "title");

The error is thrown for:
      tags = this.childrenByTagNameNS(itemNode, FeedUtils.FEEDBURNER_NS, "origLink");

An error should be thrown for line 278 (no idea if the error is thrown on the first or second run of the loop):
      let tags = this.childrenByTagNameNS(itemNode, nsURI, "category");

Sorry if I got it wrong.
Running the script with the standalone js parser with the instructions from bug 1001090 comment 151 doesn't throw an error.
(In reply to Archaeopteryx [:aryx] from comment #0)
> Created attachment 8492077 [details]
> file with issue
> 
> With the file attached (part of Thunderbird), I get the following error:
> 
> Error: ReferenceError: can't access let declaration `tags' before
> initialization
> Source File: chrome://messenger-newsblog/content/feed-parser.js
> Line: 133
> 
> But |tags| is defined on the top level of the method:
>     let tags = this.childrenByTagNameNS(channel, nsURI, "title");
> 
> The error is thrown for:
>       tags = this.childrenByTagNameNS(itemNode, FeedUtils.FEEDBURNER_NS,
> "origLink");
> 
> An error should be thrown for line 278 (no idea if the error is thrown on
> the first or second run of the loop):
>       let tags = this.childrenByTagNameNS(itemNode, nsURI, "category");
> 
> Sorry if I got it wrong.

From just reading the file, I think what happens here is that another |let tags| is defined on line 275, and since lets are hoisted to the top of their block in ES6, this is in the same scope as the use on line 133. The gotcha here is that even though the binding itself is hoisted (e.g., if I closed over 'tags' at line 133, I get the 'tags' binding declared on line 275), it can't be accessed until line 275 is executed.

(In reply to Archaeopteryx [:aryx] from comment #1)
> Running the script with the standalone js parser with the instructions from
> bug 1001090 comment 151 doesn't throw an error.

Right, because this is a runtime error, not a parser one.
It probably thinks there is another 'tags' variable local to the 'for' loop in which it is declared and this second copy is really used before it is declared. Removing the second 'let' declaration makes the engine understand this is actually the same variable as declared on the top level of the function.
So maybe the engine could warn about "shadowing" of the variable in this case, in constructs like
let x
for (...) {
  let x
}
So a minimal testcase here is basically:

  (function() {
    let x = 5;
    {
      alert(x);
      let x = 7;
    }
  })()

right?
Attachment #8492077 - Attachment mime type: application/x-javascript → application/javascript
Attachment #8492077 - Attachment mime type: application/javascript → text/plain
Yes, and the error says "can't access let declaration 'x' before initialization" on the line of "alert(x)".
(In reply to Boris Zbarsky [:bz] from comment #5)
> So a minimal testcase here is basically:
> 
>   (function() {
>     let x = 5;
>     {
>       alert(x);
>       let x = 7;
>     }
>   })()
> 
> right?

Yes. It is supposed to error on alert(x) because that's a hoisted use of the inner x.
I think this is working as designed. It's just that the spaghetti code in newsblog obscures the location of the hoisted let.
(In reply to Boris Zbarsky [:bz] from comment #5)
> So a minimal testcase here is basically:

More like this (i.e. a set of the variable before its declaration, not a use):

  (function() {
    let x = 5;
    {
      x = 3;
      let x = 7;
    }
  })()

Just moving the last |let| up to the first initialization is the right fix.  The first assignment is of a loop-variant value, so clearly the |let| should be there, and the assignment targets the variable inside the loop, not the pre-existing one outside it.
Actually not. In the case described in comment 0, the right fix was to remove the 'let' at 'x = 7'. The top level variable was the right one to be used.
But it is true that in some cases, moving 'let' to 'x = 3' would be the right fix. However, why would anybody use the same name for the local variable? I think a warning about "shadowing" would be in order.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: