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

Decompilation of function with "let" expression in for's initial-expression does not compile

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: brendan)

Tracking

(Blocks: 1 bug, {testcase, verified1.8.1})

Trunk
mozilla1.8.1
testcase, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 ?
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
> function () { for ((let (y) 3); ;) { } }
function () { for (let (y) 3;;) { } }

> function () { for (let (y) 3;;) { } }
SyntaxError on line 1: missing variable name

According to http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Statements:for the first thing in a for loop can be an expression *or* a variable declaration, but not any statement.  So I guess the reason the second doesn't compile is that the compiler sees the "let" and thinks "this must be a variable declaration rather than an expression", but then it fails to compile as a variable declaration because it's written as a "let" expression rather than a "let" statement.

I can imagine this being fixed in one of two ways:
(1) Make the decompiler not drop the parens when decompiling the first.
(2) Make the compiler accept the second, since it seems to me that the second should be legal.
(Reporter)

Updated

11 years ago
Summary: Decompilation of function with "let" in for's initial-expression does not compile → Decompilation of function with "let" expression in for's initial-expression does not compile
(Assignee)

Comment 1

11 years ago
Created attachment 236572 [details] [diff] [review]
fix

This is important not only for decompiler hygiene, but for JS1.7 semantics, which should match the ES4 wiki (which is being clarified on this point).

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #236572 - Flags: review?(mrbkap)
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1

Updated

11 years ago
Attachment #236572 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 2

11 years ago
Fixed on trunk.

/be
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #236572 - Flags: approval1.8.1?
js> function() { let x = 5; while (x-- > 0) { for (let x = x, q = 5;;); } }
Segmentation fault

I'm 99% sure it was the checkin for this bug that did it.
(Assignee)

Updated

11 years ago
Attachment #236572 - Flags: approval1.8.1?
(Assignee)

Comment 4

11 years ago
Thanks for catching that -- I should have seen it.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

11 years ago
I backed out.  The problem, of course, is that for (let i = 0; ...) is not the same as let i; for (i = 0; ...).  But re-using Statement to process the let decl or expr in the first part of a for loop will morph the nearest enclosing block into a lexical scope.

/be
Status: REOPENED → ASSIGNED
(Assignee)

Updated

11 years ago
Blocks: 336378
(Assignee)

Comment 6

11 years ago
Created attachment 236589 [details] [diff] [review]
much simpler, and correct, fix

The grammatical type of the thing at the front of a for loop head is expression, not statement, so the whole original patch that tried to use Statement was misconceived.  This may be as confusing for users as for implementors.  The three let forms are not anywhere near the same in meaning.  At least the one that looks like var resembles var otherwise, and the let(head)... block and expression forms look new and different.

/be
Attachment #236572 - Attachment is obsolete: true
Attachment #236589 - Flags: review?(mrbkap)

Updated

11 years ago
Attachment #236589 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

11 years ago
Attachment #236589 - Flags: approval1.8.1?
(Assignee)

Comment 7

11 years ago
Fixed on trunk, for good.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 8

11 years ago
Checking in regress-350991.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-350991.js,v  <--  regress-350991.js
initial revision: 1.1
Flags: in-testsuite+

Comment 9

11 years ago
verified fixed 1.9 20060903 windows/mac*/linux
Status: RESOLVED → VERIFIED

Comment 10

11 years ago
Brendan - quick risk/reward thoughts for 1.8 branch?
(Assignee)

Comment 11

11 years ago
Comment on attachment 236589 [details] [diff] [review]
much simpler, and correct, fix

Sorry, I should have said: this is essentially a two-line addition to handle an unhandled case that is required in ES4/JS2, and that ought to be in JS1.7 along with the rest of 'let'.  It reuses LetBlock, which is used in exactly the same way in the general 'let x...' case, and just needs to handled here in the start of a for loop head.  The risk is as close to zero as any, since it calls existing code in a consistent way. The reward is JS1.7 not being broken in this 'for (let (...) ...; ...)' case.

/be

Comment 12

11 years ago
Comment on attachment 236589 [details] [diff] [review]
much simpler, and correct, fix

a=schrep for drivers given comment 11
Attachment #236589 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 13

11 years ago
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1

Comment 14

11 years ago
verified fixed 1.8 20060906 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1
You need to log in before you can comment on or make changes to this bug.