Skip to content

Commit 40f4331

Browse files
guillaume-dequenneguillaume-dequenne
authored
SONARPY-587 Rule S1515: functions and lambdas should not reference va… (SonarSource#658)
1 parent 919cc5c commit 40f4331

9 files changed

Lines changed: 562 additions & 1 deletion

File tree

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
{
2+
'project:buildbot-0.8.6p1/buildbot/status/build.py':[
3+
299,
4+
],
5+
'project:buildbot-0.8.6p1/buildbot/status/builder.py':[
6+
498,
7+
],
8+
'project:buildbot-0.8.6p1/buildbot/status/buildstep.py':[
9+
235,
10+
],
11+
'project:buildbot-0.8.6p1/buildbot/status/mail.py':[
12+
456,
13+
681,
14+
],
15+
'project:buildbot-0.8.6p1/buildbot/test/unit/test_status_web_authz_Authz.py':[
16+
71,
17+
],
18+
'project:django-2.2.3/django/db/models/sql/compiler.py':[
19+
904,
20+
907,
21+
],
22+
'project:numpy-1.16.4/numpy/lib/tests/test_stride_tricks.py':[
23+
276,
24+
276,
25+
],
26+
'project:numpy-1.16.4/numpy/testing/tests/test_utils.py':[
27+
1119,
28+
1119,
29+
1120,
30+
1122,
31+
1122,
32+
1123,
33+
1125,
34+
1125,
35+
1126,
36+
1128,
37+
1128,
38+
1129,
39+
1131,
40+
1131,
41+
1132,
42+
],
43+
'project:twisted-12.1.0/twisted/conch/test/test_transport.py':[
44+
2011,
45+
2024,
46+
2037,
47+
2050,
48+
],
49+
'project:twisted-12.1.0/twisted/mail/imap4.py':[
50+
2125,
51+
],
52+
'project:twisted-12.1.0/twisted/mail/relaymanager.py':[
53+
425,
54+
],
55+
'project:twisted-12.1.0/twisted/mail/test/test_mail.py':[
56+
835,
57+
844,
58+
],
59+
'project:twisted-12.1.0/twisted/names/root.py':[
60+
234,
61+
],
62+
'project:twisted-12.1.0/twisted/web/distrib.py':[
63+
58,
64+
59,
65+
60,
66+
],
67+
}

‎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
@@ -102,6 +102,7 @@ public static Iterable<Class> getChecks() {
102102
FixmeCommentCheck.class,
103103
FunctionComplexityCheck.class,
104104
FunctionNameCheck.class,
105+
FunctionUsingLoopVariableCheck.class,
105106
GenericExceptionRaisedCheck.class,
106107
HardCodedCredentialsCheck.class,
107108
HardcodedIPCheck.class,

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

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
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.Collections;
24+
import java.util.List;
25+
import java.util.Set;
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.Symbol;
30+
import org.sonar.plugins.python.api.symbols.Usage;
31+
import org.sonar.plugins.python.api.tree.AssignmentStatement;
32+
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
33+
import org.sonar.plugins.python.api.tree.CallExpression;
34+
import org.sonar.plugins.python.api.tree.ClassDef;
35+
import org.sonar.plugins.python.api.tree.ComprehensionExpression;
36+
import org.sonar.plugins.python.api.tree.Expression;
37+
import org.sonar.plugins.python.api.tree.FileInput;
38+
import org.sonar.plugins.python.api.tree.FunctionDef;
39+
import org.sonar.plugins.python.api.tree.FunctionLike;
40+
import org.sonar.plugins.python.api.tree.LambdaExpression;
41+
import org.sonar.plugins.python.api.tree.Name;
42+
import org.sonar.plugins.python.api.tree.ParameterList;
43+
import org.sonar.plugins.python.api.tree.ReturnStatement;
44+
import org.sonar.plugins.python.api.tree.Tree;
45+
import org.sonar.plugins.python.api.tree.YieldStatement;
46+
import org.sonar.python.semantic.SymbolUtils;
47+
import org.sonar.python.tree.TreeUtils;
48+
49+
@Rule(key = "S1515")
50+
public class FunctionUsingLoopVariableCheck extends PythonSubscriptionCheck {
51+
@Override
52+
public void initialize(Context context) {
53+
context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, FunctionUsingLoopVariableCheck::checkFunctionLike);
54+
context.registerSyntaxNodeConsumer(Tree.Kind.LAMBDA, FunctionUsingLoopVariableCheck::checkFunctionLike);
55+
}
56+
57+
private static void checkFunctionLike(SubscriptionContext ctx) {
58+
FunctionLike functionLike = (FunctionLike) ctx.syntaxNode();
59+
Tree enclosingLoop = enclosingLoop(functionLike);
60+
if(enclosingLoop == null || !enclosingLoop.is(Tree.Kind.WHILE_STMT, Tree.Kind.FOR_STMT, Tree.Kind.GENERATOR_EXPR, Tree.Kind.LIST_COMPREHENSION)) {
61+
return;
62+
}
63+
if (isReturnedOrCalledWithinLoop(functionLike, enclosingLoop)) {
64+
return;
65+
}
66+
Set<Symbol> enclosingScopeSymbols = getEnclosingScopeSymbols(enclosingLoop);
67+
for (Symbol symbol : enclosingScopeSymbols) {
68+
List<Tree> problematicUsages = new ArrayList<>();
69+
List<Tree> bindingUsages = new ArrayList<>();
70+
for (Usage usage : symbol.usages()) {
71+
Tree usageTree = usage.tree();
72+
if (isUsedInFunctionLike(usageTree, functionLike) && !usage.isBindingUsage()) {
73+
problematicUsages.add(usageTree);
74+
}
75+
if (usage.isBindingUsage() && isWithinEnclosingLoop(usageTree, enclosingLoop)) {
76+
bindingUsages.add(usageTree);
77+
}
78+
}
79+
reportIssue(ctx, functionLike, problematicUsages, bindingUsages);
80+
}
81+
}
82+
83+
private static void reportIssue(SubscriptionContext ctx, FunctionLike functionLike, List<Tree> problematicUsages, List<Tree> bindingUsages) {
84+
if (!problematicUsages.isEmpty() && !bindingUsages.isEmpty()) {
85+
PreciseIssue issue = ctx.addIssue(problematicUsages.get(0), "Pass this variable as a parameter with a default value.")
86+
.secondary(bindingUsages.get(bindingUsages.size() - 1), "Assignment in the loop");
87+
if (functionLike.is(Tree.Kind.FUNCDEF)) {
88+
issue.secondary(((FunctionDef) functionLike).name(), "Function definition");
89+
}
90+
}
91+
}
92+
93+
private static boolean isUsedInFunctionLike(Tree usageTree, FunctionLike functionLike) {
94+
ParameterList parameters = functionLike.parameters();
95+
if (parameters != null && parameters.nonTuple().stream().anyMatch(p -> usageTree.equals(p.defaultValue()))) {
96+
return false;
97+
}
98+
return TreeUtils.hasDescendant(functionLike, tree -> tree.equals(usageTree));
99+
}
100+
101+
private static boolean isWithinEnclosingLoop(Tree usageTree, Tree enclosingLoop) {
102+
return TreeUtils.hasDescendant(enclosingLoop, tree -> tree.equals(usageTree));
103+
}
104+
105+
private static Set<Symbol> getEnclosingScopeSymbols(Tree enclosingLoop) {
106+
if (enclosingLoop.is(Tree.Kind.LIST_COMPREHENSION) || enclosingLoop.is(Tree.Kind.GENERATOR_EXPR)) {
107+
return ((ComprehensionExpression) enclosingLoop).localVariables();
108+
}
109+
Tree enclosingScope = TreeUtils.firstAncestor(enclosingLoop, tree -> tree.is(Tree.Kind.FUNCDEF, Tree.Kind.CLASSDEF, Tree.Kind.FILE_INPUT, Tree.Kind.LIST_COMPREHENSION,
110+
Tree.Kind.GENERATOR_EXPR));
111+
if (enclosingScope == null) {
112+
return Collections.emptySet();
113+
}
114+
if (enclosingScope.is(Tree.Kind.FUNCDEF)) {
115+
return ((FunctionLike) enclosingScope).localVariables();
116+
}
117+
if (enclosingScope.is(Tree.Kind.CLASSDEF)) {
118+
return ((ClassDef) enclosingScope).classFields();
119+
}
120+
return ((FileInput) enclosingScope).globalVariables();
121+
}
122+
123+
private static Tree enclosingLoop(FunctionLike functionLike) {
124+
return TreeUtils.firstAncestor(functionLike, t -> t.is(Tree.Kind.FUNCDEF, Tree.Kind.CLASSDEF, Tree.Kind.WHILE_STMT, Tree.Kind.FOR_STMT,
125+
Tree.Kind.RETURN_STMT, Tree.Kind.YIELD_STMT, Tree.Kind.GENERATOR_EXPR, Tree.Kind.COMP_FOR, Tree.Kind.LIST_COMPREHENSION));
126+
}
127+
128+
private static boolean isReturnedOrCalledWithinLoop(FunctionLike functionLike, Tree enclosingLoop) {
129+
Tree parentCallExpr = TreeUtils.firstAncestor(functionLike, t -> !t.is(Tree.Kind.PARENTHESIZED));
130+
if (parentCallExpr != null && parentCallExpr.is(Tree.Kind.CALL_EXPR)) {
131+
return true;
132+
}
133+
CallOrReturnVisitor callOrReturnVisitor = new CallOrReturnVisitor(functionLike, enclosingLoop);
134+
enclosingLoop.accept(callOrReturnVisitor);
135+
return callOrReturnVisitor.isReturned || callOrReturnVisitor.isCalled;
136+
}
137+
138+
static class CallOrReturnVisitor extends BaseTreeVisitor {
139+
Tree enclosingLoop;
140+
FunctionLike functionLike;
141+
boolean isReturned = false;
142+
boolean isCalled = false;
143+
144+
public CallOrReturnVisitor(FunctionLike functionLike, Tree enclosingLoop) {
145+
this.functionLike = functionLike;
146+
this.enclosingLoop = enclosingLoop;
147+
}
148+
149+
@Override
150+
public void visitCallExpression(CallExpression callExpression) {
151+
Symbol calleeSymbol = callExpression.calleeSymbol();
152+
if (calleeSymbol != null) {
153+
if (functionLike.is(Tree.Kind.FUNCDEF)) {
154+
isCalled |= calleeSymbol.equals(((FunctionDef) functionLike).name().symbol());
155+
} else {
156+
// lambda expression
157+
Name name = variableAssigned((LambdaExpression) functionLike);
158+
if (name != null) {
159+
isCalled |= calleeSymbol.equals(name.symbol());
160+
}
161+
}
162+
}
163+
super.visitCallExpression(callExpression);
164+
}
165+
166+
@Override
167+
public void visitReturnStatement(ReturnStatement returnStatement) {
168+
isReturned |= isFunctionLikeReturned(returnStatement);
169+
}
170+
171+
@Override
172+
public void visitYieldStatement(YieldStatement yieldStatement) {
173+
isReturned |= isFunctionLikeReturned(yieldStatement);
174+
}
175+
176+
private static Name variableAssigned(LambdaExpression lambdaExpression) {
177+
Tree parentAssignment = TreeUtils.firstAncestorOfKind(lambdaExpression, Tree.Kind.ASSIGNMENT_STMT);
178+
if (parentAssignment != null) {
179+
AssignmentStatement assignmentStatement = (AssignmentStatement) parentAssignment;
180+
if (assignmentStatement.lhsExpressions().get(0).expressions().get(0).is(Tree.Kind.NAME)) {
181+
Name name = (Name) assignmentStatement.lhsExpressions().get(0).expressions().get(0);
182+
Expression expression = Expressions.singleAssignedValue(name);
183+
if (expression != null && expression.equals(lambdaExpression)) {
184+
return name;
185+
}
186+
}
187+
}
188+
return null;
189+
}
190+
191+
private boolean isFunctionLikeReturned(Tree yieldOrReturnTree) {
192+
if (functionLike.is(Tree.Kind.FUNCDEF)) {
193+
Symbol functionSymbol = ((FunctionDef) functionLike).name().symbol();
194+
return TreeUtils.hasDescendant(yieldOrReturnTree, d -> TreeUtils.getSymbolFromTree(d).filter(symbol -> symbol.equals(functionSymbol)).isPresent());
195+
} else {
196+
// lambda expression
197+
return isLambdaReturned((LambdaExpression) functionLike, yieldOrReturnTree);
198+
}
199+
}
200+
201+
private static boolean isLambdaReturned(LambdaExpression lambdaExpression, Tree yieldOrReturnTree) {
202+
Tree parentAssignment = TreeUtils.firstAncestorOfKind(lambdaExpression, Tree.Kind.ASSIGNMENT_STMT);
203+
if (parentAssignment != null) {
204+
AssignmentStatement assignmentStatement = (AssignmentStatement) parentAssignment;
205+
// If the lambda expression is used to construct a returned variable, we don't raise issues to avoid FPs, even if the lambda is not returned explicitly
206+
return SymbolUtils.assignmentsLhs(assignmentStatement).stream()
207+
.map(TreeUtils::getSymbolFromTree)
208+
.anyMatch(symbol -> TreeUtils.hasDescendant(yieldOrReturnTree, d -> TreeUtils.getSymbolFromTree(d).equals(symbol)));
209+
}
210+
return false;
211+
}
212+
}
213+
}

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<p>Nested functions and lambdas can reference variables defined in enclosing scopes. This can create tricky bugs when the variable and the function
2+
are defined in a loop. If the function is called after the loop, it will see the variables last value instead of seeing the values corresponding to
3+
the iteration where the function was defined.</p>
4+
<p>This rule raises an issue when a nested function or lambda references a variable defined in an enclosing loop.</p>
5+
<h2>Noncompliant Code Example</h2>
6+
<pre>
7+
def run():
8+
mylist = []
9+
for i in range(5):
10+
mylist.append(lambda: i) # Noncompliant
11+
12+
def func():
13+
return i # Noncompliant
14+
mylist.append(func)
15+
</pre>
16+
<h2>Compliant Solution</h2>
17+
<pre>
18+
def run():
19+
mylist = []
20+
for i in range(5):
21+
mylist.append(lambda i=i: i) # passing the variable as a parameter with a default value
22+
23+
def func(i=i): # same for nested functions
24+
return i
25+
mylist.append(func)
26+
</pre>
27+
<h2>Exceptions</h2>
28+
<p>No issue will be raised if the function or lambda is only called in the same loop.</p>
29+
<pre>
30+
def function_called_in_loop():
31+
for i in range(10):
32+
print((lambda param: param * i)(42))
33+
34+
def func(param):
35+
return param * i
36+
37+
print(func(42))
38+
</pre>
39+
<h2>See</h2>
40+
<ul>
41+
<li> <a href="https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments">The Hitchhiker's Guide to Python - Common Gotchas</a> </li>
42+
</ul>
43+

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
{
2+
"title": "Functions and lambdas should not reference variables defined in enclosing loops",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "30min"
8+
},
9+
"tags": [
10+
"suspicious"
11+
],
12+
"defaultSeverity": "Major",
13+
"ruleSpecification": "RSPEC-1515",
14+
"sqKey": "S1515",
15+
"scope": "Main"
16+
}

‎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
@@ -26,6 +26,7 @@
2626
"S1226",
2727
"S1313",
2828
"S1481",
29+
"S1515",
2930
"S1542",
3031
"S1656",
3132
"S1700",

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
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 FunctionUsingLoopVariableCheckTest {
26+
27+
@Test
28+
public void test() {
29+
PythonCheckVerifier.verify("src/test/resources/checks/functionUsingLoopVariable.py", new FunctionUsingLoopVariableCheck());
30+
}
31+
}

0 commit comments

Comments
 (0)