From 1f8341c8828f981eef8718b4d8fd09527d29f9fe Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 25 Oct 2017 16:40:30 +0700 Subject: [PATCH] DataFlowInspection: specific message for ioobe contracts, messages moved to resources Fixes IDEA-180501 Confusing warning about list get method --- .../dataFlow/ContractValue.java | 20 +++++++++++++++++++ .../dataFlow/DataFlowInspectionBase.java | 19 +++++++++++++----- .../dataFlow/fixture/ArrayLength.java | 2 +- .../dataFlow/fixture/CustomContracts.java | 2 +- .../fixture/ForEachOverEmptyCollection.java | 2 +- .../fixture/LongRangeKnownMethods.java | 4 ++-- .../src/messages/InspectionsBundle.properties | 2 ++ 7 files changed, 41 insertions(+), 10 deletions(-) diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java index 813358b8f9de..cabde8c540fc 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java @@ -33,6 +33,13 @@ public abstract class ContractValue { abstract DfaValue makeDfaValue(DfaValueFactory factory, DfaCallArguments arguments); + /** + * @return true if this contract value represents a bounds-checking condition + */ + boolean isBoundCheckingCondition() { + return false; + } + public static ContractValue qualifier() { return Qualifier.INSTANCE; } @@ -160,6 +167,19 @@ public abstract class ContractValue { myRelationType = type; } + @Override + boolean isBoundCheckingCondition() { + switch (myRelationType) { + case LE: + case LT: + case GE: + case GT: + return true; + default: + return false; + } + } + @Override DfaValue makeDfaValue(DfaValueFactory factory, DfaCallArguments arguments) { return factory.createCondition(myLeft.makeDfaValue(factory, arguments), myRelationType, myRight.makeDfaValue(factory, arguments)); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java index 99ad6247c391..2fe5b75c41c5 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java @@ -393,13 +393,21 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool private static void reportAlwaysFailingCalls(ProblemsHolder holder, DataFlowInstructionVisitor visitor, HashSet reportedAnchors) { - for (PsiCall call : visitor.getAlwaysFailingCalls()) { - if (TestUtils.isExceptionExpected(call)) continue; + visitor.getAlwaysFailingCalls().forEach((call, contracts) -> { + if (TestUtils.isExceptionExpected(call)) return; PsiMethod method = call.resolveMethod(); if (method != null && reportedAnchors.add(call)) { - holder.registerProblem(getElementToHighlight(call), "The call to '#ref' always fails, according to its method contracts"); + holder.registerProblem(getElementToHighlight(call), getContractMessage(contracts)); } + }); + } + + @NotNull + private static String getContractMessage(List contracts) { + if (contracts.stream().allMatch(mc -> mc.getConditions().stream().allMatch(cv -> cv.isBoundCheckingCondition()))) { + return InspectionsBundle.message("dataflow.message.contract.fail.index"); } + return InspectionsBundle.message("dataflow.message.contract.fail"); } @NotNull private static PsiElement getElementToHighlight(@NotNull PsiCall call) { @@ -965,8 +973,9 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return myOptionalQualifiers; } - Collection getAlwaysFailingCalls() { - return StreamEx.ofKeys(myFailingCalls, v -> v).map(MethodCallInstruction::getCallExpression).toList(); + Map> getAlwaysFailingCalls() { + return StreamEx.ofKeys(myFailingCalls, v -> v) + .mapToEntry(MethodCallInstruction::getCallExpression, MethodCallInstruction::getContracts).toMap(); } boolean isAlwaysReturnsNotNull(Instruction[] instructions) { diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ArrayLength.java b/java/java-tests/testData/inspection/dataFlow/fixture/ArrayLength.java index 6a4f3ce92f8a..2250766f8d45 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ArrayLength.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ArrayLength.java @@ -26,6 +26,6 @@ public final class ArrayLength { System.out.println("Impossible"); } Arrays.fill(x, -1); - Arrays.fill(x, -1, -1, -1); + Arrays.fill(x, -1, -1, -1); } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/CustomContracts.java b/java/java-tests/testData/inspection/dataFlow/fixture/CustomContracts.java index 942a7c3173bd..23a68ff1ed3c 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/CustomContracts.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/CustomContracts.java @@ -1,6 +1,6 @@ public class CustomContracts { public void testSubstring(String s) { - if (s.substring(-1).length() == 0) { + if (s.substring(-1).length() == 0) { System.out.println("Oops"); } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java b/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java index d346355acab7..0990c3fa1f60 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java @@ -46,7 +46,7 @@ public class ForEachOverEmptyCollection { } if(!hasItem) { System.out.println( - list.get(max == null ? 0 : 1)); + list.get(max == null ? 0 : 1)); } } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/LongRangeKnownMethods.java b/java/java-tests/testData/inspection/dataFlow/fixture/LongRangeKnownMethods.java index 54d1a5634956..278935eaefc0 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/LongRangeKnownMethods.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/LongRangeKnownMethods.java @@ -196,13 +196,13 @@ public class LongRangeKnownMethods { void testEmptyListGet(List list) { if (list.isEmpty()) { - System.out.println(list.get(0)); + System.out.println(list.get(0)); } } void testBoundError(List list) { if (list.size() < 10) { - System.out.println(list.get(10)); + System.out.println(list.get(10)); } } diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 3d10baed8a29..44f93fa276f3 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -64,6 +64,8 @@ dataflow.message.npe.field.access=Dereference of #ref #loc may prod dataflow.message.cce=Casting {0} to #ref #loc may produce java.lang.ClassCastException dataflow.message.arraystore=Storing element of type {0} to array of {1} elements may produce java.lang.ArrayStoreException dataflow.message.redundant.instanceof=Condition #ref #loc is redundant and can be replaced with != null +dataflow.message.contract.fail=The call to '#ref' always fails, according to its method contracts +dataflow.message.contract.fail.index=The call to '#ref' always fails as index is out of bounds dataflow.message.constant.condition=Condition #ref #loc is always {0} dataflow.message.constant.condition.when.reached=Condition #ref #loc is always {0} when reached dataflow.message.loop.on.empty.array=Array #ref is always empty