Add a method of dumping the AST

[rvanvelzen]

This also includes using the dumped AST to verify test results. This should be slightly safer.

[rvanvelzen]

@mishoo @vjeux @fabiosantoscode All comments are welcome, pretty sure I missed some stuff.

[vjeux]

Nice, this can be really useful for debugging :)

[fabiosantoscode]

I don't know what you might have missed, this patch touches stuff I'm not familiar with. I'm sure others will be able to better review this

[kzc]

@rvanvelzen - Looks good.

I personally would prefer if the AST prop order is significant. For example, it is easier to debug large Binary expressions if the operator type is seen first in the tree. It would avoid a lot of scrolling and parentheses matching to see what's going on:

diff --git a/lib/ast.js b/lib/ast.js
index 203beef..3a1b344 100644
--- a/lib/ast.js
+++ b/lib/ast.js
@@ -79,7 +79,7 @@ function DEFNODE(type, props, methods, base) {
     if (base) base.SUBCLASSES.push(ctor);
     ctor.prototype.CTOR = ctor;
     ctor.PROPS = props || null;
-    ctor.DUMP_PROPS = dump_props.sort();
+    ctor.DUMP_PROPS = dump_props;
     ctor.SELF_PROPS = self_props;
     ctor.SUBCLASSES = [];
     if (type) {
@@ -706,7 +706,7 @@ var AST_UnaryPostfix = DEFNODE("UnaryPostfix", null, {
     $documentation: "Unary postfix expression, i.e. `i++`"
 }, AST_Unary);

-var AST_Binary = DEFNODE("Binary", "left operator right", {
+var AST_Binary = DEFNODE("Binary", "operator left right", {
     $documentation: "Binary expression, i.e. `a + b`",
     $propdoc: {
         left: "[AST_Node] left-hand side expression",

[kzc]

I suspect it wouldn't be very much work to import and export UglifyJS2 trees in this "native" JSON AST form.

[mishoo]

Sorry for chiming in so late, but why not using to_mozilla_ast and JSON-ize that instead? Seems to me the use case was to inspect an AST for correctness, and the SpiderMonkey AST would serve just as well…

[kzc]

why not using to_mozilla_ast and JSON-ize that instead

My comment about import/export aside, this feature is to more to debug UglifyJS2 itself or learn about its internal structure in order to create new optimizations.

A different command line flag --dump-mozilla-ast would also be useful.

[rvanvelzen]

Sorry for chiming in so late, but why not using to_mozilla_ast and JSON-ize that instead? Seems to me the use case was to inspect an AST for correctness, and the SpiderMonkey AST would serve just as well…

You're right, for using it in tests that's way more appropriate.

I'm not entirely sure what the right direction would be to dump the internal AST.

[kzc]

I'm not entirely sure what the right direction would be to dump the internal AST.

No reason not to support both internal AST and mozilla-style AST with different command line flags as they each serve a different purpose.

[kzc]

When I cherry picked this pull request and applied it to my local latest UglifyJS2 master and ran npm test it failed in test/compress/new.js for some reason.

But once I reverted this part of the patch, npm test runs without issue:

--- a/test/run-tests.js
+++ b/test/run-tests.js
@@ -86,7 +86,7 @@ function run_compress_tests() {
             var cmp = new U.Compressor(options, true);
             var expect;
             if (test.expect) {
-                expect = make_code(as_toplevel(test.expect), false);
+                expect = make_ast(as_toplevel(test.expect));
             } else {
                 expect = test.expect_exact;
             }
@@ -97,7 +97,7 @@ function run_compress_tests() {
             }
             var output = input.transform(cmp);
             output.figure_out_scope();
-            output = make_code(output, false);
+            output = make_ast(output);
             if (expect != output) {
                 log("!!! failed\n---INPUT---\n{input}\n---OUTPUT---\n{output}\n---EXPECTED---\n{expected}\n\n", {
                     input: input_code,
@@ -195,6 +195,10 @@ function make_code(ast, beautify) {
     return stream.get();
 }

+function make_ast(ast) {
+    return U.parse(make_code(ast, false)).dump();
+}
+
 function evaluate(code) {
     if (code instanceof U.AST_Node)
         code = make_code(code);

[fabiosantoscode]

What did the failure in test/compress/new.js look like?

[kzc]

It output the AST instead of the code. I'm assuming that the test code wasn't exercised since reverting test/run-tests.js seemed to resolve the issue.

[kzc]

It would be nice to get this PR merged to master as it's super useful for debugging uglify issues.

[fabiosantoscode]

+1 for that. I've been adding lots of syntax, and it's been quite hard to see if I'm doing it right, I have to throw sand and parentheses at stuff.

[rvanvelzen]

It output the AST instead of the code. I'm assuming that the test code wasn't exercised since reverting test/run-tests.js seemed to resolve the issue.

Tests using expect_exact should compare the generated code instead of the AST. :-)

[avdg]

I have maintained my own version here: https://github.com/avdg/UglifyJS2/blob/ast-dump/bin/uglifyjs

[rvanvelzen]

I've just rebased this to be pushed to master. It probably needs some work to work with harmony properly.

@avdg are there any modifications in your tree that would be worthwhile to have?

[avdg]

It was a bit cleaned up and updated to the new branch (there were some conflicts at the time). Though it has been a long time ago (and maybe the mess is gone by now, but I didn't check the new patch just yet).

[avdg]

And yes, there needs to be some fixing for harmony, bit tricky to maintain (need to keep track what happens on a ast-tree structure that isn't stable yet, also the code is currently maintained separately as this pr isn't merged) but the patch should be fairly obvious once the patch mechanics are understood.