487209 - erroneous redeclaration of let ... with try {...} catch(e) {var e...}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

ecma_3/ExecutionContexts/regress-448595-01.js and js1_5/Regress/regress-146596.js

fail with redeclaration of let ... errors due to 

try {...} catch(e) { var e ...}

regressed by upvar2 bug 452498

Comment 1

[Brendan Eich [:brendan]]

Dammit, can't restrict this case -- catch makes an implicit let, we must tolerate the oddly-hoisted var e. It's easier to handle because catch does not hoist.

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

I think we should permit var-vs-let for FF3.5, since we made compatibility promises to extension authors as of b3.  Is this change "merely" virtuous strictness, or is it required for something else in upvar2?

Comment 3

[Brendan Eich [:brendan]]

var vs. let is not only future-proofing, it helped avoid a conundrum in one-pass name binding judgments in upvar2. We don't have time to redo using two passes and doing so has risks for perf as well as correctness.

Maybe a fresh look would find a way, but var vs. let is another bug than this one, which clearly must be fixed.

Again the problem is hoisting.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 371758 [details] [diff] [review]
fix

I worried to brendan in person than this will lead to inconsistent results with

(function() { {let x; var x }})
vs.
(function() { {let x; try{}catch(x){var x} }})

Comment 6

[Brendan Eich [:brendan]]

Attachment: testcase

Comment 7

[Brendan Eich [:brendan]]

Attachment: testcase with outer let declaration, showing the bug mrbkap pointed out

Comment 8

[Brendan Eich [:brendan]]

Attachment: variation on testcase with outer let, this time a let block

Comment 9

[Brendan Eich [:brendan]]

Attachment: fix, v2

C++ optional parameters ftw.

/be

Comment 10

[Brendan Eich [:brendan]]

Testcases attached in case there's something I reconstructed that's not in the testsuite.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: testcase nesting catches to show how smart mrbkap is

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix, v3

Yeah, the testsuite definitely does not cover the last case.

/be

Comment 13

[Brendan Eich [:brendan]]

Fixed in tm:

http://hg.mozilla.org/tracemonkey/rev/d9eafbebdd78

/be

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d9eafbebdd78

Comment 15

[Robert Sayre]

I botched the 1.9.1 checkin comment for this, but landed it correctly:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58633e0035ee