Skip to content

SONARPY-477 Rule S1871: Avoid raising multiple issues on the same duplicated block #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pynicolas merged 2 commits into from
Nov 4, 2019
Merged

SONARPY-477 Rule S1871: Avoid raising multiple issues on the same duplicated block #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60fda44
Rule S1871: Ignore conditional expressions to avoid overlap with S3923
guillaume-dequenne Nov 4, 2019
1593cc8
SONARPY-477 Rule S1871: Avoid raising multiple issues on the same dup…
guillaume-dequenne Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions its/ruling/src/test/resources/expected/python-S1871.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
],
Expand All @@ -55,6 +52,7 @@
295,
],
'project:twisted-12.1.0/twisted/protocols/ftp.py':[
1590,
1592,
],
'project:twisted-12.1.0/twisted/spread/banana.py':[
Expand Down
62 changes: 19 additions & 43 deletions python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -61,16 +58,6 @@ public void initialize(Context context) {
List<StatementList> branches = getIfBranches(ifStmt);
findSameBranches(branches, ctx);
});

context.registerSyntaxNodeConsumer(Tree.Kind.CONDITIONAL_EXPR, ctx -> {
ConditionalExpression conditionalExpression = (ConditionalExpression) ctx.syntaxNode();
if (ignoreList.contains(conditionalExpression)) {
return;
}
List<Expression> expressions = new ArrayList<>();
addConditionalExpressionBranches(expressions, conditionalExpression);
findSameBranches(expressions, ctx);
});
}

private static boolean allIdenticalBranches(IfStatement ifStmt) {
Expand Down Expand Up @@ -98,20 +85,34 @@ private static void checkBranches(List<? extends Tree> 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<Token> issueTokens = TreeUtils.nonWhitespaceTokens(duplicateBlock);
PreciseIssue issue = ctx.addIssue(issueTokens.get(0), issueTokens.get(issueTokens.size() - 1), message);
equivalentBlocks.forEach(e -> {
List<Token> 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<Token> 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<Token> tokens = TreeUtils.nonWhitespaceTokens(body);
return IssueLocation.preciseLocation(tokens.get(0), tokens.get(tokens.size() - 1), message);
}

private List<StatementList> getIfBranches(IfStatement ifStmt) {
List<StatementList> branches = new ArrayList<>();
branches.add(ifStmt.body());
Expand All @@ -124,31 +125,6 @@ private List<StatementList> getIfBranches(IfStatement ifStmt) {
return branches;
}

private void addConditionalExpressionBranches(List<Expression> 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<StatementList> branches, ElseClause elseBranch) {
IfStatement singleIfChild = singleIfChild(elseBranch.body());
if (singleIfChild != null) {
Expand Down
50 changes: 19 additions & 31 deletions python-checks/src/test/resources/checks/sameBranch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -50,13 +60,22 @@
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:
print("2")
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")
Expand All @@ -74,37 +93,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")
Expand Down