Generated by 🚫 Danger

I've added a simple description of the Node API

Very nice!

First in PMD 7: Should we really provide two different ways to traverse the AST: via NodeStream children() and via Node getChild(int)? In general, I think, the API should provide only one way: It's easier to use, harder to misuse, only one implementation to maintain/optimize.

About the tree traversal methods that are on Node (getParent/getChild/etc): I think we need them to implement NodeStreams.

The cool thing about the current node stream implementation is that it works for any Node, using just the four basic primitives getParent/getChild/getNumChildren/getIndexInParent. Having them on the node interface gives a very clear implementation path for someone wanting to make a new Node:

• implement those four primitives
• get NodeStreams and all the other traversal methods for free, including all the optimized NodeStream implementations

We could imagine a Node interface that strongly relies on NodeStream, ie not exposing those primitives. But then, an implementor of Node will need to implement a NodeStream for the children! This means, there will be more implementations of NodeStream to maintain, and they'll live outside of pmd-core's internal packages, so we'll have to publish part of the node stream implementations. I'm not sure this is something we want..

FWIW I like the mental model that NodeStream is just free stuff built on top of Node, and not integral to the Node interface. I also think methods like getParent and getChild are harmless: even if there is an equivalent NodeStream way, those methods are really basic, well-specified and well-understood (every tree data structure ever written has some combination of those methods).

If we add this second way of traversing the AST, then I would right now add directly List getChildren() instead of having to call two methods (getNumChildren(), getChild(int)) in order to iterate.

I would rather make it a Iterable<Node> children(). This makes it possible to use foreach right now, and the transition to NodeStream<Node> children() will be binary & source compatible (NodeStream is an Iterable).

As you have seen by my questions, I didn't understand the comment, so we need to rewrite it.
Suggestion: [...]

Your suggestion seems very fine to me

More things to say here:

I think the methods about XPath should be removed from the interface and handled by an external object. This would decouple our Node -> DOM transformation from node implementations. For now we only map nodes to a DOM element with some attributes. But to support a more complete transformation, some additional methods would be required (eg access to comments, namespace, etc), and adding them directly to the node interface makes it messy. What if we want to change the XPath API, eg change the Attribute class? That's not really the problem of the Node, the AST is still the same.

Node instances are meant to be used by eg developers writing Java rules. They don't care about the XPath representation, so won't miss these methods. Also the Attribute objects, if they're created directly by Nodes, cannot know about the DOM they're used in. This is sad, because it would give a simple solution to the problem of reporting usages of deprecated attributes.

I imagine we could use something like:

interface DomAdapter {

  Iterator<Attribute> getAttributes(Node node);

  String getElementName(Node node);

}

provided by a LanguageVersionHandler.