From 2ca4b347c34b9a9391b6d8c4129d0dacb8ec8770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 16 Jan 2018 23:42:29 +0100 Subject: [PATCH] Make TCC consider local classes. If a local class is declared inside a method of the outer class, then attribute accesses made by the local class count as if they are made by the enclosing method. --- .../pmd/lang/java/ast/JavaQualifiedName.java | 6 + .../pmd/lang/java/metrics/impl/TccMetric.java | 5 +- ....java => TccAttributeAccessCollector.java} | 86 ++++++++++---- .../pmd/lang/java/ast/QualifiedNameTest.java | 5 + .../metrics/impl/AbstractMetricTestRule.java | 4 +- .../java/metrics/impl/AllMetricsTest.java | 2 + .../java/rule/design/DesignRulesTest.java | 2 + .../lang/java/metrics/impl/xml/TccTest.xml | 105 +++++++++++++++++- .../lang/java/rule/design/xml/GodClass.xml | 53 +++++++-- .../pmd/testframework/RuleTst.java | 2 +- 10 files changed, 229 insertions(+), 41 deletions(-) rename pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/{TccMethodPairVisitor.java => TccAttributeAccessCollector.java} (51%) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaQualifiedName.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaQualifiedName.java index 1b6ecfe4d9..9896663f32 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaQualifiedName.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaQualifiedName.java @@ -95,6 +95,12 @@ public final class JavaQualifiedName implements QualifiedName { localIndices[0] = NOTLOCAL_PLACEHOLDER; } + + /* default, test only */ static void resetLocalIndicesCounter() { + LOCAL_INDICES.clear(); + } + + /** * Builds the qualified name of a method declaration. * diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java index 5823e27b56..6405f242df 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java @@ -11,7 +11,7 @@ import java.util.Map; import java.util.Set; import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; -import net.sourceforge.pmd.lang.java.metrics.impl.visitors.TccMethodPairVisitor; +import net.sourceforge.pmd.lang.java.metrics.impl.visitors.TccAttributeAccessCollector; import net.sourceforge.pmd.lang.metrics.MetricOptions; /** @@ -25,8 +25,7 @@ public class TccMetric extends AbstractJavaClassMetric { @Override public double computeFor(ASTAnyTypeDeclaration node, MetricOptions options) { - @SuppressWarnings("unchecked") - Map> usagesByMethod = (Map>) node.jjtAccept(new TccMethodPairVisitor(), null); + Map> usagesByMethod = new TccAttributeAccessCollector(node).start(); int numPairs = numMethodsRelatedByAttributeAccess(usagesByMethod); int maxPairs = maxMethodPairs(usagesByMethod.size()); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccAttributeAccessCollector.java similarity index 51% rename from pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java rename to pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccAttributeAccessCollector.java index e804812656..115f1f2b3c 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccAttributeAccessCollector.java @@ -6,44 +6,65 @@ package net.sourceforge.pmd.lang.java.metrics.impl.visitors; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; -import java.util.Stack; import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; +import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; import net.sourceforge.pmd.lang.java.ast.JavaParserVisitorReducedAdapter; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; import net.sourceforge.pmd.lang.symboltable.Scope; + /** - * Returns the map of method names to the set local attributes accessed when visiting a class. + * Returns the map of method names to the set of local attributes accessed when visiting a class. * * @author Clément Fournier * @since 6.0.0 */ -public class TccMethodPairVisitor extends JavaParserVisitorReducedAdapter { +public class TccAttributeAccessCollector extends JavaParserVisitorReducedAdapter { + + private final ASTAnyTypeDeclaration exploredClass; - /** - * Collects for each method of the current class, which local attributes are accessed. - */ - Stack>> methodAttributeAccess = new Stack<>(); /** The name of the current method. */ private String currentMethodName; + private Map> methodAttributeAccess; + + + public TccAttributeAccessCollector(ASTAnyTypeDeclaration exploredClass) { + this.exploredClass = exploredClass; + } + + + /** + * Collects the attribute accesses by method into a map. + */ + @SuppressWarnings("unchecked") + public Map> start() { + return (Map>) this.visit(exploredClass, new HashMap>()); + } + @Override public Object visit(ASTAnyTypeDeclaration node, Object data) { - methodAttributeAccess.push(new HashMap>()); - super.visit(node, data); - - methodAttributeAccess.peek().remove(null); - return methodAttributeAccess.pop(); + if (node == exploredClass) { + methodAttributeAccess = new HashMap<>(); + super.visit(node, data); + } else if (node instanceof ASTClassOrInterfaceDeclaration + && ((ASTClassOrInterfaceDeclaration) node).isLocal()) { + super.visit(node, data); + } + return methodAttributeAccess; } @@ -51,26 +72,37 @@ public class TccMethodPairVisitor extends JavaParserVisitorReducedAdapter { public Object visit(ASTMethodDeclaration node, Object data) { if (!node.isAbstract()) { - currentMethodName = node.getQualifiedName().getOperation(); - methodAttributeAccess.peek().put(currentMethodName, new HashSet()); + if (node.getFirstParentOfType(ASTAnyTypeDeclaration.class) == exploredClass) { + currentMethodName = node.getQualifiedName().getOperation(); + methodAttributeAccess.put(currentMethodName, new HashSet()); - super.visit(node, data); - - currentMethodName = null; + super.visit(node, data); + currentMethodName = null; + } else { + super.visit(node, data); + } } return null; } + @Override + public Object visit(ASTConstructorDeclaration node, Object data) { + return data; // we're only looking for method pairs + } + + /** - * The primary expression node is used to detect access to attributes and method calls. If the access is not for a - * foreign class, then the {@link #methodAttributeAccess} map is updated for the current method. + * The primary expression node is used to detect access + * to attributes and method calls. If the access is not for a + * foreign class, then the {@link #methodAttributeAccess} + * map is updated for the current method. */ @Override public Object visit(ASTPrimaryExpression node, Object data) { if (currentMethodName != null) { - Set methodAccess = methodAttributeAccess.peek().get(currentMethodName); + Set methodAccess = methodAttributeAccess.get(currentMethodName); String variableName = getVariableName(node); if (isLocalAttributeAccess(variableName, node.getScope())) { methodAccess.add(variableName); @@ -81,9 +113,18 @@ public class TccMethodPairVisitor extends JavaParserVisitorReducedAdapter { return super.visit(node, data); } - private String getVariableName(ASTPrimaryExpression node) { ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); + + if (prefix.usesThisModifier()) { + List suffixes = node.findChildrenOfType(ASTPrimarySuffix.class); + if (suffixes.size() > 1) { + if (!suffixes.get(1).isArguments()) { // not a method call + return suffixes.get(0).getImage(); + } + } + } + ASTName name = prefix.getFirstDescendantOfType(ASTName.class); String variableName = null; @@ -107,7 +148,8 @@ public class TccMethodPairVisitor extends JavaParserVisitorReducedAdapter { while (currentScope != null) { for (VariableNameDeclaration decl : currentScope.getDeclarations(VariableNameDeclaration.class).keySet()) { if (decl.getImage().equals(varName)) { - if (currentScope instanceof ClassScope) { + if (currentScope instanceof ClassScope + && ((ClassScope) currentScope).getClassDeclaration().getNode() == exploredClass) { return true; } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/QualifiedNameTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/QualifiedNameTest.java index fdfa158563..37d2e78837 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/QualifiedNameTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/QualifiedNameTest.java @@ -25,6 +25,11 @@ import net.sourceforge.pmd.lang.java.ParserTstUtil; */ public class QualifiedNameTest { + /** Provides a hook into the package-private reset method for the local indices counter. */ + public static void resetLocalIndicesCounterHook() { + JavaQualifiedName.resetLocalIndicesCounter(); + } + @Test public void testEmptyPackage() { diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java index 6c48f87f4e..083297d419 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java @@ -132,7 +132,7 @@ public abstract class AbstractMetricTestRule extends AbstractJavaMetricsRule { if (val == (int) val) { return String.valueOf((int) val); } else { - return String.format(Locale.ROOT, "%." + 4 + "f", val); + return String.format(Locale.ROOT, "%.4f", val); } } @@ -165,6 +165,6 @@ public abstract class AbstractMetricTestRule extends AbstractJavaMetricsRule { "" + niceDoubleString(methodValue), }); } } - return data; + return super.visit(node, data); } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java index f5d3b6e2a9..d61edfdbe7 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.java.metrics.impl; import net.sourceforge.pmd.Rule; +import net.sourceforge.pmd.lang.java.ast.QualifiedNameTest; import net.sourceforge.pmd.lang.java.metrics.MetricsHook; import net.sourceforge.pmd.testframework.SimpleAggregatorTst; @@ -22,6 +23,7 @@ public class AllMetricsTest extends SimpleAggregatorTst { @Override protected Rule reinitializeRule(Rule rule) { MetricsHook.reset(); + QualifiedNameTest.resetLocalIndicesCounterHook(); return rule; } 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 6a94c0f585..49748c3c73 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 @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.java.rule.design; import net.sourceforge.pmd.Rule; +import net.sourceforge.pmd.lang.java.ast.QualifiedNameTest; import net.sourceforge.pmd.lang.java.metrics.MetricsHook; import net.sourceforge.pmd.testframework.SimpleAggregatorTst; @@ -18,6 +19,7 @@ public class DesignRulesTest extends SimpleAggregatorTst { @Override protected Rule reinitializeRule(Rule rule) { MetricsHook.reset(); + QualifiedNameTest.resetLocalIndicesCounterHook(); return rule; } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml index 1a325ec2bf..4fcc4dc2b3 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml @@ -26,11 +26,11 @@ this(name, valueType, initialValue, null); public Property(String name, Class valueType, Object initialValue, Object[] values) { -_name = name; -_valueType = valueType; -_initialValue = initialValue; -_availableValues = values; -_currentValue = _initialValue; + _name = name; + _valueType = valueType; + _initialValue = initialValue; + _availableValues = values; + _currentValue = _initialValue; } public String getName() { @@ -97,4 +97,99 @@ return _name.compareTo(((Property) o)._name); ]]> + + Do not crash on local class, refs #827 + 1 + + 'com.pack.Pack$1Inner' has value 0. + + get() { + + class Inner implements IInner { + + private Map results; + + public Inner(Map results) { + this.results = results; + } + + public void method() { + this.results = new HashMap(); + } + + private void otherMethod() { + this.results.clear(); + } + } + return null; + } + } + ]]> + + + + Attribute accesses in local class count as accesses of the method + 2 + + 'com.pack.Pack' has value 1. + 'com.pack.Pack$1Inner' has value 0. + + results; + + public void paired() { + results.clear(); + } + + @Override + public Map get() { + + class Inner implements IInner { + + + public Inner(Map results) { + this.results = results; + } + + public void method() { + this.results = new HashMap(); + } + + private void otherMethod() { + this.results.clear(); + } + } + return null; + } + } + ]]> + + + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/GodClass.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/GodClass.xml index c474660ae2..77996fff47 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/GodClass.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/GodClass.xml @@ -133,15 +133,52 @@ public class Foo { #1085 NullPointerException by at net.sourceforge.pmd.lang.java.rule.design.GodClassRule.visit(GodClassRule.java:313) 0 + + + + GodClass crashes with java.lang.NullPointerException, refs #827 + 0 + + get() { + + class Inner implements IInner { + + private Map results; + + public Inner(Map results) { + this.results = results; + } + } + return null; + } + + } + ]]> + + diff --git a/pmd-test/src/main/java/net/sourceforge/pmd/testframework/RuleTst.java b/pmd-test/src/main/java/net/sourceforge/pmd/testframework/RuleTst.java index 4b18938d6c..a503b10ff5 100644 --- a/pmd-test/src/main/java/net/sourceforge/pmd/testframework/RuleTst.java +++ b/pmd-test/src/main/java/net/sourceforge/pmd/testframework/RuleTst.java @@ -163,7 +163,7 @@ public abstract class RuleTst { * * @param rule The rule to reinitialise * - * @return The rule once it has be reinitialised + * @return The rule once it has been reinitialised */ protected Rule reinitializeRule(Rule rule) { return findRule(rule.getRuleSetName(), rule.getName());