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

Status: RESOLVED FIXED

(Tamarin Graveyard :: Self-hosting compiler (ESC), defect)

Description

[Henri Torgemane]

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

Comment 1

[Henri Torgemane]

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.

Comment 3

[Lars T Hansen]

IMO for-in handling in the compiler is pretty much broken, needs a rewrite.  Sigh.

Comment 4

[Lars T Hansen]

Attachment: 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.

Comment 5

[Lars T Hansen]

Attachment: Patch for esc/bin

Complements the source patch in the previous attachment.

Comment 7

[Jeff Dyer]

Comment on attachment 323662 [details] [diff] [review]
Patch

Agree with your comments regarding TempName and PropName. TempName should removed and PropName peeled to Name.

Comment 8

[Lars T Hansen]

Pushed.