[core] Make visitors generic

[oowekyala]

TODO

• [core] Add generic visitor interface in pmd-core #2589 - Add generic visitor interface in pmd-core

Language modules to port:

• [core] Add generic visitor interface in pmd-core #2589 Java, JSP, Scala, Swift
• [apex] Make apex visitor generic #2661 Apex
• [javascript] Generic visitor for JS #2707 JS
• [java] Rename visitor methods to reduce overloading #2706 Touchups for Javacc languages (Java, JSP)
• [core, ...] Finish generic visitors #2746 Modelica
• [core, ...] Finish generic visitors #2746 PL/SQL
• [core, ...] Finish generic visitors #2746 Velocity
• [core, ...] Finish generic visitors #2746 VisualForce

Cleanup after this:

• [core, ...] Finish generic visitors #2746 Cleanup the wrapper scripts
• [ ] Remove default implementation in Node "I think it's useful to have a default as some languages do not support visitors (eg we have XML)"

Use case

It's a somewhat common pattern to use a single data Object during the full visitation of a visitor, and pass it around to children to accumulate results. A Rule for instance accumulates violations in a RuleContext, whereas in the metrics framework, there are several examples (see eg ncss or cyclo) of using a visitor to eg count some nodes.

The point isn't that it's a single object that's passed around as data, but that the data object keeps the same type throughout the visitation. As you can see in the two examples with ncss and cyclo, we use data as a counter. The fact that data is of type Object instead of MutableInt obscures the code with a lot of casts though.

Generic visitor

These casts could be spared if JavaParserVisitor were generic, eg:

public interface JavaParserVisitor<T>
{
  public T visit(JavaNode node, T data);
  public T visit(ASTCompilationUnit node, T data);
  public T visit(ASTPackageDeclaration node, T data);
  // ...
}

Compatibility

Type erasure would replace T with Object in concrete subclasses, so the generic and non generic versions would be identical after compilation. A class implementing the raw type would again be identical to the original version, so AFAIK this change is free (is it?)

Implementation

The JavaParserVisitor interface is generated by JavaCC, so we can't modify it directly. We could simply create another interface based on a copy paste and generify it, but we'd need to keep it up to date by hand. Another solution would be to modify javacc itself, which looks easy enough

[adangel]

Good idea. JavaParserVisitor is indeed generated (I shortly mixed it up with the two implementations JavaParserVisitorAdapter and AbstractJavaRule that need to be updated manually whenever the grammar is changed to add a new node type).

We are currently using javacc 5 - there is version 6 available. Maybe this one allows such code generation?

[oowekyala]

As of yet I don't think it allows for a generic interface, no. See the visitor generation source: the visit method arguments and return type are customizable with options VISITOR_DATA_TYPE and VISITOR_RETURN_TYPE, but there's no option to make the interface generic yet.

The implementation would also require the generated jjtAccept methods to be generic, like so:

public <T> T jjtAccept(JavaParserVisitor<T> visitor, T data)

I'm not certain but I think these methods may be generated here. Though we don't regenerate our node classes each time, and would need to change that by hand, right?

Btw the latest version is apparently javacc 7.0

[jsotuyod]

Sounds good.

AFAIK, JavaCC 6 only added the support to compile grammars into C++ code. I've no idea of what is in JavaCC 7, last time I checked it was still in development.

Also, remember we need to review dependencies, JavaCC is currently packaged and shipped and it shouldn't as per #710

[oowekyala]

I've opened a ticket at javacc/javacc#44 to start a dialog with javacc

[oowekyala]

Without needing to patch javacc, I noticed that we could simply find/replace the generated visitor file with Ant. Adding the following lines to alljavacc.xml:

        <replace file="${target}/net/sourceforge/pmd/lang/java/ast/JavaParserVisitor.java"
                 token="JavaParserVisitor"
                 value="JavaParserVisitor&lt;T&gt;" />
        <replace file="${target}/net/sourceforge/pmd/lang/java/ast/JavaParserVisitor.java"
                 token="Object"
                 value="T" />

The produced visitor is exactly what we want:

public interface JavaParserVisitor<T>
{
  public T visit(JavaNode node, T data);
  public T visit(ASTCompilationUnit node, T data);
  public T visit(ASTPackageDeclaration node, T data);
  public T visit(ASTImportDeclaration node, T data);

Can I open a PR?

[adangel]

I was hoping, that we one day could get rid of the ant scripts, that manually search&replace the (javacc) generated files.
It nevertheless would be a doable solution.
I'm not sure yet, how the compatibility is affected - since it would change a lot, although in a compatible way via type erasure. We'll need to double check, whether any custom rules developed outside PMD, would stop compiling... If that's the case, it's more like a 7.0.0 feature.

[oowekyala]

I've fiddled a bit with generic visitors (see https://github.com/oowekyala/pmd/tree/generic-visitors), the biggest compatibility breaker is AbstractJavaRule. We're forced to make it implement the raw type if we want to preserve source compatibility. Binary compatibility is preserved anyway.

In many other cases, such as when using a standalone visitor, genericity can be viewed as an opt-in feature, in that it doesn't break source compat if you implement the raw type

[jsotuyod]

@oowekyala sorry it took me this long to reply. I've been following this discussion and thinking thoroughly of the consequences.

Changing this to generics is indeed a breaking API change (although a slight one). Even if erasure means using Object is still valid, the compiler will start to produce warnings for using rawtypes. This warnings may in turn produce the build to fail if using -Werror when running javac. This means the change can indeed produce third parties to fail to build against 6.1.0 due to this change.

At runtime however, if using a plugin built against an older version, it will still work.