Bug 17214 - REGRESSION: Extraneous parentheses in function.toString()
Summary: REGRESSION: Extraneous parentheses in function.toString()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL: http://rgov.org/safaritest.html
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2008-02-07 22:38 PST by Ryan Govostes
Modified: 2008-02-08 21:17 PST (History)
4 users (show)

See Also:


Attachments
path o' doom (7.11 KB, patch)
2008-02-08 00:16 PST, Oliver Hunt
darin: review-
Details | Formatted Diff | Diff
Patch#2 (4.51 KB, patch)
2008-02-08 18:13 PST, Oliver Hunt
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Govostes 2008-02-07 22:38:14 PST
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.
Comment 1 Ryan Govostes 2008-02-07 22:57:21 PST
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.
Comment 2 Oliver Hunt 2008-02-08 00:16:47 PST
Created attachment 18998 [details]
path o' doom
Comment 3 Darin Adler 2008-02-08 00:20:30 PST
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.
Comment 4 Oliver Hunt 2008-02-08 00:29:32 PST
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)})
Comment 5 Darin Adler 2008-02-08 00:50:19 PST
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 6 Darin Adler 2008-02-08 02:37:21 PST
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.
Comment 7 Darin Adler 2008-02-08 02:44:34 PST
<rdar://problem/5731773>
Comment 8 Oliver Hunt 2008-02-08 18:13:38 PST
Created attachment 19011 [details]
Patch#2
Comment 9 Ryan Govostes 2008-02-08 18:57:42 PST
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.
Comment 10 Oliver Hunt 2008-02-08 20:17:51 PST
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.
Comment 11 Oliver Hunt 2008-02-08 20:34:21 PST
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 12 Maciej Stachowiak 2008-02-08 21:10:05 PST
Comment on attachment 19011 [details]
Patch#2

r=me
Comment 13 Oliver Hunt 2008-02-08 21:17:31 PST
Landed r30105