[java] NPath complexity false negative due to overflow

Fixes pmd#1226
adangel · Jul 28, 2018 · 499dcab · 499dcab
1 parent c598d3b
commit 499dcab
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 8 deletions.
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -40,6 +40,7 @@ This is a minor release.
     *   [#1216](https://github.com/pmd/pmd/issues/1216): \[java] UnnecessaryFullyQualifiedName false positive for the same name method
 *   java-design
     *   [#1217](https://github.com/pmd/pmd/issues/1217): \[java] CyclomaticComplexityRule counts ?-operator twice
+    *   [#1226](https://github.com/pmd/pmd/issues/1226): \[java] NPath complexity false negative due to overflow
 *   plsql
     *   [#980](https://github.com/pmd/pmd/issues/980): \[plsql] ParseException for CREATE TABLE
     *   [#981](https://github.com/pmd/pmd/issues/981): \[plsql] ParseException when parsing VIEW

diff --git a/...a/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/NpathBaseVisitor.java b/...a/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/NpathBaseVisitor.java
@@ -39,7 +39,16 @@ private int multiplyChildrenComplexities(JavaNode node, Object data) {
 
         for (int i = 0; i < node.jjtGetNumChildren(); i++) {
             JavaNode n = (JavaNode) node.jjtGetChild(i);
-            product *= (Integer) n.jjtAccept(this, data);
+            int childComplexity = (int) n.jjtAccept(this, data);
+
+            int newProduct = product * childComplexity;
+            if (newProduct >= product) {
+                product = newProduct;
+            } else {
+                // Overflow happened
+                product = Integer.MAX_VALUE;
+                break;
+            }
         }
 
         return product;
@@ -52,7 +61,16 @@ private int sumChildrenComplexities(JavaNode node, Object data) {
 
         for (int i = 0; i < node.jjtGetNumChildren(); i++) {
             JavaNode n = (JavaNode) node.jjtGetChild(i);
-            sum += (Integer) n.jjtAccept(this, data);
+            int childComplexity = (int) n.jjtAccept(this, data);
+
+            int newSum = sum + childComplexity;
+            if (newSum >= sum) {
+                sum = newSum;
+            } else {
+                // Overflow happened
+                sum = Integer.MAX_VALUE;
+                break;
+            }
         }
 
         return sum;
@@ -81,7 +99,7 @@ public Object visit(ASTIfStatement node, Object data) {
         int complexity = node.hasElse() ? 0 : 1;
 
         for (ASTStatement element : statementChildren) {
-            complexity += (Integer) element.jjtAccept(this, data);
+            complexity += (int) element.jjtAccept(this, data);
         }
 
         int boolCompIf = CycloMetric.booleanExpressionComplexity(node.getFirstChildOfType(ASTExpression.class));
@@ -95,7 +113,7 @@ public Object visit(ASTWhileStatement node, Object data) {
 
         int boolCompWhile = CycloMetric.booleanExpressionComplexity(node.getFirstChildOfType(ASTExpression.class));
 
-        int nPathWhile = (Integer) node.getFirstChildOfType(ASTStatement.class).jjtAccept(this, data);
+        int nPathWhile = (int) node.getFirstChildOfType(ASTStatement.class).jjtAccept(this, data);
 
         return boolCompWhile + nPathWhile + 1;
     }
@@ -107,7 +125,7 @@ public Object visit(ASTDoStatement node, Object data) {
 
         int boolCompDo = CycloMetric.booleanExpressionComplexity(node.getFirstChildOfType(ASTExpression.class));
 
-        int nPathDo = (Integer) node.getFirstChildOfType(ASTStatement.class).jjtAccept(this, data);
+        int nPathDo = (int) node.getFirstChildOfType(ASTStatement.class).jjtAccept(this, data);
 
         return boolCompDo + nPathDo + 1;
     }
@@ -119,7 +137,7 @@ public Object visit(ASTForStatement node, Object data) {
 
         int boolCompFor = CycloMetric.booleanExpressionComplexity(node.getFirstDescendantOfType(ASTExpression.class));
 
-        int nPathFor = (Integer) node.getFirstChildOfType(ASTStatement.class).jjtAccept(this, data);
+        int nPathFor = (int) node.getFirstChildOfType(ASTStatement.class).jjtAccept(this, data);
 
         return boolCompFor + nPathFor + 1;
     }
@@ -162,7 +180,7 @@ public Object visit(ASTSwitchStatement node, Object data) {
                 npath += caseRange;
                 caseRange = 1;
             } else {
-                Integer complexity = (Integer) n.jjtAccept(this, data);
+                int complexity = (int) n.jjtAccept(this, data);
                 caseRange *= complexity;
             }
         }

diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/NPathComplexityRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/NPathComplexityRule.java
@@ -9,6 +9,7 @@
 import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration;
+import net.sourceforge.pmd.lang.java.ast.MethodLikeNode;
 import net.sourceforge.pmd.lang.java.metrics.JavaMetrics;
 import net.sourceforge.pmd.lang.java.metrics.api.JavaOperationMetricKey;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaMetricsRule;
@@ -68,7 +69,7 @@ private int getReportLevel() {
 
     @Override
     public final Object visit(ASTMethodOrConstructorDeclaration node, Object data) {
-        int npath = (int) JavaMetrics.get(JavaOperationMetricKey.NPATH, node);
+        int npath = (int) JavaMetrics.get(JavaOperationMetricKey.NPATH, (MethodLikeNode) node);
         if (npath >= reportLevel) {
             addViolation(data, node, new String[]{node instanceof ASTMethodDeclaration ? "method" : "constructor",
                                                   node.getQualifiedName().getOperation(), "" + npath, });

diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/NPathTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/NPathTest.xml
@@ -459,4 +459,51 @@ class Bar {
         }
         ]]></code>
     </test-code>
+
+    <test-code>
+        <description>#1226 [java] NPath complexity false negative</description>
+        <rule-property name="reportLevel">0</rule-property>
+        <expected-problems>1</expected-problems>
+        <expected-messages>
+            <message>'NPathComplexityOverflow#complexMethod()' has value 2147483647.</message>
+        </expected-messages>
+        <code><![CDATA[
+        public class NPathComplexityOverflow {
+            public void complexMethod() {
+                boolean b = true;
+                if (b); // 2
+                if (b); // 4
+                if (b); // 8
+                if (b); // 16
+                if (b); // 32
+                if (b); // 64
+                if (b); // 128
+                if (b); // 256
+                if (b); // 512
+                if (b); // 1024
+                if (b); // 2048
+                if (b); // 4096
+                if (b); // 8192
+                if (b); // 16384
+                if (b); // 32768
+                if (b); // 65536
+                if (b); // 131072
+                if (b); // 262144
+                if (b); // 524288
+                if (b); // 1048576
+                if (b); // 2097152
+                if (b); // 4194304
+                if (b); // 8388608
+                if (b); // 16777216
+                if (b); // 33554432
+                if (b); // 67108864
+                if (b); // 134217728
+                if (b); // 268435456
+                if (b); // 536870912
+                if (b); // 1073741824
+                if (b); // 2147483648 // overflow is happening here...
+            }
+        }
+        ]]></code>
+    </test-code>
 </test-data>