Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Node support for Antlr-based languages #1658

Merged
merged 19 commits into from
Apr 1, 2019

Conversation

matifraga
Copy link
Contributor

@matifraga matifraga commented Feb 17, 2019

Before submitting a PR, please check that:

  • The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • ./mvnw clean verify passes. This will build and test PMD, execute PMD and checkstyle rules. Check this for more info

PR 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:

  • Modify Node interface names, and remove the ones that are only required for javaCC-based languages
  • Make use of java 8 to provide default methods. We thought maybe getAsDocument would be a good candidate, but we are not yet familiarized with all the usages.

For javaCC-based languages:

  • We will keep using AbstractNode, we took the liberty to remove the deprecated toString method, but we don't know that is going to happend with getXPathNodeName.

For Antlr-based languages:

  • We created an AntlrNode interface that will represent the public API for all Antlr-based Nodes and will therefore extend Node. 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 Node API.

  • 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 AntlrNode with what we think is the full list of Node methods 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.

@jsotuyod jsotuyod added this to the 7.0.0 milestone Feb 17, 2019
Copy link
Member

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Feb 17, 2019
@matifraga
Copy link
Contributor Author

matifraga commented Feb 24, 2019

@jsotuyod we have getFirstDescendantOfType and findDescendantsOfType as private methods on AbstractNode. We are going to move the code to a utility until we have java 9 support. We also took the liberty to add <T> List<T> findDescendantsOfType(final Class<T> targetType, final boolean crossBoundaries) into the Node API, i don't know if having both that and <T> List<T> findDescendantsOfType(Class<T> targetType) is wise, but we saw the comment on AbstractNode and end up adding it.

@matifraga
Copy link
Contributor Author

@jsotuyod now that #1597 was merged, if we update the PMD 7 branch, the changes will be reduced.

We will also like to know if any of the AntlrNode methods we are marking as unsupported actually need a concrete implementation. Thanks in advance :)

@matifraga matifraga marked this pull request as ready for review February 24, 2019 23:24
@pmd-test
Copy link

1 Warning
⚠️ Running pmdtester failed, this message is mainly used to remind the maintainers of PMD.

Generated by 🚫 Danger

@matifraga matifraga changed the title [core] [draft] Node support for Antlr-based languages [core] Node support for Antlr-based languages Feb 25, 2019
Copy link
Member

@jsotuyod jsotuyod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jsotuyod
Copy link
Member

@matifraga checkstyle is failing. Also, please respond to my concerns about terminal nodes.

@matifraga
Copy link
Contributor Author

matifraga commented Feb 25, 2019

@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 visitTerminal and visitErrorNode in their visitors (wich by default return null as their default values, that's something we can change overriding AbstractParseTreeVisitor).

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

Copy link
Member

@jsotuyod jsotuyod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jsotuyod jsotuyod added an:enhancement An improvement on existing features / rules and removed is:WIP For PRs that are not fully ready, or issues that are actively being tackled labels Mar 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
an:enhancement An improvement on existing features / rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants