From 9a6517561e61ae7b1ac9f350f3f6dd24fa055e55 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 4 Mar 2021 12:47:07 +0100 Subject: [PATCH 1/2] [apex] ExcessiveClassLength multiple warning on the same class - fixes #3142 --- docs/pages/release_notes.md | 3 ++ .../pmd/lang/apex/ast/ASTMethod.java | 42 +++++++++++++++++++ .../pmd/lang/apex/ast/ApexRootNode.java | 19 +++++++-- .../pmd/lang/apex/ast/ApexTreeBuilder.java | 6 ++- .../pmd/lang/apex/ast/ApexParserTest.java | 38 ++++++++++++++++- .../pmd/lang/apex/ast/InnerClassLocations.cls | 16 +++++++ .../rule/design/xml/ExcessiveClassLength.xml | 25 +++++++++++ 7 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/ast/InnerClassLocations.cls diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555e..a54415795c9 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,6 +16,9 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues +* apex-design + * [#3142](https://github.com/pmd/pmd/issues/3142): \[apex] ExcessiveClassLength multiple warning on the same class + ### API Changes ### External Contributions diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTMethod.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTMethod.java index 79f0e6bd88d..bd2b26751ec 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTMethod.java +++ b/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(); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexRootNode.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexRootNode.java index 13f756ca1d9..ffab8193bc0 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexRootNode.java +++ b/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; + } + } + } } /** diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java index a317869d2cd..9b0c4974093 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java @@ -283,7 +283,6 @@ static AbstractApexNode createNodeAdapter(T node) { public ApexNode build(T astNode) { // Create a Node AbstractApexNode node = createNodeAdapter(astNode); - node.calculateLineNumbers(sourceCodePositioner); node.handleSourceCode(sourceCode); // Append to parent @@ -305,6 +304,11 @@ public ApexNode 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; } diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java index 4ef39d585a6..d2a85511056 100644 --- a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java @@ -179,6 +179,42 @@ 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); + ApexNode 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 classes = rootNode.findDescendantsOfType(ASTUserClass.class); + Assert.assertEquals(2, classes.size()); + Assert.assertEquals("bar1", classes.get(0).getImage()); + List 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 +227,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()); diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/ast/InnerClassLocations.cls b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/ast/InnerClassLocations.cls new file mode 100644 index 00000000000..f55cd3719f8 --- /dev/null +++ b/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'); + } + } +} diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/design/xml/ExcessiveClassLength.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/design/xml/ExcessiveClassLength.xml index fe40936f7fc..bd49bb9340a 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/design/xml/ExcessiveClassLength.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/design/xml/ExcessiveClassLength.xml @@ -91,4 +91,29 @@ public class SomeClass { } ]]> + + + [apex] ExcessiveClassLength multiple warning on the same class #3142 + 10 + 1 + 1 + + From 0c16a321dc8fb2e20cb2cf6ac9c4df10e43b8e75 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 4 Mar 2021 15:03:40 +0100 Subject: [PATCH 2/2] Fix build under Windows - line endings... --- .../java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java index d2a85511056..346e6568113 100644 --- a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java @@ -183,6 +183,7 @@ public void stackOverflowDuringClassParsing() throws Exception { public void verifyLineColumnNumbersInnerClasses() throws Exception { String source = IOUtils.toString(ApexParserTest.class.getResourceAsStream("InnerClassLocations.cls"), StandardCharsets.UTF_8); + source = source.replaceAll("\r\n", "\n"); ApexNode rootNode = parse(source); Assert.assertNotNull(rootNode);