From 6b0c771f03d95f9224fc876dfd0bd3ff80d18343 Mon Sep 17 00:00:00 2001 From: kazachka Date: Wed, 1 Feb 2017 21:26:30 +0300 Subject: [PATCH] Issue #56: fix NPathComplexityCheck --- config/suppressions.xml | 1 + .../checks/metrics/NPathComplexityCheck.java | 332 +++++++++++++++--- .../metrics/NPathComplexityCheckTest.java | 79 ++++- .../checks/metrics/InputNPathComplexity.java | 155 ++++++++ 4 files changed, 521 insertions(+), 46 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/checks/metrics/InputNPathComplexity.java diff --git a/config/suppressions.xml b/config/suppressions.xml index 5f8fa9a27510..35a61be4c187 100644 --- a/config/suppressions.xml +++ b/config/suppressions.xml @@ -29,6 +29,7 @@ + diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheck.java index f7eb59905958..acb3991395ad 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheck.java @@ -51,18 +51,28 @@ public final class NPathComplexityCheck extends AbstractCheck { /** The initial current value. */ private static final BigInteger INITIAL_VALUE = BigInteger.ONE; - /** Stack of values - all but the current value. */ - private final Deque valueStack = new ArrayDeque<>(); + /** + * Stack of NP values for ranges. + */ + private final Deque rangeValues = new ArrayDeque<>(); + + /** Stack of NP values for expressions. */ + private final Deque expressionValues = new ArrayDeque<>(); + + /** + * Range of the last processed expression. Used for checking that ternary operation + * which is a part of expression won't be processed for second time. + */ + private final ExpressionRange processedExpressionRange = new ExpressionRange(); - /** The current value. */ - private BigInteger currentValue = INITIAL_VALUE; + /** NP value for current range. */ + private BigInteger currentRangeValue = INITIAL_VALUE; /** Threshold to report error for. */ private int max = DEFAULT_MAX; /** * Set the maximum threshold allowed. - * * @param max the maximum threshold */ public void setMax(int max) { @@ -87,10 +97,12 @@ public int[] getAcceptableTokens() { TokenTypes.LITERAL_IF, TokenTypes.LITERAL_ELSE, TokenTypes.LITERAL_SWITCH, - TokenTypes.LITERAL_CASE, + TokenTypes.CASE_GROUP, TokenTypes.LITERAL_TRY, TokenTypes.LITERAL_CATCH, TokenTypes.QUESTION, + TokenTypes.LITERAL_RETURN, + TokenTypes.LITERAL_DEFAULT, }; } @@ -99,6 +111,14 @@ public int[] getRequiredTokens() { return getAcceptableTokens(); } + @Override + public void beginTree(DetailAST rootAST) { + rangeValues.clear(); + expressionValues.clear(); + processedExpressionRange.resetRange(); + currentRangeValue = INITIAL_VALUE; + } + @Override public void visitToken(DetailAST ast) { switch (ast.getType()) { @@ -106,21 +126,36 @@ public void visitToken(DetailAST ast) { case TokenTypes.LITERAL_DO: case TokenTypes.LITERAL_FOR: case TokenTypes.LITERAL_IF: - case TokenTypes.QUESTION: + visitConditional(ast, 1); + break; case TokenTypes.LITERAL_TRY: - case TokenTypes.LITERAL_SWITCH: visitMultiplyingConditional(); break; - case TokenTypes.LITERAL_ELSE: + case TokenTypes.LITERAL_SWITCH: + visitConditional(ast, 0); + break; case TokenTypes.LITERAL_CATCH: - case TokenTypes.LITERAL_CASE: visitAddingConditional(); break; + case TokenTypes.QUESTION: + if (!processedExpressionRange.belongsToRange(ast)) { + visitUnitaryOperator(ast, 1); + } + break; + case TokenTypes.LITERAL_RETURN: + visitUnitaryOperator(ast, 0); + break; + case TokenTypes.CASE_GROUP: + final int caseNumber = countCaseTokens(ast); + pushValue(caseNumber); + break; + case TokenTypes.LITERAL_ELSE: + case TokenTypes.LITERAL_DEFAULT: case TokenTypes.CTOR_DEF: case TokenTypes.METHOD_DEF: case TokenTypes.INSTANCE_INIT: case TokenTypes.STATIC_INIT: - visitMethodDef(); + pushValue(0); break; default: break; @@ -134,16 +169,26 @@ public void leaveToken(DetailAST ast) { case TokenTypes.LITERAL_DO: case TokenTypes.LITERAL_FOR: case TokenTypes.LITERAL_IF: - case TokenTypes.QUESTION: - case TokenTypes.LITERAL_TRY: case TokenTypes.LITERAL_SWITCH: + case TokenTypes.LITERAL_RETURN: + leaveConditional(); + break; + case TokenTypes.LITERAL_TRY: leaveMultiplyingConditional(); break; - case TokenTypes.LITERAL_ELSE: + case TokenTypes.QUESTION: + if (!processedExpressionRange.belongsToRange(ast)) { + leaveConditional(); + } + break; case TokenTypes.LITERAL_CATCH: - case TokenTypes.LITERAL_CASE: leaveAddingConditional(); break; + case TokenTypes.LITERAL_ELSE: + case TokenTypes.CASE_GROUP: + case TokenTypes.LITERAL_DEFAULT: + leaveBranch(); + break; case TokenTypes.CTOR_DEF: case TokenTypes.METHOD_DEF: case TokenTypes.INSTANCE_INIT: @@ -157,54 +202,257 @@ public void leaveToken(DetailAST ast) { /** Visits else, catch or case. */ private void visitAddingConditional() { - pushValue(); + pushValue(0); + } + + /** + * Visits if, while, do-while, for and switch tokens - all of them have expression in + * parentheses which is used for calculation. + * @param ast visited token. + * @param basicBranchingFactor default number of branches added. + */ + private void visitConditional(DetailAST ast, int basicBranchingFactor) { + int expressionValue = basicBranchingFactor; + DetailAST bracketed; + for (bracketed = ast.findFirstToken(TokenTypes.LPAREN).getNextSibling(); + bracketed.getType() != TokenTypes.RPAREN; + bracketed = bracketed.getNextSibling()) { + expressionValue += countLogicalOperators(bracketed); + } + processedExpressionRange.setRange(ast, bracketed); + pushValue(expressionValue); + } + + /** + * Visits ternary operator (?:) and return tokens. They differ from those processed by + * visitConditional method in that their expression isn't bracketed. + * @param ast visited token. + * @param basicBranchingFactor number of branches inherently added by this token. + */ + private void visitUnitaryOperator(DetailAST ast, int basicBranchingFactor) { + processedExpressionRange.setRange(ast, getLastToken(ast)); + final int expressionValue = basicBranchingFactor + countLogicalOperators(ast); + pushValue(expressionValue); + } + + /** Leaves while, do, for, if, ternary (?::), return or switch. */ + private void leaveConditional() { + final Values valuePair = popValue(); + final BigInteger basicRangeValue = valuePair.getRangeValue(); + final BigInteger expressionValue = valuePair.getExpressionValue(); + currentRangeValue = currentRangeValue.add(expressionValue).multiply(basicRangeValue); + } + + /** Leaves else, default or case group tokens. */ + private void leaveBranch() { + final Values valuePair = popValue(); + final BigInteger basicRangeValue = valuePair.getRangeValue(); + final BigInteger expressionValue = valuePair.getExpressionValue(); + currentRangeValue = currentRangeValue + .subtract(BigInteger.ONE).add(basicRangeValue).add(expressionValue); + } + + /** + * Process the end of a method definition. + * @param ast the token type representing the method definition + */ + private void leaveMethodDef(DetailAST ast) { + final BigInteger bigIntegerMax = BigInteger.valueOf(max); + if (currentRangeValue.compareTo(bigIntegerMax) > 0) { + log(ast, MSG_KEY, currentRangeValue, bigIntegerMax); + } + popValue(); + currentRangeValue = INITIAL_VALUE; } - /** Leaves else, catch or case. */ + /** Leaves catch. */ private void leaveAddingConditional() { - currentValue = currentValue.subtract(BigInteger.ONE).add(popValue()); + currentRangeValue = currentRangeValue.subtract(BigInteger.ONE) + .add(popValue().getRangeValue()); } - /** Visits while, do, for, if, try, ? (in ?::) or switch. */ + /** + * Pushes the current range value on the range value stack. Pushes this token expression value + * on the expression value stack. + * @param expressionValue value of expression calculated for current token. + */ + private void pushValue(Integer expressionValue) { + rangeValues.push(currentRangeValue); + expressionValues.push(expressionValue); + currentRangeValue = INITIAL_VALUE; + } + + /** Visits try. */ private void visitMultiplyingConditional() { - pushValue(); + pushValue(0); + } + + /** + * Pops values from both stack of expression values and stack of range values. + * @return pair of head values from both of the stacks. + */ + private Values popValue() { + final int expressionValue = expressionValues.pop(); + return new Values(rangeValues.pop(), BigInteger.valueOf(expressionValue)); } - /** Leaves while, do, for, if, try, ? (in ?::) or switch. */ + /** Leaves try. */ private void leaveMultiplyingConditional() { - currentValue = currentValue.add(BigInteger.ONE).multiply(popValue()); + currentRangeValue = currentRangeValue.add(BigInteger.ONE) + .multiply(popValue().getRangeValue()); } - /** Push the current value on the stack. */ - private void pushValue() { - valueStack.push(currentValue); - currentValue = INITIAL_VALUE; + /** + * Calculates number of logical operators, including inline ternary operatior, for a token. + * @param ast inspected token. + * @return number of logical operators. + */ + private static int countLogicalOperators(DetailAST ast) { + int number = 0; + for (DetailAST child = ast.getFirstChild(); child != null; + child = child.getNextSibling()) { + final int type = child.getType(); + if (type == TokenTypes.LOR || type == TokenTypes.LAND) { + number++; + } + else if (type == TokenTypes.QUESTION) { + number += 2; + } + number += countLogicalOperators(child); + } + return number; } /** - * Pops a value off the stack and makes it the current value. - * @return pop a value off the stack and make it the current value + * Finds a leaf, which is the most distant from the root. + * @param ast the root of tree. + * @return the leaf. */ - private BigInteger popValue() { - currentValue = valueStack.pop(); - return currentValue; + private static DetailAST getLastToken(DetailAST ast) { + final DetailAST lastChild = ast.getLastChild(); + final DetailAST result; + if (lastChild.getFirstChild() == null) { + result = lastChild; + } + else { + result = getLastToken(lastChild); + } + return result; } - /** Process the start of the method definition. */ - private void visitMethodDef() { - pushValue(); + /** + * Counts number of case tokens subject to a case group token. + * @param ast case group token. + * @return number of case tokens. + */ + private static int countCaseTokens(DetailAST ast) { + int counter = 0; + for (DetailAST iterator = ast.getFirstChild(); iterator != null; + iterator = iterator.getNextSibling()) { + if (iterator.getType() == TokenTypes.LITERAL_CASE) { + counter++; + } + } + return counter; } /** - * Process the end of a method definition. - * - * @param ast the token representing the method definition + * A range between tokens. Used to prevent inline ternary operator from being processed + * twice. */ - private void leaveMethodDef(DetailAST ast) { - final BigInteger bigIntegerMax = BigInteger.valueOf(max); - if (currentValue.compareTo(bigIntegerMax) > 0) { - log(ast, MSG_KEY, currentValue, bigIntegerMax); + private static class ExpressionRange { + /** Start line of the range. */ + private int startLineNo; + + /** Start column of the range. */ + private int startColumnNo; + + /** End line of the range. */ + private int endLineNo; + + /** End column of the range. */ + private int endColumnNo; + + /** + * Sets range between two tokens. + * @param startToken starting token of the range. + * @param endToken ending token of the range. + */ + public void setRange(DetailAST startToken, DetailAST endToken) { + if (!belongsToRange(startToken)) { + startLineNo = startToken.getLineNo(); + startColumnNo = startToken.getColumnNo(); + } + if (!belongsToRange(endToken)) { + endLineNo = endToken.getLineNo(); + endColumnNo = endToken.getColumnNo(); + } + } + + /** Sets range to the start of the file. */ + public void resetRange() { + startLineNo = 0; + startColumnNo = 0; + endLineNo = 0; + endColumnNo = 0; + } + + /** + * Checks if token belongs to the range. + * @param ast inspected token. + * @return true, if token belongs to the range. + */ + public boolean belongsToRange(DetailAST ast) { + final int lineNo = ast.getLineNo(); + final int columnNo = ast.getColumnNo(); + boolean inRange = true; + if (lineNo < startLineNo + || lineNo > endLineNo + || lineNo == endLineNo + && (columnNo <= startColumnNo || columnNo > endColumnNo)) { + inRange = false; + } + return inRange; } - popValue(); } + + /** + * Class that store range value and expression value. + */ + private static class Values { + + /** NP value for range. */ + private final BigInteger rangeValue; + + /** NP value for expression. */ + private final BigInteger expressionValue; + + /** + * Constructor that assigns all of class fields. + * @param valueOfRange NP value for range + * @param valueOfExpression NP value for expression + */ + Values(BigInteger valueOfRange, BigInteger valueOfExpression) { + rangeValue = valueOfRange; + expressionValue = valueOfExpression; + } + + /** + * Returns NP value for range. + * @return NP value for range + */ + public BigInteger getRangeValue() { + return rangeValue; + } + + /** + * Returns NP value for expression. + * @return NP value for expression + */ + public BigInteger getExpressionValue() { + return expressionValue; + } + + } + } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheckTest.java index 11d0f26220c6..8675428da2ba 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/metrics/NPathComplexityCheckTest.java @@ -50,7 +50,7 @@ public void testCalculation() throws Exception { final String[] expected = { "4:5: " + getCheckMessage(MSG_KEY, 2, 0), "7:17: " + getCheckMessage(MSG_KEY, 2, 0), - "17:5: " + getCheckMessage(MSG_KEY, 5, 0), + "17:5: " + getCheckMessage(MSG_KEY, 10, 0), "27:5: " + getCheckMessage(MSG_KEY, 3, 0), "34:5: " + getCheckMessage(MSG_KEY, 7, 0), "48:5: " + getCheckMessage(MSG_KEY, 3, 0), @@ -63,6 +63,30 @@ public void testCalculation() throws Exception { verify(checkConfig, getPath("InputComplexity.java"), expected); } + @Test + public void testCalculation2() throws Exception { + final DefaultConfiguration checkConfig = + createCheckConfig(NPathComplexityCheck.class); + + checkConfig.addAttribute("max", "0"); + final String[] expected = { + "5:5: " + getCheckMessage(MSG_KEY, 5, 0), + "12:5: " + getCheckMessage(MSG_KEY, 5, 0), + "22:5: " + getCheckMessage(MSG_KEY, 4, 0), + "34:5: " + getCheckMessage(MSG_KEY, 4, 0), + "48:5: " + getCheckMessage(MSG_KEY, 6, 0), + "62:5: " + getCheckMessage(MSG_KEY, 15, 0), + "86:5: " + getCheckMessage(MSG_KEY, 11, 0), + "100:5: " + getCheckMessage(MSG_KEY, 10, 0), + "112:5: " + getCheckMessage(MSG_KEY, 120, 0), + "124:5: " + getCheckMessage(MSG_KEY, 21, 0), + "138:5: " + getCheckMessage(MSG_KEY, 35, 0), + "146:5: " + getCheckMessage(MSG_KEY, 25, 0), + }; + + verify(checkConfig, getPath("InputNPathComplexity.java"), expected); + } + @Test public void testIntegerOverflow() throws Exception { final DefaultConfiguration checkConfig = @@ -104,10 +128,10 @@ public void testGetAcceptableTokens() { TokenTypes.LITERAL_IF, TokenTypes.LITERAL_ELSE, TokenTypes.LITERAL_SWITCH, - TokenTypes.LITERAL_CASE, - TokenTypes.LITERAL_TRY, - TokenTypes.LITERAL_CATCH, + TokenTypes.CASE_GROUP, TokenTypes.QUESTION, + TokenTypes.LITERAL_RETURN, + TokenTypes.LITERAL_DEFAULT, }; Assert.assertNotNull(actual); Assert.assertArrayEquals(expected, actual); @@ -145,4 +169,51 @@ public void testDefaultHooks() { npathComplexityCheckObj.visitToken(ast); npathComplexityCheckObj.leaveToken(ast); } + + @Test + public void testVisitTokenBeforeExpressionRange() { + // Create first ast + final DetailAST astIf = mockAST(TokenTypes.LITERAL_IF, "if", "mockfile", 2, 2); + final DetailAST astIfLParen = mockAST(TokenTypes.LPAREN, "(", "mockfile", 3, 3); + astIf.addChild(astIfLParen); + final DetailAST astIfTrue = + mockAST(TokenTypes.LITERAL_TRUE, "true", "mockfile", 3, 3); + astIf.addChild(astIfTrue); + final DetailAST astIfRParen = mockAST(TokenTypes.RPAREN, ")", "mockfile", 4, 4); + astIf.addChild(astIfRParen); + // Create ternary ast + final DetailAST astTernary = mockAST(TokenTypes.QUESTION, "?", "mockfile", 1, 1); + final DetailAST astTernaryTrue = + mockAST(TokenTypes.LITERAL_TRUE, "true", "mockfile", 1, 2); + astTernary.addChild(astTernaryTrue); + + final NPathComplexityCheck mock = new NPathComplexityCheck(); + // visiting first ast, set expressionSpatialRange to [2,2 - 4,4] + mock.visitToken(astIf); + //visiting ternary, it lies before expressionSpatialRange + mock.visitToken(astTernary); + } + + /** + * Creates MOCK lexical token and returns AST node for this token. + * @param tokenType type of token + * @param tokenText text of token + * @param tokenFileName file name of token + * @param tokenRow token position in a file (row) + * @param tokenColumn token position in a file (column) + * @return AST node for the token + */ + private static DetailAST mockAST(final int tokenType, final String tokenText, + final String tokenFileName, final int tokenRow, final int tokenColumn) { + final CommonHiddenStreamToken tokenImportSemi = new CommonHiddenStreamToken(); + tokenImportSemi.setType(tokenType); + tokenImportSemi.setText(tokenText); + tokenImportSemi.setLine(tokenRow); + tokenImportSemi.setColumn(tokenColumn); + tokenImportSemi.setFilename(tokenFileName); + final DetailAST astSemi = new DetailAST(); + astSemi.initialize(tokenImportSemi); + return astSemi; + } + } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/metrics/InputNPathComplexity.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/metrics/InputNPathComplexity.java new file mode 100644 index 000000000000..323ea26d9473 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/metrics/InputNPathComplexity.java @@ -0,0 +1,155 @@ +package com.puppycrawl.tools.checkstyle.checks.metrics; +// Advise: for lack of ambiguity try to make all factors prime numbers +public class InputNPathComplexity { + //NP = 5 + void testIfWithExpression() { + if (true && true || (true || true)) { + + } + } + + //NP = 5 + void testIfElseWithExpression() { + if (true && true || (true || true)) { + + } + else { + + } + } + + //NP = 4 + void testSimpleSwitch() { + int a = 0; + switch(a) { + case 1: + break; + case 2: + case 3: + break; + } + } + + //NP = 4 + void testSimpleSwitchWithDefault() { + int a = 0; + switch(a) { + case 1: + break; + case 2: + case 3: + break; + default: + break; + } + } + + //NP = 6 + void testSwitchWithExpression() { + int a = 0; + switch(true ? a : a) { + case 1: + break; + case 2: + case 3: + break; + default: + break; + } + } + + //NP = 15 + void testComplexSwitch() { + int a = 0; + switch(a) { + case 1: + if (true) { + } + break; + case 2: + if (true && true || (true || true)) { + } + else { + } + if (true) { + } + case 3: + if (true) { + } + break; + default: + break; + } + } + + //NP = 11 + void testComplexIfElse() { + //NP = 2 + if (true && true) { + } + //NP += 3 + else if (true || true || true) { + } + //NP += 5 + else if (true && true && true || true || true) { + } + //NP += 1 + } + + //NP = 10 + boolean testComplexReturn() { + if (true && true) { + //NP(return) = 4 + return true && true || (true && true); + } + else { + //NP(return) = 5 + return true ? true && true : true || true; + } + } + + //NP = 120 + void testForCyclesComplex() { + //NP(for) = 2 + for (int i = 0; i < 10; i++); + //NP(for) = 3 + for (int i = 0; i < 10 && true; i++); + //NP(for) = 4 + for (int i = true ? 0 : 0; i < 10; i++); + //NP(for) = 5 + for (int i = 0; true ? i < 10 : true || true; i++); + } + + //NP = 21 + void testDoWhileCyclesComplex() { + int a = 0; + //NP(do-while) = 3 + do { + } while (a < 10 && true); + //NP(do-while) = 3 + 3 + 1 = 7 + do { + //NP(do-while) = 3 + do { + } while (a < 10 || true); + } while (true ? a > 10 : (a < 10 || true)); + } + + //NP = 35 + void testComplexTernaryOperator() { + //NP(t) = 7 + boolean a = true ? (true ? true : true) : (false ? (true || false) : true); + //NP(t) = 5 + boolean b = true ? (true ? true : true) : true || true; + } + + //NP = 25 + void testSimpleTernaryBadFormatting() { + if( + true ? true : true + ) { boolean a = true ? true : true; + } + if( + true ? true : true) { boolean b = true ? true : true; + } + } +}