[core] Cleanup Node and AbstractNode by oowekyala · Pull Request #2447 · pmd/pmd

oowekyala · 2020-04-26T20:36:28Z

Describe the PR

This removes deprecated methods of Node and AbstractNode, and tears down the functionality that is in AbstractNode but not used by all subclasses. It also cleans up some tests that were either using deprecated functionality, or were just building invalid nodes.

Changes in pmd-core:

Remove most deprecated methods from AbstractNode and Node
Move AbstractNode into the impl subpackage
Remove some fields from AbstractNode
- the image field, which is pushed down to abstract base classes that need it
- the begin/end/line/column fields, pushed down to a new AbstractNodeWithTextCoordinates base class (which eg Jjtree nodes don't extend). This base class name is ugly but I think we can remove it later if we add an object to encapsulate this position information
- the id field is pushed down to AbstractJjtreeNode
- the fields about DataflowNode are removed, instead the data flow framework uses a key in the data map
Make the remaining fields private
Make the methods for tree construction protected
- This includes eg jjtClose/jjtOpen, which are pulled down from Node to AbstractJjtreeNode
Pull up some methods that are currently on the java-grammar branch and only available to AbstractJavaNode to AbstractNode (eg insertChild, this is now used by the apex module too)
Ensure Parser returns a RootNode
NodeStream methods in Node now use upper bounded wildcards, eg Node::ancestors returns NodeStream<? extends Node>. This is to make it overridable by eg NodeStream<JavaNode>. Duplication of the overrides is removed by an interface "GenericNode", because I found it was hard to add a type parameter to Node directly (very contagious), and it also wouldn't be useful in language-independent code, only in language modules. Some type parameters are added to AbstractNode, but this keeps complexity to a minimum while improving the type safety and ease of use of the API. For example, it's impossible to add an arbitrary Node as a child to a JavaNode now, the child must be an AbstractJavaNode. This in turn enables us to say for sure, that the ancestor stream contains only JavaNodes. Of course this is Java, so unchecked conversions can still break the type safety, but since mutation methods are now protected, we can be reasonably sure of it.

Changes in language modules:

Make AbstractPLSQLNode package-private (this moves up some of its methods to the interface, removes the method dump, because now we have getText anyway)
AbstractApexNodeBase is merged into AbstractApexNode and removed
Remove all childrenAccept methods (in several modules)
In pmd-java, ClassScope doesn't build its custom method declaration itself anymore. It just asks InternalApiBridge, which lets us make all node constructors package-private
In pmd-java, Comment now extends AbstractJjtreeNode, it's technically a separate tree: the parent of a comment cannot be a JavaNode, the child of a JavaNode cannot be a Comment. Thankfully this is now enforced at compile time. This needs some more thought, I don't know if this should stay or go, or what relation this would have to a "javadoc language module".

The changes in test sources are mostly fixing the following problems:

using testingOnlySetBeginLine/Column without setting other coordinates (which makes these methods inconsistent)
setting invalid coordinates (eg not correctly ordered, argument is 0 but line and columns start at 1)

Related issues

Part of [core] Merging Javacc build scripts #2239
Follow up on [core] Deprecate jjtree methods from the Node interface #2172

Ready?

Added unit tests for fixed bug/feature
Passing all unit tests
Complete build ./mvnw clean verify passes (checked automatically by travis)
Added (in-code) documentation (if needed)

Replaced with a DataKey

No real reason to remove it, but it should be moved to pmd-core

ghost · 2020-04-26T20:50:13Z

	1 Message
📖	No java rules are changed!

Generated by 🚫 Danger

adangel

Thanks for the huge cleanup!

Consider my comments as food for thought 😄

adangel · 2020-05-02T14:13:05Z

     *
     * @param image The image to check
     */
    default boolean hasImageEqualTo(String image) {


Once we removed the generic "getImage" method in favor of more specific methods likes "getName" etc., this method also doesn't make sense and should be removed

adangel · 2020-05-02T14:23:20Z

+     * @param child The child to add
+     * @param index The index to which the child will be added
+     */
+    protected void addChild(final B child, final int index) {


Do we need three different ways to add children? (addChild, insertChild, setChildren)
Can we reduce this maybe?

setChildren in used only in tests AFAIK so could be pulled down to DummyNode.

I think addChild is a bit unsafe/hard to use:

nothing prevents you from writing addChild(child, 42), and leave 42 null slots in the array (which breaks the contract of the class)...

adding children with this method needs to be done right to left, otherwise adding n children performs n array resizes

OTOH I can't see another way to improve it right now apart from documentation

adangel · 2020-05-02T14:25:45Z

+     *
+     * @param index the child index
+     */
+    protected void setChildIndex(final int index) {


Is this needed as a protected method? It looks error prone, if this is ever forgotten to be called. And it probably shouldn't needed to be called -> addChild etc. should automatically make sure, the tree is consistent.

Indeed, I'll look into whether we can remove it

adangel · 2020-05-02T14:29:50Z

+
+/**
+ * Interface that binds the return type of some node methods to a type
+ * parameter. This enforces that eg all children of such a node are from


I guess, we should demand that all language impls use this interface. Maybe that's just in the documentation then?

adangel · 2020-05-02T14:34:22Z

+     * it must be set to some value that depends on the node type, not some
+     * arbitrary "1" or "2", and not necessarily a unique value.
+     */
+    protected AbstractJjtreeNode(int id) {


Maybe we can name the "id" something like "productionId" or "jjtNodeNameIndex", to avoid the misinterpretation of "unique id"...

adangel · 2020-05-02T14:42:13Z


-import net.sourceforge.pmd.lang.ast.AbstractNode;
 import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.ast.impl.AbstractNode;


In theory, AbstractNode should be irrelevant in this context - AbstractNode should only help to implement the Node API, but should not add to the API... (so no additional methods).

Maybe we don't need the NoAttrScope at all? If a intermediate (abstract) class adds a public method, it is public API... so why not expose it as XPath attribute?

adangel · 2020-05-02T14:53:50Z

+    public void setImage(String image) {
+        super.setImage(image);
+    }
+


Hm... why do we have a "DummyJavaNode" in src/main? Looks like, this should have gone into src/test....

Indeed, I recall it's still in main because some rules about imports create dummy nodes for ImportWrapper... Hopefully those rules may be rewritten with the newer symbol table and ImportWrapper can be nuked

adangel · 2020-05-02T15:07:49Z

+     * Normally this is uppercase, unless the names is quoted ("name").
+     * </p>
+     */
+    default String getCanonicalImage() {


hm...yes, these methods are not ideal, too generic like "getImage". but this time on plsql...

oowekyala added 30 commits April 23, 2020 19:59

Merge branch 'deprecate-find-xx-of-any-type' into tmp

9c9f9c4

Cleanup AbstractNode

13cacee

Merge AbstractApexNode[Base]

3cfa9cb

Bring back the remove[child] methods

715b28d

Move AbstractNode in .impl package

0735011

Fewer unchecked casts

1fd866e

Add GenericNode & JjtreeNode interfaces

11d7aeb

Remove childrenAccept

35c2637

Stop ClassScope from building nodes

83189fc

Cleanup tests

197f901

Fix tests

434881e

Cleanup more things

036b2d5

Remove Dataflow methods from Node/AbstractNode

d41f4d8

Replaced with a DataKey

Doc

786a09b

Hide setImage

81c3743

Merge branch '7.0.x' into cleanup-abstract-node

24a9bf5

Merge branch '7.0.x' into cleanup-abstract-node

c74446a

Doc

443703f

Ensure Parser returns a RootNode

7028157

Make children array private, fix comment nodes

3e892e1

Add insertChild

13e414c

Add a type parameter to AbstractNode

1a6d7b3

Fix compil

aaceca5

Remove tparam of JjtreeBuilder

24c4abd

Fix dataflow code

b26eb20

Reinstate ClassScope building of enum valueOf

a237ae4

Fix apex module

7e10867

Checkstyle

1bb7e3d

Bring back SimpleNodeTest

541b4c3

No real reason to remove it, but it should be moved to pmd-core

Make AbstractPLSQLNode package-private

020f2a9

This was referenced Apr 26, 2020

[core] Merging Javacc build scripts #2239

Closed

[core] Conflicting contracts of getChild between PMD & Antlr #2235

Closed

oowekyala added 2 commits April 29, 2020 09:19

Merge branch '7.0.x' into cleanup-abstract-node

bd835c0

Merge branch '7.0.x' into cleanup-abstract-node

efef060

adangel added this to the 7.0.0 milestone May 2, 2020

adangel approved these changes May 2, 2020

View reviewed changes

adangel merged commit 57177ab into pmd:pmd/7.0.x May 2, 2020

adangel mentioned this pull request May 2, 2020

Merge #2447 [core] Cleanup Node and AbstractNode in java-grammar #2460

Merged

4 tasks

oowekyala deleted the cleanup-abstract-node branch May 2, 2020 17:27

oowekyala added a commit to pmd/pmd-designer that referenced this pull request May 2, 2020

Compat for pmd/pmd#2447

87d1a8a

adangel added the in:ast About the AST structure or API, the parsing step label Jan 12, 2023

adangel mentioned this pull request Jan 23, 2023

PMD 7 Tracking Issue #3898

Closed

55 tasks

Uh oh!

Conversation

oowekyala commented Apr 26, 2020 • edited by adangel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the PR

Related issues

Ready?

Uh oh!

ghost commented Apr 26, 2020

Uh oh!

adangel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oowekyala commented Apr 26, 2020 •

edited by adangel

Loading