[core] Tree traversal boundaries #949

jsotuyod · 2018-03-03T23:42:17Z

Fixes [core] Tree traversal revision #904
Fixes [java] GuardLogStatementJavaUtil not considering lambdas #370
hasDescendantOfType, getFirstDescendantOfType and findDescendantsOfType are now consistent

oowekyala

I'm not satisfied with the behaviour of that. Now, any call to node.getFirstDescendantOfType will return null if node is a find boundary, eg if it's a body declaration inside an anonymous class, and or a nested ClassOrInterfaceDeclaration. Consequently, nested class declaration traversal logic is inconsistent with outer classes'. For example:

class Outer {
  class Nested { // ClassOrInterfaceDeclaration[@FindBoundary=true()]
    void method() {}
  }
  void bar() {}
}


// outer and nested are the ClassOrInterfaceDeclarations
outer.getFirstDescendantOfType(ASTClassOrInterfaceDeclaration.class) == nested
outer.getFirstDescendantOfType(ASTMethodDeclaration.class) // == void bar()

nested.getFirstDescendantOfType(<anything>) == null

Under those conditions, you're forced to break the boundary with an explicit jjtGetChild call, which is impractical. I think semantically, node.getFirstDescendantOfType should search the descendants even if node.isFindBoundary() is true. Otherwise the name doesn't fit the behaviour -- which is to find descendants, regardless of the self node. It should stop recursion on find boundaries though.

Correct me if I'm wrong, but this causes the change, as it currently stands, to be API breaking, even though binary compatible. For example, you had to modify some test cases to make them work. It could break some rules too.

hasDescendantOfType, getFirstDescendantOfType and findDescendantsOfType are now consistent

findDescendantsOfType still crosses boundaries by default, which is inconsistent with the other methods. There is an "overload" which allows to change the behaviour, but its interface is inconsistent with the others since it works by performing side effects on a list given in parameter instead of returning a result list... This breaks call chains and I still think it should be taken care of

Even if we changed the "true" on the previous link to "false", the behaviour is still incorrect. In that case, on the NestedAnonymousClass type res test case, the following expression returns three results instead of two:

acu.getFirstDescendantOfType(ASTClassOrInterfaceDeclaration.class)
     .findDescendantsOfType(ASTClassOrInterfaceBodyDeclaration.class)

because it crosses the anonymous class' ClassOrInterfaceBody and adds its body declaration. I think the most logical way to treat that is to implement #905 and make that AnonymousClassDeclaration a find boundary. BodyDeclarations shoulnd't be find boundaries. Until #905, I'm not sure how we could make the behaviour consistent though...
...

There are probably other inconsistencies lurking, and I think this PR lacks a test suite to define precisely the expected behaviour. The documentation of the traversal methods could be revised too.

jsotuyod · 2018-03-04T22:02:25Z

@oowekyala thanks for your comments. You make some very interesting points.

As for ignoring self as a find boundary, I had thought about it, but felt it could be a little dangerous... I still have to think it thoroughly to be sure that can't backfire. Non the less, the test would have to be edited, as we were searching for allocations, which are not the boundary, but higher up.

As for findDescendantsOfType, I knew I was missing something. Thanks for spotting that one.

I think we can do this without #905. I'm still not sure we actually need a specific node type for anonymous classes (even if the problem you report is effectively real), but I admit I haven't been able to properly think it over, so this is mostly my gut speaking. Take it with a grain of salt.

I do not think of this as a breaking API change, and for his comment on #904 I don't think @adangel does either. This is a correction of a broken behavior. If people were relying on that broken behavior, sure, things will cease to work for them (and that's why a proper notice in the changelog is in order); but it's still a broken behavior / bug, which should be fixed ASAP. The fact that nothing on our own codebase broke other than a single test is telling on how unlikely this is to happen anyway.

I agree non the less, some boundary checking unit tests could aid. I'll try to work them out.

oowekyala · 2018-03-04T23:15:11Z

I think we can do this without #905.

Maybe by moving the boundary on the AllocationExpression if it's an anonymous class ? The danger here, is that eg the type arguments or constructor arguments would be lost behind the boundary. On the other hand, moving the boundary on the ClassOrInterfaceBody would make that Body be included in searches for node.getFirstDescendantOfType(ASTClassOrInterfaceBody.class) if node is in the enclosing class, even though I think it should be considered as behind the boundary -- as is the body of a nested or local class. I'm pretty sure this tricky situation could be resolved by #905, but in the meantime, this could perhaps be worked around by reimplementing traversal methods in AbstractJavaNode to handle anonymous class specially...

This is a correction of a broken behavior.

Yeah ok that makes sense. I should've re-read #904 before posting that comment, sorry ^^

oowekyala · 2018-03-05T10:42:05Z

...java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/AbstractCommentRule.java

-        List<ASTClassOrInterfaceDeclaration> packageDecl = cUnit
-                .findDescendantsOfType(ASTClassOrInterfaceDeclaration.class);
-        addDeclarations(itemsByLineNumber, packageDecl);
+        List<ASTClassOrInterfaceDeclaration> classDecl = new ArrayList<>();


I still think it would be nice to remove that extra statement by adding an overload <T> List<T> findDescendantsOfType(Class<T>, boolean). This would be consistent with the signature and usage of all other find* methods -- which return a list instead of void. These variables could then be initialized with a single expression

This has been done on AbstractNode, but adding it to the interface will have to wait to 7.0.0 (add it to #881 when merging). The same applies to PreserveStackTraceRule where such a change could not be taken advantage of due to the usage of jjtGetParent which returns Node

- This fixes pmd#904 - `hasDescendantOfType`, `getFirstDescendantOfType` and `findDescendantsOfType` are now consistent

- Fixes pmd#370

- Also break once we have a result

- A few places actually need to do so, and some other were simply wrong - We can now cross the boundary if searching dowm from it - Anonymous inner classes are still somewhat inconsistent

oowekyala · 2018-03-22T13:13:10Z

@jsotuyod Let me know when you think this is ready for review

jsotuyod · 2018-03-23T04:25:15Z

Ok, this should be ready now.

The inconsistencies with anonymous inner classes (which predated this PR) remains, but the current grammar doesn't allow a fix. So until we take on #905 on PMD 7.0.0 that will remain as-is.

Standing in a find boundary we can now drill down without further overhead, meaning inconsistencies between outer / inner classes are no more.

Outdated review

oowekyala

LGTM. This is a really nice changeset :) You should just merge master and it would be good to merge

oowekyala · 2018-03-27T13:11:12Z

pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java

+        final List<T> list = new ArrayList<>();
+        findDescendantsOfType(this, targetType, list, crossBoundaries);
+        return list;
+    }

    @Override
    public <T> void findDescendantsOfType(Class<T> targetType, List<T> results, boolean crossBoundaries) {


We should probably deprecate the public use of that overload in 7.0.0, when the new one is added to the Node interface

good idea, will do before merging

oowekyala · 2018-03-27T13:13:41Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java

@@ -103,7 +103,7 @@ private String getMethodOrAttributeName(ASTPrimaryExpression node) {


    private boolean isAttributeAccess(ASTPrimaryExpression node) {
-        return node.findDescendantsOfType(ASTPrimarySuffix.class).isEmpty();
+        return !node.hasDescendantOfType(ASTPrimarySuffix.class);


Nice find! We could maybe make a rule for our dogfood ruleset that detects that

Definitely. There is plenty we can do in dogfooding. Avoiding calls to super.visit on rules using rulechain is at the top my list. We should maybe create an issue to track these...

oowekyala · 2018-03-27T13:18:25Z

...main/java/net/sourceforge/pmd/lang/java/rule/codestyle/CommentDefaultAccessModifierRule.java

-                }
-                if (result == false) {
+                final ASTName name = annotation.getFirstDescendantOfType(ASTName.class);
+                if (name.hasImageEqualTo("VisibleForTesting")) {


Looks to me like this rule should extend AbstractIgnoredAnnotationRule. Once you merge master you can probably do that (or in another PR)

Definitely another PR, but a great idea non the less. That refactor just keeps paying off

jsotuyod added the in:pmd-internals Affects PMD's internals label Mar 3, 2018

jsotuyod added this to the 6.2.0 milestone Mar 3, 2018

oowekyala previously requested changes Mar 4, 2018

View reviewed changes

jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Mar 5, 2018

oowekyala reviewed Mar 5, 2018

View reviewed changes

oowekyala changed the title ~~[core] Tree transversal boundaries~~ [core] Tree traversal boundaries Mar 10, 2018

jsotuyod added 8 commits March 12, 2018 01:13

[java] Honor find boundaries

f725f1e

- This fixes pmd#904 - `hasDescendantOfType`, `getFirstDescendantOfType` and `findDescendantsOfType` are now consistent

Fix failing tests

ba54b1c

Add test case for pmd#370

19c3487

- Fixes pmd#370

Update changelog, refs pmd#370 & pmd#904

180cfc1

Remove no longer needed parent check

cdc50f7

- Also break once we have a result

findDescendantsOfType defaults to not crossing boundaries

8deaf20

- A few places actually need to do so, and some other were simply wrong - We can now cross the boundary if searching dowm from it - Anonymous inner classes are still somewhat inconsistent

Simplify test

8b3fd24

Remove unused imports

240d26e

jsotuyod force-pushed the tree-transversal-boundaries branch from 8675b50 to 240d26e Compare March 12, 2018 04:20

jsotuyod added 4 commits March 12, 2018 01:41

Fix bad merge

783b449

Checkstyle fixes

0eef25e

Simplify rules when possible

46ab578

Revamp changelog

b3a9b90

jsotuyod added 4 commits March 23, 2018 00:52

Simplify calls where possible

a1ea36a

Improve javadocs

809310c

Add unit test for boundary checking

257d11e

Merge branch 'master' into tree-transversal-boundaries

c71db57

jsotuyod removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Mar 23, 2018

oowekyala mentioned this pull request Mar 23, 2018

[java] Fix type resolution inconsistency in multiple variable declaration #945

Merged

adangel removed this from the 6.2.0 milestone Mar 26, 2018

adangel modified the milestone: 6.3.0 Mar 26, 2018

jsotuyod added this to the 6.3.0 milestone Mar 26, 2018

oowekyala approved these changes Mar 27, 2018

View reviewed changes

Merge branch 'master' into tree-transversal-boundaries

1e3b1bf

jsotuyod merged commit 1e3b1bf into pmd:master Mar 29, 2018

jsotuyod deleted the tree-transversal-boundaries branch March 29, 2018 04:57

oowekyala mentioned this pull request Mar 30, 2018

[java] UnnecessaryConstructor triggered on required empty constructor (Dagger @Inject) #1010

Merged

jsotuyod mentioned this pull request May 7, 2018

[java] Performance degradation of 250% between 6.1.0 and 6.2.0 #1018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Tree traversal boundaries #949

[core] Tree traversal boundaries #949

jsotuyod commented Mar 3, 2018

oowekyala left a comment •

edited

Loading

jsotuyod commented Mar 4, 2018

oowekyala commented Mar 4, 2018

oowekyala Mar 5, 2018

jsotuyod Mar 23, 2018

oowekyala commented Mar 22, 2018

jsotuyod commented Mar 23, 2018

oowekyala left a comment

oowekyala Mar 27, 2018

jsotuyod Mar 29, 2018

oowekyala Mar 27, 2018

jsotuyod Mar 29, 2018

oowekyala Mar 27, 2018

jsotuyod Mar 29, 2018

[core] Tree traversal boundaries #949

[core] Tree traversal boundaries #949

Conversation

jsotuyod commented Mar 3, 2018

oowekyala left a comment • edited Loading

Choose a reason for hiding this comment

jsotuyod commented Mar 4, 2018

oowekyala commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oowekyala commented Mar 22, 2018

jsotuyod commented Mar 23, 2018

oowekyala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oowekyala left a comment •

edited

Loading