Skip to content

Commit 7b5dfa0

Browse files
alban-auzeillchristophe-zurn-sonarsource
alban-auzeill
authored and
christophe-zurn-sonarsource
committed
SONARPY-256 Update S1871 to cover single line of code exception (SonarSource#174)
1 parent acf0c4f commit 7b5dfa0

4 files changed

Lines changed: 60 additions & 110 deletions

File tree

‎its/ruling/src/test/resources/expected/python-S1871.json‎

Lines changed: 0 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -1,116 +1,35 @@
11
{
2-
'project:buildbot-0.8.6p1/buildbot/sourcestamp.py':[
3-
183,
4-
],
52
'project:buildbot-0.8.6p1/buildbot/steps/python_twisted.py':[
63
522,
74
],
85
'project:buildbot-0.8.6p1/buildbot/test/fake/fakedb.py':[
96
664,
107
],
11-
'project:buildbot-0.8.6p1/contrib/bzr_buildbot.py':[
12-
229,
13-
],
148
'project:django-1.4/django/contrib/auth/tests/auth_backends.py':[
15-
120,
169
122,
1710
],
1811
'project:django-1.4/django/contrib/auth/views.py':[
1912
46,
2013
],
21-
'project:django-1.4/django/contrib/gis/geos/coordseq.py':[
22-
51,
23-
],
24-
'project:django-1.4/django/contrib/gis/utils/wkt.py':[
25-
46,
26-
],
27-
'project:django-1.4/django/contrib/localflavor/mk/forms.py':[
28-
86,
29-
],
30-
'project:django-1.4/django/db/backends/__init__.py':[
31-
845,
32-
],
33-
'project:django-1.4/django/db/backends/oracle/base.py':[
34-
190,
35-
],
36-
'project:django-1.4/django/db/models/query.py':[
37-
837,
38-
],
39-
'project:django-1.4/django/forms/models.py':[
40-
280,
41-
282,
42-
287,
43-
],
44-
'project:django-1.4/django/test/_doctest.py':[
45-
869,
46-
871,
47-
],
48-
'project:django-1.4/django/utils/archive.py':[
49-
115,
50-
],
51-
'project:django-1.4/django/utils/regex_helper.py':[
52-
183,
53-
],
54-
'project:django-1.4/django/views/generic/date_based.py':[
55-
160,
56-
285,
57-
],
5814
'project:tornado-2.3/demos/appengine/markdown.py':[
59-
514,
6015
765,
6116
],
6217
'project:tornado-2.3/demos/blog/markdown.py':[
63-
514,
6418
765,
6519
],
66-
'project:twisted-12.1.0/doc/core/examples/threadedselect/pygamedemo.py':[
67-
73,
68-
],
69-
'project:twisted-12.1.0/twisted/conch/insults/insults.py':[
70-
539,
71-
],
72-
'project:twisted-12.1.0/twisted/conch/telnet.py':[
73-
947,
74-
958,
75-
],
7620
'project:twisted-12.1.0/twisted/mail/test/pop3testserver.py':[
77-
152,
7821
295,
7922
],
80-
'project:twisted-12.1.0/twisted/news/nntp.py':[
81-
905,
82-
],
83-
'project:twisted-12.1.0/twisted/persisted/aot.py':[
84-
491,
85-
],
8623
'project:twisted-12.1.0/twisted/protocols/ftp.py':[
87-
1590,
8824
1592,
8925
],
9026
'project:twisted-12.1.0/twisted/spread/banana.py':[
9127
198,
9228
],
93-
'project:twisted-12.1.0/twisted/spread/ui/tkutil.py':[
94-
111,
95-
],
9629
'project:twisted-12.1.0/twisted/test/test_doc.py':[
9730
69,
9831
],
99-
'project:twisted-12.1.0/twisted/trial/runner.py':[
100-
626,
101-
],
102-
'project:twisted-12.1.0/twisted/web/client.py':[
103-
69,
104-
],
105-
'project:twisted-12.1.0/twisted/web/static.py':[
106-
419,
107-
],
10832
'project:twisted-12.1.0/twisted/web/sux.py':[
109-
333,
11033
556,
11134
],
112-
'project:twisted-12.1.0/twisted/words/protocols/oscar.py':[
113-
159,
114-
161,
115-
],
11635
}

‎python-checks/src/main/java/org/sonar/python/checks/SameBranchCheck.java‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import com.sonar.sslr.api.AstNode;
2323
import com.sonar.sslr.api.AstNodeType;
24+
import java.util.ArrayList;
2425
import java.util.Collections;
2526
import java.util.LinkedList;
2627
import java.util.List;
@@ -88,12 +89,20 @@ private void findSameBranches(List<AstNode> branches) {
8889

8990
private void checkBranch(List<AstNode> branches, int index) {
9091
AstNode duplicateBlock = branches.get(index);
92+
boolean isOnASingleLine = isOnASingleLine(duplicateBlock);
93+
List<AstNode> equivalentBlocks = new ArrayList<>();
9194
for (int j = 0; j < index; j++) {
9295
AstNode originalBlock = branches.get(j);
9396
if (CheckUtils.equalNodes(originalBlock, duplicateBlock)) {
94-
String message = String.format(MESSAGE, originalBlock.getToken().getLine() + 1);
95-
addIssue(location(duplicateBlock, message))
96-
.secondary(location(originalBlock, "Original"));
97+
equivalentBlocks.add(originalBlock);
98+
boolean isLastComparisonInBranches = j == branches.size() - 2;
99+
if (!isOnASingleLine || isLastComparisonInBranches) {
100+
String message = String.format(MESSAGE, originalBlock.getToken().getLine() + 1);
101+
PreciseIssue issue = addIssue(location(duplicateBlock, message));
102+
equivalentBlocks.forEach(original -> issue.secondary(location(original, "Original")));
103+
return;
104+
}
105+
} else if (isOnASingleLine) {
97106
return;
98107
}
99108
}
@@ -136,4 +145,12 @@ private static AstNode singleIfChild(AstNode suite) {
136145
}
137146
return null;
138147
}
148+
149+
public static boolean isOnASingleLine(AstNode parent) {
150+
List<AstNode> statements = parent.getChildren(PythonGrammar.STATEMENT);
151+
if (statements.isEmpty()) {
152+
return true;
153+
}
154+
return statements.get(0).getTokenLine() == statements.get(statements.size() - 1).getLastToken().getLine();
155+
}
139156
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S1871.html‎

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,38 +3,29 @@
33
<h2>Noncompliant Code Example</h2>
44
<pre>
55
if 0 &lt;= a &lt; 10:
6-
do_the_thing()
6+
do_first()
7+
do_second()
78
elif 10 &lt;= a &lt; 20:
89
do_the_other_thing()
910
elif 20 &lt;= a &lt; 50:
10-
do_the_thing() # Noncompliant; duplicates first condition
11-
else:
12-
do_the_rest()
13-
14-
b = 4 if a &gt; 12 else 4
11+
do_first() # Noncompliant; duplicates first condition
12+
do_second()
1513
</pre>
16-
<h2>Compliant Solution</h2>
14+
<h2>Exceptions</h2>
15+
<p>Blocks in an <code>if</code> chain that contain a single line of code are ignored.</p>
1716
<pre>
18-
if (0 &lt;= a &lt; 10) or (20 &lt;= a &lt; 50):
19-
do_the_thing()
17+
if 0 &lt;= a &lt; 10:
18+
do_first()
2019
elif 10 &lt;= a &lt; 20:
2120
do_the_other_thing()
22-
else:
23-
do_the_rest()
24-
25-
b = 4
21+
elif 20 &lt;= a &lt; 50:
22+
do_first() # no issue, usually this is done on purpose to increase the readability
2623
</pre>
27-
<p>or </p>
24+
<p>But this exception does not apply when all branches have the same single line of code.</p>
2825
<pre>
2926
if 0 &lt;= a &lt; 10:
30-
do_the_thing()
31-
elif 10 &lt;= a &lt; 20:
32-
do_the_other_thing()
27+
do_first()
3328
elif 20 &lt;= a &lt; 50:
34-
do_the_third_thing()
35-
else:
36-
do_the_rest()
37-
38-
b = 8 if a &gt; 12 else 4
29+
do_first() # Noncompliant, this might have been done on purpose but probably not
3930
</pre>
4031

‎python-checks/src/test/resources/checks/sameBranch.py‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,11 @@
1414

1515
if param == 1:
1616
print(1)
17+
foo()
1718
else:
1819
if param == 2:
19-
print(1) # Noncompliant [[secondary=-3]]
20-
20+
print(1) # Noncompliant [[secondary=-4]]
21+
foo()
2122

2223
if param == 1:
2324
if True:
@@ -33,9 +34,31 @@
3334
if True:
3435
print(1)
3536
else:
36-
print(1) # Noncompliant {{Either merge this branch with the identical one on line "34" or change one of the implementations.}}
37+
print(1) # Noncompliant {{Either merge this branch with the identical one on line "35" or change one of the implementations.}}
3738

3839
if 1: print("1"); foo()
39-
# Noncompliant@+1 [[secondary=-2;sc=9;ec=26;el=+0]]
4040
elif 2: print("1"); foo()
4141
else: print("2")
42+
43+
if 1:
44+
print("1")
45+
elif 2:
46+
print("2")
47+
else:
48+
print("1")
49+
50+
if 1:
51+
print("1")
52+
elif 2:
53+
print("1")
54+
else:
55+
print("1") # Noncompliant
56+
57+
if 1:
58+
print("1")
59+
foo()
60+
elif 2:
61+
print("1") # Noncompliant [[secondary=-3;sc=5;ec=10;el=+1]]
62+
foo()
63+
else:
64+
print("2")

0 commit comments

Comments
 (0)