Bug 12878 – Support for each (var in collection) syntax

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Justin Haygood]

JavaScript 1.6 supports for each ( var in collection ) syntax, like:

var array = ["foo","bar"];

for each ( var string in array )
{
document.write( string + "<br>" )
}

Should result in:

foo<br>
bar<br>

Currently, it gets an error.

This is one area where we can make the first effort at supporting E4X, but this is useful in any JavaScript/DOM collection as well.

[Justin Haygood]

Created attachment 13362 [details]
mini test case

Test case for bug. Works in Firefox, Fails elsewhere.

[Justin Haygood]

Created attachment 13364 [details]
Adds support for for..each..in

This is an initial implementation of for...each...in, supported by Mozilla Firefox currently. This also is required for support of E4X, but is useable elsewhere, such as iterating through arrays more naturally.

[Alexey Proskuryakov]

Comment on attachment 13364 [details]
Adds support for for..each..in

r- for purely stylistic issues discussed via IRC (star and comma positioning, assert vs. ASSERT). And clearly, this cannot be landed to trunk before the stabilization period ends.

A couple additional stylistic notes:

+        if (varDecl)
+    s << "var " << varDecl;

  Indentation looks broken here.

+            if ((c.complType() == Break) && ls.contains(c.target()))
+            break;

  And here.

+    JSValue* e;
...
+    e = expr->evaluate(exec);

  There is no need to declare local variables at the beginning of the function in C++. Also, this and other variables may benefit from a more descriptive name.

[Justin Haygood]

Created attachment 13368 [details]
Implements for each ( var <identifier> in <collection> )

Same as above mostly, except:
 * Removes unused variables
 * Follows code style guidelines

[Mark Rowe (bdash)]

Comment on attachment 13368 [details]
Implements for each ( var <identifier> in <collection> )

+void ForEachInNode::streamTo(SourceStream &s) const
+{
+    s << SourceStream::Endl << "for each (";
+        if (varDecl)
+    s << "var " << varDecl;
+        else
+    s << lexpr;
+

Indentation is incorrect here.

+            if ((c.complType() == Break) && ls.contains(c.target()))
+            break;

And again here.

The ForEachInNode constructor should set varDecl and lexpr in the initializer list, and the formatting of this list should match the coding style guidelines.

It looks as though varDecl is only used in streamTo, which means that "for each (a in b)" and "for each (var a in b)" are treated identically when they should have different semantics.

+        JSValue* str = v->get(exec, name);

"str" seems to be a bit misleading for the new use of this variable, I gather it's now the value of the property retrieved from the iterator.

It seems that almost all of the code in ForEachInNode::execute is duplicated from ForInNode which is less than ideal.  It's also not clear why supporting only a single feature from the E4X specification is desirable.

[Mark Rowe (bdash)]

Note that this feature comes from ECMA 357 (<http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-357.pdf>), the ECMAScript for XML specification.  You should verify that your implementation matches this specification and definitely include test cases to support this.

[Justin Haygood]

Created attachment 15782 [details]
Updated Patch

1. Implemented by subclassing from ForInNode, so that it doesn't duplicate code effort
2. Testcases are on the way
3. Doesn't cause any regressions
4. This is an experimental patch. Questions/Concerns, etc.. are very much welcome!

[Mark Rowe (bdash)]

Comment on attachment 15782 [details]
Updated Patch

I pointed out a few issues in the patch to Justin via IRC.  There are numerous coding style inconsistencies, and the redeclaration of ForInNode's instance variables in the subclass is wrong.  I also suggested that the implementation of streamTo could easily be shared between the two classes which should allow ForInNode's members to remain private.

[Mark Rowe (bdash)]

Oh, and test cases!  Test cases, test cases, test cases! ;-)

[Justin Haygood]

Created attachment 15790 [details]
for each...in test case

Testcase

[Justin Haygood]

Created attachment 15791 [details]
Updated Patch for for each...in

This has been modified to follow code style guidelines.

It also merges streamTo between ForInNode and ForEachInNode and restores the "private" in ForInNode

[Mark Rowe (bdash)]

Comment on attachment 15791 [details]
Updated Patch for for each...in

There is still incorrect whitespace and * placement in many places.  You should also include a test case as part of the patch, along with the related changelog entry.

+                                            Node *n = $4->nodeInsideAllParens();

+JSValue* ForInNode::getIteratorValue( ExecState*, JSObject* inValue, const Identifier name )

+      JSValue *str = getIteratorValue(exec,v,name);

+JSValue* ForEachInNode::getIteratorValue(ExecState* exec, JSObject* inValue, const Identifier name )

+    virtual JSValue* getIteratorValue( ExecState *exec, JSObject *inValue, const Identifier name ) KJS_FAST_CALL;


+    ForEachInNode(Node *l, Node *e, StatementNode *s) KJS_FAST_CALL;
+    ForEachInNode(const Identifier &i, AssignExprNode *in, Node *e, StatementNode *s) KJS_FAST_CALL;
+  protected:
+    virtual JSValue* getIteratorValue( ExecState *exec, JSObject *inValue, const Identifier name ) KJS_FAST_CALL;

[Cameron Zwarich (cpst)]

*** Bug 20728 has been marked as a duplicate of this bug. ***

[Gavin Barraclough]

Our goal is ES-262 compliance, and I don't think it looks "for each" looks like it's going to make it into ES6.  As such, I don't think we plan to support this.  If "for each" makes it onto the standardization track, we should revisit this.