Skip to content

Commit 12a57ca

Browse files
guillaume-dequennepynicolas
guillaume-dequenne
and
pynicolas
authored
SONARPY-574 Rule S5713: A subclass should not be in the same "except"… (SonarSource#614)
* SONARPY-574 Rule S5713: A subclass should not be in the same "except" clause as a parent class * SONARPY-574 Refactor S5713 and add ClassSymbol.isOrExtends(ClassSymbol) Co-authored-by: Pierre-Yves Nicolas <[email protected]>
1 parent 0551cc4 commit 12a57ca

11 files changed

Lines changed: 240 additions & 0 deletions

File tree

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
'project:django-2.2.3/django/template/defaultfilters.py':[
3+
816,
4+
],
5+
}

‎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
@@ -64,6 +64,7 @@ public static Iterable<Class> getChecks() {
6464
BooleanExpressionInExceptCheck.class,
6565
BreakContinueOutsideLoopCheck.class,
6666
CaughtExceptionsCheck.class,
67+
ChildAndParentExceptionCaughtCheck.class,
6768
ClassComplexityCheck.class,
6869
ClassNameCheck.class,
6970
ClearTextProtocolsCheck.class,

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

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
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.ArrayList;
23+
import java.util.HashMap;
24+
import java.util.List;
25+
import java.util.Map;
26+
import org.sonar.check.Rule;
27+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
28+
import org.sonar.plugins.python.api.SubscriptionContext;
29+
import org.sonar.plugins.python.api.symbols.ClassSymbol;
30+
import org.sonar.plugins.python.api.symbols.Symbol;
31+
import org.sonar.plugins.python.api.tree.ExceptClause;
32+
import org.sonar.plugins.python.api.tree.Expression;
33+
import org.sonar.plugins.python.api.tree.HasSymbol;
34+
import org.sonar.plugins.python.api.tree.Tree;
35+
import org.sonar.python.tree.TreeUtils;
36+
37+
@Rule(key = "S5713")
38+
public class ChildAndParentExceptionCaughtCheck extends PythonSubscriptionCheck {
39+
@Override
40+
public void initialize(Context context) {
41+
context.registerSyntaxNodeConsumer(Tree.Kind.EXCEPT_CLAUSE, ctx -> {
42+
ExceptClause exceptClause = (ExceptClause) ctx.syntaxNode();
43+
Map<ClassSymbol, List<Expression>> caughtExceptionsBySymbol = new HashMap<>();
44+
Expression exceptionExpression = exceptClause.exception();
45+
if (exceptionExpression == null) {
46+
return;
47+
}
48+
TreeUtils.flattenTuples(exceptionExpression).forEach(e -> addExceptionExpression(e, caughtExceptionsBySymbol));
49+
checkCaughtExceptions(ctx, caughtExceptionsBySymbol);
50+
});
51+
}
52+
53+
private static void checkCaughtExceptions(SubscriptionContext ctx, Map<ClassSymbol, List<Expression>> caughtExceptionsBySymbol) {
54+
caughtExceptionsBySymbol.forEach((currentSymbol, caughtExceptionsWithSameSymbol) -> {
55+
Expression currentException = caughtExceptionsWithSameSymbol.get(0);
56+
if (caughtExceptionsWithSameSymbol.size() > 1) {
57+
PreciseIssue issue = ctx.addIssue(currentException, "Remove this duplicate Exception class.");
58+
caughtExceptionsWithSameSymbol.stream().skip(1).forEach(e -> issue.secondary(e, null));
59+
}
60+
PreciseIssue issue = null;
61+
for (Map.Entry<ClassSymbol, List<Expression>> otherEntry : caughtExceptionsBySymbol.entrySet()) {
62+
ClassSymbol comparedSymbol = otherEntry.getKey();
63+
if (currentSymbol != comparedSymbol && currentSymbol.isOrExtends(comparedSymbol)) {
64+
if (issue == null) {
65+
issue = ctx.addIssue(currentException, "Remove this redundant Exception class; it derives from another which is already caught.");
66+
}
67+
addSecondaryLocations(issue, otherEntry.getValue());
68+
}
69+
}
70+
});
71+
}
72+
73+
private static void addExceptionExpression(Expression exceptionExpression, Map<ClassSymbol, List<Expression>> caughtExceptionsByFQN) {
74+
if (exceptionExpression instanceof HasSymbol) {
75+
Symbol symbol = ((HasSymbol) exceptionExpression).symbol();
76+
if (symbol != null && symbol.kind().equals(Symbol.Kind.CLASS)) {
77+
ClassSymbol classSymbol = (ClassSymbol) symbol;
78+
caughtExceptionsByFQN.computeIfAbsent(classSymbol, k -> new ArrayList<>()).add(exceptionExpression);
79+
}
80+
}
81+
}
82+
83+
private static void addSecondaryLocations(PreciseIssue issue, List<Expression> others) {
84+
for (Expression other : others) {
85+
issue.secondary(other, null);
86+
}
87+
}
88+
}

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<p>Repeating an exception class in a single <code>except</code> statement will not fail but it is not what the developer intended. Either the class is
2+
not the one which should be caught, or this is dead code.</p>
3+
<p>Having a subclass and a parent class in the same <code>except</code> statement is also useless. It is enough to keep only the parent class.</p>
4+
<p>This rule raises an issue when an exception class is duplicated in an <code>except</code> statement, or when an exception class has a parent class
5+
in the same <code>except</code> statement.</p>
6+
<h2>Noncompliant Code Example</h2>
7+
<pre>
8+
try:
9+
raise NotImplementedError()
10+
except (NotImplementedError, RuntimeError): # Noncompliant. NotImplementedError inherits from RuntimeError
11+
print("Foo")
12+
13+
try:
14+
raise NotImplementedError()
15+
except (RuntimeError, RuntimeError): # Noncompliant.
16+
print("Foo")
17+
</pre>
18+
<h2>Compliant Solution</h2>
19+
<pre>
20+
try:
21+
raise NotImplementedError()
22+
except RuntimeError:
23+
print("Foo")
24+
</pre>
25+
<h2>See</h2>
26+
<ul>
27+
<li> Python Documentation - <a href="https://docs.python.org/3/tutorial/errors.html#handling-exceptions">Handling Exceptions</a> </li>
28+
</ul>
29+

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
"title": "A subclass should not be in the same \"except\" statement as a parent class",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"tags": [
6+
"unused"
7+
],
8+
"defaultSeverity": "Major",
9+
"ruleSpecification": "RSPEC-5713",
10+
"sqKey": "S5713",
11+
"scope": "All"
12+
}

‎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
@@ -86,6 +86,7 @@
8686
"S5707",
8787
"S5708",
8888
"S5712",
89+
"S5713",
8990
"S5714",
9091
"S5727",
9192
"S5747"

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

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
def child_with_parent():
2+
try:
3+
raise NotImplementedError()
4+
except (NotImplementedError, RuntimeError): # Noncompliant {{Remove this redundant Exception class; it derives from another which is already caught.}}
5+
# ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^<
6+
print("Foo")
7+
8+
def parent_with_child():
9+
try:
10+
raise NotImplementedError()
11+
except (RuntimeError, NotImplementedError): # Noncompliant {{Remove this redundant Exception class; it derives from another which is already caught.}}
12+
# ^^^^^^^^^^^^> ^^^^^^^^^^^^^^^^^^^
13+
print("Foo")
14+
15+
16+
def duplicate_exception_caught():
17+
try:
18+
raise NotImplementedError()
19+
except (RuntimeError, RuntimeError): # Noncompliant {{Remove this duplicate Exception class.}}
20+
# ^^^^^^^^^^^^ ^^^^^^^^^^^^<
21+
print("Foo")
22+
23+
def multiple_parents():
24+
try:
25+
raise NotImplementedError()
26+
except (UnicodeDecodeError, UnicodeError, ValueError): # Noncompliant 2
27+
print("Foo")
28+
29+
def duplicate_and_parent_with_child():
30+
try:
31+
raise NotImplementedError()
32+
except (RuntimeError, NotImplementedError, RuntimeError): # Noncompliant 2
33+
print("Foo")
34+
35+
def python2_supports_nested_tuples():
36+
try:
37+
...
38+
except (ValueError, (RuntimeError, NotImplementedError)): # Noncompliant
39+
...
40+
41+
def compliant():
42+
try:
43+
raise NotImplementedError()
44+
except RuntimeError:
45+
print("Foo")
46+
def unknown():
47+
def method():
48+
pass
49+
try:
50+
raise ValueError()
51+
except ((A | B), C):
52+
pass
53+
except (method, Exception):
54+
pass
55+
except:
56+
pass

‎python-frontend/src/main/java/org/sonar/plugins/python/api/symbols/ClassSymbol.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,7 @@ public interface ClassSymbol extends Symbol {
3636

3737
@Beta
3838
boolean isOrExtends(String fullyQualifiedClassName);
39+
40+
@Beta
41+
boolean isOrExtends(ClassSymbol other);
3942
}

‎python-frontend/src/main/java/org/sonar/python/semantic/ClassSymbolImpl.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.LinkedHashSet;
2828
import java.util.List;
2929
import java.util.Map;
30+
import java.util.Objects;
3031
import java.util.Optional;
3132
import java.util.Set;
3233
import java.util.stream.Collectors;
@@ -111,6 +112,12 @@ public boolean isOrExtends(String fullyQualifiedClassName) {
111112
return allSuperClasses().stream().anyMatch(c -> fullyQualifiedClassName.equals(c.fullyQualifiedName()));
112113
}
113114

115+
@Override
116+
public boolean isOrExtends(ClassSymbol other) {
117+
// TODO there should be only 1 class with a given fullyQualifiedName when analyzing a python file
118+
return allSuperClasses().stream().anyMatch(c -> Objects.equals(c.fullyQualifiedName(), other.fullyQualifiedName()));
119+
}
120+
114121
private Map<String, Symbol> membersByName() {
115122
if (membersByName == null) {
116123
membersByName = declaredMembers().stream().collect(Collectors.toMap(Symbol::name, m -> m, (s1, s2) -> s1));

0 commit comments

Comments
 (0)