Skip to content

[java] Use default method on generated visitor interfaces#1446

Merged
adangel merged 12 commits into
pmd:pmd/7.0.xfrom
oowekyala:java-visitor-default-methods
Nov 29, 2018
Merged

[java] Use default method on generated visitor interfaces#1446
adangel merged 12 commits into
pmd:pmd/7.0.xfrom
oowekyala:java-visitor-default-methods

Conversation

@oowekyala

@oowekyala oowekyala commented Nov 9, 2018

Copy link
Copy Markdown
Member
  • Use default methods on JavaParserVisitor, which allows to remove the duplication between AbstractJavaRule and JavaParserVisitorAdapter
    • That also means default methods are generated based on the grammar and we don't need to maintain it by hand when we change the grammar (save for [java] Deprecate visitor decorators #1437), which is nice to have considering 7.0.0 is coming.
  • Add a new visitor type that takes a generic argument and returns no result
    • This is typically the kind of interface that AstProcessingStages would use, ie they perform side effects on the AST and carry around a context object. Rules also fit this description since they perform side-effects on a RuleContext, but changing that would be a pain to update.
  • The java parser is now generated anew if the ant file changes, which allows updating to the file more simply

@oowekyala oowekyala added this to the 7.0.0 milestone Nov 9, 2018
@oowekyala oowekyala force-pushed the java-visitor-default-methods branch from e02190d to 82aa804 Compare November 9, 2018 17:56
@ghost

ghost commented Nov 9, 2018

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

Generated by 🚫 Danger

@adangel adangel self-assigned this Nov 25, 2018

@adangel adangel 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.

Looks good!

I'm not sure yet about the name of the "SideEffectingVisitor". Is the (long-term) plan, that the SideEffectingVisitor replaces the current JavaParserVisitor?

@adangel adangel merged commit 360d8d2 into pmd:pmd/7.0.x Nov 29, 2018
@oowekyala oowekyala deleted the java-visitor-default-methods branch November 29, 2018 23:16
@oowekyala

Copy link
Copy Markdown
Member Author

@adangel I don't think new visitor types should replace JavaParserVisitor, but rather they should complement it. Having yet other visitors would help us fit different use cases more closely. E.g. I can see use cases for the following signatures too:

void visit(XXNode node); // no parameter, void return type
T visit(XXNode node); // no parameter, generic return type

These could of course be implemented with a JavaParserVisitor, but it's inconvenient bc of cast and unnecessary parameter. Visitors with different signatures would just be different tools in the toolbox, and I think fleshing out this toolbox now can be beneficial in the long term.

Since they can be generated the maintenance cost would be low, the only thing would be that we'd have to add an accept method to each node, for each type of visitor. That would be verbose, but only needs to be done once, so I think long-term it's a good investment. Wdyt?

@adangel adangel added the in:pmd-internals Affects PMD's internals label Jan 12, 2023
@adangel adangel mentioned this pull request Jan 23, 2023
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in:pmd-internals Affects PMD's internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants