phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

pmd: fix #1032 ImmutableField Rule: Private field in inner class gives false positive

Browse Source 
Improved symbol table resolution to check inner classes, too.
This commit is contained in:
Andreas Dangel 2013-03-15 21:37:29 +01:00
parent 8eac048c03
commit 31f6b9af1a
7 changed files with 95 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pmd/etc/changelog.txt
View File

@ -2,6 +2,7 @@
Fixed bug 1002: False +: FinalFieldCouldBeStatic on inner class
Fixed bug 1005: False + for ConstructorCallsOverridableMethod - overloaded methods
Fixed bug 1032: ImmutableField Rule: Private field in inner class gives false positive
Fixed bug 1064: Exception running PrematureDeclaration
Fixed bug 1068: CPD fails on broken symbolic links
Fixed bug 1073: Hard coded violation messages CommentSize

4
pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ImmutableFieldRule.java
View File

@ -10,6 +10,7 @@ import java.util.Map;
import java.util.Set;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBody;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
@ -113,7 +114,8 @@ public class ImmutableFieldRule extends AbstractJavaRule {
private List<ASTConstructorDeclaration> findAllConstructors(ASTClassOrInterfaceDeclaration node) {
List<ASTConstructorDeclaration> cons = new ArrayList<ASTConstructorDeclaration>();
node.findDescendantsOfType(ASTConstructorDeclaration.class, cons, false);
node.getFirstChildOfType(ASTClassOrInterfaceBody.class)
.findDescendantsOfType(ASTConstructorDeclaration.class, cons, false);
return cons;
}
}

5
pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java
View File

@ -122,7 +122,10 @@ public class SingularFieldRule extends AbstractJavaRule {
if (decl == null) {
decl = method;
continue;
} else if (decl != method) {
} else if (decl != method
// handle inner classes
&& decl.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class)
== method.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class)) {
violation = false;
break; //Optimization
}

36
pmd/src/main/java/net/sourceforge/pmd/lang/java/symboltable/ClassScope.java
View File

@ -68,6 +68,14 @@ public class ClassScope extends AbstractScope {
List<NameOccurrence> nameOccurrences = variableNames.get(decl);
if (nameOccurrences == null) {
// TODO may be a class name
// search inner classes
for (ClassNameDeclaration innerClass : classNames.keySet()) {
Scope innerClassScope = innerClass.getScope();
if (innerClassScope.contains(occurrence)) {
innerClassScope.addVariableNameOccurrence(occurrence);
}
}
} else {
nameOccurrences.add(occurrence);
Node n = occurrence.getLocation();
@ -155,26 +163,38 @@ public class ClassScope extends AbstractScope {
}
ImageFinderFunction finder = new ImageFinderFunction(images);
Applier.apply(finder, variableNames.keySet().iterator());
return finder.getDecl();
NameDeclaration result = finder.getDecl();
// search inner classes
if (result == null && !classNames.isEmpty()) {
for (ClassNameDeclaration innerClass : classNames.keySet()) {
Applier.apply(finder, innerClass.getScope().getVariableDeclarations().keySet().iterator());
result = finder.getDecl();
if (result != null) {
break;
}
}
}
return result;
}
public String toString() {
String res = "ClassScope (" + className + "): ";
StringBuilder res = new StringBuilder("ClassScope (").append(className).append("): ");
if (!classNames.isEmpty()) {
res += "(" + glomNames(classNames.keySet()) + ")";
res.append("Inner Classes ").append(glomNames(classNames.keySet())).append("; ");
}
if (!methodNames.isEmpty()) {
for (MethodNameDeclaration mnd: methodNames.keySet()) {
res += mnd.toString();
res.append(mnd.toString());
int usages = methodNames.get(mnd).size();
res += "(begins at line " + mnd.getNode().getBeginLine() + ", " + usages + " usages)";
res += ",";
res.append("(begins at line ").append(mnd.getNode().getBeginLine()).append(", ").append(usages).append(" usages)");
res.append(", ");
}
}
if (!variableNames.isEmpty()) {
res += "(" + glomNames(variableNames.keySet()) + ")";
res.append("Variables ").append(glomNames(variableNames.keySet()));
}
return res;
return res.toString();
}
private String clipClassName(String s) {

4
pmd/src/main/java/net/sourceforge/pmd/lang/java/symboltable/NameFinder.java
View File

@ -7,6 +7,7 @@ import java.util.ArrayList;
import java.util.List;
import java.util.StringTokenizer;
import net.sourceforge.pmd.PMD;
import net.sourceforge.pmd.lang.java.ast.ASTArguments;
import net.sourceforge.pmd.lang.java.ast.ASTMemberSelector;
import net.sourceforge.pmd.lang.java.ast.ASTName;
@ -71,7 +72,8 @@ public class NameFinder {
public String toString() {
StringBuilder result = new StringBuilder();
for (NameOccurrence occ: names) {
result.append(occ.getImage());
result.append(occ);
result.append(PMD.EOL);
}
return result.toString();
}

24
pmd/src/test/java/net/sourceforge/pmd/symboltable/AcceptanceTest.java
View File

@ -125,6 +125,16 @@ public class AcceptanceTest extends STBBaseTst {
assertEquals(9, usages.get(1).getLocation().getBeginLine());
}
@Test
public void testInnerOuterClass() {
parseCode(TEST_INNER_CLASS);
ASTVariableDeclaratorId vdi = acu.findDescendantsOfType(ASTVariableDeclaratorId.class).get(0);
List<NameOccurrence> usages = vdi.getUsages();
assertEquals(2, usages.size());
assertEquals(5, usages.get(0).getLocation().getBeginLine());
assertEquals(10, usages.get(1).getLocation().getBeginLine());
}
private static final String TEST_DEMO =
"public class Foo {" + PMD.EOL +
" void bar(ArrayList buz) { " + PMD.EOL +
@ -175,6 +185,20 @@ public class AcceptanceTest extends STBBaseTst {
" }" + PMD.EOL +
"}" + PMD.EOL;
public static final String TEST_INNER_CLASS =
"public class Outer {" + PMD.EOL +
" private static class Inner {" + PMD.EOL +
" private int i;" + PMD.EOL +
" private Inner(int i) {" + PMD.EOL +
" this.i = i;" + PMD.EOL +
" }" + PMD.EOL +
" }" + PMD.EOL +
" public int modify(int i) {" + PMD.EOL +
" Inner in = new Inner(i);" + PMD.EOL +
" return in.i;" + PMD.EOL +
" }" + PMD.EOL +
"}" + PMD.EOL;
public static junit.framework.Test suite() {
return new junit.framework.JUnit4TestAdapter(AcceptanceTest.class);
}

32
pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ImmutableField.xml
View File

@ -330,6 +330,38 @@ public class pmd_inc {
public void inc_test(int val) {
this.test += val;
}
}
]]></code>
</test-code>
<test-code>
<description>#946 ImmutableField false +</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class MyClass {
private long counter = 0;
public <E> void run(final E entity, final Class<? extends TemplatedClass<E>> m) throws MyException {
this.counter += 1;
}
}
]]></code>
</test-code>
<test-code>
<description>#1032 ImmutableField Rule: Private field in inner class gives false positive</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
private static class Bar {
private int i;
Bar(final int i) {
this.i = i;
}
}
public int foo() {
final Bar b = new Bar(1);
b.i=0;
return b.i;
}
}
]]></code>
</test-code>