diff --git a/pmd/rulesets/controversial.xml b/pmd/rulesets/controversial.xml
index 14767a7908..fb0b6dbbbc 100644
--- a/pmd/rulesets/controversial.xml
+++ b/pmd/rulesets/controversial.xml
@@ -381,8 +381,37 @@ public class Foo {
+
+ The dataflow analysis tracks local definitions, undefinitions and references to variables on different paths on the data flow.
+From those informations there can be found various problems.
+
+1. UR - Anomaly: There is a reference to a variable that was not defined before. This is a bug and leads to an error.
+2. DU - Anomaly: A recently defined variable is undefined. These anomalies may appear in normal source text.
+3. DD - Anomaly: A recently defined variable is redefined. This is ominous but don't have to be a bug.
+
+ 5
+
+
+
+
+
+ dd-anomaly
+ foo(buz);
+ buz = 2;
+ } // buz is undefined when leaving scope -> du-anomaly
+}
+]]>
+
+
+
-
diff --git a/pmd/src/net/sourceforge/pmd/dfa/DaaRule.java b/pmd/src/net/sourceforge/pmd/dfa/DaaRule.java
index 1918c257a1..a23a192236 100644
--- a/pmd/src/net/sourceforge/pmd/dfa/DaaRule.java
+++ b/pmd/src/net/sourceforge/pmd/dfa/DaaRule.java
@@ -5,6 +5,7 @@ package net.sourceforge.pmd.dfa;
import net.sourceforge.pmd.AbstractRule;
import net.sourceforge.pmd.RuleContext;
+import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.ast.SimpleNode;
import net.sourceforge.pmd.dfa.pathfinder.CurrentPath;
@@ -12,6 +13,7 @@ import net.sourceforge.pmd.dfa.pathfinder.DAAPathFinder;
import net.sourceforge.pmd.dfa.pathfinder.Executable;
import net.sourceforge.pmd.dfa.variableaccess.VariableAccess;
+import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Hashtable;
import java.util.Iterator;
@@ -23,19 +25,38 @@ import java.util.List;
* Starts path search for each method and runs code if found.
*/
public class DaaRule extends AbstractRule implements Executable {
-
private RuleContext rc;
- private int counter;
- private static final int MAX_PATHS = 5000;
-
+ private List daaRuleViolations;
+ private final static String PROPERTY_MAX_PATH = "maxpaths";
+ private final static String PROPERTY_MAX_VIOLATIONS = "maxviolations";
+ private final static int DEFAULT_MAX_VIOLATIONS = 1000;
+ private int maxRuleViolations;
+ private int currentRuleViolationCount;
+
+ public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
+ if (this.hasProperty(PROPERTY_MAX_VIOLATIONS)) {
+ this.maxRuleViolations = this.getIntProperty(PROPERTY_MAX_VIOLATIONS);
+ } else {
+ this.maxRuleViolations = DEFAULT_MAX_VIOLATIONS;
+ }
+
+ this.currentRuleViolationCount = 0;
+ return super.visit(node, data);
+ }
+
public Object visit(ASTMethodDeclaration node, Object data) {
this.rc = (RuleContext) data;
- counter = 0;
-
+ this.daaRuleViolations = new ArrayList();
+
IDataFlowNode n = (IDataFlowNode) node.getDataFlowNode().getFlow().get(0);
- System.out.println("In DaaRule, IDataFlowNode n = " + n);
-
- DAAPathFinder a = new DAAPathFinder(n, this);
+
+ DAAPathFinder a;
+ if (this.hasProperty(PROPERTY_MAX_PATH)) {
+ a = new DAAPathFinder(n, this, this.getIntProperty(PROPERTY_MAX_PATH));
+ } else {
+ a = new DAAPathFinder(n, this);
+ }
+
a.run();
super.visit(node, data);
@@ -43,12 +64,13 @@ public class DaaRule extends AbstractRule implements Executable {
}
public void execute(CurrentPath path) {
- Hashtable hash = new Hashtable();
- counter++;
- if (counter == 5000) {
- System.out.print("|");
- counter = 0;
+ if (maxNumberOfViolationsReached()) {
+ // dont execute this path if the limit is already reached
+ return;
}
+
+ Hashtable hash = new Hashtable();
+
for (Iterator d = path.iterator(); d.hasNext();) {
IDataFlowNode inode = (IDataFlowNode) d.next();
if (inode.getVariableAccess() != null) {
@@ -81,7 +103,7 @@ public class DaaRule extends AbstractRule implements Executable {
// undefinition outside, get the node of the definition
SimpleNode lastSimpleNode = (SimpleNode)array.get(1);
if (lastSimpleNode != null) {
- addDaaViolation(rc, lastSimpleNode, "DU", va.getVariableName(), startLine, endLine);
+ addDaaViolation(rc, lastSimpleNode, "DU", va.getVariableName(), startLine, endLine);
}
}
}
@@ -103,8 +125,50 @@ public class DaaRule extends AbstractRule implements Executable {
* @param node the node that produces the violation
* @param msg specific message to put in the report
*/
- private final void addDaaViolation(Object data, SimpleNode node, String msg, String var, int startLine, int endLine) {
- RuleContext ctx = (RuleContext) data;
- ctx.getReport().addRuleViolation(new DaaRuleViolation(this, ctx, node, msg, var, startLine, endLine));
+ private final void addDaaViolation(Object data, SimpleNode node, String type, String var, int startLine, int endLine) {
+ if (!maxNumberOfViolationsReached()
+ && !violationAlreadyExists(type, var, startLine, endLine)) {
+ RuleContext ctx = (RuleContext) data;
+ Object[] params = new Object[] { type, var, new Integer(startLine), new Integer(endLine) };
+ String msg = type;
+ if (getMessage() != null) {
+ msg = MessageFormat.format(getMessage(), params);
+ }
+ DaaRuleViolation violation = new DaaRuleViolation(this, ctx, node, type, msg, var, startLine, endLine);
+ ctx.getReport().addRuleViolation(violation);
+ this.daaRuleViolations.add(violation);
+ this.currentRuleViolationCount++;
+ }
+ }
+
+ /**
+ * Maximum number of violations was already reached?
+ * @return
+ */
+ private boolean maxNumberOfViolationsReached() {
+ return this.currentRuleViolationCount >= this.maxRuleViolations;
+ }
+
+ /**
+ * Checks if a violation already exists.
+ * This is needed because on the different paths same anomalies can occur.
+ * @param type
+ * @param var
+ * @param startLine
+ * @param endLine
+ * @return true if the violation already was added to the report
+ */
+ private boolean violationAlreadyExists(String type, String var, int startLine, int endLine) {
+ Iterator violationIterator = this.daaRuleViolations.iterator();
+ while (violationIterator.hasNext()) {
+ DaaRuleViolation violation = (DaaRuleViolation)violationIterator.next();
+ if ((violation.getBeginLine() == startLine)
+ && (violation.getEndLine() == endLine)
+ && violation.getType().equals(type)
+ && violation.getVariableName().equals(var)) {
+ return true;
+ }
+ }
+ return false;
}
}
diff --git a/pmd/src/net/sourceforge/pmd/dfa/DaaRuleViolation.java b/pmd/src/net/sourceforge/pmd/dfa/DaaRuleViolation.java
index 28be58aeaf..3cf436482b 100644
--- a/pmd/src/net/sourceforge/pmd/dfa/DaaRuleViolation.java
+++ b/pmd/src/net/sourceforge/pmd/dfa/DaaRuleViolation.java
@@ -16,12 +16,14 @@ public class DaaRuleViolation extends RuleViolation {
private String variableName;
private int beginLine;
private int endLine;
+ private String type;
- public DaaRuleViolation(Rule rule, RuleContext ctx, SimpleNode node, String specificMsg, String variableName, int beginLine, int endLine) {
- super(rule, ctx, node, specificMsg);
- this.variableName = variableName;
+ public DaaRuleViolation(Rule rule, RuleContext ctx, SimpleNode node, String type, String msg, String var, int beginLine, int endLine) {
+ super(rule, ctx, node, msg);
+ this.variableName = var;
this.beginLine = beginLine;
this.endLine = endLine;
+ this.type = type;
}
public String getVariableName() {
@@ -35,4 +37,8 @@ public class DaaRuleViolation extends RuleViolation {
public int getEndLine() {
return endLine;
}
+
+ public String getType() {
+ return type;
+ }
}
diff --git a/pmd/src/net/sourceforge/pmd/dfa/pathfinder/CurrentPath.java b/pmd/src/net/sourceforge/pmd/dfa/pathfinder/CurrentPath.java
index 4a342e6b2c..aa190008e5 100644
--- a/pmd/src/net/sourceforge/pmd/dfa/pathfinder/CurrentPath.java
+++ b/pmd/src/net/sourceforge/pmd/dfa/pathfinder/CurrentPath.java
@@ -14,6 +14,10 @@ public class CurrentPath {
list = new LinkedList();
}
+ public int getLength() {
+ return list.size();
+ }
+
public Iterator iterator() {
return list.iterator();
}
diff --git a/pmd/src/net/sourceforge/pmd/dfa/pathfinder/DAAPathFinder.java b/pmd/src/net/sourceforge/pmd/dfa/pathfinder/DAAPathFinder.java
index 54b1e6d9f1..2eda85eb18 100644
--- a/pmd/src/net/sourceforge/pmd/dfa/pathfinder/DAAPathFinder.java
+++ b/pmd/src/net/sourceforge/pmd/dfa/pathfinder/DAAPathFinder.java
@@ -15,17 +15,25 @@ import javax.swing.tree.DefaultMutableTreeNode;
* 2 paths. This is special to the data flow anomaly analysis.
*/
public class DAAPathFinder {
-
private static final int MAX_PATHS = 5000;
+ private static final int MAX_PATH_LENGTH = 5000;
private IDataFlowNode rootNode;
private Executable shim;
private CurrentPath currentPath = new CurrentPath();
private DefaultMutableTreeNode stack = new DefaultMutableTreeNode();
+ private int maxPaths;
public DAAPathFinder(IDataFlowNode rootNode, Executable shim) {
this.rootNode = rootNode;
this.shim = shim;
+ this.maxPaths = MAX_PATHS;
+ }
+
+ public DAAPathFinder(IDataFlowNode rootNode, Executable shim, int maxPaths) {
+ this.rootNode = rootNode;
+ this.shim = shim;
+ this.maxPaths = maxPaths;
}
public void run() {
@@ -44,7 +52,7 @@ public class DAAPathFinder {
phase2(flag);
shim.execute(currentPath);
flag = false;
- } while (i < MAX_PATHS && phase3());
+ } while (i < maxPaths && phase3());
//System.out.println("found: " + i + " path(s)");
}
@@ -52,7 +60,7 @@ public class DAAPathFinder {
* Builds up the path.
* */
private void phase2(boolean flag) {
- while (!currentPath.isEndNode()) {
+ while (!currentPath.isEndNode() && currentPath.getLength() < MAX_PATH_LENGTH) {
if (currentPath.isBranch() || currentPath.isFirstDoStatement()) {
if (flag) {
addNodeToTree();
diff --git a/pmd/src/net/sourceforge/pmd/dfa/variableaccess/VariableAccessVisitor.java b/pmd/src/net/sourceforge/pmd/dfa/variableaccess/VariableAccessVisitor.java
index 28366ad11d..733a3ab44d 100644
--- a/pmd/src/net/sourceforge/pmd/dfa/variableaccess/VariableAccessVisitor.java
+++ b/pmd/src/net/sourceforge/pmd/dfa/variableaccess/VariableAccessVisitor.java
@@ -5,7 +5,10 @@ package net.sourceforge.pmd.dfa.variableaccess;
import net.sourceforge.pmd.ast.ASTClassOrInterfaceBodyDeclaration;
import net.sourceforge.pmd.ast.ASTConstructorDeclaration;
+import net.sourceforge.pmd.ast.ASTFormalParameter;
+import net.sourceforge.pmd.ast.ASTFormalParameters;
import net.sourceforge.pmd.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.ast.ASTVariableInitializer;
import net.sourceforge.pmd.ast.JavaParserVisitorAdapter;
import net.sourceforge.pmd.ast.SimpleNode;
import net.sourceforge.pmd.dfa.IDataFlowNode;
@@ -21,7 +24,7 @@ import java.util.Map;
import java.util.Set;
/**
- * @author raik
+ * @author raik, Sven Jacob
*
* Searches for special nodes and computes based on the sequence, the type of
* access of a variable.
@@ -43,10 +46,11 @@ public class VariableAccessVisitor extends JavaParserVisitorAdapter {
List undefinitions = markUsages(inode);
- // is this necessary? Why does the first node need undefs?
+ // all variables are first in state undefinition
IDataFlowNode firstINode = (IDataFlowNode) inode.getFlow().get(0);
firstINode.setVariableAccess(undefinitions);
+ // all variables are getting undefined when leaving scope
IDataFlowNode lastINode = (IDataFlowNode) inode.getFlow().get(inode.getFlow().size() - 1);
lastINode.setVariableAccess(undefinitions);
}
@@ -58,10 +62,24 @@ public class VariableAccessVisitor extends JavaParserVisitorAdapter {
for (Iterator i = variableDeclarations.iterator(); i.hasNext();) {
Map declarations = (Map) i.next();
for (Iterator j = declarations.entrySet().iterator(); j.hasNext();) {
- Map.Entry entry = (Map.Entry) j.next();
+ Map.Entry entry = (Map.Entry) j.next();
VariableNameDeclaration vnd = (VariableNameDeclaration) entry.getKey();
- addVariableAccess(vnd.getNode().getBeginLine(), new VariableAccess(VariableAccess.DEFINITION, vnd.getImage()), inode.getFlow());
+
+ if (vnd.getAccessNodeParent() instanceof ASTFormalParameter) {
+ // add definition for parameters
+ addVariableAccess(
+ (SimpleNode)vnd.getNode().getFirstParentOfType(ASTFormalParameters.class),
+ new VariableAccess(VariableAccess.DEFINITION, vnd.getImage()),
+ inode.getFlow());
+ } else if (vnd.getAccessNodeParent().getFirstChildOfType(ASTVariableInitializer.class) != null) {
+ // add definition for initialized variables
+ addVariableAccess(
+ vnd.getNode(),
+ new VariableAccess(VariableAccess.DEFINITION, vnd.getImage()),
+ inode.getFlow());
+ }
undefinitions.add(new VariableAccess(VariableAccess.UNDEFINITION, vnd.getImage()));
+
for (Iterator k = ((List) entry.getValue()).iterator(); k.hasNext();) {
addAccess(k, inode);
}
@@ -89,19 +107,35 @@ public class VariableAccessVisitor extends JavaParserVisitorAdapter {
private void addAccess(Iterator k, IDataFlowNode inode) {
NameOccurrence occurrence = (NameOccurrence) k.next();
if (occurrence.isOnLeftHandSide()) {
- this.addVariableAccess(occurrence.getLocation().getBeginLine(), new VariableAccess(VariableAccess.DEFINITION, occurrence.getImage()), inode.getFlow());
+ this.addVariableAccess(occurrence.getLocation(), new VariableAccess(VariableAccess.DEFINITION, occurrence.getImage()), inode.getFlow());
} else if (occurrence.isOnRightHandSide() || (!occurrence.isOnLeftHandSide() && !occurrence.isOnRightHandSide())) {
- this.addVariableAccess(occurrence.getLocation().getBeginLine(), new VariableAccess(VariableAccess.REFERENCING, occurrence.getImage()), inode.getFlow());
+ this.addVariableAccess(occurrence.getLocation(), new VariableAccess(VariableAccess.REFERENCING, occurrence.getImage()), inode.getFlow());
}
}
- private void addVariableAccess(int line, VariableAccess va, List flow) {
- for (int i = 1; i < flow.size(); i++) {
+ /**
+ * Adds a VariableAccess to a dataflow node.
+ * @param node location of the access of a variable
+ * @param va variable access to add
+ * @param flow dataflownodes that can contain the node.
+ */
+ private void addVariableAccess(SimpleNode node, VariableAccess va, List flow) {
+ // backwards to find the right inode (not a method declaration)
+ for (int i = flow.size()-1; i > 0; i--) {
IDataFlowNode inode = (IDataFlowNode) flow.get(i);
- if (line == inode.getLine()) {
- List v = new ArrayList();
- v.add(va);
- inode.setVariableAccess(v);
+ if (inode.getSimpleNode() == null) {
+ continue;
+ }
+
+ List children = inode.getSimpleNode().findChildrenOfType(node.getClass());
+ Iterator childrenIterator = children.iterator();
+ while (childrenIterator.hasNext()) {
+ if (node.equals(childrenIterator.next())) {
+ List v = new ArrayList();
+ v.add(va);
+ inode.setVariableAccess(v);
+ return;
+ }
}
}
}