using for(in) on a function argument yields "undefined" keys.

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: henri_torgemane, Assigned: lhansen)

Tracking

Details

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: 

Smallest testcase:
var obj={a:1,b:2};
function test(o) {
  var s="";
  for (var k in o) s+=k;
  return s;
}
print(test(obj))

expected result is "ab", but actual result is "undefinedundefined".

Reproducible: Always
(Reporter)

Comment 1

11 years ago
This occurs because ESC uses getlocal/setlocal to access function-scoped variables, but cgForInStmt() always uses its index as a named variable.

If the loop index variable is scoped outside of the function, things work:
var obj={a:1,b:2}
var loopIndex;
function test(o) {
  var s="";
  for (loopIndex in o) s+=loopIndex;
  return s;
}
print(test(obj));

I have a broken fix, which involves commenting out every statement that mentions the variable "name" in cgForInStmt(), and instead calling cgSetProp(ctx, init); right before the call to cgStmt.
It's a fix because it resolves the loop index correctly.
It's broken because it only accepts the form "for (index in expr)". even "for (var index in expr)" breaks. 
I suspect there's a neat function somewhere that would convert "init" into something cgSetProp can consume, but I don't know the codebase enough.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 436191
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: nobody → lhansen
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
IMO for-in handling in the compiler is pretty much broken, needs a rewrite.  Sigh.
(Assignee)

Comment 4

11 years ago
Created attachment 323662 [details] [diff] [review]
Patch

Broad reengineering of for-in loops; this should handle most cases (it handles all the cases in this bug and in the dup) and should be set up for eg proper destructuring support when we get to that.

Also some other cleanup here, this fixes the abcencoder (heavy use of for-in there) and adds some editorializing comments to the ast file, where it also fixes some array-structural types.
Attachment #323662 - Flags: review?(jodyer)
(Assignee)

Comment 5

11 years ago
Created attachment 323663 [details]
Patch for esc/bin

Complements the source patch in the previous attachment.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 424543

Comment 7

11 years ago
Comment on attachment 323662 [details] [diff] [review]
Patch

Agree with your comments regarding TempName and PropName. TempName should removed and PropName peeled to Name.
Attachment #323662 - Flags: review?(jodyer) → review+
(Assignee)

Comment 8

11 years ago
Pushed.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.