[core] Add remove operation over nodes by MatiasComercio · Pull Request #696 · pmd/pmd

MatiasComercio · 2017-10-29T16:26:50Z

Please, prefix the PR title with the language it applies to within brackets, such as [java] or [apex]. If not specific to a language, you can use [core]

Before submitting a PR, please check that:

The PR is submitted against master. The PMD team will merge back to support branches as needed.
mvn test passes.
mvn checkstyle:check passes. Check this for more info

PR Description:
Implements: #693: Add remove operation over nodes

List of changes

Add remove & removeChildAtIndex to Node interface
- Implement these methods at AbstractNode.
- Add AbstractNodeTest class with test cases for the implemented methods.
  - Add JUnitParams dependency: https://github.com/Pragmatists/JUnitParams
  - Implement these test cases using JUnitParams features
Other changes:
- Fix missing validation on cast for AbstractJavaRule

- Implement these methods at AbstractNode. - Add AbstractNodeTest class with test cases for the implemented methods. - Add JUnitParams dependency: https://github.com/Pragmatists/JUnitParams - Implement these test cases using JUnitParams features

jsotuyod · 2017-10-29T18:50:28Z

+            // Remove the child at the given index
+            children = ArrayUtils.remove(children, childIndex);
+            // Update the remaining children indexes
+            for (int i = 0; i < jjtGetNumChildren(); i++) {


This loop can safely start at childIndex, since previous indexes should not be affected

jsotuyod · 2017-10-29T18:57:33Z

+        // Check that the child has the expected properties
+        assertEquals(NUM_CHILDREN, rootNode.jjtGetNumChildren());
+        assertEquals(rootNode, child.jjtGetParent());
+        assertEquals(NUM_GRAND_CHILDREN, child.jjtGetNumChildren());


these asserts are just checking your tree setup was correct? you really shouldn't, tests should:

setup

exercise the tested behavior (in this case remove())

assert the output is as expected

jsotuyod · 2017-10-29T19:07:48Z

+
+    @Override
+    public void remove() {
+        // Detach current node of all its children, if any


why do we need to do this? can't I just remove a subtree and add it as is somewhere else? I believe this will actually make operations harder.

For instance, I want to replace a Node instance but keep it's children untouched. If I remove first, I loose the children. If I remove last, the children will be detached of it's parent (whichever it may be). I'd have to remove the child nodes, one by one, and add them to the new parent, but that would translate the problem to grandchildren...

Suppose I want to reorder nodes (ie, to autofix a rule regarding field / constructor / method declaration ordering). The easiest thing would be to take the out of place node (ASTFieldDeclaration, ASTMethodDeclaration, etc.) and put it back in at the correct place. This method once again would make that impossible to do. I'd have to add an extra method to "swap" elements, meaning more boilerplate and harder implementations for other grammar engines such as Antlr4 (soon to come to PMD /cc @matifraga @tomidelucca )

@jsotuyod I got your point.

This has been done so because it has been the less intrusive way that I have found to stop the visitor pattern through the recently removed tree, since all language-specific abstract nodes seem to have the visitor chain implemented as follows: if the current node's children array is not null it is iterated so as to visit all its children.

The solution I have come up with to handle both cases (supporting the usage you mention and stopping the visitor pattern) is to not detach the children of the node being removed and to add a new field to the abstract node class, a kind of areChildrenAccepted, with its default value being true, so as to update all language-specific abstract nodes to check this condition too before visiting node's children.
Then, when a node is removed, the only thing we have to do to stop the visitor is to set this field to false. When a node is add as a child of another node, we should set this field in true. Caution should be taken if a root node is removed and it wants to be enabled again as root node, as the areChildrenAccepted field should be explicitly set to true - however, I think this case is much less common than the other ones mentioned.

What do you think about this?

Ok, I get a better picture now.

The bottom line here however is: when do you intend to apply the change to the AST? Altering the AST during visitation has deeper problems than what happens to the children, but also, what happens to parents / siblings.

As the code stands, suppose I have a root node with 2 children. Upon visiting the first one, this one will remove itself from the tree.

This will happen as follows:

we visit the root node

we call childrenAccept on the root node

we visit the first node, call remove, and the parent is updated to have only 1 children

as the childrenAccept loop resumes, our current index (i) is incremented from 0 to 1, we check against children.length (now 1 since we removed a node), and stop.

We never visited the second child.

I think it would be far saner to never attempt a tree modification during transversal. The autofix needs to be in 2 stages:

we visit all nodes and detect violations.

we apply fixes.

we move on to the next rule

Notice that ParametricRuleViolation, which is the base class for RuleViolations does receive both the Rule and Node as parameters, allowing you to eventually come up with autofixes later on. Just never keep the Node indefinitely, since the RuleViolation instance is kept during the whole PMD execution and that would prevent all ASTs from being garbage collected.

Good catch & idea!

I'll leave a reference to this comment in the main issue.
I'll then remove the detachment of all the children.

As the remove method may be called during transversal (although it shouldn't), what do you think about adding some kind of lock for node modifications during transversal in another PR?
This lock is also intended to work with the add and replace methods that we are going to develop.

Just let me know and I'll add that feature to the main issue too 😄

I thought about the lock too. I like the idea in general, I'm hesitant as to how to effectively implement it...

adding a boolean field to all nodes implies a big memory footprint

having it at the root of the AST means navigating up the tree all the time to check it

I think it needs some thinking, but definitely not part of this PR

Great! Then I'll add that feature to the main issue.
We may discuss the implementation details soon. I'll let you know if I come up with any idea on how to solve this.

jsotuyod · 2017-10-29T19:09:01Z

+    public void testRemoveRootNode() {
+        // Check that the root node has the expected properties
+        assertEquals(NUM_CHILDREN, rootNode.jjtGetNumChildren());
+        assertNull(rootNode.jjtGetParent());


jsotuyod · 2017-10-29T19:09:12Z

+        // Check that the child has the expected properties
+        assertEquals(NUM_GRAND_CHILDREN, child.jjtGetNumChildren());
+        assertEquals(0, grandChild.jjtGetNumChildren());
+        assertEquals(child, grandChild.jjtGetParent());


jsotuyod · 2017-10-29T19:09:21Z

+                assertNotEquals(originalChildren[i - 1], originalChildren[i]);
+            }
+        }
+        assertEquals(NUM_CHILDREN, rootNode.jjtGetNumChildren());


jsotuyod · 2017-10-29T19:11:04Z

+     */
+    @Test
+    public void testRemoveChildAtIndexWithInvalidIndex() {
+        // No assert as the test is considered passed if no exception is thrown


to make this explicit, you should try-catch the whole block, and add a fail on the catch. That would also comply with our JUnitTestsShouldIncludeAssert rule

jsotuyod · 2017-10-29T19:11:15Z

+    public void testRemoveChildAtIndexOnNodeWithNoChildren() {
+        final Node grandChild = rootNode.jjtGetChild(0).jjtGetChild(0);
+        // Check that this node does not have any children
+        assertEquals(0, grandChild.jjtGetNumChildren());


- Remove unnecessary asserts for checking setup - Improve `for` loop performance in AbstractNode.removeChildAtIndex method - Remove unnecessary detachment of the node being removed children

MatiasComercio added 2 commits October 28, 2017 19:47

Fix missing validation on cast for AbstractJavaRule

8e120da

MatiasComercio mentioned this pull request Oct 29, 2017

[core] Autofixes feature development #693

Open

14 tasks

jsotuyod reviewed Oct 29, 2017

View reviewed changes

jsotuyod self-assigned this Oct 29, 2017

jsotuyod added an:enhancement An improvement on existing features / rules in:pmd-internals Affects PMD's internals labels Oct 29, 2017

jsotuyod added this to the 6.0.0 milestone Oct 29, 2017

Fix PR - 1

9348c54

- Remove unnecessary asserts for checking setup - Improve `for` loop performance in AbstractNode.removeChildAtIndex method - Remove unnecessary detachment of the node being removed children

MatiasComercio force-pushed the add-remove-node-feature branch from 2b8cd11 to 9348c54 Compare October 30, 2017 18:39

jsotuyod merged commit 9348c54 into pmd:master Nov 4, 2017

jsotuyod added a commit that referenced this pull request Nov 4, 2017

Update changelog, refs #696

9e894fd

MatiasComercio deleted the add-remove-node-feature branch November 7, 2017 11:38

oowekyala added the in:autofixes Affects the autofixes framework label Jan 23, 2018

Uh oh!

Conversation

MatiasComercio commented Oct 29, 2017

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!

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!

3 participants