Witness at http://rgov.org/safaritest.html If you write the following code: function foo() { var x = "a", y = "b", z = "c"; } alert(foo.toString()); You'll get different results with the current shipping Safari 3: function foo() { var x = "1", y = "2", z = "3"; } and the latest build (30051): function foo() { var (x = "1", y = "2"), z = "3"; } As you can see, it tries to add parentheses around the assignments, but this is not valid syntax. Consequently, the printed code cannot actually be used.
I added some debug info to my nodes2string.h to help trace this: function foo() [ScopeNode] { [VarStatementNode] var [CommaNode] ( [CommaNode] [AssignResolveNode] x = [StringNode] "a" [/StringNode] [/AssignResolveNode] , [AssignResolveNode] y = [StringNode] "b" [/StringNode] [/AssignResolveNode] [/CommaNode] ), [AssignResolveNode] z = [StringNode] "c" [/StringNode] [/AssignResolveNode] [/CommaNode] ; [/VarStatementNode] }[/ScopeNode] It looks like where the first faulty parenthesis is going to be added, the needsParens check in SourceStream::operator<<(const Node* n) is true because 1. m_precedence = 17 2. n->precedence() = 18 In other words, it's because the CommaNode's precedence is greater than the AssignResolveNode. Someone please double check this for me but I think the function should be rewritten as: ----- SourceStream& SourceStream::operator<<(const Node* n) { if (!n) { m_precedence = PrecExpression; return *this; } bool needParens = (m_precedence < PrecAssignment && n->precedence() > m_precedence) || (m_atStartOfStatement && n->needsParensIfLeftmost()); // and so on ----- (I moved the n == NULL check to the top because I think it could cause a NULL pointer dereference as it stands in the current build.) Furthermore, a test case should be added in the parenthesization tests to ensure this problem does not creep up again.
Created attachment 18998 [details] path o' doom
Comment on attachment 18998 [details] path o' doom I'm not sure this is the right fix. I need to think about why those parentheses are showing up.
Var node contains a comma node, comma node appears to attempt to ensure a non-ambiguous parse by forcing PrecAssingment on the left hand side, and therefore parenthesising recursively, just try: javascript:alert(function(){(a,b,c,d)})
This regression happened when we did the variable optimization. It started using the CommaNode for variable declarations, but that has the wrong precedence. Here's my suggested fix: Make a class derived from CommaNode that is VariableDeclarationCommaNode -- the only difference for this derived node class is that its streamTo function uses PrecExpression, not PrecAssignment. Use that new class in the combineVarInitializers function.
Comment on attachment 18998 [details] path o' doom Tests good, but fix should be done without adding a new boolean. We just need a different node type.
<rdar://problem/5731773>
Created attachment 19011 [details] Patch#2
I would recommend additional tests that do a mix of assignments and plain declarations. Also, is my NULL pointer dereference fix worth including? I don't know what situation would cause it, but as-is the check is useless.
We can never have a null node in this case -- i can't think of any major place (other than maybe some of the linked lists) when we can have null in the AST. Generally null is uncommon so we have noop nodes to avoid a null check in the general case :D I'll add a few more tests.
I realised that there's no value in testing a mix of initialised and uninitialised declarations as all var declarations without initialisers get stripped from the dom and are regenerated from the symbol table at the head of the function -- thus reducing them to a var decl list of uninitialised variables, and a var decl list where everything is initialised, eg. function () { var a, b=a, c=d, d=e; var e, f=a; } becomes: function () { var a, e; var b = a, c = d, d = e; var f = a; } (note the hoisting of a and e)
Comment on attachment 19011 [details] Patch#2 r=me
Landed r30105