From e027871f2a909e01c5a7d9a0638afe34dd864708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Wed, 8 Feb 2017 10:25:45 -0300 Subject: [PATCH 1/3] [java] Add AccessorMethodGeneration rule - This resolves #1496 - There is still an outstanding false negative, which is down to a bug in symbol table analysis, I'm leaving that out for the moment, and address it on a separate PR. --- .../design/AccessorMethodGenerationRule.java | 74 ++++++++ .../main/resources/rulesets/java/design.xml | 32 ++++ .../java/rule/design/DesignRulesTest.java | 1 + .../design/xml/AccessorMethodGeneration.xml | 169 ++++++++++++++++++ 4 files changed, 276 insertions(+) create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorMethodGenerationRule.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorMethodGenerationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorMethodGenerationRule.java new file mode 100644 index 0000000000..43a86d19c9 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/AccessorMethodGenerationRule.java @@ -0,0 +1,74 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.design; + +import java.util.List; +import java.util.Map; + +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.ASTEnumDeclaration; +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.rule.AbstractJavaRule; +import net.sourceforge.pmd.lang.java.symboltable.AbstractJavaScope; +import net.sourceforge.pmd.lang.java.symboltable.ClassNameDeclaration; +import net.sourceforge.pmd.lang.java.symboltable.ClassScope; +import net.sourceforge.pmd.lang.java.symboltable.MethodNameDeclaration; +import net.sourceforge.pmd.lang.java.symboltable.SourceFileScope; +import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; +import net.sourceforge.pmd.lang.symboltable.NameOccurrence; + +public class AccessorMethodGenerationRule extends AbstractJavaRule { + + public Object visit(final ASTCompilationUnit node, final Object data) { + final SourceFileScope file = node.getScope().getEnclosingScope(SourceFileScope.class); + analyzeScope(file, data); + + return data; // Stop tree navigation + } + + private void analyzeScope(final AbstractJavaScope file, final Object data) { + for (final ClassNameDeclaration classDecl : file.getDeclarations(ClassNameDeclaration.class).keySet()) { + final ClassScope classScope = (ClassScope) classDecl.getScope(); + + // Check fields + for (final Map.Entry> varDecl : classScope.getVariableDeclarations().entrySet()) { + final ASTFieldDeclaration field = varDecl.getKey().getNode().getFirstParentOfType(ASTFieldDeclaration.class); + analyzeMember(field, varDecl.getValue(), classScope, data); + } + + // Check methods + for (final Map.Entry> methodDecl : classScope.getMethodDeclarations().entrySet()) { + final ASTMethodDeclaration method = methodDecl.getKey().getNode().getFirstParentOfType(ASTMethodDeclaration.class); + analyzeMember(method, methodDecl.getValue(), classScope, data); + } + + // Check inner classes + analyzeScope(classScope, data); + } + } + + public void analyzeMember(final AbstractJavaAccessNode node, final List occurrences, + final ClassScope classScope, final Object data) { + if (!node.isPrivate()) { + return; + } + + for (final NameOccurrence no : occurrences) { + Node n = no.getLocation(); + while (n != null && !(n instanceof ASTClassOrInterfaceDeclaration) && !(n instanceof ASTEnumDeclaration)) { + n = n.jjtGetParent(); + } + + // Are we within the same class that defines the field / method? + if (!n.getImage().equals(classScope.getClassName())) { + addViolation(data, no.getLocation()); + } + } + } +} diff --git a/pmd-java/src/main/resources/rulesets/java/design.xml b/pmd-java/src/main/resources/rulesets/java/design.xml index 9801f3f7bc..7dd30bf0af 100644 --- a/pmd-java/src/main/resources/rulesets/java/design.xml +++ b/pmd-java/src/main/resources/rulesets/java/design.xml @@ -2003,6 +2003,38 @@ public interface YetAnotherConstantInterface { public static final int CONST1 = 1; // no violation int anyMethod(); +} + ]]> + + + + + +When accessing a private field / method from another class, the Java compiler will generate a accessor methods +with package-private visibility. This adds overhead, and to the dex method count on Android. This situation can +be avoided by changing the visibility of the field / method from private to package-private. + + 3 + + diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java index 4e5e2f0b15..0e3173845b 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java @@ -16,6 +16,7 @@ public class DesignRulesTest extends SimpleAggregatorTst { public void setUp() { addRule(RULESET, "AbstractClassWithoutAbstractMethod"); addRule(RULESET, "AbstractClassWithoutAnyMethod"); + addRule(RULESET, "AccessorMethodGeneration"); addRule(RULESET, "AccessorClassGeneration"); addRule(RULESET, "AssignmentToNonFinalStatic"); addRule(RULESET, "AvoidConstantsInterface"); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml new file mode 100644 index 0000000000..1b1cdd52bf --- /dev/null +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml @@ -0,0 +1,169 @@ + + + + + 1 + 8 + + + + + 1 + 8 + + + + + 1 + 9 + + + + + 0 + + + + + 1 + 4 + + + + + 1 + 4 + + + + + 0 + + + + + 0 + + + From 070011c3ae50d4dc280d093d39ff13884f06edb5 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 13 Feb 2017 20:02:21 +0100 Subject: [PATCH 2/3] Update changelog with new rule "AccessorMethodGeneration" References #244 --- .../main/resources/rulesets/releases/554.xml | 14 +++++++ src/site/markdown/overview/changelog.md | 41 ++++++++++++++++--- 2 files changed, 50 insertions(+), 5 deletions(-) create mode 100644 pmd-core/src/main/resources/rulesets/releases/554.xml diff --git a/pmd-core/src/main/resources/rulesets/releases/554.xml b/pmd-core/src/main/resources/rulesets/releases/554.xml new file mode 100644 index 0000000000..4e6b8f2d15 --- /dev/null +++ b/pmd-core/src/main/resources/rulesets/releases/554.xml @@ -0,0 +1,14 @@ + + + + +This ruleset contains links to rules that are new in PMD v5.5.4 + + + + + + diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index dd0f5ddb73..f9bb81f557 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -7,22 +7,53 @@ The PMD team is pleased to announce PMD 5.5.4 ### Table Of Contents -* [New and noteworthy](#New_and_noteworthy) -* [Fixed Issues](#Fixed_Issues) -* [API Changes](#API_Changes) -* [External Contributions](#External_Contributions) +* [New and noteworthy](#New_and_noteworthy) + * [New Rules](#New_Rules) +* [Fixed Issues](#Fixed_Issues) +* [API Changes](#API_Changes) +* [External Contributions](#External_Contributions) ### New and noteworthy +#### New Rules + +##### AccessorMethodGeneration (java-design) + +When accessing a private field / method from another class, the Java compiler will generate a accessor methods +with package-private visibility. This adds overhead, and to the dex method count on Android. This situation can +be avoided by changing the visibility of the field / method from private to package-private. + +For instance, it would report violations on code such as: + +``` +public class OuterClass { + private int counter; + /* package */ int id; + + public class InnerClass { + InnerClass() { + OuterClass.this.counter++; // wrong, accessor method will be generated + } + + public int getOuterClassId() { + return OuterClass.this.id; // id is package-private, no accessor method needed + } + } +} +``` + +This new rule is part of the `java-design` ruleset. + ### Fixed Issues -* general +* General * [#234](https://github.com/pmd/pmd/issues/234): \[core] Zip file stream closes spuriously when loading rulesets * java-basic * [#232](https://github.com/pmd/pmd/issues/232): \[java] SimplifiedTernary: Incorrect ternary operation can be simplified. * java-design + * [#1496](https://sourceforge.net/p/pmd/bugs/1496/): \[java] New Rule: AccesorMethodGeneration - complements accessor class rule * [#216](https://github.com/pmd/pmd/issues/216): \[java] \[doc] NonThreadSafeSingleton: Be more explicit as to why double checked locking is not recommended * [#219](https://github.com/pmd/pmd/issues/219): \[java] UnnecessaryLocalBeforeReturn: ClassCastException in switch case with local variable returned * java-optimizations From 983a0dc9b87b31581607aa1e8562f92b5fa879ed Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 13 Feb 2017 21:04:12 +0100 Subject: [PATCH 3/3] Remove the to be ignored test from the regressionTests --- .../pmd/lang/java/rule/design/DesignRulesTest.java | 2 +- .../lang/java/rule/design/xml/AccessorMethodGeneration.xml | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java index 0e3173845b..5a27b792f1 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java @@ -16,8 +16,8 @@ public class DesignRulesTest extends SimpleAggregatorTst { public void setUp() { addRule(RULESET, "AbstractClassWithoutAbstractMethod"); addRule(RULESET, "AbstractClassWithoutAnyMethod"); - addRule(RULESET, "AccessorMethodGeneration"); addRule(RULESET, "AccessorClassGeneration"); + addRule(RULESET, "AccessorMethodGeneration"); addRule(RULESET, "AssignmentToNonFinalStatic"); addRule(RULESET, "AvoidConstantsInterface"); addRule(RULESET, "AvoidDeeplyNestedIfStmts"); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml index 1b1cdd52bf..c1d04a8abd 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AccessorMethodGeneration.xml @@ -121,11 +121,12 @@ public class Foo { } ]]> - + - 0 + 1 + 9