Skip to content

Commit 591af6f

Browse files
carlosu7carlosu7
authored
Improve switch-literal-order codemod to recognize non-null sources (#330)
Method `isSimpleNameANotNullInitializedVariableDeclarator` was updated to review expression when is a method call expression. This new update checks if method used in the expression belongs to a library that is considered a null safe imported library (based on a given list of null safe libraries). Issue #316
1 parent f99775e commit 591af6f

File tree

4 files changed

+316
-22
lines changed

4 files changed

+316
-22
lines changed

‎core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ void it_transforms_webgoat_normally() throws Exception {
9595
.flatMap(Collection::stream)
9696
.collect(Collectors.toList());
9797

98-
assertThat(fileChanges.size(), is(54));
98+
assertThat(fileChanges.size(), is(58));
9999

100100
// we only inject into a couple files
101101
verifyStandardCodemodResults(fileChanges);
@@ -133,7 +133,7 @@ void it_transforms_webgoat_with_codeql() throws Exception {
133133
.flatMap(Collection::stream)
134134
.collect(Collectors.toList());
135135

136-
assertThat(fileChanges.size(), is(58));
136+
assertThat(fileChanges.size(), is(62));
137137

138138
verifyStandardCodemodResults(fileChanges);
139139

‎core-codemods/src/main/java/io/codemodder/codemods/SwitchLiteralFirstComparisonsCodemod.java‎

Lines changed: 178 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,24 @@
33
import com.contrastsecurity.sarif.Result;
44
import com.github.javaparser.Range;
55
import com.github.javaparser.ast.CompilationUnit;
6+
import com.github.javaparser.ast.ImportDeclaration;
67
import com.github.javaparser.ast.Node;
78
import com.github.javaparser.ast.NodeList;
89
import com.github.javaparser.ast.body.FieldDeclaration;
910
import com.github.javaparser.ast.body.Parameter;
1011
import com.github.javaparser.ast.body.VariableDeclarator;
1112
import com.github.javaparser.ast.expr.AnnotationExpr;
1213
import com.github.javaparser.ast.expr.BinaryExpr;
14+
import com.github.javaparser.ast.expr.ConditionalExpr;
1315
import com.github.javaparser.ast.expr.Expression;
16+
import com.github.javaparser.ast.expr.FieldAccessExpr;
17+
import com.github.javaparser.ast.expr.LiteralExpr;
1418
import com.github.javaparser.ast.expr.MethodCallExpr;
1519
import com.github.javaparser.ast.expr.Name;
1620
import com.github.javaparser.ast.expr.NameExpr;
1721
import com.github.javaparser.ast.expr.NullLiteralExpr;
1822
import com.github.javaparser.ast.expr.SimpleName;
23+
import com.github.javaparser.ast.expr.StringLiteralExpr;
1924
import com.github.javaparser.ast.nodeTypes.NodeWithAnnotations;
2025
import com.github.javaparser.ast.nodeTypes.NodeWithSimpleName;
2126
import com.github.javaparser.resolution.UnsolvedSymbolException;
@@ -68,16 +73,15 @@ public ChangesResult onResultFound(
6873
* This codemod will not be executed if:
6974
*
7075
* <ol>
71-
* <li>Variable was previously initialized to a not null value
7276
* <li>Variable has a previous not null assertion
7377
* <li>Variable has a {@link @NotNull} or {@link @Nonnull} annotation
78+
* <li>Variable was previously initialized to a not null value
7479
* </ol>
7580
*/
7681
if (simpleNameOptional.isPresent()
77-
&& (isSimpleNameANotNullInitializedVariableDeclarator(
78-
variableDeclarators, simpleNameOptional.get())
79-
|| hasSimpleNameNotNullAnnotation(cu, simpleNameOptional.get(), variableDeclarators)
80-
|| hasSimpleNamePreviousNullAssertion(cu, simpleNameOptional.get()))) {
82+
&& (hasSimpleNameNotNullAnnotation(cu, simpleNameOptional.get(), variableDeclarators)
83+
|| hasSimpleNamePreviousNullAssertion(cu, simpleNameOptional.get())
84+
|| isSimpleNameANotNullInitializedVariableDeclarator(cu, simpleNameOptional.get()))) {
8185
return ChangesResult.noChanges;
8286
}
8387

@@ -238,21 +242,20 @@ private boolean isNotNullOrNonnullAnnotation(final AnnotationExpr annotation) {
238242

239243
/**
240244
* Checks if the provided {@link SimpleName} variable corresponds to a {@link VariableDeclarator}
241-
* that was previously initialized to a non-null value.
245+
* that was previously initialized to a non-null expression.
242246
*/
243247
private boolean isSimpleNameANotNullInitializedVariableDeclarator(
244-
final List<VariableDeclarator> variableDeclarators, final SimpleName targetName) {
245-
246-
return targetName != null
247-
&& variableDeclarators.stream()
248-
.filter(declarator -> declarator.getName().equals(targetName))
249-
.filter(declarator -> isPreviousNodeBefore(targetName, declarator.getName()))
250-
.anyMatch(
251-
declarator ->
252-
declarator
253-
.getInitializer()
254-
.map(expr -> !(expr instanceof NullLiteralExpr))
255-
.orElse(false));
248+
final CompilationUnit cu, final SimpleName targetName) {
249+
250+
final Optional<VariableDeclarator> variableDeclaratorOptional =
251+
getDeclaredVariable(cu, targetName);
252+
253+
if (variableDeclaratorOptional.isEmpty()
254+
|| variableDeclaratorOptional.get().getInitializer().isEmpty()) {
255+
return false;
256+
}
257+
258+
return isNullSafeExpression(cu, variableDeclaratorOptional.get().getInitializer().get());
256259
}
257260

258261
/**
@@ -269,6 +272,163 @@ private Optional<SimpleName> getSimpleNameFromMethodCallExpr(
269272
return simpleNames.isEmpty() ? Optional.empty() : Optional.of(simpleNames.get(0));
270273
}
271274

275+
private boolean isNullSafeExpression(final CompilationUnit cu, final Expression expression) {
276+
if (expression instanceof NullLiteralExpr) {
277+
return false;
278+
}
279+
280+
if (expression instanceof MethodCallExpr methodCallExpr) {
281+
return isNullSafeMethodExpr(cu, methodCallExpr);
282+
}
283+
284+
if (expression instanceof ConditionalExpr conditionalExpr) {
285+
return isNullSafeExpression(cu, conditionalExpr.getThenExpr())
286+
&& isNullSafeExpression(cu, conditionalExpr.getElseExpr());
287+
}
288+
289+
if (expression instanceof NameExpr nameExpr) {
290+
return isSimpleNameANotNullInitializedVariableDeclarator(cu, nameExpr.getName());
291+
}
292+
293+
return expression instanceof LiteralExpr;
294+
}
295+
296+
private boolean isNullSafeMethodExpr(
297+
final CompilationUnit cu, final MethodCallExpr methodCallExpr) {
298+
final Optional<Expression> optionalScope = methodCallExpr.getScope();
299+
300+
final String method = methodCallExpr.getName().getIdentifier();
301+
302+
// Static import case for example: import static
303+
// org.apache.commons.lang3.StringUtils.defaultString
304+
if (optionalScope.isEmpty()) {
305+
return isNullSafeImportLibrary(cu, methodCallExpr.getName().getIdentifier(), method);
306+
}
307+
308+
final Expression scope = optionalScope.get();
309+
310+
// Using java.lang.String's method
311+
if (scope instanceof StringLiteralExpr) {
312+
return commonMethodsThatCantReturnNull.contains("java.lang.String#".concat(method));
313+
}
314+
315+
// Using full import name as scope of method, for example
316+
// String str = org.apache.commons.lang3.StringUtils.defaultString("")
317+
if (scope instanceof FieldAccessExpr fieldAccessExpr) {
318+
final String fullImportName = fieldAccessExpr.toString();
319+
return commonMethodsThatCantReturnNull.contains(fullImportName.concat("#").concat(method));
320+
}
321+
322+
if (scope instanceof NameExpr scopeName) {
323+
324+
if (!isVariable(cu, scopeName)) {
325+
// check if scope is non-static import like: import org.apache.commons.lang3.StringUtils
326+
return isNullSafeImportLibrary(cu, scopeName.getName().getIdentifier(), method);
327+
}
328+
329+
final Optional<VariableDeclarator> variableDeclaratorOptional =
330+
getDeclaredVariable(cu, scopeName.getName());
331+
332+
if (variableDeclaratorOptional.isEmpty()) {
333+
return false;
334+
}
335+
336+
final String type = variableDeclaratorOptional.get().getTypeAsString();
337+
338+
// when scope is an object variable, check class type to determine if it is an implicit or
339+
// explicit import
340+
return isClassObjectMethodNullSafe(cu, type, method);
341+
}
342+
343+
return false;
344+
}
345+
346+
/** Some basic java lang type classes */
347+
private boolean isClassObjectMethodNullSafe(
348+
final CompilationUnit cu, final String type, final String method) {
349+
switch (type) {
350+
case "String" -> {
351+
return commonMethodsThatCantReturnNull.contains("java.lang.String#".concat(method));
352+
}
353+
case "Integer" -> {
354+
return commonMethodsThatCantReturnNull.contains("java.lang.Integer#".concat(method));
355+
}
356+
case "Double" -> {
357+
return commonMethodsThatCantReturnNull.contains("java.lang.Double#".concat(method));
358+
}
359+
case "Character" -> {
360+
return commonMethodsThatCantReturnNull.contains("java.lang.Character#".concat(method));
361+
}
362+
case "Long" -> {
363+
return commonMethodsThatCantReturnNull.contains("java.lang.Long#".concat(method));
364+
}
365+
default -> {
366+
return isNullSafeImportLibrary(cu, type, method);
367+
}
368+
}
369+
}
370+
371+
private boolean isVariable(final CompilationUnit cu, final NameExpr nameExpr) {
372+
final SimpleName simpleName = nameExpr.getName();
373+
final Optional<VariableDeclarator> variableDeclaratorOptional =
374+
getDeclaredVariable(cu, simpleName);
375+
return variableDeclaratorOptional.isPresent();
376+
}
377+
378+
private boolean isNullSafeImportLibrary(
379+
final CompilationUnit cu, final String identifier, final String method) {
380+
final Optional<ImportDeclaration> optionalImport =
381+
cu.getImports().stream()
382+
.filter(importName -> importName.getName().getIdentifier().equals(identifier))
383+
.findFirst();
384+
385+
if (optionalImport.isEmpty()) {
386+
return false;
387+
}
388+
389+
if (optionalImport.get().isStatic()
390+
&& optionalImport.get().getName().getQualifier().isEmpty()) {
391+
return false;
392+
}
393+
394+
final Name importDeclaration =
395+
optionalImport.get().isStatic()
396+
? optionalImport.get().getName().getQualifier().get()
397+
: optionalImport.get().getName();
398+
399+
return commonMethodsThatCantReturnNull.contains(
400+
importDeclaration.asString().concat("#").concat(method));
401+
}
402+
403+
private Optional<VariableDeclarator> getDeclaredVariable(
404+
final CompilationUnit cu, final SimpleName simpleName) {
405+
final List<VariableDeclarator> variableDeclarators = cu.findAll(VariableDeclarator.class);
406+
return variableDeclarators.stream()
407+
.filter(declarator -> declarator.getName().equals(simpleName))
408+
.filter(declarator -> isPreviousNodeBefore(simpleName, declarator.getName()))
409+
.findFirst();
410+
}
411+
272412
private static final Set<String> flippableComparisonMethods =
273413
Set.of("equals", "equalsIgnoreCase");
414+
415+
private static final List<String> commonMethodsThatCantReturnNull =
416+
List.of(
417+
"org.apache.commons.lang3.StringUtils#defaultString",
418+
"java.lang.String#concat",
419+
"java.lang.String#replace",
420+
"java.lang.String#replaceAll",
421+
"java.lang.String#replaceFirst",
422+
"java.lang.String#join",
423+
"java.lang.String#substring",
424+
"java.lang.String#substring",
425+
"java.lang.String#toLowerCase",
426+
"java.lang.String#toUpperCase",
427+
"java.lang.String#trim",
428+
"java.lang.String#strip",
429+
"java.lang.String#stripLeading",
430+
"java.lang.String#stripTrailing",
431+
"java.lang.String#toString",
432+
"java.lang.String#valueOf",
433+
"java.lang.String#formatted");
274434
}

‎core-codemods/src/test/resources/switch-literal-first-comparisons/Test.java.after‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
package com.acme.testcode;
22

3+
import org.apache.commons.lang3.StringUtils;
4+
import com.pibe.PibeUtils;
5+
import static org.apache.commons.lang3.StringUtils.defaultString;
6+
import io.codemodder.codemods.Codemod;
7+
38
final class Test {
49

510
public boolean fine1 = "foo".equals("bar");
611
private boolean fine2 = "foo".equals(bar);
712
static boolean cantChange = bar.equals("foo"); // can't change because we don't know the type of bar
813
final boolean change2 = "foo".equalsIgnoreCase(getBar());
914

10-
void foo(String foo, @NotNull String foo1, String nullAssertion) {
15+
void foo(String foo, @NotNull String foo1, String nullAssertion, String paramValue) {
1116
boolean change1 = "bar".equals(foo);
1217
String initializedVar = "init";
1318
if("bar".equals(foo)) { // should change
@@ -36,6 +41,68 @@ final class Test {
3641
System.out.println("outer");
3742
}
3843
}
44+
45+
String nullString = null;
46+
if("hola".equals(nullString)){
47+
System.out.println(nullString);
48+
}
49+
50+
// shouldn't be change because var is initialized using a method contained in commonMethodsThatCantReturnNull (Implicit String library)
51+
String replacedString = "hola".replace("hola", "fibonacci");
52+
if(replacedString.equals("fibonacci")){
53+
System.out.println(replacedString);
54+
}
55+
56+
// should change name expr var since it is a string that is initialized using a method not contained in commonMethodsThatCantReturnNull (Implicit String library)
57+
String invalidStringMethodString2 = invalidStringMethodString.randomMethod(100);
58+
if("pibeString".equals(invalidStringMethodString2)){
59+
System.out.println(pibeUtilsStr);
60+
}
61+
62+
// shouldn't be change because var is initialized using a method contained in commonMethodsThatCantReturnNull (Explicit non static import library)
63+
String defaultString = StringUtils.defaultString(str);
64+
if(defaultString.equals("defaultString")){
65+
System.out.println(defaultString);
66+
}
67+
68+
// should change - var is initialized using a method NOT contained in commonMethodsThatCantReturnNull (Explicit non static import library)
69+
String pibeUtilsStr = PibeUtils.parse(str);
70+
if("pibeString".equals(pibeUtilsStr)){
71+
System.out.println(pibeUtilsStr);
72+
}
73+
74+
// shouldn't be change because var is initialized using a method contained in commonMethodsThatCantReturnNull (Full explicit non static import library)
75+
String fieldExprStr = org.apache.commons.lang3.StringUtils.defaultString(str);
76+
if(fieldExprStr.equals("defaultString")){
77+
System.out.println(defaultString);
78+
}
79+
80+
// shouldn't be change because var is initialized using a method contained in commonMethodsThatCantReturnNull (Explicit static import library)
81+
String staticImportMethodStr = defaultString(str);
82+
if(staticImportMethodStr.equals("defaultString")){
83+
System.out.println(defaultString);
84+
}
85+
86+
// should not change because org.apache.commons.lang3.StringUtils#defaultString is contained in commonMethodsThatCantReturnNull
87+
StringUtils stringUtils = new StringUtils();
88+
String stringUtilsDefaultString = stringUtils.defaultString();
89+
if(stringUtilsDefaultString.equals("defaultString")){
90+
System.out.println(defaultString);
91+
}
92+
93+
// should change because anotherVar can be null
94+
String anotherVar = 1 == 2 ? "hola": null;
95+
String myVar = anotherVar;
96+
if ("test".equals(myVar)) {
97+
System.out.println("Hello, World!");
98+
}
99+
100+
// should not change because anotherExample will never be null
101+
String anotherExample = 1 == 2 ? "hola": "";
102+
String example = anotherExample;
103+
if (example.equals("test")) {
104+
System.out.println("Hello, World!");
105+
}
39106
}
40107

41108
private String getBar() { return null; }

0 commit comments

Comments
 (0)