Skip to content

Commit 7471867

Browse files
mpaladinivandalbosco
mpaladin
authored and
ivandalbosco
committed
SONARPY-141 Rule: Identical expressions should not be used on both sides of a binary operator (SonarSource#62)
* SONARPY-141 Rule: Identical expressions should not be used on both sides of a binary operator * correct message, add test
1 parent 63e209f commit 7471867

8 files changed

Lines changed: 304 additions & 1 deletion

File tree

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

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
{
2+
'project:buildbot-slave-0.8.6p1/buildslave/commands/svn.py':[
3+
184,
4+
],
5+
'project:django-1.4/django/contrib/gis/gdal/tests/test_geom.py':[
6+
36,
7+
],
8+
'project:django-1.4/django/contrib/gis/tests/test_measure.py':[
9+
155,
10+
313,
11+
],
12+
'project:django-1.4/django/forms/fields.py':[
13+
304,
14+
],
15+
'project:django-1.4/django/template/defaultfilters.py':[
16+
103,
17+
],
18+
'project:django-1.4/django/utils/simplejson/encoder.py':[
19+
224,
20+
],
21+
'project:django-1.4/tests/modeltests/many_to_one/tests.py':[
22+
412,
23+
],
24+
'project:django-1.4/tests/regressiontests/defaultfilters/tests.py':[
25+
55,
26+
],
27+
'project:django-1.4/tests/regressiontests/m2m_regress/tests.py':[
28+
86,
29+
87,
30+
],
31+
'project:django-1.4/tests/regressiontests/queries/tests.py':[
32+
187,
33+
],
34+
'project:tornado-2.3/demos/appengine/markdown.py':[
35+
375,
36+
],
37+
'project:tornado-2.3/demos/blog/markdown.py':[
38+
375,
39+
],
40+
'project:tornado-2.3/tornado/test/simple_httpclient_test.py':[
41+
120,
42+
],
43+
'project:twisted-12.1.0/twisted/internet/test/test_address.py':[
44+
27,
45+
28,
46+
],
47+
'project:twisted-12.1.0/twisted/internet/test/test_base.py':[
48+
211,
49+
212,
50+
224,
51+
225,
52+
236,
53+
237,
54+
249,
55+
250,
56+
260,
57+
261,
58+
271,
59+
272,
60+
],
61+
'project:twisted-12.1.0/twisted/manhole/explorer.py':[
62+
26,
63+
],
64+
'project:twisted-12.1.0/twisted/names/test/test_dns.py':[
65+
940,
66+
943,
67+
],
68+
'project:twisted-12.1.0/twisted/python/test/test_constants.py':[
69+
54,
70+
55,
71+
],
72+
'project:twisted-12.1.0/twisted/python/test/test_util.py':[
73+
572,
74+
573,
75+
575,
76+
576,
77+
585,
78+
596,
79+
],
80+
'project:twisted-12.1.0/twisted/python/test/test_versions.py':[
81+
75,
82+
84,
83+
],
84+
'project:twisted-12.1.0/twisted/test/test_internet.py':[
85+
670,
86+
],
87+
'project:twisted-12.1.0/twisted/test/test_paths.py':[
88+
974,
89+
976,
90+
987,
91+
],
92+
'project:twisted-12.1.0/twisted/trial/test/test_testcase.py':[
93+
37,
94+
],
95+
}

‎its/ruling/src/test/resources/profile.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,5 +265,10 @@
265265
<key>S2772</key>
266266
<priority>INFO</priority>
267267
</rule>
268+
<rule>
269+
<repositoryKey>python</repositoryKey>
270+
<key>S1764</key>
271+
<priority>INFO</priority>
272+
</rule>
268273
</rules>
269274
</profile>

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ public static Iterable<Class> getChecks() {
8181
TooManyReturnsCheck.class,
8282
NeedlessPassCheck.class,
8383
AfterJumpStatementCheck.class,
84+
IdenticalExpressionOnBinaryOperatorCheck.class,
8485
MethodShouldBeStaticCheck.class
8586
);
8687
}

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

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2016 SonarSource SA
4+
* mailto:contact AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks;
21+
22+
import com.google.common.collect.ImmutableList;
23+
import com.sonar.sslr.api.AstNode;
24+
import org.sonar.check.Priority;
25+
import org.sonar.check.Rule;
26+
import org.sonar.python.PythonCheck;
27+
import org.sonar.python.api.PythonGrammar;
28+
import org.sonar.squidbridge.annotations.ActivatedByDefault;
29+
import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
30+
31+
import java.util.List;
32+
33+
@Rule(
34+
key = "S1764",
35+
name = "Identical expressions should not be used on both sides of a binary operator",
36+
priority = Priority.MAJOR,
37+
tags = {Tags.BUG, Tags.CERT})
38+
@SqaleConstantRemediation("2min")
39+
@ActivatedByDefault
40+
public class IdenticalExpressionOnBinaryOperatorCheck extends PythonCheck {
41+
42+
private static final List<String> EXCLUDED_OPERATOR_TYPES = ImmutableList.of(
43+
"*",
44+
"+"
45+
);
46+
47+
@Override
48+
public void init() {
49+
subscribeTo(
50+
PythonGrammar.M_EXPR,
51+
PythonGrammar.A_EXPR,
52+
PythonGrammar.SHIFT_EXPR,
53+
PythonGrammar.AND_EXPR,
54+
PythonGrammar.XOR_EXPR,
55+
PythonGrammar.OR_EXPR,
56+
PythonGrammar.COMPARISON,
57+
PythonGrammar.OR_TEST,
58+
PythonGrammar.AND_TEST
59+
);
60+
}
61+
62+
@Override
63+
public void visitNode(AstNode expression) {
64+
List<AstNode> children = expression.getChildren();
65+
AstNode leftOperand = children.get(0);
66+
String operator = children.get(1).getTokenValue();
67+
AstNode rightOperand = children.get(2);
68+
if (!EXCLUDED_OPERATOR_TYPES.contains(operator) && CheckUtils.equalNodes(leftOperand, rightOperand) && !isLeftShiftBy1(leftOperand, operator)) {
69+
addIssue(rightOperand, "Correct one of the identical sub-expressions on both sides of operator \"" + operator + "\".")
70+
.secondary(leftOperand, "");
71+
}
72+
}
73+
74+
private static boolean isLeftShiftBy1(AstNode leftOperand, String operator) {
75+
return "<<".equals(operator) && "1".equals(leftOperand.getTokenValue());
76+
}
77+
}

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<p>
2+
Using the same value on either side of a binary operator is almost always a mistake. In the case of logical operators, it is either a copy/paste
3+
error and therefore a bug, or it is simply wasted code, and should be simplified. In the case of bitwise operators and most binary mathematical
4+
operators, having the same value on both sides of an operator yields predictable results, and should be simplified.
5+
</p>
6+
7+
<p>This rule ignores <code>*</code>, <code>+</code>, and <code>=</code>.</p>
8+
9+
<h2>Noncompliant Code Example</h2>
10+
<pre>
11+
if a == a: # Noncompliant
12+
work()
13+
14+
if a != a: # Noncompliant
15+
work()
16+
17+
if a == b and a == b: # Noncompliant
18+
work()
19+
20+
if a == b or a == b: # Noncompliant
21+
work()
22+
23+
j = 5 / 5 # Noncompliant
24+
k = 5 - 5 # Noncompliant
25+
</pre>
26+
27+
<h2>Compliant Solution</h2>
28+
<pre>
29+
work()
30+
31+
if a == b:
32+
work()
33+
34+
if a == b:
35+
work()
36+
37+
j = 1
38+
k = 0
39+
</pre>
40+
41+
<h2>Exceptions</h2>
42+
<p>The following are ignored:</p>
43+
<ul>
44+
<li> The expression <code>1 &lt;&lt; 1</code> </li>
45+
</ul>
46+
<h2>See</h2>
47+
<ul>
48+
<li> <a href="https://www.securecoding.cert.org/confluence/x/NYA5">CERT, MSC12-C.</a> - Detect and remove code that has no effect or is never
49+
executed </li>
50+
<li> <a href="https://www.securecoding.cert.org/confluence/x/SIIyAQ">CERT, MSC12-CPP.</a> - Detect and remove code that has no effect </li>
51+
</ul>

‎python-checks/src/test/java/org/sonar/python/checks/IdenticalExpressionOnBinaryOperatorCheckTest.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2016 SonarSource SA
4+
* mailto:contact AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks;
21+
22+
import org.junit.Test;
23+
import org.sonar.python.checks.utils.PythonCheckVerifier;
24+
25+
public class IdenticalExpressionOnBinaryOperatorCheckTest {
26+
27+
@Test
28+
public void test() {
29+
PythonCheckVerifier.verify("src/test/resources/checks/identicalExpressionOnBinaryOperator.py", new IdenticalExpressionOnBinaryOperatorCheck());
30+
}
31+
32+
}

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
a = 1
2+
b = 1
3+
4+
def work():
5+
pass
6+
7+
if a == a: # Noncompliant [[secondary=+0;sc=9;ec=10]] {{Correct one of the identical sub-expressions on both sides of operator "==".}}
8+
# ^
9+
work()
10+
11+
if a != \
12+
a: # Noncompliant [[secondary=-1;sc=9;ec=10]]
13+
work()
14+
15+
if a == b and a == b: # Noncompliant
16+
work()
17+
18+
if a == b or a == b: # Noncompliant
19+
# ^^^^^^
20+
work()
21+
22+
j = 5 / 5 # Noncompliant
23+
k = 5 - 5 # Noncompliant
24+
l = 5 + 5
25+
m = 5 * 5
26+
n = 3 << 3 # Noncompliant
27+
o = 3 & 3 # Noncompliant
28+
p = 3 ^ 3 # Noncompliant
29+
q = 3 | 3 # Noncompliant
30+
r = 3 and 3 # Noncompliant
31+
s = 3 or 3 # Noncompliant
32+
c1 = 3 < 3 # Noncompliant
33+
c2 = 3 <= 3 # Noncompliant
34+
c3 = 3 > 3 # Noncompliant
35+
c4 = 3 >= 3 # Noncompliant
36+
c5 = 3 <> 3 # Noncompliant
37+
c6 = 3 is 3 # Noncompliant
38+
c7 = 3 is not 3 # Noncompliant
39+
c8 = 3 not in 3 # Noncompliant
40+
c9 = 3 in 3 # Noncompliant
41+
exclusion = 1 << 1
42+
exclusion2 = (a * b) << 1

‎sonar-python-plugin/src/test/java/org/sonar/plugins/python/PythonRuleRepositoryTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public void createRulesTest() {
4242

4343
List<RulesDefinition.Rule> rules = repository.rules();
4444
assertThat(rules).isNotNull();
45-
assertThat(rules).hasSize(50);
45+
assertThat(rules).hasSize(51);
4646
}
4747

4848
}

0 commit comments

Comments
 (0)