From 98e27294f0735680585117e90f4d7f0071f6ceef Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 3 Aug 2013 15:47:38 +0200 Subject: [PATCH] pmd: fix #1115 commentRequiredRule in pmd 5.1 is not working properly --- pmd/etc/changelog.txt | 1 + .../rule/comments/AbstractCommentRule.java | 124 +++++++++++------- .../pmd/testframework/RuleTst.java | 28 +++- .../rule/comments/xml/CommentRequired.xml | 26 ++++ 4 files changed, 130 insertions(+), 49 deletions(-) diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index d4a66cd9f7..594a55e55f 100644 --- a/pmd/etc/changelog.txt +++ b/pmd/etc/changelog.txt @@ -1,6 +1,7 @@ ????? ??, 2013 - 5.1.0: Fixed bug 1059: Change rule name "Use Singleton" should be "Use Utility class" +Fixed bug 1115: commentRequiredRule in pmd 5.1 is not working properly New EcmaScript rules and ruleset: Controversial ruleset, featuring diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/comments/AbstractCommentRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/comments/AbstractCommentRule.java index db9c4f9a65..a31985b9e7 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/comments/AbstractCommentRule.java +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/comments/AbstractCommentRule.java @@ -3,19 +3,21 @@ */ package net.sourceforge.pmd.lang.java.rule.comments; -import java.util.List; -import java.util.Map.Entry; import java.util.ArrayList; import java.util.Collections; +import java.util.List; +import java.util.Map.Entry; import java.util.SortedMap; import java.util.TreeMap; +import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.AbstractJavaAccessNode; +import net.sourceforge.pmd.lang.java.ast.AbstractJavaAccessTypeNode; import net.sourceforge.pmd.lang.java.ast.Comment; import net.sourceforge.pmd.lang.java.ast.FormalComment; import net.sourceforge.pmd.lang.java.ast.MultiLineComment; @@ -37,7 +39,7 @@ public abstract class AbstractCommentRule extends AbstractJavaRule { int atPos = comments.indexOf('@'); if (atPos < 0) - return Collections.EMPTY_LIST; + return Collections.emptyList(); List ints = new ArrayList(); ints.add(atPos); @@ -146,63 +148,89 @@ public abstract class AbstractCommentRule extends AbstractJavaRule { } protected void assignCommentsToDeclarations(ASTCompilationUnit cUnit) { - SortedMap itemsByLineNumber = orderedCommentsAndDeclarations(cUnit); + SortedMap itemsByLineNumber = orderedCommentsAndDeclarations(cUnit); FormalComment lastComment = null; + AbstractJavaAccessNode lastNode = null; + + for (Entry entry : itemsByLineNumber.entrySet()) { + Node value = entry.getValue(); - for (Entry entry : itemsByLineNumber.entrySet()) { - Object value = entry.getValue(); if (lastComment == null) { if (value instanceof FormalComment) { lastComment = (FormalComment) value; + } else { + // else this is declaration without comment + if (!(value instanceof AbstractJavaAccessTypeNode)) { + lastNode = (AbstractJavaAccessNode) value; + } } - // else this is declaration without comment } else if (value instanceof AbstractJavaAccessNode) { AbstractJavaAccessNode node = (AbstractJavaAccessNode) value; - node.comment(lastComment); - lastComment = null; + + // maybe the last comment is within the last node + if (isCommentNotWithin(lastComment, lastNode) && isCommentBefore(lastComment, node)) { + node.comment(lastComment); + lastComment = null; + } + if (!(node instanceof AbstractJavaAccessTypeNode)) { + lastNode = node; + } } } } - /** - * - * @since - * @param cUnit - * @return bla - */ - protected SortedMap orderedCommentsAndDeclarations( - ASTCompilationUnit cUnit) { + private boolean isCommentNotWithin(FormalComment n1, Node n2) { + if (n1 == null || n2 == null) { + return true; + } + if ((n1.getEndLine() < n2.getEndLine()) + || (n1.getEndLine() == n2.getEndLine() && n1.getEndColumn() < n2.getEndColumn())) { + return false; + } else { + return true; + } + } - SortedMap itemsByLineNumber = new TreeMap(); - - List packageDecl = cUnit - .findDescendantsOfType(ASTClassOrInterfaceDeclaration.class); - for (ASTClassOrInterfaceDeclaration decl : packageDecl) { - itemsByLineNumber.put(decl.getBeginLine(), decl); - } - - for (Comment comment : cUnit.getComments()) { - itemsByLineNumber.put(comment.getBeginLine(), comment); - } - - List fields = cUnit - .findDescendantsOfType(ASTFieldDeclaration.class); - for (ASTFieldDeclaration fieldDecl : fields) { - itemsByLineNumber.put(fieldDecl.getBeginLine(), fieldDecl); - } - - List methods = cUnit - .findDescendantsOfType(ASTMethodDeclaration.class); - for (ASTMethodDeclaration methodDecl : methods) { - itemsByLineNumber.put(methodDecl.getBeginLine(), methodDecl); - } - - List constructors = cUnit - .findDescendantsOfType(ASTConstructorDeclaration.class); - for (ASTConstructorDeclaration constructorDecl : constructors) { - itemsByLineNumber.put(constructorDecl.getBeginLine(), constructorDecl); - } - - return itemsByLineNumber; + private boolean isCommentBefore(FormalComment n1, Node n2) { + if ((n1.getEndLine() < n2.getBeginLine()) + || (n1.getEndLine() == n2.getBeginLine() && n1.getEndColumn() < n2.getBeginColumn())) { + return true; + } else { + return false; + } } + + /** + * + * @since + * @param cUnit + * @return bla + */ + protected SortedMap orderedCommentsAndDeclarations(ASTCompilationUnit cUnit) { + + SortedMap itemsByLineNumber = new TreeMap(); + + List packageDecl = cUnit + .findDescendantsOfType(ASTClassOrInterfaceDeclaration.class); + addDeclarations(itemsByLineNumber, packageDecl); + + addDeclarations(itemsByLineNumber, cUnit.getComments()); + + List fields = cUnit.findDescendantsOfType(ASTFieldDeclaration.class); + addDeclarations(itemsByLineNumber, fields); + + List methods = cUnit.findDescendantsOfType(ASTMethodDeclaration.class); + addDeclarations(itemsByLineNumber, methods); + + List constructors = cUnit.findDescendantsOfType(ASTConstructorDeclaration.class); + addDeclarations(itemsByLineNumber, constructors); + + return itemsByLineNumber; + } + + private void addDeclarations(SortedMap map, List nodes) { + for (Node node : nodes) { + map.put((node.getBeginLine() << 16) + node.getBeginColumn(), node); + } + } } diff --git a/pmd/src/test/java/net/sourceforge/pmd/testframework/RuleTst.java b/pmd/src/test/java/net/sourceforge/pmd/testframework/RuleTst.java index 48cbedda9f..1cf930caaf 100644 --- a/pmd/src/test/java/net/sourceforge/pmd/testframework/RuleTst.java +++ b/pmd/src/test/java/net/sourceforge/pmd/testframework/RuleTst.java @@ -9,6 +9,7 @@ import static org.junit.Assert.fail; import java.io.IOException; import java.io.InputStream; import java.io.StringReader; +import java.io.StringWriter; import java.util.Map; import java.util.Properties; @@ -29,6 +30,7 @@ import net.sourceforge.pmd.RuleSetNotFoundException; import net.sourceforge.pmd.RuleSets; import net.sourceforge.pmd.lang.Language; import net.sourceforge.pmd.lang.LanguageVersion; +import net.sourceforge.pmd.renderers.TextRenderer; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -75,6 +77,7 @@ public abstract class RuleTst { Map, Object> oldProperties = rule.getPropertiesByPropertyDescriptor(); try { int res; + Report report; try { // Set test specific properties onto the Rule if (test.getProperties() != null) { @@ -90,11 +93,13 @@ public abstract class RuleTst { } } - res = processUsingStringReader(test.getCode(), rule, test.getLanguageVersion()).size(); + report = processUsingStringReader(test.getCode(), rule, test.getLanguageVersion()); + res = report.size(); } catch (Throwable t) { t.printStackTrace(); throw new RuntimeException('"' + test.getDescription() + "\" failed", t); } + printReport(test, report); assertEquals('"' + test.getDescription() + "\" resulted in wrong number of failures,", test.getNumberOfProblemsExpected(), res); } finally { @@ -107,6 +112,27 @@ public abstract class RuleTst { } } + private void printReport(TestDescriptor test, Report report) { + if (test.getNumberOfProblemsExpected() != report.size()) { + System.out.println("--------------------------------------------------------------"); + System.out.println("Test Failure: " + test.getDescription()); + System.out.println(" -> Expected " + test.getNumberOfProblemsExpected() + " problem(s), but " + + report.size() + " problem(s) found."); + System.out.println(); + TextRenderer renderer = new TextRenderer(); + renderer.setWriter(new StringWriter()); + try { + renderer.start(); + renderer.renderFileReport(report); + renderer.end(); + } catch (IOException e) { + throw new RuntimeException(e); + } + System.out.println(renderer.getWriter().toString()); + System.out.println("--------------------------------------------------------------"); + } + } + private Report processUsingStringReader(String code, Rule rule, LanguageVersion languageVersion) throws PMDException { Report report = new Report(); diff --git a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/comments/xml/CommentRequired.xml b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/comments/xml/CommentRequired.xml index ee90e44c87..759a6114e3 100755 --- a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/comments/xml/CommentRequired.xml +++ b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/comments/xml/CommentRequired.xml @@ -61,4 +61,30 @@ public class Foo { Unwanted + + #1115 commentRequiredRule in pmd 5.1 is not working properly + 2 + + + + #1115 commentRequiredRule in pmd 5.1 is not working properly - without new lines + 2 + +