[core] Node support for Antlr-based languages#1658
Conversation
oowekyala
left a comment
There was a problem hiding this comment.
Nice! I think if we're going to go remove methods from the Node interface, then we should probably remove most setter methods (setImage, setBeginLine, etc., maybe not setUserData), which for the most part are only relevant to node construction.
|
@jsotuyod we have |
Generated by 🚫 Danger |
jsotuyod
left a comment
There was a problem hiding this comment.
This is looking really good, but I'm still unsure on how you will deal with terminal nodes given the current AntlrBaseNode simply extends ParserRuleContext
|
@matifraga checkstyle is failing. Also, please respond to my concerns about terminal nodes. |
|
@jsotuyod As far as we see now, we don't need to make Terminal Nodes implement any PMD interface, given that even leaf nodes are NOT antlr terminal nodes, the only reason you will find a Terminal Node while traversing the AST will be if you explicitly need to look for them, in wich case Antlr provides you with a separete set of methods We are currently working on some rules on Swift and we are using this implementation as it is now, I also think there won't be changes regarding terminal nodes in this PR because of the extension it has, if we found in the near future a need to add some implementation to cover some terminal node scenarios I don't think we would need to change this classes |
jsotuyod
left a comment
There was a problem hiding this comment.
This looks good to me. I think we should remove most setters, and javacc specific methods from the mode interface, but that require changes to the master branch first and is therefore outside the scope of this PR.
Before submitting a PR, please check that:
master. The PMD team will merge back to support branches as needed../mvnw clean verifypasses. This will build and test PMD, execute PMD and checkstyle rules. Check this for more infoPR Description
Disclaimer: We don't have much knowledge about the project as aw hole so we would like as much input as possible given that we are interacting with really sensible classes. We would also like to take the opportunity the breaking changes of PMD 7 gave us to make this right.
This pull request is intended to provide a clean Node API that will also be useful to Antlr-based languages. Some things we think we need to do in order to provide this API are:
Nodeinterface names, and remove the ones that are only required for javaCC-based languagesgetAsDocumentwould be a good candidate, but we are not yet familiarized with all the usages.For javaCC-based languages:
AbstractNode, we took the liberty to remove the deprecatedtoStringmethod, but we don't know that is going to happend withgetXPathNodeName.For Antlr-based languages:
We created an
AntlrNodeinterface that will represent the public API for all Antlr-based Nodes and will therefore extendNode. As it is now, we make a default implementation with most of the Node API methods that we think are meant to be unsupported for now.We think jjtOpen and jjtClose shouldn't be on
NodeAPI.We are reviewing the usages of get/set parent in order to understand where it is needed.
We created AntlrBaseNode as a concrete implementation of
AntlrNodewith what we think is the full list ofNodemethods we need to override.We are NOT sure if line/column boundaries are correct, we coudn't find documentation regarding this aspect, we will add gladly add it.
Team Raptor.