Closed Bug 346749 Opened 14 years ago Closed 11 years ago

let declarations at function level should turn into var declarations


(Core :: JavaScript Engine, defect, P4)






(Reporter: Waldo, Assigned: mrbkap)




(Keywords: perf, testcase)


(1 file)
Assignee: general → mrbkap
Priority: -- → P3
Target Milestone: --- → mozilla1.8.1
Summary: |var x=5;let x=7; print(this.x)| prints 7, not 5 → let declarations at top level are turned into var declarations
Duplicate of this bug: 377267
We might want to fix this for js1.8. Blake, do you have any time? Security via wrappers only takes precedence.

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

Blocks: js1.8
Flags: wanted1.9+
Target Milestone: mozilla1.8.1 → mozilla1.9 M11
Sure, take it. I'm still in XOW-land.
Adding perf keyword based on the second paragraph of bug 410894 comment 2.
Keywords: perf
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.

Bumping to JS1.9.
Blocks: js1.8.5
No longer blocks: js1.8
Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.
Flags: wanted1.9.1?
Priority: P3 → P4
Flags: wanted1.9.1? → wanted1.9.1+
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.

Summary: let declarations at top level are turned into var declarations → let declarations at function level should turn into var declarations
Attached patch FixSplinter Review
Attachment #331885 - Flags: review?(brendan)
Comment on attachment 331885 [details] [diff] [review]

Looks good -- need tests?

Attachment #331885 - Flags: review?(brendan) → review+
Pushed as

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.
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 457618
Flags: in-testsuite?
Flags: in-litmus-
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.
Bob, the easiest testcase should be:

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

since that's the only behavior change from the patch here.
Keywords: testcase
/cvsroot/mozilla/js/tests/js1_8/decompilation/regress-346749.js,v  <--  regress-346749.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.