Skip to content

Commit 9967790

Browse files
pynicolaspynicolas
authored
SONARPY-212 Rule S3801: Functions should use "return" consistently (#1097)
1 parent fca007c commit 9967790

File tree

9 files changed

+1606
-1
lines changed

9 files changed

+1606
-1
lines changed

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

Lines changed: 1196 additions & 0 deletions
Large diffs are not rendered by default.

‎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
@@ -101,6 +101,7 @@ public static Iterable<Class> getChecks() {
101101
ComparisonToNoneCheck.class,
102102
ConfusingTypeCheckingCheck.class,
103103
ConfusingWalrusCheck.class,
104+
ConsistentReturnCheck.class,
104105
ConstantConditionCheck.class,
105106
CorsCheck.class,
106107
CsrfDisabledCheck.class,

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

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2022 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.List;
23+
import java.util.stream.Collectors;
24+
import org.sonar.check.Rule;
25+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
26+
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.cfg.CfgBranchingBlock;
28+
import org.sonar.plugins.python.api.cfg.ControlFlowGraph;
29+
import org.sonar.plugins.python.api.tree.FunctionDef;
30+
import org.sonar.plugins.python.api.tree.ReturnStatement;
31+
import org.sonar.plugins.python.api.tree.Statement;
32+
import org.sonar.plugins.python.api.tree.Tree;
33+
import org.sonar.plugins.python.api.tree.Tree.Kind;
34+
import org.sonar.plugins.python.api.tree.WhileStatement;
35+
36+
@Rule(key = "S3801")
37+
public class ConsistentReturnCheck extends PythonSubscriptionCheck {
38+
39+
public static final String MESSAGE = "Refactor this function to use \"return\" consistently.";
40+
41+
@Override
42+
public void initialize(Context context) {
43+
context.registerSyntaxNodeConsumer(Kind.FUNCDEF, ctx -> {
44+
FunctionDef functionDef = (FunctionDef) ctx.syntaxNode();
45+
ControlFlowGraph cfg = ControlFlowGraph.build(functionDef, ctx.pythonFile());
46+
if (cfg == null || hasExceptOrFinally(cfg)) {
47+
return;
48+
}
49+
List<Tree> endStatements = cfg.end().predecessors().stream()
50+
.map(block -> parentStatement(block.elements().get(block.elements().size() - 1)))
51+
.filter(s -> !s.is(Kind.RAISE_STMT, Kind.ASSERT_STMT, Kind.WITH_STMT) && !isWhileTrue(s))
52+
.collect(Collectors.toList());
53+
54+
List<Tree> returnsWithValue = endStatements.stream()
55+
.filter(s -> s.is(Kind.RETURN_STMT) && hasValue((ReturnStatement) s))
56+
.collect(Collectors.toList());
57+
58+
if (returnsWithValue.size() != endStatements.size() && !returnsWithValue.isEmpty()) {
59+
addIssue(ctx, functionDef, endStatements);
60+
}
61+
});
62+
}
63+
64+
private static boolean hasExceptOrFinally(ControlFlowGraph cfg) {
65+
return cfg.blocks().stream().anyMatch(block ->
66+
block instanceof CfgBranchingBlock && ((CfgBranchingBlock) block).branchingTree().is(Kind.EXCEPT_CLAUSE, Kind.FINALLY_CLAUSE));
67+
}
68+
69+
private static boolean isWhileTrue(Statement statement) {
70+
return statement.is(Kind.WHILE_STMT) && Expressions.isTruthy(((WhileStatement) statement).condition());
71+
}
72+
73+
private static void addIssue(SubscriptionContext ctx, FunctionDef functionDef, List<Tree> endStatements) {
74+
PreciseIssue issue = ctx.addIssue(functionDef.name(), MESSAGE);
75+
for (Tree statement : endStatements) {
76+
if (statement.is(Kind.RETURN_STMT)) {
77+
ReturnStatement returnStatement = (ReturnStatement) statement;
78+
boolean hasValue = hasValue(returnStatement);
79+
issue.secondary(statement, String.format("Return %s value", hasValue ? "with" : "without"));
80+
} else if (statement.is(Kind.IF_STMT, Kind.FOR_STMT, Kind.WHILE_STMT)) {
81+
issue.secondary(statement.firstToken(), "Implicit return without value if the condition is false");
82+
} else if (statement.is(Kind.MATCH_STMT)) {
83+
issue.secondary(statement.firstToken(), "Implicit return without value when no case matches");
84+
} else if (statement.is(Kind.FUNCDEF, Kind.CLASSDEF)) {
85+
issue.secondary(statement.firstToken(), "Implicit return without value");
86+
} else {
87+
issue.secondary(statement, "Implicit return without value");
88+
}
89+
}
90+
}
91+
92+
private static boolean hasValue(ReturnStatement returnStatement) {
93+
return !returnStatement.expressions().isEmpty();
94+
}
95+
96+
private static Statement parentStatement(Tree tree) {
97+
while (!(tree instanceof Statement)) {
98+
tree = tree.parent();
99+
}
100+
return (Statement) tree;
101+
}
102+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,26 @@ public static boolean isFalsy(@Nullable Expression expression) {
7474
}
7575
}
7676

77+
// https://docs.python.org/3/library/stdtypes.html#truth-value-testing
78+
public static boolean isTruthy(@Nullable Expression expression) {
79+
if (expression == null) {
80+
return false;
81+
}
82+
switch (expression.getKind()) {
83+
case NAME:
84+
return "True".equals(((Name) expression).name());
85+
case STRING_LITERAL:
86+
case NUMERIC_LITERAL:
87+
case LIST_LITERAL:
88+
case TUPLE:
89+
case SET_LITERAL:
90+
case DICTIONARY_LITERAL:
91+
return !isFalsy(expression);
92+
default:
93+
return false;
94+
}
95+
}
96+
7797
public static Expression singleAssignedValue(Name name) {
7898
Symbol symbol = name.symbol();
7999
if (symbol == null) {

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<p>Because it is dynamically typed, Python does not enforce a return type on a function. This means that different paths through a function can return
2+
different types of values, which can be very confusing to the user and significantly harder to maintain.</p>
3+
<p>In particular, it is consequently also possible to mix empty <code>return</code> statements (implicitly returning <code>None</code>) with some
4+
returning an expression. This rule verifies that all the <code>return</code> statements from a function are consistent.</p>
5+
<h2>Noncompliant Code Example</h2>
6+
<pre>
7+
def foo(a): # Noncompliant, function will return "true" or None
8+
if a == 1:
9+
return True
10+
return
11+
</pre>
12+
<h2>Compliant Solution</h2>
13+
<pre>
14+
def foo(a):
15+
if a == 1:
16+
return True
17+
return False
18+
</pre>
19+
<p>or</p>
20+
<pre>
21+
def foo(a):
22+
if a == 1:
23+
return True
24+
return None
25+
</pre>
26+

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
{
2+
"title": "Functions should use \"return\" consistently",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "20min"
8+
},
9+
"tags": [
10+
"api-design",
11+
"confusing"
12+
],
13+
"defaultSeverity": "Major",
14+
"ruleSpecification": "RSPEC-3801",
15+
"sqKey": "S3801",
16+
"scope": "Main",
17+
"quickfix": "unknown"
18+
}

‎python-checks/src/test/java/org/sonar/python/checks/ConsistentReturnCheckTest.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-2022 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 org.junit.Test;
23+
import org.sonar.python.checks.utils.PythonCheckVerifier;
24+
25+
public class ConsistentReturnCheckTest {
26+
27+
@Test
28+
public void test() {
29+
PythonCheckVerifier.verify("src/test/resources/checks/consistentReturn.py", new ConsistentReturnCheck());
30+
}
31+
32+
}

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

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636

3737
import static org.assertj.core.api.Assertions.assertThat;
3838
import static org.sonar.python.checks.Expressions.isFalsy;
39+
import static org.sonar.python.checks.Expressions.isTruthy;
3940
import static org.sonar.python.checks.Expressions.removeParentheses;
4041
import static org.sonar.python.checks.Expressions.unescape;
4142
import static org.sonar.python.checks.Expressions.unescapeString;
@@ -70,7 +71,7 @@ public void falsy() {
7071

7172
assertThat(isFalsy(exp("[x]"))).isFalse();
7273
assertThat(isFalsy(exp("[]"))).isTrue();
73-
assertThat(isFalsy(exp("(x)"))).isFalse();
74+
assertThat(isFalsy(exp("(x,)"))).isFalse();
7475
assertThat(isFalsy(exp("()"))).isTrue();
7576
assertThat(isFalsy(exp("{x:y}"))).isFalse();
7677
assertThat(isFalsy(exp("{x}"))).isFalse();
@@ -79,6 +80,31 @@ public void falsy() {
7980
assertThat(isFalsy(exp("x.y"))).isFalse();
8081
}
8182

83+
@Test
84+
public void truthy() {
85+
assertThat(isTruthy(null)).isFalse();
86+
87+
assertThat(isTruthy(exp("True"))).isTrue();
88+
assertThat(isTruthy(exp("False"))).isFalse();
89+
assertThat(isTruthy(exp("x"))).isFalse();
90+
assertThat(isTruthy(exp("None"))).isFalse();
91+
92+
assertThat(isTruthy(exp("0"))).isFalse();
93+
assertThat(isTruthy(exp("1"))).isTrue();
94+
assertThat(isTruthy(exp("42"))).isTrue();
95+
96+
assertThat(isTruthy(exp("''"))).isFalse();
97+
assertThat(isTruthy(exp("'0'"))).isTrue();
98+
99+
assertThat(isTruthy(exp("[x]"))).isTrue();
100+
assertThat(isTruthy(exp("[]"))).isFalse();
101+
assertThat(isTruthy(exp("(x,)"))).isTrue();
102+
assertThat(isTruthy(exp("()"))).isFalse();
103+
assertThat(isTruthy(exp("{x:y}"))).isTrue();
104+
assertThat(isTruthy(exp("{x}"))).isTrue();
105+
assertThat(isTruthy(exp("{}"))).isFalse();
106+
}
107+
82108
@Test
83109
public void remove_parentheses() {
84110
assertThat(removeParentheses(exp("42")).getKind()).isEqualTo(Kind.NUMERIC_LITERAL);

0 commit comments

Comments
 (0)