Implement better built-in AST dumping support

[jD91mZM2]

This should

1. Aid users to actually debug their things
2. Aid unit testing by making it easy to create a dump function with a stable output, unlike the default debug function

Thoughts?

[matklad]

I am definitely all for an ability to just print SyntaxNode (outputting numric codes for syntax kinds).

I am not so sure about tweakable output format.... The interface of dump, with two closures, seems pretty complicated. If you need to tweak ouptput precisely, wouldn't it be easier to just roll your own formatting by calling into preorder_with_tokens?

[jD91mZM2]

The idea was a fast way to create a debug print for the AST that does the typical indention you'd expect since it's such a standard, but I agree my function might be the wrong way to achieve that. Would a builder-style interface to the function - or perhaps just a simple iterator that gives both the levels deep and the WalkEvent, help? Or do you disagree with the idea as a whole?

[matklad]

Yeah, I understand the motivation. My main complaint is that the resulting API looks harder than the problem it is trying to solve. That is, I suppose, if you are new to the library, it would be easier to roll-your-own printing on top of descendents_with_tokens than to understand the closure based API.

This is basically a template method pattern, and it is usually problematic for two reasons:

• it gives you only a limited amount of flexibility
• the callback-based control flow obscures what really happens on the call site

Perhaps we should add printing the nodes as an example to the tree-walking API (a literal example in the doc comment)?

[jD91mZM2]

Okay then. Could we at least supply a standard, stable, format that is meant to be used for both printing and in unit tests? I don't want to retype the whole thing every time :)

[matklad]

Okay then. Could we at least supply a standard, stable, format that is meant to be used for both printing and in unit tests?

Maybe, but this seems like an X/Y problem. In rust-analyzer, the printed AST format is the same as it was initially, despite the fact that rowan changed a lot since then. I am looking at rnix right now, so I'll take a closer look at how unit tests are done.

[matklad]

Aha, I see what is the difference between analyzer and rnix!

rnix uses rowan::SyntaxNode directly, while analyzer wraps the node:

https://github.com/rust-analyzer/rust-analyzer/blob/35b60d68932926c6f5cef6ece44a82363a7061c5/crates/ra_syntax/src/syntax_node.rs#L104

This wrapping is intentional, and in part exist because of debug impls. Rowan deliberately doesn't care about node's string names, it cares only about integers.

However, if a library uses rowan, it statically knows which syntax kind has which name. So, wrapping rowan::SyntaxNode into struct SyntaxNode(rowan::SyntaxNode) allows you to inject this static info (at the cost of duplicating the API)

[jD91mZM2]

allows you to inject this static info (at the cost of duplicating the API)

FYI, rnix does wrap the tokens, but only when using the types interface. Wrapping makes sense but I almost refuse to duplicate the API. Perhaps, if it really is the best solution to wrap the node, most functions can be put in a trait with a default implementations for as much as possible?

Anyway, maybe I'm not clear: The idea here wasn't that the user could use the dump function in this PR, it's so each implementation (rnix, rust-analyzer, etc) could easily create a stable and recursive debug format. That's why I thought a complicated interface didn't matter - only a few of us would need to use it, and this is to avoid duplicating code. But I agree it's not perhaps not flexible enough to be worth it.

[matklad]

Let's discuss the general wrapping problem in https://gitlab.com/jD91mZM2/rnix/merge_requests/10 :-)

[matklad]

SyntaxNodes are now fmt::Debug