Skip to content

Commit 777a966

Browse files
guillaume-dequenneguillaume-dequenne
authored
SONARPY-667 Rule S905: Non-empty statements sould change control flow… (SonarSource#705)
1 parent 84e1d1c commit 777a966

9 files changed

Lines changed: 530 additions & 0 deletions

File tree

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

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
{
2+
'project:docker-compose-1.24.1/tests/unit/config/interpolation_test.py':[
3+
452,
4+
453,
5+
],
6+
'project:docker-compose-1.24.1/tests/unit/service_test.py':[
7+
524,
8+
551,
9+
],
10+
'project:numpy-1.16.4/benchmarks/benchmarks/bench_core.py':[
11+
87,
12+
90,
13+
93,
14+
96,
15+
],
16+
'project:numpy-1.16.4/benchmarks/benchmarks/bench_ufunc.py':[
17+
36,
18+
95,
19+
102,
20+
109,
21+
116,
22+
],
23+
'project:numpy-1.16.4/numpy/core/arrayprint.py':[
24+
696,
25+
],
26+
'project:numpy-1.16.4/numpy/core/tests/test_datetime.py':[
27+
881,
28+
1162,
29+
1809,
30+
1829,
31+
],
32+
'project:numpy-1.16.4/numpy/core/tests/test_defchararray.py':[
33+
631,
34+
643,
35+
667,
36+
],
37+
'project:numpy-1.16.4/numpy/core/tests/test_errstate.py':[
38+
27,
39+
30,
40+
],
41+
'project:numpy-1.16.4/numpy/core/tests/test_half.py':[
42+
497,
43+
498,
44+
499,
45+
503,
46+
504,
47+
505,
48+
506,
49+
],
50+
'project:numpy-1.16.4/numpy/core/tests/test_indexing.py':[
51+
1243,
52+
1244,
53+
],
54+
'project:numpy-1.16.4/numpy/core/tests/test_multiarray.py':[
55+
3545,
56+
3547,
57+
3549,
58+
3551,
59+
3553,
60+
3595,
61+
3611,
62+
3686,
63+
],
64+
'project:numpy-1.16.4/numpy/core/tests/test_numeric.py':[
65+
475,
66+
478,
67+
614,
68+
617,
69+
620,
70+
],
71+
'project:numpy-1.16.4/numpy/core/tests/test_regression.py':[
72+
315,
73+
416,
74+
2279,
75+
2285,
76+
],
77+
'project:numpy-1.16.4/numpy/core/tests/test_ufunc.py':[
78+
1589,
79+
],
80+
'project:numpy-1.16.4/numpy/lib/tests/test_mixins.py':[
81+
153,
82+
155,
83+
171,
84+
173,
85+
],
86+
'project:numpy-1.16.4/numpy/ma/tests/test_core.py':[
87+
2041,
88+
2360,
89+
],
90+
'project:numpy-1.16.4/numpy/ma/tests/test_regression.py':[
91+
26,
92+
33,
93+
34,
94+
],
95+
'project:numpy-1.16.4/numpy/matrixlib/tests/test_defmatrix.py':[
96+
270,
97+
274,
98+
],
99+
'project:numpy-1.16.4/numpy/matrixlib/tests/test_regression.py':[
100+
25,
101+
],
102+
'project:tornado-2.3/tornado/test/gen_test.py':[
103+
51,
104+
59,
105+
64,
106+
],
107+
'project:tornado-2.3/tornado/test/web_test.py':[
108+
589,
109+
596,
110+
610,
111+
621,
112+
],
113+
'project:twisted-12.1.0/twisted/conch/test/test_knownhosts.py':[
114+
797,
115+
],
116+
'project:twisted-12.1.0/twisted/internet/iocpreactor/abstract.py':[
117+
362,
118+
],
119+
'project:twisted-12.1.0/twisted/python/test/test_release.py':[
120+
479,
121+
],
122+
'project:twisted-12.1.0/twisted/test/reflect_helper_ZDE.py':[
123+
4,
124+
],
125+
'project:twisted-12.1.0/twisted/test/test_defgen.py':[
126+
118,
127+
],
128+
'project:twisted-12.1.0/twisted/test/test_log.py':[
129+
86,
130+
261,
131+
267,
132+
],
133+
'project:twisted-12.1.0/twisted/trial/test/erroneous.py':[
134+
63,
135+
],
136+
'project:twisted-12.1.0/twisted/trial/test/test_pyunitcompat.py':[
137+
147,
138+
],
139+
}

‎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
@@ -188,6 +188,7 @@ public static Iterable<Class> getChecks() {
188188
UnverifiedHostnameCheck.class,
189189
UselessParenthesisAfterKeywordCheck.class,
190190
UselessParenthesisCheck.class,
191+
UselessStatementCheck.class,
191192
VerifiedSslTlsCertificateCheck.class,
192193
WeakSSLProtocolCheck.class,
193194
WrongAssignmentOperatorCheck.class

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

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2020 SonarSource SA
4+
* mailto:info 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 java.util.Arrays;
23+
import java.util.List;
24+
import java.util.Optional;
25+
import java.util.stream.Collectors;
26+
import java.util.stream.Stream;
27+
import org.sonar.check.Rule;
28+
import org.sonar.check.RuleProperty;
29+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
30+
import org.sonar.plugins.python.api.SubscriptionContext;
31+
import org.sonar.plugins.python.api.symbols.ClassSymbol;
32+
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
33+
import org.sonar.plugins.python.api.symbols.Symbol;
34+
import org.sonar.plugins.python.api.tree.BinaryExpression;
35+
import org.sonar.plugins.python.api.tree.ClassDef;
36+
import org.sonar.plugins.python.api.tree.FileInput;
37+
import org.sonar.plugins.python.api.tree.FunctionDef;
38+
import org.sonar.plugins.python.api.tree.Name;
39+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
40+
import org.sonar.plugins.python.api.tree.StringLiteral;
41+
import org.sonar.plugins.python.api.tree.Token;
42+
import org.sonar.plugins.python.api.tree.Tree;
43+
import org.sonar.plugins.python.api.tree.Tree.Kind;
44+
import org.sonar.plugins.python.api.tree.UnaryExpression;
45+
import org.sonar.python.tree.TreeUtils;
46+
47+
@Rule(key = "S905")
48+
public class UselessStatementCheck extends PythonSubscriptionCheck {
49+
50+
private static final boolean DEFAULT_REPORT_ON_STRINGS = false;
51+
52+
@RuleProperty(
53+
key = "reportOnStrings",
54+
description = "Enable issues on unread attributes with a single underscore prefix",
55+
defaultValue = "" + DEFAULT_REPORT_ON_STRINGS)
56+
public boolean reportOnStrings = DEFAULT_REPORT_ON_STRINGS;
57+
58+
@RuleProperty(
59+
key = "ignoredOperators",
60+
description = "Comma separated list of ignored operators",
61+
defaultValue = "")
62+
public String ignoredOperators = "";
63+
64+
List<String> ignoredOperatorsList;
65+
66+
private List<String> ignoredOperators() {
67+
if (ignoredOperatorsList == null) {
68+
ignoredOperatorsList = Stream.of(ignoredOperators.split(","))
69+
.map(String::trim).collect(Collectors.toList());
70+
}
71+
return ignoredOperatorsList;
72+
}
73+
74+
private static final List<Kind> regularKinds = Arrays.asList(Kind.NUMERIC_LITERAL, Kind.LIST_LITERAL, Kind.SET_LITERAL, Kind.DICTIONARY_LITERAL,
75+
Kind.NONE, Kind.CONDITIONAL_EXPR, Kind.LAMBDA);
76+
77+
private static final List<Kind> binaryExpressionKinds = Arrays.asList(Kind.AND, Kind.OR, Kind.PLUS, Kind.MINUS,
78+
Kind.MULTIPLICATION, Kind.DIVISION, Kind.FLOOR_DIVISION, Kind.MODULO, Kind.MATRIX_MULTIPLICATION, Kind.SHIFT_EXPR,
79+
Kind.BITWISE_AND, Kind.BITWISE_OR, Kind.BITWISE_XOR, Kind.COMPARISON, Kind.POWER);
80+
81+
private static final List<Kind> unaryExpressionKinds = Arrays.asList(Kind.UNARY_PLUS, Kind.UNARY_MINUS, Kind.BITWISE_COMPLEMENT, Kind.NOT);
82+
83+
private static final String MESSAGE = "Remove or refactor this statement; it has no side effects.";
84+
85+
@Override
86+
public void initialize(Context context) {
87+
context.registerSyntaxNodeConsumer(Kind.STRING_LITERAL, this::checkStringLiteral);
88+
context.registerSyntaxNodeConsumer(Kind.NAME, UselessStatementCheck::checkName);
89+
context.registerSyntaxNodeConsumer(Kind.QUALIFIED_EXPR, UselessStatementCheck::checkQualifiedExpression);
90+
binaryExpressionKinds.forEach(b -> context.registerSyntaxNodeConsumer(b, this::checkBinaryExpression));
91+
unaryExpressionKinds.forEach(u -> context.registerSyntaxNodeConsumer(u, this::checkUnaryExpression));
92+
regularKinds.forEach(r -> context.registerSyntaxNodeConsumer(r, UselessStatementCheck::checkNode));
93+
}
94+
95+
private static void checkNode(SubscriptionContext ctx) {
96+
Tree tree = ctx.syntaxNode();
97+
Tree tryParent = TreeUtils.firstAncestorOfKind(tree, Kind.TRY_STMT);
98+
if (tryParent != null) {
99+
return;
100+
}
101+
if (isBooleanExpressionWithCalls(tree)) {
102+
return;
103+
}
104+
Tree parent = tree.parent();
105+
if (parent == null || !parent.is(Kind.EXPRESSION_STMT)) {
106+
return;
107+
}
108+
ctx.addIssue(tree, MESSAGE);
109+
}
110+
111+
private static boolean isBooleanExpressionWithCalls(Tree tree) {
112+
return (tree.is(Kind.AND) || tree.is(Kind.OR) || tree.is(Kind.NOT)) && (TreeUtils.hasDescendant(tree, t -> t.is(Kind.CALL_EXPR)));
113+
}
114+
115+
private void checkStringLiteral(SubscriptionContext ctx) {
116+
StringLiteral stringLiteral = (StringLiteral) ctx.syntaxNode();
117+
if (!reportOnStrings || isDocString(stringLiteral)) {
118+
return;
119+
}
120+
checkNode(ctx);
121+
}
122+
123+
private static void checkName(SubscriptionContext ctx) {
124+
Name name = (Name) ctx.syntaxNode();
125+
Symbol symbol = name.symbol();
126+
if (symbol != null && symbol.is(Symbol.Kind.CLASS)) {
127+
ClassSymbol classSymbol = (ClassSymbol) symbol;
128+
// Creating an exception without raising it is covered by S3984
129+
if (classSymbol.canBeOrExtend("BaseException")) {
130+
return;
131+
}
132+
}
133+
checkNode(ctx);
134+
}
135+
136+
private static void checkQualifiedExpression(SubscriptionContext ctx) {
137+
QualifiedExpression qualifiedExpression = (QualifiedExpression) ctx.syntaxNode();
138+
Symbol symbol = qualifiedExpression.symbol();
139+
if (symbol != null && symbol.is(Symbol.Kind.FUNCTION) && ((FunctionSymbol) symbol).decorators().stream().noneMatch(d -> d.matches("property"))) {
140+
checkNode(ctx);
141+
}
142+
}
143+
144+
private void checkBinaryExpression(SubscriptionContext ctx) {
145+
BinaryExpression binaryExpression = (BinaryExpression) ctx.syntaxNode();
146+
Token operator = binaryExpression.operator();
147+
if (ignoredOperators().contains(operator.value())) {
148+
return;
149+
}
150+
checkNode(ctx);
151+
}
152+
153+
private void checkUnaryExpression(SubscriptionContext ctx) {
154+
UnaryExpression unaryExpression = (UnaryExpression) ctx.syntaxNode();
155+
Token operator = unaryExpression.operator();
156+
if (ignoredOperators().contains(operator.value())) {
157+
return;
158+
}
159+
checkNode(ctx);
160+
}
161+
162+
private static boolean isDocString(StringLiteral stringLiteral) {
163+
Tree parent = TreeUtils.firstAncestorOfKind(stringLiteral, Kind.FILE_INPUT, Kind.CLASSDEF, Kind.FUNCDEF);
164+
return Optional.ofNullable(parent)
165+
.map(p -> ((p.is(Kind.FILE_INPUT) && stringLiteral.equals(((FileInput) p).docstring()))
166+
|| (p.is(Kind.CLASSDEF) && stringLiteral.equals(((ClassDef) p).docstring()))
167+
|| (p.is(Kind.FUNCDEF) && stringLiteral.equals(((FunctionDef) p).docstring()))))
168+
.orElse(false);
169+
}
170+
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<p>Any statement, other than a <code>pass</code>, <code>...</code> (ellipsis) or an empty statement (i.e. a single semicolon "<code>;</code>"), which
2+
has no side effect and does not result in a change of control flow will normally indicate a programming error, and therefore should be refactored.</p>
3+
<h2>Noncompliant Code Example</h2>
4+
<pre>
5+
a == 1 # Noncompliant; was assignment intended?
6+
a &lt; b # Noncompliant; have we forgotten to assign the result to a variable?
7+
</pre>
8+
<h2>Exceptions</h2>
9+
<p><strong>Strings</strong></p>
10+
<p>Some projects use string literals as comments. By default, this rule will not raise an issue on these strings. Reporting on string literals can be
11+
enabled by setting the rule parameter "reportOnStrings" to "true".</p>
12+
<pre>
13+
class MyClass:
14+
myattr = 42
15+
"""This is an attribute""" # Noncompliant by default. Set "reportOnStrings" to "false"
16+
</pre>
17+
<p><strong>Operators</strong></p>
18+
<p>By default, this rule considers that no arithmetic operator has a side effect. Some rare projects redefine operators and add a side effect. You can
19+
list such operators in the rule parameter "ignoredOperators".</p>
20+
<pre>
21+
def process(p, beam):
22+
"""
23+
Apache Beam redefines "|" and "&gt;&gt;" operators and they have a side effect.
24+
Thus for Apache Beam projects "ignoredOperators"should be set to "|,&gt;&gt;"
25+
"""
26+
p | "create" &gt;&gt; beam.Create() # Noncompliant by default
27+
</pre>
28+
<h2>See</h2>
29+
<ul>
30+
<li> <a href="http://cwe.mitre.org/data/definitions/482">MITRE, CWE-482</a> - Comparing instead of Assigning </li>
31+
</ul>
32+

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S905.json‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
{
2+
"title": "Non-empty statements should change control flow or have at least one side-effect",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "10min"
8+
},
9+
"tags": [
10+
"cwe",
11+
"unused"
12+
],
13+
"defaultSeverity": "Major",
14+
"ruleSpecification": "RSPEC-905",
15+
"sqKey": "S905",
16+
"scope": "Main",
17+
"securityStandards": {
18+
"CWE": [
19+
482
20+
]
21+
}
22+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
"S116",
1515
"S117",
1616
"S125",
17+
"S905",
1718
"S930",
1819
"S1045",
1920
"S1066",

0 commit comments

Comments
 (0)