Merge pull request #3156 from

adangel:issue-3142-apex-excessive-class-length

[apex] ExcessiveClassLength multiple warning on the same class #3156

docs/pages/release_notes.md

@@ -22,6 +22,8 @@ the parser. More information can be found in the [language specific documentatio
 
 ### Fixed Issues
 
+*   apex-design
+    *   [#3142](https://github.com/pmd/pmd/issues/3142): \[apex] ExcessiveClassLength multiple warning on the same class
 *   java
     *   [#3117](https://github.com/pmd/pmd/issues/3117): \[java] Infinite loop when parsing invalid code nested in lambdas
     *   [#3145](https://github.com/pmd/pmd/issues/3145): \[java] Parse exception when using "record" as variable name

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTMethod.java

@@ -34,8 +34,45 @@ public String getCanonicalName() {
         return node.getMethodInfo().getCanonicalName();
     }
 
+    @Override
+    public int getBeginLine() {
+        if (!hasRealLoc()) {
+            // this is a synthetic method, only in the AST, not in the source
+            // search for the last sibling with real location from the end
+            // and place this synthetic method after it.
+            for (int i = getParent().getNumChildren() - 1; i >= 0; i--) {
+                ApexNode<?> sibling = getParent().getChild(i);
+                if (sibling.hasRealLoc()) {
+                    return sibling.getEndLine();
+                }
+            }
+        }
+        return super.getBeginLine();
+    }
+
+    @Override
+    public int getBeginColumn() {
+        if (!hasRealLoc()) {
+            // this is a synthetic method, only in the AST, not in the source
+            // search for the last sibling with real location from the end
+            // and place this synthetic method after it.
+            for (int i = getParent().getNumChildren() - 1; i >= 0; i--) {
+                ApexNode<?> sibling = getParent().getChild(i);
+                if (sibling.hasRealLoc()) {
+                    return sibling.getEndColumn();
+                }
+            }
+        }
+        return super.getBeginColumn();
+    }
+
     @Override
     public int getEndLine() {
+        if (!hasRealLoc()) {
+            // this is a synthetic method, only in the AST, not in the source
+            return this.getBeginLine();
+        }
+
         ASTBlockStatement block = getFirstChildOfType(ASTBlockStatement.class);
         if (block != null) {
             return block.getEndLine();
@@ -46,6 +83,11 @@ public int getEndLine() {
 
     @Override
     public int getEndColumn() {
+        if (!hasRealLoc()) {
+            // this is a synthetic method, only in the AST, not in the source
+            return this.getBeginColumn();
+        }
+
         ASTBlockStatement block = getFirstChildOfType(ASTBlockStatement.class);
         if (block != null) {
             return block.getEndColumn();

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexRootNode.java

@@ -20,12 +20,25 @@ public ApexRootNode(T node) {
         super(node);
     }
 
-    // For top level classes, the end is the end of file.
     @Override
     void calculateLineNumbers(SourceCodePositioner positioner) {
         super.calculateLineNumbers(positioner);
-        this.endLine = positioner.getLastLine();
-        this.endColumn = positioner.getLastLineColumn();
+
+        if (getParent() == null) {
+            // For top level classes, the end is the end of file.
+            this.endLine = positioner.getLastLine();
+            this.endColumn = positioner.getLastLineColumn();
+        } else {
+            // For nested classes, look for the position of the last child, which has a real location
+            for (int i = getNumChildren() - 1; i >= 0; i--) {
+                ApexNode<?> child = getChild(i);
+                if (child.hasRealLoc()) {
+                    this.endLine = child.getEndLine();
+                    this.endColumn = child.getEndColumn();
+                    break;
+                }
+            }
+        }
     }
 
     /**

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java

@@ -283,7 +283,6 @@ static <T extends AstNode> AbstractApexNode<T> createNodeAdapter(T node) {
     public <T extends AstNode> ApexNode<T> build(T astNode) {
         // Create a Node
         AbstractApexNode<T> node = createNodeAdapter(astNode);
-        node.calculateLineNumbers(sourceCodePositioner);
         node.handleSourceCode(sourceCode);
 
         // Append to parent
@@ -305,6 +304,11 @@ public <T extends AstNode> ApexNode<T> build(T astNode) {
             addFormalComments();
         }
 
+        // calculate line numbers after the tree is built
+        // so that we can look at parent/children to figure
+        // out the positions if necessary.
+        node.calculateLineNumbers(sourceCodePositioner);
+
         return node;
     }
 

pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java

@@ -179,6 +179,43 @@ public void stackOverflowDuringClassParsing() throws Exception {
         Assert.assertEquals(487, count);
     }
 
+    @Test
+    public void verifyLineColumnNumbersInnerClasses() throws Exception {
+        String source = IOUtils.toString(ApexParserTest.class.getResourceAsStream("InnerClassLocations.cls"),
+                StandardCharsets.UTF_8);
+        source = source.replaceAll("\r\n", "\n");
+        ApexNode<Compilation> rootNode = parse(source);
+        Assert.assertNotNull(rootNode);
+
+        visitPosition(rootNode, 0);
+
+        Assert.assertEquals("InnerClassLocations", rootNode.getImage());
+        // Note: Apex parser doesn't provide positions for "public class" keywords. The
+        // position of the UserClass node is just the identifier. So, the node starts
+        // with the identifier and not with the first keyword in the file...
+        assertPosition(rootNode, 1, 14, 16, 2);
+
+        List<ASTUserClass> classes = rootNode.findDescendantsOfType(ASTUserClass.class);
+        Assert.assertEquals(2, classes.size());
+        Assert.assertEquals("bar1", classes.get(0).getImage());
+        List<ASTMethod> methods = classes.get(0).findChildrenOfType(ASTMethod.class);
+        Assert.assertEquals(2, methods.size()); // m() and synthetic clone()
+        Assert.assertEquals("m", methods.get(0).getImage());
+        assertPosition(methods.get(0), 4, 21, 7, 9);
+        Assert.assertEquals("clone", methods.get(1).getImage());
+        assertPosition(methods.get(1), 7, 9, 7, 9);
+
+        // Position of the first inner class: starts with the identifier "bar1" and ends with
+        // the last real method m(). The last bracket it actually on the next line 8, but we
+        // don't see this in the AST.
+        assertPosition(classes.get(0), 3, 18, 7, 9);
+
+        Assert.assertEquals("bar2", classes.get(1).getImage());
+        assertPosition(classes.get(1), 10, 18, 14, 9);
+    }
+
+    // TEST HELPER
+
     private int visitPosition(Node node, int count) {
         int result = count + 1;
         Assert.assertTrue(node.getBeginLine() > 0);
@@ -191,8 +228,6 @@ private int visitPosition(Node node, int count) {
         return result;
     }
 
-    // TEST HELPER
-
     private static void assertPosition(Node node, int beginLine, int beginColumn, int endLine, int endColumn) {
         assertEquals("Wrong begin line", beginLine, node.getBeginLine());
         assertEquals("Wrong begin column", beginColumn, node.getBeginColumn());

pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/ast/InnerClassLocations.cls

@@ -0,0 +1,16 @@
+public class InnerClassLocations {
+
+    public class bar1 {
+        public void m() {
+            System.out.println('foo');
+            System.out.println('foo');
+        }
+    }
+
+    public class bar2 {
+        public void m() {
+            System.out.println('foo');
+            System.out.println('foo');
+        }
+    }
+}

pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/design/xml/ExcessiveClassLength.xml

@@ -91,4 +91,29 @@ public class SomeClass {
 }
        ]]></code>
     </test-code>
+
+    <test-code>
+        <description>[apex] ExcessiveClassLength multiple warning on the same class #3142</description>
+        <rule-property name="minimum">10</rule-property>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>1</expected-linenumbers>
+        <code><![CDATA[
+public class FooSmaller {
+
+    public class bar1 {
+        public void m() {
+            System.out.println('foo');
+            System.out.println('foo');
+        }
+    }
+
+    public class bar2 {
+        public void m() {
+            System.out.println('foo');
+            System.out.println('foo');
+        }
+    }
+}
+        ]]></code>
+    </test-code>
 </test-data>