Unifying printing logic in ASTDumper

I’ve been looking at the code in ASTDumper.cpp and noticed that most of it basically uses one of three approaches:

  • Some of it uses the ASTDumper’s own classes, like PrintExpr and PrintStmt
  • Some of it uses an external ASTPrinter class that implements some printing logic used in other parts of the compiler
  • And some of it just prints directly to the output stream, with (seemingly) no centralized logic

While I imagine this is just the result of several unrelated changes throughout the compiler’s development, I wanted to ask, is there a specific motivation behind these decisions? And is there a plan (or a desire, perhaps) to eventually simplify and unify these approaches?

I think it's just the result of differing needs:

  • ASTPrinter needs to print a source-like view of code rather than a tree structure. (This is extra important now with the work on textual interfaces.) ASTDumper usually uses this when referring to some other AST node that isn't really a child, like the underlying type of a typealias.

  • PrintExpr and PrintStmt are subclasses of ASTVisitor; keeping them separate helps keep the code clean by separating concerns. (At least, that's the theory, anyway.)

  • If none of this functionality is needed, there's no reason not to print directly to the output stream.

Anything that makes the code cleaner is probably a welcome patch, but I wouldn't say the current division of labor is something we consider a problem.

Hmmm that makes a lot more sense now.

What about methods like Stmt::print and Stmt::dump? It seems to me like they’re a way for other parts of the compiler to print AST nodes in a tree structure, is that about right?

dump() is 99% of the time present to make debugging easier, since a real dumper would usually want to control the output stream rather than just using stderr. print(…) seems to be a way to hide the details of PrintExpr, but it might make more sense if that went consistently through ASTPrinter (as noted by FIXME in one of the APIs). The convenience of having a print(…) method right on an AST node type might still be worth it, though.

Aside: we also haven't really put effort into pretty-printing Exprs and Stmts, only Decls.

Ok, I think I get it.

So, do you think this make sense?

  • Standardize dump() methods to always print the S-Expression trees to a given output stream.
  • Standardize print() methods to always print the more source-like representation to a given output stream, using the ASTPrinter when appropriate.

This seems like a good first step for me :) it might make things clearer in the code, and it should take care of that FIXME you mentioned too.

Well, we do want to keep the no-argument dump() methods, because they're convenient for debugging. Other than that that seems reasonable.

Hey @jrose, I submitted a pull request (#19985) on this issue. It's my first, so it might not be quite right, but I'd love some feedback if you have the time.