congo-parser-generator icon indicating copy to clipboard operation
congo-parser-generator copied to clipboard

I really wonder about the value of certain newer tree-building annotations

Open revusky opened this issue 9 months ago • 4 comments

Well, specifically, as a case in point, consider this line. I mean, specifically, that one writes:

  ( @importDeclarations :+= ImportDeclaration )*!

instead of just:

 (ImportDeclaration)*!

causes the CompilationUnit node to have an extra List<Node> importDeclarations field and the various ImportDeclaration child nodes are added to that list. However, without that extra annotation, we already can write:

 List<ImportDeclaration> importDeclarations = compilationUnit.childrenOfType(ImportDeclaration.class);

I mean, we have the above possibility available for free, but aside from it just being automtically there, this gives us a List<ImportDeclaration> while the disposition with the extra annotation gives us a List<Node>, so that if we want to iterate over the nodes in the container, we would typically have to downcast. So, instead of writing simply:

     for (ImportDeclaration idecl : compilationUnit.childrenOfType(ImportDeclaration.class) {
               ...
     }

we would have to write:

    for (Node n : compilationUnit.getImportDeclarations()) {
              ImportDeclaration idecl = (ImportDeclaration) n;
              ...
    }

i.e. something that is more typical of pre-generics Java code. But I really just can't figure out what this is buying one, when you already have API that gives you all the ImportDeclaration child nodes and makes better use of the type system.

Well, maybe I'm just missing something about all this. I really honestly can't see where the extra feature is paying its weight. I recently patched this class so as not to use the getNamedChild (or the getNamedChildList) feature and, again, it's very hard to see what the real gain of this was.

Well, okay, there is a problem at times where a grammar production creates a new node or just passes up a single node that is on the stack. For example, if you wrote a production:

      AnnotatedIdentifier : (Annotation)* <IDENTIFIER> ;

This production, by default, will just put an IDENTIFIER token on the stack if there is no annotation, but otherwise rolls up a Annotated Identifier node. So it is true that if you had something like:

     AnnotatedIdentifier  /annotatedIdentifier/

then the getNamedChild("annotatedIdentifier") would potentially return just an IDENTIFIER token or an AnnotatedIdentifier node. And this means that it would have to return the base Node, since we don't know which node type we have.

But the other possibility would be just to make this an unconditionally built node, so we could have something like:

     PossiblyAnnotatedIdentifier# : (Annotation)* <IDENTIFIER>;

In that case, we would always build a node even if there are no annotations and the only thing the node contains is a single IDENTIFIER child. And actually, you can see that I addressed the issue with EnumConstant that way. I changed EnumConstant to an unconditional node.

But anyway, I would at least like to get rid of these tree-building annotations like the aforementioned @importDeclarations :+= ImportDeclarationfrom the Java grammar. I mean, aside from the fact that this is not really documented anywhere, I think that usually it is better to use the buit-in API that makes better use of the type system.

revusky avatar May 27 '24 11:05 revusky