StringConcatenationInLoopsInspection: null-safe fixes

Fixes IDEA-183139 "String to StringBuilder" quick fix may cause NPE
This commit is contained in:
Tagir Valeev
2017-12-26 17:15:41 +07:00
parent 74ce1e1a43
commit 2e7492686e
12 changed files with 306 additions and 40 deletions

View File

@@ -5,7 +5,7 @@ public class Main {
StringBuilder res = null;
for (String s : strings) {
if(res == null) {
res = s == null ? null : new StringBuilder(s);
res = new StringBuilder(s);
} else {
res.append(s);
}

View File

@@ -0,0 +1,15 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
class Main {
String test(String[] strings) {
StringBuilder res = null;
for (String s : strings) {
if(res == null) {
res = s == null ? null : new StringBuilder(s);
} else {
res.append(s);
}
}
return res == null ? null : res.toString();
}
}

View File

@@ -0,0 +1,16 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
public class Main {
String test(String[] strings) {
StringBuilder res = null;
for (String s : strings) {
if (s == null) continue;
if(res == null) {
res = new StringBuilder(s);
} else {
res.append(s);
}
}
return res == null ? null : res.toString();
}
}

View File

@@ -0,0 +1,13 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
public class Main {
String test(String[] strings) {
StringBuilder res = null;
res = (res == null ? new StringBuilder("null") : res).append("foo");
for (String s : strings) {
res.append(", ");
res.append(s);
}
return res.toString(); // known to be not-null at this point
}
}

View File

@@ -0,0 +1,26 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
import java.util.Optional;
public class Main {
String test(String[] strings) {
StringBuilder res = null;
for (String s : strings) {
if(res == null) {
res = Optional.ofNullable(s.isEmpty() ? null : s).map(StringBuilder::new).orElse(null);
} else {
res.append(", ").append(s);
}
res = (res == null ? new StringBuilder("null") : res).append(", ");
res.append(s);
}
System.out.println(res);
consume(res.toString());
return res.toString(); // known to be not-null at this point
}
// NotNull parameter inferred
static void consume(String s) {
System.out.println(s.trim());
}
}

View File

@@ -0,0 +1,18 @@
// "Convert variable 's1' from String to StringBuilder (null-safe)" "false"
import java.util.List;
class Test {
static void test(List<String> list) {
String s2 = "bar";
String s1 = s2;
for (String s : list) {
s1 +<caret>= "baz";
}
s1 += "foo";
System.out.println(s1);
}
}

View File

@@ -0,0 +1,15 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
class Main {
String test(String[] strings) {
String res = null;
for (String s : strings) {
if(res == null) {
res = s;
} else {
res<caret>+=s;
}
}
return res;
}
}

View File

@@ -0,0 +1,16 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
public class Main {
String test(String[] strings) {
String res = null;
for (String s : strings) {
if (s == null) continue;
if(res == null) {
res = s;
} else {
res<caret>+=s;
}
}
return res;
}
}

View File

@@ -0,0 +1,13 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
public class Main {
String test(String[] strings) {
String res = null;
res += "foo";
for (String s : strings) {
res+<caret>=", ";
res+=s;
}
return res; // known to be not-null at this point
}
}

View File

@@ -0,0 +1,24 @@
// "Convert variable 'res' from String to StringBuilder (null-safe)" "true"
public class Main {
String test(String[] strings) {
String res = null;
for (String s : strings) {
if(res == null) {
res = s.isEmpty() ? null : s;
} else {
res<caret>+=", "+s;
}
res+=", ";
res+=s;
}
System.out.println(res);
consume(res);
return res; // known to be not-null at this point
}
// NotNull parameter inferred
static void consume(String s) {
System.out.println(s.trim());
}
}

View File

@@ -15,10 +15,11 @@
*/
package com.siyeh.ig.performance;
import com.intellij.codeInsight.NullableNotNullManager;
import com.intellij.codeInsight.PsiEquivalenceUtil;
import com.intellij.codeInspection.ProblemDescriptor;
import com.intellij.codeInspection.dataFlow.CommonDataflow;
import com.intellij.codeInspection.dataFlow.DfaFactType;
import com.intellij.codeInspection.dataFlow.Nullness;
import com.intellij.codeInspection.dataFlow.NullnessUtil;
import com.intellij.codeInspection.util.ChangeToAppendUtil;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.util.text.StringUtil;
@@ -32,22 +33,21 @@ import com.intellij.psi.util.PsiTreeUtil;
import com.intellij.psi.util.PsiUtil;
import com.intellij.util.ArrayUtil;
import com.intellij.util.ObjectUtils;
import com.intellij.util.Processor;
import com.intellij.util.Query;
import com.siyeh.InspectionGadgetsBundle;
import com.siyeh.ig.BaseInspection;
import com.siyeh.ig.BaseInspectionVisitor;
import com.siyeh.ig.InspectionGadgetsFix;
import com.siyeh.ig.psiutils.*;
import one.util.streamex.IntStreamEx;
import one.util.streamex.StreamEx;
import org.jetbrains.annotations.Contract;
import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.*;
import java.util.function.Predicate;
import java.util.regex.Pattern;
@@ -295,34 +295,65 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
PsiExpression expression = ObjectUtils.tryCast(ArrayUtil.getFirstElement(infos), PsiExpression.class);
PsiVariable var = getAppendedVariable(expression);
if (var == null) return InspectionGadgetsFix.EMPTY_ARRAY;
boolean needNullSafe = canBeNull(var);
List<InspectionGadgetsFix> fixes = new ArrayList<>();
if (var instanceof PsiLocalVariable) {
fixes.add(new ReplaceWithStringBuilderFix(var));
fixes.add(new ReplaceWithStringBuilderFix(var, false));
if (needNullSafe) {
fixes.add(new ReplaceWithStringBuilderFix(var, true));
}
PsiLoopStatement loop = getOutermostCommonLoop(expression, var);
// Do not add IntroduceStringBuilderFix if there's only 0 or 1 reference to the variable outside loop:
// in this case the result is usually similar to ReplaceWithStringBuilderFix or worse
if (ReferencesSearch.search(var).findAll().stream()
.map(PsiReference::getElement).filter(e -> !PsiTreeUtil.isAncestor(loop, e, true))
.limit(2).count() > 1) {
fixes.add(new IntroduceStringBuilderFix(var));
fixes.add(new IntroduceStringBuilderFix(var, false));
if (needNullSafe) {
fixes.add(new IntroduceStringBuilderFix(var, true));
}
}
}
else if (var instanceof PsiParameter) {
fixes.add(new IntroduceStringBuilderFix(var));
fixes.add(new IntroduceStringBuilderFix(var, false));
if (needNullSafe) {
fixes.add(new IntroduceStringBuilderFix(var, true));
}
}
return fixes.toArray(InspectionGadgetsFix.EMPTY_ARRAY);
}
private static boolean canBeNull(PsiVariable var) {
PsiExpression initializer = var.getInitializer();
if (initializer != null && NullnessUtil.getExpressionNullness(initializer, true) != Nullness.NOT_NULL) {
return true;
}
Processor<PsiReference> isNotNullableWrite = (PsiReference ref) -> {
if (!(ref instanceof PsiExpression)) return true;
PsiExpression expression = (PsiExpression)ref;
if (!PsiUtil.isOnAssignmentLeftHand(expression)) return true;
PsiAssignmentExpression assignment = PsiTreeUtil.getParentOfType(expression, PsiAssignmentExpression.class);
if (assignment == null || assignment.getOperationTokenType() != JavaTokenType.EQ) return true;
PsiExpression rExpression = assignment.getRExpression();
return rExpression == null || NullnessUtil.getExpressionNullness(rExpression, true) == Nullness.NOT_NULL;
};
boolean notNull = ReferencesSearch.search(var).forEach(isNotNullableWrite);
return !notNull;
}
static abstract class AbstractStringBuilderFix extends InspectionGadgetsFix {
static final Pattern PRINT_OR_PRINTLN = Pattern.compile("print|println");
String myName;
String myTargetType;
final String myName;
final String myTargetType;
final boolean myNullSafe;
Set<PsiExpression> myNullables = Collections.emptySet();
public AbstractStringBuilderFix(PsiVariable variable) {
AbstractStringBuilderFix(PsiVariable variable, boolean nullSafe) {
myName = variable.getName();
myTargetType = PsiUtil.isLanguageLevel5OrHigher(variable) ?
CommonClassNames.JAVA_LANG_STRING_BUILDER : CommonClassNames.JAVA_LANG_STRING_BUFFER;
myNullSafe = nullSafe;
}
@NotNull
@@ -332,10 +363,18 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
}
String text = initializer == null || ExpressionUtils.isLiteral(initializer, "") ? "" : ct.text(initializer);
String stringBuilderText = "new " + myTargetType + "(" + text + ")";
PsiReferenceExpression ref = ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(initializer), PsiReferenceExpression.class);
if (ref != null && ref.getQualifierExpression() == null &&
!Boolean.FALSE.equals(CommonDataflow.getExpressionFact(ref, DfaFactType.CAN_BE_NULL))) {
return ref.getText() + "==null?null:" + stringBuilderText;
if (myNullables.contains(initializer)) {
if (ExpressionUtils.isSimpleExpression(initializer)) {
return initializer.getText() + "==null?null:" + stringBuilderText;
}
if (PsiUtil.isLanguageLevel8OrHigher(initializer)) {
return CommonClassNames.JAVA_UTIL_OPTIONAL +
".ofNullable(" +
initializer.getText() +
").map(" +
myTargetType +
"::new).orElse(null)";
}
}
return stringBuilderText;
}
@@ -355,6 +394,9 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
Query<PsiReference> query =
scope == null ? ReferencesSearch.search(variable) : ReferencesSearch.search(variable, new LocalSearchScope(scope));
Collection<PsiReference> refs = query.findAll();
if (myNullSafe) {
fillNullables(variable, refs);
}
for(PsiReference ref : refs) {
PsiElement target = ref.getElement();
if(target instanceof PsiReferenceExpression && target.isValid() && !skip.test((PsiReferenceExpression)target)) {
@@ -363,6 +405,30 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
}
}
private void fillNullables(PsiVariable variable, Collection<PsiReference> refs) {
if (myNullables instanceof HashSet) return; // already filled
myNullables = new HashSet<>();
PsiExpression initializer = variable.getInitializer();
if (initializer != null && NullnessUtil.getExpressionNullness(initializer, true) != Nullness.NOT_NULL) {
myNullables.add(initializer);
}
for (PsiReference ref : refs) {
if (ref instanceof PsiExpression) {
PsiExpression refExpr = (PsiExpression)ref;
if (NullnessUtil.getExpressionNullness(refExpr, true) != Nullness.NOT_NULL) {
myNullables.add(refExpr);
}
if(PsiUtil.isOnAssignmentLeftHand(refExpr)) {
PsiExpression rExpr =
ExpressionUtils.getAssignmentTo(PsiTreeUtil.getParentOfType(refExpr, PsiAssignmentExpression.class), variable);
if (rExpr != null && NullnessUtil.getExpressionNullness(rExpr, true) != Nullness.NOT_NULL) {
myNullables.add(rExpr);
}
}
}
}
}
private void replace(PsiVariable variable,
PsiVariable builderVariable,
PsiReferenceExpression ref,
@@ -409,7 +475,26 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
}
if (operands.length > 1 && operands[0] == ref && TypeUtils.isJavaLangString(operands[1].getType())) return;
}
ct.replace(ref, builderVariable.getName() + ".toString()");
String text = builderVariable.getName() + ".toString()";
if (myNullables.contains(ref) && !isNotNullContext(ref)) {
text = builderVariable.getName() + "==null?null:" + text;
if (parent instanceof PsiExpression) {
text = "(" + text + ")";
}
}
ct.replace(ref, text);
}
private static boolean isNotNullContext(PsiExpression ref) {
PsiElement parent = PsiUtil.skipParenthesizedExprUp(ref.getParent());
if (!(parent instanceof PsiExpressionList) || !(parent.getParent() instanceof PsiMethodCallExpression)) return false;
int argIndex = IntStreamEx.ofIndices(((PsiExpressionList)parent).getExpressions(), arg -> PsiTreeUtil.isAncestor(arg, ref, false))
.findFirst().orElse(-1);
if (argIndex < 0) return false;
PsiMethod method = ((PsiMethodCallExpression)parent.getParent()).resolveMethod();
if (method == null) return false;
PsiParameter[] parameters = method.getParameterList().getParameters();
return parameters.length > argIndex && NullableNotNullManager.isNotNull(parameters[argIndex]);
}
private static boolean canAcceptBuilderInsteadOfString(PsiMethodCallExpression call) {
@@ -492,16 +577,13 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
StringBuilder replacement =
ChangeToAppendUtil.buildAppendExpression(rValue, false, new StringBuilder(builderName));
if (replacement != null) {
PsiMethodCallExpression append = (PsiMethodCallExpression)ct.replace(assignment, replacement.toString());
while(true) {
PsiMethodCallExpression qualifierCall = MethodCallUtils.getQualifierMethodCall(append);
if (qualifierCall == null) break;
append = qualifierCall;
}
PsiMethodCallExpression result = (PsiMethodCallExpression)ct.replace(assignment, replacement.toString());
PsiMethodCallExpression append = getDeepestQualifierCall(result);
PsiExpression qualifier = append.getMethodExpression().getQualifierExpression();
if (qualifier != null) {
append.replace(qualifier);
}
makeNullSafe(operands[0], result);
}
return;
}
@@ -510,7 +592,7 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
if (ExpressionUtils.isReferenceTo(lastOp, variable)) {
ct.delete(concat.getTokenBeforeOperand(lastOp), lastOp);
replaceAll(variable, builderVariable, rValue, ct);
ct.replace(assignment, builderName + ".insert(0," + ct.text(rValue) + ")");
makeNullSafe(lastOp, (PsiMethodCallExpression)ct.replace(assignment, builderName + ".insert(0," + ct.text(rValue) + ")"));
return;
}
}
@@ -530,16 +612,39 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
replacement = sb.toString();
}
}
ct.replace(assignment, replacement);
makeNullSafe(assignment.getLExpression(), (PsiMethodCallExpression)ct.replace(assignment, replacement));
} else if(assignment.getOperationTokenType().equals(JavaTokenType.EQ)) {
ct.replace(assignment, builderName + "=" + generateNewStringBuilder(rValue, ct));
JavaCodeStyleManager.getInstance(variable.getProject())
.shortenClassReferences(ct.replace(assignment, builderName + "=" + generateNewStringBuilder(rValue, ct)));
}
}
@NotNull
private static PsiMethodCallExpression getDeepestQualifierCall(PsiMethodCallExpression result) {
PsiMethodCallExpression append = result;
while (true) {
PsiMethodCallExpression qualifierCall = MethodCallUtils.getQualifierMethodCall(append);
if (qualifierCall == null) break;
append = qualifierCall;
}
return append;
}
private void makeNullSafe(PsiExpression expression, PsiMethodCallExpression result) {
if (!myNullables.contains(expression)) return;
PsiExpression qualifier = getDeepestQualifierCall(result).getMethodExpression().getQualifierExpression();
if (qualifier == null) return;
String builder = qualifier.getText();
PsiElementFactory factory = JavaPsiFacade.getElementFactory(result.getProject());
qualifier
.replace(factory.createExpressionFromText("(" + builder + "==null?new " + myTargetType + "(\"null\"):" + builder + ")", qualifier));
result.replace(factory.createExpressionFromText(builder + "=" + result.getText(), result));
}
}
static class IntroduceStringBuilderFix extends AbstractStringBuilderFix {
public IntroduceStringBuilderFix(PsiVariable variable) {
super(variable);
public IntroduceStringBuilderFix(PsiVariable variable, boolean nullSafe) {
super(variable, nullSafe);
}
@Override
@@ -551,7 +656,8 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
PsiLoopStatement loop = getOutermostCommonLoop(expression, variable);
if (loop == null) return;
ControlFlowUtils.InitializerUsageStatus status = ControlFlowUtils.getInitializerUsageStatus(variable, loop);
String newName = JavaCodeStyleManager.getInstance(project).suggestUniqueVariableName(variable.getName() + "Builder", loop, true);
JavaCodeStyleManager javaCodeStyleManager = JavaCodeStyleManager.getInstance(project);
String newName = javaCodeStyleManager.suggestUniqueVariableName(variable.getName() + "Builder", loop, true);
String newStringBuilder =
myTargetType + " " + newName + "=new " + myTargetType + "(" + variable.getName() + ");";
PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
@@ -575,7 +681,7 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
// Put original variable declaration after the loop and use its original initializer in StringBuilder constructor
PsiTypeElement typeElement = variable.getTypeElement();
if (typeElement != null && initializer != null) {
ct.replace(builderInitializer, generateNewStringBuilder(initializer, ct));
javaCodeStyleManager.shortenClassReferences(ct.replace(builderInitializer, generateNewStringBuilder(initializer, ct)));
ct.replace(initializer, newName + ".toString()");
toString = variable.getText();
ct.delete(variable);
@@ -584,21 +690,21 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
case AT_WANTED_PLACE_ONLY:
// Move original initializer to the StringBuilder constructor
if (initializer != null) {
ct.replace(builderInitializer, generateNewStringBuilder(initializer, ct));
javaCodeStyleManager.shortenClassReferences(ct.replace(builderInitializer, generateNewStringBuilder(initializer, ct)));
initializer.delete();
}
break;
case AT_WANTED_PLACE:
// Copy original initializer to the StringBuilder constructor if possible
if (ExpressionUtils.isSimpleExpression(initializer)) {
ct.replace(builderInitializer, generateNewStringBuilder(initializer, ct));
javaCodeStyleManager.shortenClassReferences(ct.replace(builderInitializer, generateNewStringBuilder(initializer, ct)));
}
break;
case UNKNOWN:
PsiElement prevStatement = PsiTreeUtil.skipWhitespacesAndCommentsBackward(declaration);
PsiExpression prevAssignment = ExpressionUtils.getAssignmentTo(prevStatement, variable);
if (prevAssignment != null) {
ct.replace(builderInitializer, generateNewStringBuilder(prevAssignment, ct));
javaCodeStyleManager.shortenClassReferences(ct.replace(builderInitializer, generateNewStringBuilder(prevAssignment, ct)));
ct.delete(prevStatement);
}
break;
@@ -611,7 +717,8 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
@NotNull
@Override
public String getName() {
return InspectionGadgetsBundle.message("string.concatenation.introduce.fix.name", myName, StringUtil.getShortName(myTargetType));
return InspectionGadgetsBundle.message("string.concatenation.introduce.fix.name", myName, StringUtil.getShortName(myTargetType))
+ (myNullSafe ? " (null-safe)" : "");
}
@Nls
@@ -623,8 +730,8 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
}
static class ReplaceWithStringBuilderFix extends AbstractStringBuilderFix {
public ReplaceWithStringBuilderFix(PsiVariable variable) {
super(variable);
public ReplaceWithStringBuilderFix(PsiVariable variable, boolean nullSafe) {
super(variable, nullSafe);
}
@Override
@@ -641,7 +748,8 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
ct.replace(typeElement, myTargetType);
PsiExpression initializer = variable.getInitializer();
if (initializer != null) {
ct.replace(initializer, generateNewStringBuilder(initializer, ct));
JavaCodeStyleManager.getInstance(project)
.shortenClassReferences(ct.replace(initializer, generateNewStringBuilder(initializer, ct)));
}
PsiStatement commentPlace = PsiTreeUtil.getParentOfType(variable, PsiStatement.class);
ct.insertCommentsBefore(commentPlace == null ? variable : commentPlace);
@@ -651,7 +759,8 @@ public class StringConcatenationInLoopsInspection extends BaseInspection {
@NotNull
@Override
public String getName() {
return InspectionGadgetsBundle.message("string.concatenation.replace.fix.name", myName, StringUtil.getShortName(myTargetType));
return InspectionGadgetsBundle.message("string.concatenation.replace.fix.name", myName, StringUtil.getShortName(myTargetType))
+ (myNullSafe ? " (null-safe)" : "");
}
@Nls

View File

@@ -6,7 +6,8 @@ it with explicit calls to <b>StringBuilder.append()</b> or <b>StringBuffer.appen
<p>
Sometimes quick-fix actions are available which allow you to convert <code>String</code> variable to <code>StringBuilder</code> or
introduce a new <code>StringBuilder</code>. Be careful if the original code handles <code>null</code> value specially: the replacement
might not be semantically correct after that. Also it's not guaranteed that the automatic replacement will always be more performant.
might not be semantically correct after that. If <code>null</code> value is possible, null-safe fixes are suggested which generate
necessary null-checks. Also it's not guaranteed that the automatic replacement will always be more performant.
</p>
</body>
</html>