Bug 17214 – REGRESSION: Extraneous parentheses in function.toString()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ryan Govostes]

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.

[Ryan Govostes]

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.

[Oliver Hunt]

Created attachment 18998 [details]
path o' doom

[Darin Adler]

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.

[Oliver Hunt]

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)})

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

<rdar://problem/5731773>

[Oliver Hunt]

Created attachment 19011 [details]
Patch#2

[Ryan Govostes]

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.

[Oliver Hunt]

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.

[Oliver Hunt]

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)

[Maciej Stachowiak]

Comment on attachment 19011 [details]
Patch#2

r=me

[Oliver Hunt]

Landed r30105