491621 - VariableDeclaration.toSource() sometimes returns invalid source code

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Daniel Gredler]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10
Build Identifier: Trunk (1.7R3pre)

When parsing the following JavaScript code:

"var a = 1; x = 1;"

Calling AstRoot.toSource() returns invalid code (note the missing semicolon):

"var a = 1x = 1;"

Calling AstRoot.debugPrint() returns:

0	SCRIPT 0 17
0	  VAR 0 10
4	    VAR 4 9
4	      NAME 0 1 a
8	      NUMBER 4 1
11	  EXPR_RESULT 11 6
11	    ASSIGN 0 5
11	      NAME 0 1 x
15	      NUMBER 4 1

I'm not sure if VariableDeclaration.toSource(int) needs to append a semicolon sometimes, or if the VariableDeclaration instance should be wrapped in an ExpressionStatement. The latter may be the case, because the javadoc for VariableDeclaration states that:

"A standalone variable declaration in a statement context is wrapped with an {@link ExpressionStatement}."

It looks like this was reported on the mailing list last year:

http://www.mail-archive.com/[e-mail]/msg01287.html

Reproducible: Always

Comment 1

[Daniel Gredler]

Attachment: Possible Fix

Attaching a possible fix.

Comment 2

[Daniel Gredler]

Regarding the patch: it may be best to wrap the node *after* the line number has been set, or set the line number on both nodes... not sure, I'm not very familiar with how the AstNode hierarchy is supposed to work.

Comment 3

[Frank Groeneveld]

Although the line numbers changed, the patch still works. This is what I needed, thanks!

Comment 4

[Frank Groeneveld]

Attachment: Update the previous patch to apply cleanly against cvshead

Comment 5

[Oliver Hutchison]

Supplies patch does not work with current cvshead - see following exception. Perhaps the VariableDeclaration.toSource() method should just append the semicolon? That is my workaround (see attached patch). Is there a situation where this would result in invalid code?

Sorry don't have time to investigate further. 

java.lang.RuntimeException: 129
	at org.mozilla.javascript.CodeGenerator.badTree(CodeGenerator.java:272)
	at org.mozilla.javascript.CodeGenerator.visitExpression(CodeGenerator.java:998)
	at org.mozilla.javascript.CodeGenerator.visitStatement(CodeGenerator.java:429)
	at org.mozilla.javascript.CodeGenerator.visitStatement(CodeGenerator.java:322)
	at org.mozilla.javascript.CodeGenerator.generateICodeFromTree(CodeGenerator.java:159)
	at org.mozilla.javascript.CodeGenerator.generateFunctionICode(CodeGenerator.java:150)
	at org.mozilla.javascript.CodeGenerator.generateNestedFunctions(CodeGenerator.java:235)
	at org.mozilla.javascript.CodeGenerator.generateICodeFromTree(CodeGenerator.java:155)
	at org.mozilla.javascript.CodeGenerator.generateFunctionICode(CodeGenerator.java:150)
	at org.mozilla.javascript.CodeGenerator.generateNestedFunctions(CodeGenerator.java:235)
	at org.mozilla.javascript.CodeGenerator.generateICodeFromTree(CodeGenerator.java:155)
	at org.mozilla.javascript.CodeGenerator.compile(CodeGenerator.java:124)
	at org.mozilla.javascript.Interpreter.compile(Interpreter.java:235)
	at org.mozilla.javascript.Context.compileImpl(Context.java:2425)
	at org.mozilla.javascript.Context.compileString(Context.java:1367)
	at org.mozilla.javascript.Context.compileString(Context.java:1356)

Comment 6

[Oliver Hutchison]

Attachment: Alternative patch. Appends missing semicolon in VariableDeclaration.toString().

Comment 7

[Frank Groeneveld]

How can we debug your problem if you don't provide the JavaScript that makes it crash? Can you please post that full source code?

I've parsed all big (web) JavaScript frameworks with the first patch and never had problems.

Comment 8

[Hannes Wallnoefer]

(In reply to comment #7)
> 
> I've parsed all big (web) JavaScript frameworks with the first patch and never
> had problems.

I can confirm that the original patch breaks compilation - not parsing, but transforming the AST tree to runnable code in the optimizer or interpreter. 

The alternative patch seems to work for top level var statements, but breaks var declarations in for loops (e.g. for (var x = 0; ...) ...)

Comment 9

[Oliver Hutchison]

Frank my apologies I'm extremely busy at the moment and if I'd had more time I would have investigated further. I added the comment as it is clearly better to have a broken toSource() than a broken code generator. Anyway here's some code that will break with the 1st patch.

function foo() {
function() {
		var bar = 1;
	}	
}

Hannes, thanks for the update - of course I totally forgot about for loops. i'm using the parser to transform a very simple expression language and I'm not using for loops. I've added some to my testcases and have now patched Block.toSource()

Comment 10

[Oliver Hutchison]

[hit enter too soon]

I've now patched Block.toSource() to add the missing semicolon after any VariableDeclaration nodes - which is working fine with my minimal expression language.

Comment 11

[Hannes Wallnoefer]

Oliver, would you mind posting your Block.toSource() patch?

Comment 12

[Oliver Hutchison]

Attachment: Appends a semicolon where required after VariableDeclaration nodes

Comment 13

[Oliver Hutchison]

Hannes, I've attached a patch and basic test case. It turns out that the missing semicolon may need to be added in Scope, Block and AstRoot nodes. 

It's possible there may be other nodes types that need this modification - so perhaps a better solution is to always add the semicolon in VariableDeclaration.toSource() and then modify ForLoop.toSource() to not output the semi after the initializer if it's a variable declaration.

Comment 14

[Oliver Hutchison]

Attachment: Appends missing semicolon in VariableDeclaration.toString()

Does not break for loops like the 1st patch. Also updated the test to cover some more cases and added a compilation test.

Comment 15

[Marius Dumitru Florea]

Is there another way to decompile the AST tree than using the toSource() method? If not then this bug is critical IMO.

Comment 16

[Marius Dumitru Florea]

I attached a patch to https://sourceforge.net/tracker/?func=detail&atid=448266&aid=3105264&group_id=47038 (htmlunit-core-js, a rhino fork). My patch modifies Parser to wrap VariableDeclaration in an ExpressionStatement and also modifies IRFactory to "skip" ExpressionStatements that wrap VariableDeclarations. The patch also includes a unit test.

Comment 18

[Hannes Wallnoefer]

Attachment: Add isStatement flag to VariableDeclaration

Thanks for all the patches. I considered them all, but in the end I decided to come up with my own patch that does not change the AST tree representation (as there may be programs out there depending on the current AST layout) and keeps hackiness at a minimum at the same time.

This patch adds an isStatement() flag to VariableDeclaration so it knows whether to render itself with or without semicolon. The flag is set right in the parser depending on the context. I think this is the soundest solution yet.

Comment 19

[Hannes Wallnoefer]

Committed patch to git master:

https://github.com/mozilla/rhino/commit/832d89110a4302d2731b549e4313e4945fc6f79f

Comment 20

[Frank Groeneveld]

Great, thanks for fixing this!