Added support for dumping AST to console, text file or dot file

Added support for dumping AST to console, text file or dot file
Committed: https://code.google.com/p/dart/source/detail?r=1587

[shauvik, 2011-11-14 23:27:58]

Hi,

Here is the CL for the AST dump option that I demoed last Friday. Let me know
your comments.

Regards,
Shauvik

[zundel, 2011-11-15 14:49:54]

(You can run the new files through the Eclipse formatter to get rid of a lot of
the formatting issues.)

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/CommandLineOptions.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/CommandLineOptions.java:77: usage = "Dump
parse tree to a suppoted format (console, text, dot)")
Formatting looks funny.
suppoted --> supported

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:676: BaseASTWriter
astWriter = null;
One way you could do this without intruding into the calling code when you add
new writers is to use the Factory pattern.  You would create a static method
like:

 BaseASTWriter AstWriterFactory.create(String type)

Then you could move the if ladder into the factory.  The advantage to this is
that you don't have to disturb callers of the factory when you change the types
of writers supported.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:677:
if("console".equals(astFormat)) {
add space between if and ( 
(here and elsewhere)

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:679: }else
if("text".equals(astFormat)) {
add space between } and else  (here and elsewhere)

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:692: if(astWriter !=
null) astWriter.process(unit);
style: in Java, we always surround the if block with { }

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:85: public
abstract class BaseASTWriter implements DartPlainVisitor<Object> {
instead of making the base class implement DartPlainVisitor<Object>

1) use encasulation - declare an inner class.  None of the derived classes need
to override the visitor's methods.

2) Use DartNodeTraverser instead.  We need to deprecate or remove
DartPlainVisitor.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:92: public
BaseASTWriter() {
If its unused, why do you need it..?

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:96: public
BaseASTWriter(String outputDir) {
do you really want people to instantiate the base class?  You could make this
protected or package protected.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:100: public
abstract void write(String nodeType, DartNode node);
please add javadoc to abstract methods so we know what it is they are supposed
to implement.

the implementation of write(String, Node) seems to be the same in all
subclasses.  Why make it abstract?

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:130: public
void visitChildren(DartNode node) {
Does this need to be public or protected? Its intended to be overridden, that
might be worth javadoc.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:34: protected
void endHook() {
Guava has a shortcut for closing streams like this:

Closeables.close(out, true);

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:62:
out.write(" ("+data+")");
style: spaces before and after +

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:72: //Setup
Indentation
style: space after //

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:76: }else{
style: } else {

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:22:
Map<DartNode,String> nodeMap;
make final

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:24: StringBuffer
edges = new StringBuffer();
make final

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:25: StringBuffer
nodes = new StringBuffer();
make final

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:80:
label.append(String.format("\\n%s",data.replaceAll("\"", "'")));
space after ,

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:98: public void
visitChildren(DartNode node) {
is this override necessary?

[shauvik, 2011-11-15 23:53:14]

Hi,

I've made changes based on zundel's comments. Check the latest patch!

Cheers,
Shauvik

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/CommandLineOptions.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/CommandLineOptions.java:77: usage = "Dump
parse tree to a suppoted format (console, text, dot)")
Fixed. It now reads "Dump parse tree. Supported formats include console, text or
dot". Does it sound okay?

On 2011/11/15 14:49:54, zundel wrote:
> Formatting looks funny.
> suppoted --> supported

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:676: BaseASTWriter
astWriter = null;
I moved most of the code in the factory. i.e. passed the config to the factory
as it needs to access outDir as well.
Looks much cleaner now.

On 2011/11/15 14:49:54, zundel wrote:
> One way you could do this without intruding into the calling code when you add
> new writers is to use the Factory pattern.  You would create a static method
> like:
> 
>  BaseASTWriter AstWriterFactory.create(String type)
> 
> Then you could move the if ladder into the factory.  The advantage to this is
> that you don't have to disturb callers of the factory when you change the
types
> of writers supported.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:677:
if("console".equals(astFormat)) {
Ctrl+Shift+F fixed it. Thanks.

On 2011/11/15 14:49:54, zundel wrote:
> add space between if and ( 
> (here and elsewhere)

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:679: }else
if("text".equals(astFormat)) {
Fixed. Thanks.
On 2011/11/15 14:49:54, zundel wrote:
> add space between } and else  (here and elsewhere)

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:692: if(astWriter !=
null) astWriter.process(unit);
Okay, Done!

On 2011/11/15 14:49:54, zundel wrote:
> style: in Java, we always surround the if block with { }

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:85: public
abstract class BaseASTWriter implements DartPlainVisitor<Object> {
On 2011/11/15 14:49:54, zundel wrote:
> instead of making the base class implement DartPlainVisitor<Object>
> 
> 1) use encasulation - declare an inner class.  None of the derived classes
need
> to override the visitor's methods.
> 
> 2) Use DartNodeTraverser instead.  We need to deprecate or remove
> DartPlainVisitor.

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:92: public
BaseASTWriter() {
On 2011/11/15 14:49:54, zundel wrote:
> If its unused, why do you need it..?

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:96: public
BaseASTWriter(String outputDir) {
On 2011/11/15 14:49:54, zundel wrote:
> do you really want people to instantiate the base class?  You could make this
> protected or package protected.

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:100: public
abstract void write(String nodeType, DartNode node);
On 2011/11/15 14:49:54, zundel wrote:
> please add javadoc to abstract methods so we know what it is they are supposed
> to implement.
> 
> the implementation of write(String, Node) seems to be the same in all
> subclasses.  Why make it abstract?

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:130: public
void visitChildren(DartNode node) {
On 2011/11/15 14:49:54, zundel wrote:
> Does this need to be public or protected? Its intended to be overridden, that
> might be worth javadoc.

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:34: protected
void endHook() {
Cool!
On 2011/11/15 14:49:54, zundel wrote:
> Guava has a shortcut for closing streams like this:
> 
> Closeables.close(out, true);
>

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:34: protected
void endHook() {
On 2011/11/15 14:49:54, zundel wrote:
> Guava has a shortcut for closing streams like this:
> 
> Closeables.close(out, true);
> 

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:62:
out.write(" ("+data+")");
On 2011/11/15 14:49:54, zundel wrote:
> style: spaces before and after +

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:72: //Setup
Indentation
On 2011/11/15 14:49:54, zundel wrote:
> style: space after //

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:76: }else{
On 2011/11/15 14:49:54, zundel wrote:
> style: } else {

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:22:
Map<DartNode,String> nodeMap;
Can't make final. I am reusing this class for all units. This comment helped me
find a bug. Basically nodes need to be reinitialized for every Dart Unit. Added
a fix for that.

On 2011/11/15 14:49:54, zundel wrote:
> make final

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:24: StringBuffer
edges = new StringBuffer();
same as above

On 2011/11/15 14:49:54, zundel wrote:
> make final

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:25: StringBuffer
nodes = new StringBuffer();
same as above

On 2011/11/15 14:49:54, zundel wrote:
> make final

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:80:
label.append(String.format("\\n%s",data.replaceAll("\"", "'")));
On 2011/11/15 14:49:54, zundel wrote:
> space after ,

Done.

http://codereview.chromium.org/8566019/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:98: public void
visitChildren(DartNode node) {
On 2011/11/15 14:49:54, zundel wrote:
> is this override necessary? 

Done.

[zundel, 2011-11-16 14:31:23]

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/DartCompiler.java:670: //Dump the
compiler parse tree if dump format is set in arguments
style: space between // and first letter of comment

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:87: final
String TAB = "  ";
static final  

use "\t" instead of embedding a literal tab character in the string.  (using \t
directly in a string is what I do instead of defining a constant)

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:88: String[]
ignoredLibs = {"corelib", "corelib_impl", "dom", "html"};
private static final

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:89: DartUnit
currentUnit;
protected

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:90: String
outputDir;
(protected?) final

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:91:
ASTNodeTraverser visitor;
either make this private final, or just re-instantiate ASTNodeTraverser each
time in process and don't keep it as an instance variable.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:112: //Hooks
called before and after visiting tree
style: space after //

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:113: protected
abstract void endHook();
maybe it would be cleaner to pass 'currentUnit' as a parameter to startHook()
and endHook() instead of relying on them as protected fields.  It would be
consistent with isIgnored.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:151: boolean
isIgnored(DartUnit unit) {
protected

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:176:
write("DartArrayAccess", node);
I'm not sure if we want to do this, but assuming all nodes being passed really
are the type specified (and not some supertype), you could use reflection to get
these strings:

node.getClass().getSimpleName().  This would allow you to create a general
'write(node);' method.

[shauvik, 2011-11-16 18:56:42]

Hi Eric,

I have fixed and committed the new patch.
Please let me know if you have any other comments.

Thanks,
Shauvik

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/DartCompiler.java:670: //Dump the
compiler parse tree if dump format is set in arguments
On 2011/11/16 14:31:23, zundel wrote:
> style: space between // and first letter of comment

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:87: final
String TAB = "  ";
On 2011/11/16 14:31:23, zundel wrote:
> static final  
> 
> use "\t" instead of embedding a literal tab character in the string.  (using
\t
> directly in a string is what I do instead of defining a constant)

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:88: String[]
ignoredLibs = {"corelib", "corelib_impl", "dom", "html"};
On 2011/11/16 14:31:23, zundel wrote:
> private static final

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:89: DartUnit
currentUnit;
On 2011/11/16 14:31:23, zundel wrote:
> protected

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:90: String
outputDir;
On 2011/11/16 14:31:23, zundel wrote:
> (protected?) final

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:91:
ASTNodeTraverser visitor;
On 2011/11/16 14:31:23, zundel wrote:
> either make this private final, or just re-instantiate ASTNodeTraverser each
> time in process and don't keep it as an instance variable.

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:112: //Hooks
called before and after visiting tree
On 2011/11/16 14:31:23, zundel wrote:
> style: space after //

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:113: protected
abstract void endHook();
On 2011/11/16 14:31:23, zundel wrote:
> maybe it would be cleaner to pass 'currentUnit' as a parameter to startHook()
> and endHook() instead of relying on them as protected fields.  It would be
> consistent with isIgnored.

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:151: boolean
isIgnored(DartUnit unit) {
On 2011/11/16 14:31:23, zundel wrote:
> protected

Done.

http://codereview.chromium.org/8566019/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:176:
write("DartArrayAccess", node);
I tried that but I was seeing some some inner classes and the simple names were
like "Class$InnerClass". Let me know if that's okay and i'll switch back to
that.

On 2011/11/16 14:31:23, zundel wrote:
> I'm not sure if we want to do this, but assuming all nodes being passed really
> are the type specified (and not some supertype), you could use reflection to
get
> these strings:
> 
> node.getClass().getSimpleName().  This would allow you to create a general
> 'write(node);' method.

[zundel, 2011-11-16 19:57:47]

http://codereview.chromium.org/8566019/diff/20001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8566019/diff/20001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:80: private String
getLabel(String nodeType, String data) {
A long chain of string compares looks unecessarily expensive and complicated.

In this case, what I'd do is create a static  final Set<Class> printDataNodes
member, and initialize it with the node class constants that you want to print
data for in a static constructor or with Sets.newHashSet().  Then, the if test
becomes O(1):

if (printDataNodes.contains(node.getClass)) {
  ... append data to the string ...
};

[shauvik, 2011-11-16 21:03:10]

Hi Eric,

Fixed this issue. 
Please check and let me know if it looks okay.

Thanks,
Shauvik

http://codereview.chromium.org/8566019/diff/20001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8566019/diff/20001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:80: private String
getLabel(String nodeType, String data) {
On 2011/11/16 19:57:47, zundel wrote:
> A long chain of string compares looks unecessarily expensive and complicated.
> 
> In this case, what I'd do is create a static  final Set<Class> printDataNodes
> member, and initialize it with the node class constants that you want to print
> data for in a static constructor or with Sets.newHashSet().  Then, the if test
> becomes O(1):
> 
> if (printDataNodes.contains(node.getClass)) {
>   ... append data to the string ...
> };

Done.

[zundel, 2011-11-16 21:16:18]

lgtm

http://codereview.chromium.org/8566019/diff/21003/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8566019/diff/21003/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:34: String[]
labels = {
nit: if you use class constants instead of strings here, your code will get
refactored if anyone renames a node or looks for a reference to this node using
the IDE.