Skip to content

Commit 999302a

Browse files
nils-werner-sonarsourcenils-werner-sonarsource
authored
SONARPY-1029 Add quick fixes for S5717 (ModifiedParameterValueCheck) (#1146)
1 parent c86dd55 commit 999302a

File tree

6 files changed

+274
-107
lines changed

6 files changed

+274
-107
lines changed

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

Lines changed: 57 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.sonar.plugins.python.api.symbols.Symbol;
3737
import org.sonar.plugins.python.api.symbols.Usage;
3838
import org.sonar.plugins.python.api.tree.AssignmentStatement;
39+
import org.sonar.plugins.python.api.tree.CallExpression;
3940
import org.sonar.plugins.python.api.tree.CompoundAssignmentStatement;
4041
import org.sonar.plugins.python.api.tree.Expression;
4142
import org.sonar.plugins.python.api.tree.FunctionDef;
@@ -45,21 +46,27 @@
4546
import org.sonar.plugins.python.api.tree.SubscriptionExpression;
4647
import org.sonar.plugins.python.api.tree.Tree;
4748
import org.sonar.plugins.python.api.types.BuiltinTypes;
49+
import org.sonar.python.quickfix.IssueWithQuickFix;
50+
import org.sonar.python.quickfix.PythonQuickFix;
4851
import org.sonar.python.tree.TreeUtils;
4952

5053
import static org.sonar.plugins.python.api.tree.Tree.Kind.ASSIGNMENT_STMT;
54+
import static org.sonar.plugins.python.api.tree.Tree.Kind.CALL_EXPR;
5155
import static org.sonar.plugins.python.api.tree.Tree.Kind.COMPOUND_ASSIGNMENT;
5256
import static org.sonar.plugins.python.api.tree.Tree.Kind.DEL_STMT;
5357
import static org.sonar.plugins.python.api.tree.Tree.Kind.FUNCDEF;
5458
import static org.sonar.plugins.python.api.tree.Tree.Kind.NAME;
5559
import static org.sonar.plugins.python.api.tree.Tree.Kind.QUALIFIED_EXPR;
5660
import static org.sonar.plugins.python.api.tree.Tree.Kind.SUBSCRIPTION;
61+
import static org.sonar.python.quickfix.PythonTextEdit.insertLineBefore;
62+
import static org.sonar.python.quickfix.PythonTextEdit.replace;
5763
import static org.sonar.python.tree.TreeUtils.getSymbolFromTree;
5864
import static org.sonar.python.tree.TreeUtils.nonTupleParameters;
5965

6066
@Rule(key = "S5717")
6167
public class ModifiedParameterValueCheck extends PythonSubscriptionCheck {
6268

69+
private static final String MESSAGE = "Change this default value to \"None\" and initialize this parameter inside the function/method.";
6370
private static final String MODIFIED_SECONDARY = "The parameter is modified.";
6471
private static final String ASSIGNED_SECONDARY = "The parameter is stored in another object.";
6572

@@ -113,16 +120,55 @@ public void initialize(Context context) {
113120
if (TreeUtils.firstAncestorOfKind(functionDef, FUNCDEF) != null) {
114121
return;
115122
}
123+
116124
for (Parameter parameter : nonTupleParameters(functionDef)) {
117-
Map<Tree, String> mutations = getMutations(parameter);
118-
if (!mutations.isEmpty()) {
119-
PreciseIssue preciseIssue = ctx.addIssue(parameter, "Change this default value to \"None\" and initialize this parameter inside the function/method.");
120-
mutations.keySet().forEach(t -> preciseIssue.secondary(t, mutations.get(t)));
125+
Expression defaultValue = parameter.defaultValue();
126+
if (defaultValue == null) {
127+
continue;
121128
}
129+
130+
getSymbolFromTree(parameter.name())
131+
.filter(symbol -> !isUsingMemoization(symbol))
132+
.ifPresent(paramSymbol -> {
133+
Map<Tree, String> mutations = getMutations(defaultValue, paramSymbol);
134+
if (!mutations.isEmpty()) {
135+
IssueWithQuickFix issue = (IssueWithQuickFix) ctx.addIssue(parameter, MESSAGE);
136+
mutations.keySet().forEach(t -> issue.secondary(t, mutations.get(t)));
137+
138+
getQuickFix(functionDef, defaultValue, paramSymbol)
139+
.ifPresent(issue::addQuickFix);
140+
}
141+
}
142+
);
122143
}
123144
});
124145
}
125146

147+
// We use "\n" systematically, the IDE will decide which one to use,
148+
// therefore suppressing java:S3457 (Printf-style format strings should be used correctly)
149+
@SuppressWarnings("java:S3457")
150+
private static Optional<PythonQuickFix> getQuickFix(FunctionDef functionDef, Expression defaultValue, Symbol paramSymbol) {
151+
Tree firstStatement = functionDef.body().statements().get(0);
152+
String paramName = paramSymbol.name();
153+
154+
return parameterInitialization(defaultValue).map(
155+
paramInit -> PythonQuickFix.newQuickFix("Initialize this parameter inside the function/method")
156+
.addTextEdit(replace(defaultValue, "None"))
157+
.addTextEdit(insertLineBefore(firstStatement, String.format("if %1$s is None:\n %1$s = %2$s()\n", paramName, paramInit)))
158+
.build()
159+
);
160+
}
161+
162+
private static Optional<String> parameterInitialization(Expression defaultValue) {
163+
if (defaultValue.is(CALL_EXPR)) {
164+
CallExpression call = (CallExpression) defaultValue;
165+
return Optional.ofNullable(call.calleeSymbol())
166+
.map(symbol -> call.callee().is(QUALIFIED_EXPR) ? symbol.fullyQualifiedName() : symbol.name());
167+
} else {
168+
return Optional.ofNullable(defaultValueType(defaultValue));
169+
}
170+
}
171+
126172
@CheckForNull
127173
private static String defaultValueType(Expression expression) {
128174
for (String nonCompliantType : MUTATING_METHODS.keySet()) {
@@ -137,19 +183,9 @@ private static boolean isUsingMemoization(Symbol symbol) {
137183
return symbol.name().contains("cache") || symbol.name().contains("memo");
138184
}
139185

140-
private static Map<Tree, String> getMutations(Parameter parameter) {
141-
Expression defaultValue = parameter.defaultValue();
142-
if (defaultValue == null) {
143-
return Collections.emptyMap();
144-
}
145-
146-
Optional<Symbol> paramSymbol = getSymbolFromTree(parameter.name());
147-
if (!paramSymbol.isPresent() || isUsingMemoization(paramSymbol.get())) {
148-
return Collections.emptyMap();
149-
}
150-
186+
private static Map<Tree, String> getMutations(Expression defaultValue, Symbol paramSymbol) {
151187
if (!defaultValue.type().canOnlyBe(BuiltinTypes.NONE_TYPE)) {
152-
List<Tree> attributeSet = getAttributeSet(paramSymbol.get());
188+
List<Tree> attributeSet = getAttributeSet(paramSymbol);
153189
if (!attributeSet.isEmpty()) {
154190
return attributeSet.stream().collect(Collectors.toMap(tree -> tree, tree -> MODIFIED_SECONDARY));
155191
}
@@ -160,18 +196,19 @@ private static Map<Tree, String> getMutations(Parameter parameter) {
160196
return Collections.emptyMap();
161197
}
162198
Map<Tree, String> mutations = new HashMap<>();
163-
for (Usage usage : paramSymbol.get().usages()) {
199+
for (Usage usage : paramSymbol.usages()) {
164200
getKindOfWriteUsage(paramSymbol, defaultValueType, typeMutatingMethods, usage).ifPresent(s -> mutations.put(usage.tree().parent(), s));
165201
}
166202
return mutations;
167203
}
168204

169-
private static Optional<String> getKindOfWriteUsage(Optional<Symbol> paramSymbol, @Nullable String defaultValueType, Set<String> typeMutatingMethods, Usage usage) {
205+
private static Optional<String> getKindOfWriteUsage(Symbol paramSymbol, @Nullable String defaultValueType, Set<String> typeMutatingMethods, Usage usage) {
170206
Tree parent = usage.tree().parent();
171207
if (parent.is(QUALIFIED_EXPR)) {
172208
QualifiedExpression qualifiedExpression = (QualifiedExpression) parent;
173-
return getSymbolFromTree(qualifiedExpression.qualifier()).equals(paramSymbol) && isMutatingMethod(typeMutatingMethods, qualifiedExpression.name().name()) ?
174-
Optional.of(MODIFIED_SECONDARY) : Optional.empty();
209+
210+
return getSymbolFromTree(qualifiedExpression.qualifier()).filter(paramSymbol::equals).isPresent()
211+
&& isMutatingMethod(typeMutatingMethods, qualifiedExpression.name().name()) ? Optional.of(MODIFIED_SECONDARY) : Optional.empty();
175212
}
176213
if (isUsedInDelStatement(usage.tree()) ||
177214
isUsedInLhsOfAssignment(usage.tree(), exp -> isAccessingExpression(exp, usage.tree())) ||

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

Lines changed: 143 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,155 @@
2020
package org.sonar.python.checks;
2121

2222
import org.junit.Test;
23+
import org.sonar.plugins.python.api.PythonCheck;
24+
import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
2325
import org.sonar.python.checks.utils.PythonCheckVerifier;
2426

2527
public class ModifiedParameterValueCheckTest {
2628

29+
private final PythonCheck check = new ModifiedParameterValueCheck();
2730
@Test
2831
public void test() {
29-
PythonCheckVerifier.verify("src/test/resources/checks/modifiedParameterValue.py", new ModifiedParameterValueCheck());
32+
PythonCheckVerifier.verify("src/test/resources/checks/modifiedParameterValue.py", check);
3033
}
3134

35+
@Test
36+
public void list_modified_quickfix() {
37+
String codeWithIssue = "" +
38+
"def list_modified(param=list()):\n" +
39+
" param.append('a')";
40+
41+
String fixedCode = "" +
42+
"def list_modified(param=None):\n" +
43+
" if param is None:\n" +
44+
" param = list()\n" +
45+
" param.append('a')";
46+
47+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
48+
}
49+
50+
@Test
51+
public void method_with_quickfix() {
52+
String codeWithIssue = "" +
53+
"class Foo:\n" +
54+
" def list_modified(param=list()):\n" +
55+
" param.append('a')";
56+
57+
String fixedCode = "" +
58+
"class Foo:\n" +
59+
" def list_modified(param=None):\n" +
60+
" if param is None:\n" +
61+
" param = list()\n" +
62+
" param.append('a')";
63+
64+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
65+
}
66+
67+
@Test
68+
public void default_with_spaces_quickfix() {
69+
String codeWithIssue = "" +
70+
"def list_modified(param = list()):\n" +
71+
" param.append('a')";
72+
73+
String fixedCode = "" +
74+
"def list_modified(param = None):\n" +
75+
" if param is None:\n" +
76+
" param = list()\n" +
77+
" param.append('a')";
78+
79+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
80+
}
81+
82+
@Test
83+
public void annotated_parameter_quickfix() {
84+
String codeWithIssue = "" +
85+
"def list_modified(param:list=list()):\n" +
86+
" param.append('a')";
87+
88+
String fixedCode = "" +
89+
"def list_modified(param:list=None):\n" +
90+
" if param is None:\n" +
91+
" param = list()\n" +
92+
" param.append('a')";
93+
94+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
95+
}
96+
97+
@Test
98+
public void annotated_parameter_with_space_quickfix() {
99+
String codeWithIssue = "" +
100+
"def list_modified(param: list = list()):\n" +
101+
" param.append('a')";
102+
103+
String fixedCode = "" +
104+
"def list_modified(param: list = None):\n" +
105+
" if param is None:\n" +
106+
" param = list()\n" +
107+
" param.append('a')";
108+
109+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
110+
}
111+
112+
@Test
113+
public void set_modified_quickfix() {
114+
String codeWithIssue = "" +
115+
"def set_modified(param=set()):\n" +
116+
" param.add('a')";
117+
118+
String fixedCode = "" +
119+
"def set_modified(param=None):\n" +
120+
" if param is None:\n" +
121+
" param = set()\n" +
122+
" param.add('a')";
123+
124+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
125+
}
126+
127+
@Test
128+
public void counter_modified_quickfix() {
129+
String codeWithIssue = "" +
130+
"import collections\n" +
131+
"def counter_modified(param=collections.Counter()):\n" +
132+
" param.subtract()";
133+
134+
String fixedCode = "" +
135+
"import collections\n" +
136+
"def counter_modified(param=None):\n" +
137+
" if param is None:\n" +
138+
" param = collections.Counter()\n" +
139+
" param.subtract()";
140+
141+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
142+
}
143+
144+
@Test
145+
public void import_from_quickfix() {
146+
String codeWithIssue = "" +
147+
"from collections import Counter\n" +
148+
"def list_modified(param=Counter()):\n" +
149+
" param.subtract()";
150+
151+
String fixedCode = "" +
152+
"from collections import Counter\n" +
153+
"def list_modified(param=None):\n" +
154+
" if param is None:\n" +
155+
" param = Counter()\n" +
156+
" param.subtract()";
157+
158+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
159+
}
160+
161+
@Test
162+
public void literal_dict_quickfix() {
163+
String codeWithIssue = "def literal_dict(param={}):\n" +
164+
" param.pop('a')";
165+
String fixedCode = "def literal_dict(param=None):\n" +
166+
" if param is None:\n" +
167+
" param = dict()\n" +
168+
" param.pop('a')";
169+
170+
PythonQuickFixVerifier.verify(check, codeWithIssue, fixedCode);
171+
}
172+
173+
32174
}

‎python-frontend/pom.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@
5050
<groupId>org.sonarsource.analyzer-commons</groupId>
5151
<artifactId>sonar-regex-parsing</artifactId>
5252
</dependency>
53+
<dependency>
54+
<groupId>org.sonarsource.analyzer-commons</groupId>
55+
<artifactId>sonar-analyzer-commons</artifactId>
56+
<scope>compile</scope>
57+
</dependency>
5358
</dependencies>
5459
<build>
5560
<extensions>

‎python-frontend/src/main/java/org/sonar/python/quickfix/PythonQuickFix.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
import java.util.Arrays;
2424
import java.util.List;
2525

26+
/**
27+
* For internal use only. Can not be used outside SonarPython analyzer.
28+
*/
2629
public class PythonQuickFix {
2730
private final String description;
2831
private final List<PythonTextEdit> textEdits;

‎python-frontend/src/main/java/org/sonar/python/quickfix/PythonTextEdit.java

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@
2222
import org.sonar.plugins.python.api.tree.Token;
2323
import org.sonar.plugins.python.api.tree.Tree;
2424

25+
/**
26+
* For internal use only. Can not be used outside SonarPython analyzer.
27+
*/
2528
public class PythonTextEdit {
2629

2730
private final String message;
@@ -38,25 +41,37 @@ public PythonTextEdit(String message, int startLine, int startLineOffset, int en
3841
this.endLineOffset = endLineOffset;
3942
}
4043

44+
/**
45+
* Insert a line with the same offset as the given tree, before the given tree.
46+
* Offset is applied to multiline insertions.
47+
*/
48+
public static PythonTextEdit insertLineBefore(Tree tree, String textToInsert) {
49+
String lineOffset = " ".repeat(tree.firstToken().column());
50+
String textWithOffset = textToInsert.replace("\n", "\n" + lineOffset);
51+
return insertBefore(tree, textWithOffset);
52+
}
53+
4154
public static PythonTextEdit insertBefore(Tree tree, String textToInsert) {
4255
Token token = tree.firstToken();
43-
return new PythonTextEdit(textToInsert, token.line(), token.column(), token.line(), token.column());
56+
return insertAtPosition(token.line(), token.column(), textToInsert);
4457
}
4558

4659
public static PythonTextEdit insertAfter(Tree tree, String textToInsert) {
4760
Token token = tree.firstToken();
4861
int lengthToken = token.value().length();
49-
return new PythonTextEdit(textToInsert, token.line(), token.column() + lengthToken, token.line(), token.column() + lengthToken);
62+
return insertAtPosition(token.line(), token.column() + lengthToken, textToInsert);
63+
}
64+
65+
private static PythonTextEdit insertAtPosition(int line, int column, String textToInsert) {
66+
return new PythonTextEdit(textToInsert, line, column, line, column);
5067
}
5168

5269
public static PythonTextEdit replace(Tree toReplace, String replacementText) {
53-
Token token = toReplace.firstToken();
54-
Token last = token.lastToken();
55-
return new PythonTextEdit(replacementText, token.line(), token.column(), last.line(), last.column() + last.value().length());
70+
return replaceRange(toReplace, toReplace, replacementText);
5671
}
5772

5873
public static PythonTextEdit replaceRange(Tree start, Tree end, String replacementText) {
59-
Token first =start.firstToken();
74+
Token first = start.firstToken();
6075
Token last = end.lastToken();
6176
return new PythonTextEdit(replacementText, first.line(), first.column(), last.line(), last.column() + last.value().length());
6277
}

0 commit comments

Comments
 (0)