346749 - let declarations at function level should turn into var declarations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P4)

Description

[Jeff Walden [:Waldo]]

See also <http://lxr.mozilla.org/mozilla/source/js/src/jsparse.c#2934>.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsparse.c&rev=3.183&root=/cvsroot#2934

Comment 3

[Brendan Eich [:brendan]]

We might want to fix this for js1.8. Blake, do you have any time? Security via wrappers only takes precedence.

/be

Comment 4

[Brendan Eich [:brendan]]

Blake, I'll take this off your list if you are willing to give it up.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Sure, take it. I'm still in XOW-land.

Comment 6

[Jesse Ruderman]

Adding perf keyword based on the second paragraph of bug 410894 comment 2.

Comment 7

[Jesse Ruderman]

I mean bug 410894 comment 31.

Comment 8

[Brendan Eich [:brendan]]

There are enough closures formed at top level, however, that a quick fix here could easily de-optimize compared to the current translation of let to var at top level. More work analyzing outer name usage in closures to minimize reflection of compile-time lexical bindings into runtime Block object properties is needed. Not sure this will make 1.9 but we should think about it.

/be

Comment 9

[John Resig]

Bumping to JS1.9.

Comment 10

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.

Comment 11

[Brendan Eich [:brendan]]

I thought I updated this bug. At the newton ES4 working group meeting, we agreed to avoid a block implicitly scoping the body of a function or top-level program, which allows let x to shadow var x. This means let == var at top level.

So this bug should morph to request that SpiderMonkey no longer make a function body blocks.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 331885 [details] [diff] [review]
Fix

Looks good -- need tests?

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Pushed as http://hg.mozilla.org/mozilla-central/index.cgi/rev/24546515566f

As a note: we're now optimizing global variable access more aggressively in SpiderMonkey and var and let are both optimized where possible in function bodies, so this shouldn't be any sort of perf hit.

Comment 15

[Bob Clary [:bc] (inactive)]

mrbkap, how do i test this? current 1.9.1 and 7/31 trunk both agree with attachment 261359 [details]. I'm not sure what the changed/correct behavior is.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Bob, the easiest testcase should be:

function(){let k} == "function ( ) { var k }"

since that's the only behavior change from the patch here.

Comment 17

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/e0eece29c686
/cvsroot/mozilla/js/tests/js1_8/decompilation/regress-346749.js,v  <--  regress-346749.js
initial revision: 1.1