diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 834c546dfa..1bdd8ea2d5 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -19,15 +19,24 @@ This is a {{ site.pmd.release_type }} release. This PMD release ships a new version of the pmd-designer. For the changes, see [PMD Designer Changelog](https://github.com/pmd/pmd-designer/releases/tag/6.21.0). -### Apex Suppressions +#### Apex Suppressions In addition to suppressing violation with the `@SuppressWarnings` annotation, Apex now also supports the suppressions with a `NOPMD` comment. See [Suppressing warnings](pmd_userdocs_suppressing_warnings.html). +#### New Rules + +* The Rule {% rule "apex/design/CognitiveComplexity" %} (`apex-design`) finds methods and classes + that are highly complex and therefore difficult to read and more costly to maintain. In contrast + to cyclomatic complexity, this rule uses "Cognitive Complexity", which is a measure of how + difficult it is for humans to read and understand a method. + ### Fixed Issues * apex * [#1087](https://github.com/pmd/pmd/issues/1087): \[apex] Support suppression via //NOPMD +* apex-design + * [#2162](https://github.com/pmd/pmd/issues/2162): \[apex] Cognitive Complexity rule * doc * [#2274](https://github.com/pmd/pmd/issues/2274): \[doc] Java API documentation for PMD * java @@ -125,6 +134,7 @@ methods on {% jdoc apex::lang.apex.ast.ApexParserVisitor %} and its implementati * [#2276](https://github.com/pmd/pmd/pull/2276): \[java] AppendCharacterWithCharRule ignore literals in expressions - [Kris Scheibe](https://github.com/kris-scheibe) * [#2278](https://github.com/pmd/pmd/pull/2278): \[java] fix UnusedImports rule for ambiguous static on-demand imports - [Kris Scheibe](https://github.com/kris-scheibe) * [#2279](https://github.com/pmd/pmd/pull/2279): \[apex] Add support for suppressing violations using the // NOPMD comment - [Gwilym Kuiper](https://github.com/gwilymatgearset) +* [#2297](https://github.com/pmd/pmd/pull/2297): \[apex] Cognitive complexity metrics - [Gwilym Kuiper](https://github.com/gwilymatgearset) {% endtocmaker %} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexClassMetricKey.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexClassMetricKey.java index 2ffae23a42..73a08d5f2c 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexClassMetricKey.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexClassMetricKey.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.apex.metrics.api; import net.sourceforge.pmd.lang.apex.ast.ASTUserClassOrInterface; +import net.sourceforge.pmd.lang.apex.metrics.impl.ClassCognitiveComplexityMetric; import net.sourceforge.pmd.lang.apex.metrics.impl.WmcMetric; import net.sourceforge.pmd.lang.metrics.MetricKey; @@ -12,6 +13,7 @@ import net.sourceforge.pmd.lang.metrics.MetricKey; * @author Clément Fournier */ public enum ApexClassMetricKey implements MetricKey> { + COGNITIVE(new ClassCognitiveComplexityMetric()), WMC(new WmcMetric()); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexOperationMetricKey.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexOperationMetricKey.java index 502dbbd069..e5816c5e6c 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexOperationMetricKey.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/api/ApexOperationMetricKey.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.apex.metrics.api; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.metrics.impl.CognitiveComplexityMetric; import net.sourceforge.pmd.lang.apex.metrics.impl.CycloMetric; import net.sourceforge.pmd.lang.metrics.MetricKey; @@ -12,7 +13,8 @@ import net.sourceforge.pmd.lang.metrics.MetricKey; * @author Clément Fournier */ public enum ApexOperationMetricKey implements MetricKey { - CYCLO(new CycloMetric()); + CYCLO(new CycloMetric()), + COGNITIVE(new CognitiveComplexityMetric()); private final ApexOperationMetric calculator; diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/ClassCognitiveComplexityMetric.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/ClassCognitiveComplexityMetric.java new file mode 100644 index 0000000000..90e0527c14 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/ClassCognitiveComplexityMetric.java @@ -0,0 +1,23 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.metrics.impl; + +import net.sourceforge.pmd.lang.apex.ast.ASTUserClassOrInterface; +import net.sourceforge.pmd.lang.apex.metrics.ApexMetrics; +import net.sourceforge.pmd.lang.apex.metrics.api.ApexOperationMetricKey; +import net.sourceforge.pmd.lang.metrics.MetricOptions; +import net.sourceforge.pmd.lang.metrics.ResultOption; + +/** + * The sum of the cognitive complexities of all the methods within a class. + * + * @author Gwilym Kuiper + */ +public class ClassCognitiveComplexityMetric extends AbstractApexClassMetric { + @Override + public double computeFor(ASTUserClassOrInterface node, MetricOptions options) { + return ApexMetrics.get(ApexOperationMetricKey.COGNITIVE, node, ResultOption.SUM); + } +} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/CognitiveComplexityMetric.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/CognitiveComplexityMetric.java new file mode 100644 index 0000000000..740bfd29bb --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/CognitiveComplexityMetric.java @@ -0,0 +1,24 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.metrics.impl; + +import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.metrics.impl.visitors.CognitiveComplexityVisitor; +import net.sourceforge.pmd.lang.metrics.MetricOptions; + +/** + * Measures the cognitive complexity of a Class / Method in Apex. + * + * See https://www.sonarsource.com/docs/CognitiveComplexity.pdf for information about the metric + * + * @author Gwilym Kuiper + */ +public class CognitiveComplexityMetric extends AbstractApexOperationMetric { + @Override + public double computeFor(ASTMethod node, MetricOptions options) { + CognitiveComplexityVisitor.State resultingState = (CognitiveComplexityVisitor.State) node.jjtAccept(new CognitiveComplexityVisitor(), new CognitiveComplexityVisitor.State()); + return resultingState.getComplexity(); + } +} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/visitors/CognitiveComplexityVisitor.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/visitors/CognitiveComplexityVisitor.java new file mode 100644 index 0000000000..8cdcddcb55 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/metrics/impl/visitors/CognitiveComplexityVisitor.java @@ -0,0 +1,234 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.metrics.impl.visitors; + +import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTBooleanExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTBreakStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTCatchBlockStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTContinueStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTDoLoopStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTForEachStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTForLoopStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTIfElseBlockStatement; +import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTPrefixExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTTernaryExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTWhileLoopStatement; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexParserVisitorAdapter; + +import apex.jorje.data.ast.BooleanOp; +import apex.jorje.data.ast.PrefixOp; + +/** + * @author Gwilym Kuiper + */ +public class CognitiveComplexityVisitor extends ApexParserVisitorAdapter { + public static class State { + private int complexity = 0; + private int nestingLevel = 0; + + private BooleanOp currentBooleanOperation = null; + private String methodName = null; + + public double getComplexity() { + return complexity; + } + + void structureComplexity() { + complexity += 1; + } + + void nestingComplexity() { + complexity += nestingLevel; + } + + void booleanOperation(BooleanOp op) { + if (currentBooleanOperation != op) { + if (op != null) { + structureComplexity(); + } + + currentBooleanOperation = op; + } + } + + void increaseNestingLevel() { + structureComplexity(); + nestingComplexity(); + nestingLevel++; + } + + void decreaseNestingLevel() { + nestingLevel--; + } + + void methodCall(String methodCalledName) { + if (methodCalledName.equals(methodName)) { + structureComplexity(); + } + } + + void setMethodName(String name) { + methodName = name; + } + } + + @Override + public Object visit(ASTIfElseBlockStatement node, Object data) { + State state = (State) data; + + boolean hasElseStatement = node.getNode().hasElseStatement(); + for (ApexNode child : node.children()) { + // If we don't have an else statement, we get an empty block statement which we shouldn't count + if (!hasElseStatement && child instanceof ASTBlockStatement) { + break; + } + + state.increaseNestingLevel(); + super.visit(child, data); + state.decreaseNestingLevel(); + } + + return data; + } + + @Override + public Object visit(ASTForLoopStatement node, Object data) { + State state = (State) data; + + state.increaseNestingLevel(); + super.visit(node, data); + state.decreaseNestingLevel(); + + return data; + } + + @Override + public Object visit(ASTForEachStatement node, Object data) { + State state = (State) data; + + state.increaseNestingLevel(); + super.visit(node, data); + state.decreaseNestingLevel(); + + return data; + } + + @Override + public Object visit(ASTContinueStatement node, Object data) { + State state = (State) data; + + state.structureComplexity(); + return super.visit(node, data); + } + + @Override + public Object visit(ASTBreakStatement node, Object data) { + State state = (State) data; + + state.structureComplexity(); + return super.visit(node, data); + } + + @Override + public Object visit(ASTWhileLoopStatement node, Object data) { + State state = (State) data; + + state.increaseNestingLevel(); + super.visit(node, data); + state.decreaseNestingLevel(); + + return data; + } + + @Override + public Object visit(ASTCatchBlockStatement node, Object data) { + State state = (State) data; + + state.increaseNestingLevel(); + super.visit(node, data); + state.decreaseNestingLevel(); + + return data; + } + + @Override + public Object visit(ASTDoLoopStatement node, Object data) { + State state = (State) data; + + state.increaseNestingLevel(); + super.visit(node, data); + state.decreaseNestingLevel(); + + return data; + } + + @Override + public Object visit(ASTTernaryExpression node, Object data) { + State state = (State) data; + + state.increaseNestingLevel(); + super.visit(node, data); + state.decreaseNestingLevel(); + + return data; + } + + @Override + public Object visit(ASTBooleanExpression node, Object data) { + State state = (State) data; + + BooleanOp op = node.getNode().getOp(); + if (op == BooleanOp.AND || op == BooleanOp.OR) { + state.booleanOperation(op); + } + + return super.visit(node, data); + } + + @Override + public Object visit(ASTPrefixExpression node, Object data) { + State state = (State) data; + + PrefixOp op = node.getNode().getOp(); + if (op == PrefixOp.NOT) { + state.booleanOperation(null); + } + + return super.visit(node, data); + } + + @Override + public Object visit(ASTBlockStatement node, Object data) { + State state = (State) data; + + for (ApexNode child : node.children()) { + child.jjtAccept(this, data); + + // This needs to happen because the current 'run' of boolean operations is terminated + // once we finish a statement. + state.booleanOperation(null); + } + + return data; + } + + @Override + public Object visit(ASTMethod node, Object data) { + State state = (State) data; + state.setMethodName(node.getNode().getMethodInfo().getCanonicalName()); + return super.visit(node, data); + } + + @Override + public Object visit(ASTMethodCallExpression node, Object data) { + State state = (State) data; + state.methodCall(node.getNode().getMethodName()); + return super.visit(node, data); + } +} diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/design/CognitiveComplexityRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/design/CognitiveComplexityRule.java new file mode 100644 index 0000000000..319afdd529 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/design/CognitiveComplexityRule.java @@ -0,0 +1,107 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.design; + +import static net.sourceforge.pmd.properties.constraints.NumericConstraints.positive; + +import java.util.Stack; + +import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; +import net.sourceforge.pmd.lang.apex.ast.ASTUserTrigger; +import net.sourceforge.pmd.lang.apex.metrics.ApexMetrics; +import net.sourceforge.pmd.lang.apex.metrics.api.ApexClassMetricKey; +import net.sourceforge.pmd.lang.apex.metrics.api.ApexOperationMetricKey; +import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; +import net.sourceforge.pmd.lang.metrics.MetricsUtil; +import net.sourceforge.pmd.lang.metrics.ResultOption; +import net.sourceforge.pmd.properties.PropertyDescriptor; +import net.sourceforge.pmd.properties.PropertyFactory; + +public class CognitiveComplexityRule extends AbstractApexRule { + + private static final PropertyDescriptor CLASS_LEVEL_DESCRIPTOR + = PropertyFactory.intProperty("classReportLevel") + .desc("Total class cognitive complexity reporting threshold") + .require(positive()) + .defaultValue(50) + .build(); + + private static final PropertyDescriptor METHOD_LEVEL_DESCRIPTOR + = PropertyFactory.intProperty("methodReportLevel") + .desc("Cognitive complexity reporting threshold") + .require(positive()) + .defaultValue(15) + .build(); + + private Stack classNames = new Stack<>(); + private boolean inTrigger; + + + public CognitiveComplexityRule() { + definePropertyDescriptor(CLASS_LEVEL_DESCRIPTOR); + definePropertyDescriptor(METHOD_LEVEL_DESCRIPTOR); + } + + + @Override + public Object visit(ASTUserTrigger node, Object data) { + inTrigger = true; + super.visit(node, data); + inTrigger = false; + return data; + } + + + @Override + public Object visit(ASTUserClass node, Object data) { + + classNames.push(node.getImage()); + super.visit(node, data); + classNames.pop(); + + if (ApexClassMetricKey.COGNITIVE.supports(node)) { + int classCognitive = (int) MetricsUtil.computeMetric(ApexClassMetricKey.COGNITIVE, node); + + if (classCognitive >= getProperty(CLASS_LEVEL_DESCRIPTOR)) { + int classHighest = (int) ApexMetrics.get(ApexOperationMetricKey.COGNITIVE, node, ResultOption.HIGHEST); + + String[] messageParams = { + "class", + node.getImage(), + " total", + classCognitive + " (highest " + classHighest + ")", + }; + + addViolation(data, node, messageParams); + } + } + return data; + } + + + @Override + public final Object visit(ASTMethod node, Object data) { + + if (ApexOperationMetricKey.COGNITIVE.supports(node)) { + int cognitive = (int) MetricsUtil.computeMetric(ApexOperationMetricKey.COGNITIVE, node); + if (cognitive >= getProperty(METHOD_LEVEL_DESCRIPTOR)) { + String opType = inTrigger ? "trigger" + : node.getImage().equals(classNames.peek()) ? "constructor" + : "method"; + + addViolation(data, node, new String[] { + opType, + node.getQualifiedName().getOperation(), + "", + "" + cognitive, + }); + } + } + + return data; + } + +} diff --git a/pmd-apex/src/main/resources/category/apex/design.xml b/pmd-apex/src/main/resources/category/apex/design.xml index 790c5cd416..ffac0783e4 100644 --- a/pmd-apex/src/main/resources/category/apex/design.xml +++ b/pmd-apex/src/main/resources/category/apex/design.xml @@ -86,6 +86,65 @@ public class Complicated { + + +Methods that are highly complex are difficult to read and more costly to maintain. If you include too much decisional +logic within a single method, you make its behavior hard to understand and more difficult to modify. + +Cognitive complexity is a measure of how difficult it is for humans to read and understand a method. Code that contains +a break in the control flow is more complex, whereas the use of language shorthands doesn't increase the level of +complexity. Nested control flows can make a method more difficult to understand, with each additional nesting of the +control flow leading to an increase in cognitive complexity. + +Information about Cognitive complexity can be found in the original paper here: +https://www.sonarsource.com/docs/CognitiveComplexity.pdf + +By default, this rule reports methods with a complexity of 15 or more. Reported methods should be broken down into less +complex components. + + 3 + + contacts) { + List contactsToUpdate = new List(); + + for (Contact contact : contacts) { // +1 + if (contact.Department == 'Finance') { // +2 + contact.Title = 'Finance Specialist'; + contactsToUpdate.add(contact); + } else if (contact.Department == 'Sales') { // +2 + contact.Title = 'Sales Specialist'; + contactsToUpdate.add(contact); + } + } + + update contactsToUpdate; + } +} +]]> + + + + + + If statements have complexity 1 + 1 + + 'c__Foo#foo(Integer)' has value 1. + + + + + + + + Nested if statements bump complexity level + 1 + + 'c__Foo#foo(Integer)' has value 3. + + + 0) { // +1 + if (n == 1) { // +2 + return "one"; + } + + return "positive"; + } + + return "negative or 0"; + } + } + ]]> + + + + + Non nested if statements don't incur a penalty + 1 + + 'c__Foo#foo(Integer)' has value 3. + + + 0) { // +1 + return "positive"; + } + + if (n == 0) { // +1 + return "zero"; + } + + if (n < 0) { // +1 + return "negative"; + } + } + } + ]]> + + + + + Else-if blocks count as non-nested + 1 + + 'c__Foo#foo(Integer)' has value 3. + + + 0) { // +1 + return "positive"; + } else if (n < 0) { // +1 + return "negative"; + } else { // +1 + return "zero"; + } + } + } + ]]> + + + + + For loops increment nesting + 1 + + 'c__Foo#foo()' has value 3. + + + + + + + + Foreach loops increment nesting + 1 + + 'c__Foo#foo()' has value 3. + + + + + + + + Continue statements increase complexity + 1 + + 'c__Foo#foo()' has value 4. + + + + + + + + Break statements increase complexity + 1 + + 'c__Foo#foo()' has value 4. + + + + + + + + While statements increase nesting + 1 + + 'c__Foo#foo()' has value 3. + + + 1000) { // +2 + return "big"; + } + } + + return "small"; + } + } + ]]> + + + + + Only the catch statement increases nesting + 1 + + 'c__Foo#foo()' has value 4. + + + + + + + + Do-while loops cause nesting + 1 + + 'c__Foo#foo()' has value 5. + + + + + + + + Ternary operators cause nesting + 1 + + 'c__Foo#foo(Integer)' has value 3. + + + 0 ? // +2 + 1 : 0; + } + } + ]]> + + + + + Boolean operators + 6 + + 'c__Foo#a(Integer)' has value 1. + 'c__Foo#b(Integer)' has value 1. + 'c__Foo#c(Integer)' has value 1. + 'c__Foo#d(Boolean,Boolean,Boolean,Boolean,Boolean,Boolean)' has value 3. + 'c__Foo#e(Boolean,Boolean,Boolean)' has value 2. + 'c__Foo#f()' has value 2. + + + 0 && n > 1; // +1 + } + + Boolean b(Integer n) { + return n > 0 && n > 1 && n > 2; // +1 + } + + Boolean c(Integer n) { + return n > 0 || n < 0; // +1 + } + + Boolean d(Boolean a, Boolean b, Boolean c, Boolean d, Boolean e, Boolean f) { + return (a + && b && c) // +1 + || (d || e) // +1 + && f; // +1 + } + + Boolean e(Boolean a, Boolean b, Boolean c) { + return a + && // +1 + !(b && c); // +1 + } + + Boolean f() { + Boolean a = true && false; // +1 + return a && true; // +1 + } + } + ]]> + + + + + Recursion bumps complexity value + 1 + + 'c__Foo#foo(Integer)' has value 3. + + + + + + \ No newline at end of file diff --git a/pmd-apex/src/test/resources/rulesets/apex/metrics_test.xml b/pmd-apex/src/test/resources/rulesets/apex/metrics_test.xml index b9980f7571..cbb1c7324f 100644 --- a/pmd-apex/src/test/resources/rulesets/apex/metrics_test.xml +++ b/pmd-apex/src/test/resources/rulesets/apex/metrics_test.xml @@ -19,4 +19,9 @@ class="net.sourceforge.pmd.lang.apex.metrics.impl.WmcTestRule"> + + + diff --git a/pmd-core/src/main/resources/rulesets/releases/6220.xml b/pmd-core/src/main/resources/rulesets/releases/6220.xml new file mode 100644 index 0000000000..e810572d8e --- /dev/null +++ b/pmd-core/src/main/resources/rulesets/releases/6220.xml @@ -0,0 +1,13 @@ + + + + +This ruleset contains links to rules that are new in PMD v6.22.0 + + + + +