From 60fda440a56ca1e2a090f62f9ceeae7467ae1b2f Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Mon, 4 Nov 2019 15:36:34 +0100 Subject: [PATCH 1/2] Rule S1871: Ignore conditional expressions to avoid overlap with S3923 --- .../test/resources/expected/python-S1871.json | 3 -- .../sonar/python/checks/SameBranchCheck.java | 38 ------------------- .../src/test/resources/checks/sameBranch.py | 31 --------------- 3 files changed, 72 deletions(-) diff --git a/its/ruling/src/test/resources/expected/python-S1871.json b/its/ruling/src/test/resources/expected/python-S1871.json index 212b4a383e..fe57ef5ff2 100644 --- a/its/ruling/src/test/resources/expected/python-S1871.json +++ b/its/ruling/src/test/resources/expected/python-S1871.json @@ -42,9 +42,6 @@ 'project:numpy-1.16.4/numpy/testing/_private/utils.py':[ 2206, ], -'project:numpy-1.16.4/tools/npy_tempita/__init__.py':[ -801, -], 'project:tornado-2.3/demos/appengine/markdown.py':[ 765, ], diff --git a/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java b/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java index 17725f5fd2..aa21583ba8 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java +++ b/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java @@ -25,11 +25,8 @@ import org.sonar.check.Rule; import org.sonar.plugins.python.api.PythonSubscriptionCheck; import org.sonar.plugins.python.api.SubscriptionContext; -import org.sonar.plugins.python.api.tree.ConditionalExpression; import org.sonar.plugins.python.api.tree.ElseClause; -import org.sonar.plugins.python.api.tree.Expression; import org.sonar.plugins.python.api.tree.IfStatement; -import org.sonar.plugins.python.api.tree.ParenthesizedExpression; import org.sonar.plugins.python.api.tree.Statement; import org.sonar.plugins.python.api.tree.StatementList; import org.sonar.plugins.python.api.tree.Token; @@ -61,16 +58,6 @@ public void initialize(Context context) { List branches = getIfBranches(ifStmt); findSameBranches(branches, ctx); }); - - context.registerSyntaxNodeConsumer(Tree.Kind.CONDITIONAL_EXPR, ctx -> { - ConditionalExpression conditionalExpression = (ConditionalExpression) ctx.syntaxNode(); - if (ignoreList.contains(conditionalExpression)) { - return; - } - List expressions = new ArrayList<>(); - addConditionalExpressionBranches(expressions, conditionalExpression); - findSameBranches(expressions, ctx); - }); } private static boolean allIdenticalBranches(IfStatement ifStmt) { @@ -124,31 +111,6 @@ private List getIfBranches(IfStatement ifStmt) { return branches; } - private void addConditionalExpressionBranches(List branches, ConditionalExpression conditionalExpression) { - Expression trueExpression = removeParentheses(conditionalExpression.trueExpression()); - Expression falseExpression = removeParentheses(conditionalExpression.falseExpression()); - if (trueExpression.is(Tree.Kind.CONDITIONAL_EXPR)) { - ignoreList.add(trueExpression); - addConditionalExpressionBranches(branches, (ConditionalExpression) trueExpression); - } else { - branches.add(trueExpression); - } - if (falseExpression.is(Tree.Kind.CONDITIONAL_EXPR)) { - ignoreList.add(falseExpression); - addConditionalExpressionBranches(branches, (ConditionalExpression) falseExpression); - } else { - branches.add(falseExpression); - } - } - - private static Expression removeParentheses(Expression expression) { - if (expression.is(Tree.Kind.PARENTHESIZED)) { - return removeParentheses(((ParenthesizedExpression) expression).expression()); - } else { - return expression; - } - } - private void lookForElseIfs(List branches, ElseClause elseBranch) { IfStatement singleIfChild = singleIfChild(elseBranch.body()); if (singleIfChild != null) { diff --git a/python-checks/src/test/resources/checks/sameBranch.py b/python-checks/src/test/resources/checks/sameBranch.py index aac1d44dc8..76afc36ff3 100644 --- a/python-checks/src/test/resources/checks/sameBranch.py +++ b/python-checks/src/test/resources/checks/sameBranch.py @@ -74,37 +74,6 @@ else: print("2") -a = 1 if 1 else 2 -a = 1 if x else 1 # Noncompliant [[secondary=+0]] -# ^ -a = 1 if x else 2 if y else 2 -a = 2 if x else 1 if y else 2 -a = 2 if x else 2 if y else 2 # Noncompliant -# ^ -a = 2 \ - if x else 2 if y \ - else 2 -# Noncompliant@-1 -a = 2 if x else (t + 4) if y else (t + - 4) -# Noncompliant@-2 -a = (1 if x else 3) if y else 5 - -a = 1 if x else (1) # Noncompliant -# ^ -a = ((1)) if x else (1) # Noncompliant -# ^ -a = ((1)) if x else 1 # Noncompliant -# ^ -a = 2 if x else ((2) if y else 2) # Noncompliant -# ^ -a = (1, 2) if x else (1, 3) -a = (1, 2) if x else (1, 2) # Noncompliant -# ^^^^^^ -a = [1] if x else [2] -a = [1] if x else [1] # Noncompliant -# ^^^ - if x in ('a', 'b'): print("1") print("2") From 1593cc835e59e98d831fb73120886f179ff49e23 Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Mon, 4 Nov 2019 15:55:44 +0100 Subject: [PATCH 2/2] SONARPY-477 Rule S1871: Avoid raising multiple issues on the same duplicated block --- .../test/resources/expected/python-S1871.json | 1 + .../sonar/python/checks/SameBranchCheck.java | 24 +++++++++++++++---- .../src/test/resources/checks/sameBranch.py | 19 +++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/its/ruling/src/test/resources/expected/python-S1871.json b/its/ruling/src/test/resources/expected/python-S1871.json index fe57ef5ff2..e5335b205e 100644 --- a/its/ruling/src/test/resources/expected/python-S1871.json +++ b/its/ruling/src/test/resources/expected/python-S1871.json @@ -52,6 +52,7 @@ 295, ], 'project:twisted-12.1.0/twisted/protocols/ftp.py':[ +1590, 1592, ], 'project:twisted-12.1.0/twisted/spread/banana.py':[ diff --git a/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java b/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java index aa21583ba8..c984866de0 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java +++ b/python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java @@ -85,20 +85,34 @@ private static void checkBranches(List branches, int index, Subs if (CheckUtils.areEquivalent(originalBlock, duplicateBlock)) { equivalentBlocks.add(originalBlock); boolean allBranchesIdentical = equivalentBlocks.size() == branches.size() - 1; - if (!isOnASingleLine || allBranchesIdentical) { + if (!isOnASingleLine && !allBranchesIdentical) { int line = TreeUtils.nonWhitespaceTokens(originalBlock).get(0).line(); String message = String.format(MESSAGE, line); List issueTokens = TreeUtils.nonWhitespaceTokens(duplicateBlock); - PreciseIssue issue = ctx.addIssue(issueTokens.get(0), issueTokens.get(issueTokens.size() - 1), message); - equivalentBlocks.forEach(e -> { - List tokens = TreeUtils.nonWhitespaceTokens(e); - issue.secondary(IssueLocation.preciseLocation(tokens.get(0), tokens.get(tokens.size() - 1), "Original")); + ctx.addIssue(issueTokens.get(0), issueTokens.get(issueTokens.size() - 1), message) + .secondary(issueLocation(originalBlock, "Original")); + break; + } + if (allBranchesIdentical) { + equivalentBlocks.add(duplicateBlock); + Tree firstBlock = branches.get(0); + int line = TreeUtils.nonWhitespaceTokens(firstBlock).get(0).line(); + String message = String.format(MESSAGE, line); + equivalentBlocks.stream().skip(1).forEach(e -> { + List issueTokens = TreeUtils.nonWhitespaceTokens(e); + ctx.addIssue(issueTokens.get(0), issueTokens.get(issueTokens.size() - 1), message) + .secondary(issueLocation(firstBlock, "Original")); }); } } } } + private static IssueLocation issueLocation(Tree body, String message) { + List tokens = TreeUtils.nonWhitespaceTokens(body); + return IssueLocation.preciseLocation(tokens.get(0), tokens.get(tokens.size() - 1), message); + } + private List getIfBranches(IfStatement ifStmt) { List branches = new ArrayList<>(); branches.add(ifStmt.body()); diff --git a/python-checks/src/test/resources/checks/sameBranch.py b/python-checks/src/test/resources/checks/sameBranch.py index 76afc36ff3..6d80dcd453 100644 --- a/python-checks/src/test/resources/checks/sameBranch.py +++ b/python-checks/src/test/resources/checks/sameBranch.py @@ -32,6 +32,16 @@ #^[el=+2;ec=16] pass +if cond: + foo() + bar() +elif cond: + foo() #Noncompliant + bar() +elif cond: + foo() #Noncompliant + bar() + if True: #In this case, S3923 will raise a bug if True: @@ -50,6 +60,7 @@ elif 2: print("1"); foo() else: print("2") +#ok, no issue raised for branches with 1 line of code if 1: print("1") elif 2: @@ -57,6 +68,14 @@ else: print("1") +#exception: all branches identical without else clause +if cond: + foo() +elif cond: + foo() # Noncompliant +elif cond: + foo() # Noncompliant + #In this case, S3923 will raise a bug if 1: print("1")