From c0f0d3c63679aa2fe06bb6e4944f53aeed5110b6 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Thu, 8 Jun 2017 17:30:20 +0700 Subject: [PATCH] IDEA-174151 'Replace with single Map method' suggests incorrect rewrites to computeIfAbsent Option added (on by default) to disable warning if side effects are possible. --- .../java18api/Java8MapApiInspection.java | 28 +++++++++++++------ .../afterComputeIfAbsentSingleLine.java | 2 +- .../afterComputeIfAbsentUsedKey.java | 2 +- .../java8MapApi/afterMergeSimple.java | 2 +- .../beforeComputeIfAbsentSingleLine.java | 2 +- .../beforeComputeIfAbsentUsedKey.java | 2 +- .../java8MapApi/beforeMergeSimple.java | 2 +- ...hedStringBuilderQueryUpdateInspection.java | 2 +- .../siyeh/ig/psiutils/SideEffectChecker.java | 4 +-- .../inspectionDescriptions/Java8MapApi.html | 3 ++ 10 files changed, 31 insertions(+), 18 deletions(-) diff --git a/java/java-impl/src/com/intellij/codeInspection/java18api/Java8MapApiInspection.java b/java/java-impl/src/com/intellij/codeInspection/java18api/Java8MapApiInspection.java index 1f480ae2ba79..bda102be9a0c 100644 --- a/java/java-impl/src/com/intellij/codeInspection/java18api/Java8MapApiInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/java18api/Java8MapApiInspection.java @@ -52,6 +52,7 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { public boolean mySuggestMapPutIfAbsent = true; public boolean mySuggestMapMerge = true; public boolean myTreatGetNullAsContainsKey = false; + public boolean mySideEffects = false; @Nullable @Override @@ -62,6 +63,7 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { panel.addCheckbox("Suggest conversion to Map.putIfAbsent", "mySuggestMapPutIfAbsent"); panel.addCheckbox("Suggest conversion to Map.merge", "mySuggestMapMerge"); panel.addCheckbox("Treat 'get(k) != null' the same as 'containsKey(k)' (may change semantics)", "myTreatGetNullAsContainsKey"); + panel.addCheckbox("Suggest replacement even if lambda may have side effects", "mySideEffects"); return panel; } @@ -121,7 +123,9 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { if (PsiTreeUtil.collectElements(presentValue, e -> PsiEquivalenceUtil.areElementsEquivalent(e, absentValue)).length == 0) { return; } - condition.register(holder, new ReplaceWithSingleMapOperation("merge", PsiTreeUtil + boolean informationLevel = + !mySideEffects && SideEffectChecker.mayHaveSideEffects(presentValue, ex -> condition.extractGetCall(ex) != null); + condition.register(holder, informationLevel, new ReplaceWithSingleMapOperation("merge", PsiTreeUtil .getParentOfType(absentValue, PsiMethodCallExpression.class), presentValue, noneBranch)); } } @@ -139,11 +143,11 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { condition.isMap(putCall.getMethodExpression().getQualifierExpression())) { PsiExpression[] putArgs = putCall.getArgumentList().getExpressions(); if (putArgs.length != 2 || !condition.isKey(putArgs[0]) || !ExpressionUtils.isSimpleExpression(putArgs[1])) return; - condition.register(holder, new ReplaceWithSingleMapOperation("putIfAbsent", getCall, putArgs[1], result)); + condition.register(holder, false, new ReplaceWithSingleMapOperation("putIfAbsent", getCall, putArgs[1], result)); } if (mySuggestMapGetOrDefault && condition.isContainsKey() && ExpressionUtils.isSimpleExpression(noneExpression) && condition.isMapValueType(noneExpression.getType())) { - condition.register(holder, new ReplaceWithSingleMapOperation("getOrDefault", getCall, noneExpression, result)); + condition.register(holder, false, new ReplaceWithSingleMapOperation("getOrDefault", getCall, noneExpression, result)); } } @@ -159,7 +163,7 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { PsiExpression rValue = assignment.getRExpression(); if (ExpressionUtils.isSimpleExpression(rValue) && condition.isValueReference(assignment.getLExpression()) && !condition.isValueReference(rValue) && condition.isMapValueType(rValue.getType())) { - condition.register(holder, ReplaceWithSingleMapOperation.fromIf("getOrDefault", condition, rValue)); + condition.register(holder, false, ReplaceWithSingleMapOperation.fromIf("getOrDefault", condition, rValue)); } } else if (condition.isGetNull()) { /* @@ -171,25 +175,30 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { */ PsiExpression lambdaCandidate = extractLambdaCandidate(condition, noneBranch); if (lambdaCandidate != null && mySuggestMapComputeIfAbsent) { - condition.register(holder, ReplaceWithSingleMapOperation.fromIf("computeIfAbsent", condition, lambdaCandidate)); + boolean informationLevel = !mySideEffects && SideEffectChecker.mayHaveSideEffects(lambdaCandidate); + condition + .register(holder, informationLevel, ReplaceWithSingleMapOperation.fromIf("computeIfAbsent", condition, lambdaCandidate)); } if (lambdaCandidate == null) { PsiExpression expression = extractPutValue(condition, noneBranch); if(expression != null) { String replacement = null; + boolean informationLevel = false; if (mySuggestMapPutIfAbsent && ExpressionUtils.isSimpleExpression(expression) && !condition.isValueReference(expression)) { replacement = "putIfAbsent"; } else if (mySuggestMapComputeIfAbsent && !condition.hasVariable()) { + informationLevel = !mySideEffects && SideEffectChecker.mayHaveSideEffects(expression); replacement = "computeIfAbsent"; } if(replacement != null) { if(condition.hasVariable()) { - condition.register(holder, ReplaceWithSingleMapOperation.fromIf(replacement, condition, expression)); + condition.register(holder, informationLevel, ReplaceWithSingleMapOperation.fromIf(replacement, condition, expression)); } else { PsiMethodCallExpression call = PsiTreeUtil.getParentOfType(expression, PsiMethodCallExpression.class); LOG.assertTrue(call != null); - condition.register(holder, new ReplaceWithSingleMapOperation(replacement, call, expression, noneBranch)); + condition + .register(holder, informationLevel, new ReplaceWithSingleMapOperation(replacement, call, expression, noneBranch)); } } } @@ -554,9 +563,10 @@ public class Java8MapApiInspection extends BaseJavaBatchLocalInspectionTool { return myFullCondition; } - public void register(ProblemsHolder holder, ReplaceWithSingleMapOperation fix) { + public void register(ProblemsHolder holder, boolean informationLevel, ReplaceWithSingleMapOperation fix) { //noinspection DialogTitleCapitalization - holder.registerProblem(getFullCondition(), QuickFixBundle.message("java.8.map.api.inspection.description", fix.myMethodName), fix); + holder.registerProblem(getFullCondition(), QuickFixBundle.message("java.8.map.api.inspection.description", fix.myMethodName), + informationLevel ? ProblemHighlightType.INFORMATION : ProblemHighlightType.GENERIC_ERROR_OR_WARNING, fix); } public boolean isMapValueType(@Nullable PsiType type) { diff --git a/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentSingleLine.java b/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentSingleLine.java index a6caf3d589d9..9b3c5bcfacf2 100644 --- a/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentSingleLine.java +++ b/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentSingleLine.java @@ -1,4 +1,4 @@ -// "Replace with 'computeIfAbsent' method call" "true" +// "Replace with 'computeIfAbsent' method call" "GENERIC_ERROR_OR_WARNING" import java.util.ArrayList; import java.util.List; import java.util.Map; diff --git a/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentUsedKey.java b/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentUsedKey.java index 7167e9f3f07f..a35a993fe20f 100644 --- a/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentUsedKey.java +++ b/java/java-tests/testData/inspection/java8MapApi/afterComputeIfAbsentUsedKey.java @@ -1,4 +1,4 @@ -// "Replace with 'computeIfAbsent' method call" "true" +// "Replace with 'computeIfAbsent' method call" "INFORMATION" import java.util.ArrayList; import java.util.List; import java.util.Map; diff --git a/java/java-tests/testData/inspection/java8MapApi/afterMergeSimple.java b/java/java-tests/testData/inspection/java8MapApi/afterMergeSimple.java index 38fddca37cbd..06b240d35435 100644 --- a/java/java-tests/testData/inspection/java8MapApi/afterMergeSimple.java +++ b/java/java-tests/testData/inspection/java8MapApi/afterMergeSimple.java @@ -1,4 +1,4 @@ -// "Replace with 'merge' method call" "true" +// "Replace with 'merge' method call" "GENERIC_ERROR_OR_WARNING" import java.util.Map; public class Main { diff --git a/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentSingleLine.java b/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentSingleLine.java index 96071e187676..0c97d5f5694a 100644 --- a/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentSingleLine.java +++ b/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentSingleLine.java @@ -1,4 +1,4 @@ -// "Replace with 'computeIfAbsent' method call" "true" +// "Replace with 'computeIfAbsent' method call" "GENERIC_ERROR_OR_WARNING" import java.util.ArrayList; import java.util.List; import java.util.Map; diff --git a/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentUsedKey.java b/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentUsedKey.java index 3b0c3f0b3025..4ed73df5ef2d 100644 --- a/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentUsedKey.java +++ b/java/java-tests/testData/inspection/java8MapApi/beforeComputeIfAbsentUsedKey.java @@ -1,4 +1,4 @@ -// "Replace with 'computeIfAbsent' method call" "true" +// "Replace with 'computeIfAbsent' method call" "INFORMATION" import java.util.ArrayList; import java.util.List; import java.util.Map; diff --git a/java/java-tests/testData/inspection/java8MapApi/beforeMergeSimple.java b/java/java-tests/testData/inspection/java8MapApi/beforeMergeSimple.java index e4933d8bb334..4c121c191bcc 100644 --- a/java/java-tests/testData/inspection/java8MapApi/beforeMergeSimple.java +++ b/java/java-tests/testData/inspection/java8MapApi/beforeMergeSimple.java @@ -1,4 +1,4 @@ -// "Replace with 'merge' method call" "true" +// "Replace with 'merge' method call" "GENERIC_ERROR_OR_WARNING" import java.util.Map; public class Main { diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java index 358c122d33bb..0a068f2a260a 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MismatchedStringBuilderQueryUpdateInspection.java @@ -270,7 +270,7 @@ public class MismatchedStringBuilderQueryUpdateInspection extends BaseInspection if (hasReferenceToVariable(variable, qualifierExpression)) { PsiElement parent = PsiTreeUtil.getParentOfType(expression, PsiStatement.class, PsiLambdaExpression.class); if (parent instanceof PsiStatement && - !SideEffectChecker.mayHaveSideEffects((PsiStatement)parent, this::isSideEffectFreeBuilderMethodCall)) { + !SideEffectChecker.mayHaveSideEffects(parent, this::isSideEffectFreeBuilderMethodCall)) { return; } queried = true; diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java index da50102920fa..661b2ce4af8b 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/SideEffectChecker.java @@ -65,9 +65,9 @@ public class SideEffectChecker { return visitor.mayHaveSideEffects(); } - public static boolean mayHaveSideEffects(@NotNull PsiStatement statement, Predicate shouldIgnoreCall) { + public static boolean mayHaveSideEffects(@NotNull PsiElement element, Predicate shouldIgnoreCall) { final SideEffectsVisitor visitor = new SideEffectsVisitor(null, shouldIgnoreCall); - statement.accept(visitor); + element.accept(visitor); return visitor.mayHaveSideEffects(); } diff --git a/resources-en/src/inspectionDescriptions/Java8MapApi.html b/resources-en/src/inspectionDescriptions/Java8MapApi.html index fccdc8d4cff3..261992dc44ce 100644 --- a/resources-en/src/inspectionDescriptions/Java8MapApi.html +++ b/resources-en/src/inspectionDescriptions/Java8MapApi.html @@ -28,6 +28,9 @@ Reports calls to Map.get() which could be replaced with getOr +

Note that replacement with computeIfAbsent() or merge() may work incorrectly for some Map +implementations if the code extracted to lambda expression modifies the same Map. By default, +warning is not issued if this code may have side effects. If desired, use the last checkbox to issue warning always.

This inspection only reports if the project or module is configured to use a language level of 8 or higher.

New in 2016.3