Skip to content

[core] Antlr visitor rules#1774

Merged
adangel merged 9 commits into
pmd:pmd/7.0.xfrom
teamraptor:feature/antlr-visitor-rules
Jun 15, 2019
Merged

[core] Antlr visitor rules#1774
adangel merged 9 commits into
pmd:pmd/7.0.xfrom
teamraptor:feature/antlr-visitor-rules

Conversation

@lsoncini

Copy link
Copy Markdown
Contributor

This pull request depends on #1698.

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:

This pull request adds the necessary classes to implement antlr-based abstract rules and generic implementations of both RuleChainVisitor (AntlrRuleChainVisitor) and RuleViolationFactory (AntlrRuleViolationFactory).

We also added AbstractSwiftRule to serve as an example to show how to implement AbstractAntlrVisitor for a specific language and modified the ant-file antlr4.xml to make antlr's autogenerated BaseVisitors extend AbstractAntlrVisitor.

Team Raptor.

@ghost

ghost commented Apr 13, 2019

Copy link
Copy Markdown
1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@oowekyala oowekyala self-requested a review April 15, 2019 15:41

@oowekyala oowekyala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is an interesting change, it introduces some nice ideas that I think we can take further to reduce the complexity of language implementations (outside of this PR)

Comment thread pmd-core/src/main/java/net/sourceforge/pmd/lang/antlr/AbstractAntlrVisitor.java Outdated
@jsotuyod jsotuyod changed the title Feature/antlr visitor rules [core] Antlr visitor rules Apr 17, 2019
@jsotuyod jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Apr 22, 2019
@jsotuyod jsotuyod added this to the 7.0.0 milestone Apr 22, 2019

@oowekyala oowekyala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@matifraga I think, the comments I made can be handled at a later stage, as they indeed are not limited to Antlr rules. I'll open a couple of tickets to track them. This PR seems otherwise perfectly fine to me and I think it can be merged.

@adangel adangel self-assigned this Jun 15, 2019
@adangel adangel 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 Jun 15, 2019
@adangel adangel merged commit 5307f8a into pmd:pmd/7.0.x Jun 15, 2019
adangel added a commit that referenced this pull request Jun 15, 2019
@adangel

adangel commented Jun 15, 2019

Copy link
Copy Markdown
Member

@lsoncini @matifraga @tomidelucca
I've merged now your PRs to PMD 7. I don't know, if anything works, since I didn't see a unit test 😃

I added the ANTLR support on the wiki here: https://github.com/pmd/pmd/wiki/PMD-7.0.0#language-specific-improvements to track the progress a bit and see, what's still ahead.

Here are the points I see next:

  • Create a sample rule for swift
  • XPath support for Antlr based grammars
  • Unit tests for Antlr support
  • Documentation

@matifraga matifraga deleted the feature/antlr-visitor-rules branch June 15, 2019 15:19
@matifraga

Copy link
Copy Markdown

Thanks @adangel ! Sorry about unit testing, we will unit test the classes we introduced already. We do have a working rule for Swift already, we will send a PR soon.

Regarding XPath our first attempt making a rule didn't work out, we are currently digging the code to find out why. We will also document with examples all that is needed to add a new rule using antlr as we did with the CPD feature.

cc/ @lsoncini @tomidelucca @jsotuyod

@adangel

adangel commented Jun 15, 2019

Copy link
Copy Markdown
Member

Don't worry. I just wanted to make sure, you guys have this on your radar...

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.

5 participants