diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java index c6708934201b..33b97675f191 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java @@ -5,27 +5,27 @@ import com.intellij.codeInspection.dataFlow.instructions.MethodCallInstruction; import com.intellij.codeInspection.dataFlow.instructions.PushInstruction; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; import com.intellij.codeInspection.dataFlow.value.DfaValue; -import com.intellij.ide.PowerSaveMode; import com.intellij.psi.*; -import com.intellij.psi.util.CachedValueProvider; -import com.intellij.psi.util.CachedValuesManager; -import com.intellij.psi.util.PsiModificationTracker; -import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.*; import com.intellij.util.ObjectUtils; +import com.siyeh.ig.psiutils.ExpressionUtils; import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.HashMap; import java.util.Map; public class CommonDataflow { - static class DataflowResult { + /** + * Represents the result of dataflow applied to some code fragment (usually a method) + */ + public static class DataflowResult { private final Map myFacts = new HashMap<>(); - void add(PsiExpression expression, DfaMemoryStateImpl memState) { + void add(PsiExpression expression, DfaMemoryStateImpl memState, DfaValue value) { DfaFactMap existing = myFacts.get(expression); if(existing != DfaFactMap.EMPTY) { - DfaValue value = memState.peek(); DfaFactMap newMap = memState.getFactMap(value); if (!Boolean.FALSE.equals(newMap.get(DfaFactType.CAN_BE_NULL)) && memState.isNotNull(value)) { newMap = newMap.with(DfaFactType.CAN_BE_NULL, false); @@ -33,6 +33,33 @@ public class CommonDataflow { myFacts.put(expression, existing == null ? newMap : existing.union(newMap)); } } + + /** + * Returns true if given expression was visited by dataflow. Note that dataflow usually tracks deparenthesized expressions only, + * so you should deparenthesize it in advance if necessary. + * + * @param expression expression to check + * @return true if given expression was visited by dataflow. + * If false is returned, it's possible that the expression exists in unreachable branch or this expression is not tracked due to + * the dataflow implementation details. + */ + public boolean expressionWasAnalyzed(PsiExpression expression) { + return myFacts.containsKey(expression); + } + + /** + * Returns a fact of specific type which is known for given expression or null if fact is not known + * + * @param expression expression to get the fact + * @param type a fact type + * @param resulting type + * @return a fact value or null if fact of given type is not known for given expression + */ + @Nullable + public T getExpressionFact(PsiExpression expression, DfaFactType type) { + DfaFactMap map = this.myFacts.get(expression); + return map == null ? null : map.get(type); + } } @Contract("null -> null") @@ -49,23 +76,43 @@ public class CommonDataflow { PsiExpression place = instruction.getPlace(); if (place != null && !instruction.isReferenceWrite()) { for (DfaInstructionState state : states) { - dfr.add(place, (DfaMemoryStateImpl)state.getMemoryState()); + DfaMemoryState afterState = state.getMemoryState(); + dfr.add(place, (DfaMemoryStateImpl)afterState, instruction.getValue()); } } return states; } + @NotNull + @Override + protected DfaCallArguments popCall(MethodCallInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + boolean contractOnly) { + DfaCallArguments arguments = super.popCall(instruction, runner, memState, contractOnly); + PsiElement context = instruction.getContext(); + if (instruction.getMethodType() == MethodCallInstruction.MethodType.REGULAR_METHOD_CALL && + context instanceof PsiMethodCallExpression) { + PsiExpression qualifier = + PsiUtil.skipParenthesizedExprDown(((PsiMethodCallExpression)context).getMethodExpression().getQualifierExpression()); + if (qualifier != null) { + dfr.add(qualifier, (DfaMemoryStateImpl)memState, arguments.myQualifier); + } + } + return arguments; + } + @Override public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); PsiExpression context = ObjectUtils.tryCast(instruction.getContext(), PsiExpression.class); - if (context != null) { + if (context != null && ExpressionUtils.getCallForQualifier(context) == null) { for (DfaInstructionState state : states) { DfaValue value = state.getMemoryState().peek(); if(value != fail) { - dfr.add(context, (DfaMemoryStateImpl)state.getMemoryState()); + dfr.add(context, (DfaMemoryStateImpl)state.getMemoryState(), state.getMemoryState().peek()); } } } @@ -76,9 +123,13 @@ public class CommonDataflow { return result == RunnerResult.OK ? dfr : null; } - private static DataflowResult getDataflowResult(PsiElement context) { - // Disable common dataflow in powersave mode - if(PowerSaveMode.isEnabled()) return null; + /** + * Returns the dataflow result for code fragment which contains given context + * @param context a context to get the dataflow result + * @return the dataflow result or null if dataflow cannot be launched for this context (e.g. we are inside too complex method) + */ + @Nullable + public static DataflowResult getDataflowResult(PsiExpression context) { PsiMember member = PsiTreeUtil.getParentOfType(context, PsiMember.class); if(!(member instanceof PsiMethod) && !(member instanceof PsiField) && !(member instanceof PsiClassInitializer)) return null; PsiElement body = member instanceof PsiMethod ? ((PsiMethod)member).getBody() : member.getContainingClass(); @@ -100,7 +151,6 @@ public class CommonDataflow { public static T getExpressionFact(PsiExpression expression, DfaFactType type) { DataflowResult result = getDataflowResult(expression); if (result == null) return null; - DfaFactMap map = result.myFacts.get(expression); - return map == null ? null : map.get(type); + return result.getExpressionFact(expression, type); } } 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 34149ae58f70..ce5d4c91dce3 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 @@ -5,7 +5,6 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; import com.intellij.codeInsight.ExpressionUtil; import com.intellij.codeInsight.NullableNotNullManager; -import com.intellij.codeInsight.PsiEquivalenceUtil; import com.intellij.codeInsight.daemon.GroupNames; import com.intellij.codeInsight.intention.AddAnnotationPsiFix; import com.intellij.codeInspection.*; @@ -54,7 +53,6 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool public boolean REPORT_CONSTANT_REFERENCE_VALUES = true; public boolean REPORT_NULLS_PASSED_TO_NOT_NULL_PARAMETER = true; public boolean REPORT_NULLABLE_METHODS_RETURNING_NOT_NULL = true; - public boolean REPORT_UNCHECKED_OPTIONALS = true; @Override public JComponent createOptionsPanel() { @@ -80,9 +78,6 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool if (!REPORT_NULLABLE_METHODS_RETURNING_NOT_NULL) { node.addContent(new Element("option").setAttribute("name", "REPORT_NULLABLE_METHODS_RETURNING_NOT_NULL").setAttribute("value", "false")); } - if (!REPORT_UNCHECKED_OPTIONALS) { - node.addContent(new Element("option").setAttribute("name", "REPORT_UNCHECKED_OPTIONALS").setAttribute("value", "false")); - } } @Override @@ -262,8 +257,6 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool reportOptionalOfNullableImprovements(holder, reportedAnchors, visitor.getOfNullableCalls()); - reportUncheckedOptionalGet(holder, visitor.getOptionalCalls(), visitor.getOptionalQualifiers()); - visitor.getBooleanCalls().forEach((call, state) -> { if (state != ThreeState.UNSURE && reportedAnchors.add(call)) { reportConstantCondition(holder, call, state.toBoolean()); @@ -400,33 +393,6 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool }); } - private void reportUncheckedOptionalGet(ProblemsHolder holder, - Map calls, - List qualifiers) { - if (!REPORT_UNCHECKED_OPTIONALS) return; - for (Map.Entry entry : calls.entrySet()) { - ThreeState state = entry.getValue(); - if (state != ThreeState.UNSURE) continue; - PsiMethodCallExpression call = entry.getKey(); - PsiMethod method = call.resolveMethod(); - if (method == null) continue; - PsiClass optionalClass = method.getContainingClass(); - if (optionalClass == null) continue; - PsiExpression qualifier = PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()); - if (qualifier instanceof PsiMethodCallExpression && - qualifiers.stream().anyMatch(q -> PsiEquivalenceUtil.areElementsEquivalent(q, qualifier))) { - // Conservatively do not report methodCall().get() cases if methodCall().isPresent() was found in the same method - // without deep correspondence analysis - continue; - } - LocalQuickFix fix = holder.isOnTheFly() ? new SetInspectionOptionFix(this, "REPORT_UNCHECKED_OPTIONALS", InspectionsBundle - .message("inspection.data.flow.turn.off.unchecked.optional.get.quickfix"), false) : null; - holder.registerProblem(getElementToHighlight(call), - InspectionsBundle.message("dataflow.message.optional.get.without.is.present", optionalClass.getName()), - fix); - } - } - private void reportAlwaysReturnsNotNull(ProblemsHolder holder, PsiElement scope) { if (!(scope.getParent() instanceof PsiMethod)) return; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java index 36ca6d1620d1..2c9b5b2a385f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java @@ -5,12 +5,10 @@ import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.value.*; import com.intellij.openapi.util.Pair; import com.intellij.psi.*; -import com.intellij.psi.util.PsiUtil; import com.intellij.util.ObjectUtils; import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.MultiMap; -import com.siyeh.ig.psiutils.TypeUtils; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -23,13 +21,11 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { private final Map, StateInfo> myStateInfos = new LinkedHashMap<>(); private final Set myCCEInstructions = ContainerUtil.newHashSet(); private final Map myFailingCalls = new HashMap<>(); - private final Map myOptionalCalls = new HashMap<>(); private final Map myBooleanCalls = new HashMap<>(); private final Map myOfNullableCalls = new HashMap<>(); private final Map> myArrayStoreProblems = new HashMap<>(); private final Map myMethodReferenceResults = new HashMap<>(); private final Map myOutOfBoundsArrayAccesses = new HashMap<>(); - private final List myOptionalQualifiers = new ArrayList<>(); private final MultiMap myPossibleVariableValues = MultiMap.createSet(); private final Set myReceiverMutabilityViolation = new HashSet<>(); private final Set myArgumentMutabilityViolation = new HashSet<>(); @@ -87,10 +83,6 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return myArrayStoreProblems; } - Map getOptionalCalls() { - return myOptionalCalls; - } - Map getOfNullableCalls() { return myOfNullableCalls; } @@ -115,10 +107,6 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return StreamEx.ofKeys(myOutOfBoundsArrayAccesses, ThreeState.YES::equals); } - List getOptionalQualifiers() { - return myOptionalQualifiers; - } - Map> getAlwaysFailingCalls() { return StreamEx.ofKeys(myFailingCalls, v -> v) .mapToEntry(MethodCallInstruction::getCallExpression, MethodCallInstruction::getContracts).toMap(); @@ -133,21 +121,6 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - PsiMethodCallExpression call = ObjectUtils.tryCast(instruction.getCallExpression(), PsiMethodCallExpression.class); - if (call != null) { - String methodName = call.getMethodExpression().getReferenceName(); - PsiExpression qualifier = PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()); - if (qualifier != null && TypeUtils.isOptional(qualifier.getType())) { - if ("isPresent".equals(methodName) && qualifier instanceof PsiMethodCallExpression) { - myOptionalQualifiers.add(qualifier); - } - else if (DfaOptionalSupport.isOptionalGetMethodName(methodName)) { - Boolean fact = memState.getValueFact(memState.peek(), DfaFactType.OPTIONAL_PRESENCE); - ThreeState state = fact == null ? ThreeState.UNSURE : ThreeState.fromBoolean(fact); - myOptionalCalls.merge(call, state, ThreeState::merge); - } - } - } if (instruction.matches(DfaOptionalSupport.OPTIONAL_OF_NULLABLE)) { DfaValue arg = memState.peek(); ThreeState nullArg = memState.isNull(arg) ? ThreeState.YES : memState.isNotNull(arg) ? ThreeState.NO : ThreeState.UNSURE; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java index b13b1a33a61b..2d2ff5bcbd9c 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java @@ -81,7 +81,7 @@ public class DfaOptionalSupport { return new ReplaceOptionalCallFix("of", false); } - static boolean isOptionalGetMethodName(String name) { + public static boolean isOptionalGetMethodName(String name) { return "get".equals(name) || "getAsDouble".equals(name) || "getAsInt".equals(name) || "getAsLong".equals(name); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index c1a14712d7e7..55f87732daf0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java @@ -337,10 +337,10 @@ public class StandardInstructionVisitor extends InstructionVisitor { } @NotNull - private DfaCallArguments popCall(MethodCallInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - boolean contractOnly) { + protected DfaCallArguments popCall(MethodCallInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + boolean contractOnly) { PsiMethod method = instruction.getTargetMethod(); MutationSignature sig = MutationSignature.fromMethod(method); DfaValue[] argValues = popCallArguments(instruction, runner, memState, contractOnly, sig); diff --git a/java/java-impl/src/META-INF/JavaPlugin.xml b/java/java-impl/src/META-INF/JavaPlugin.xml index 3d8311bf4864..44664f0e5fb4 100644 --- a/java/java-impl/src/META-INF/JavaPlugin.xml +++ b/java/java-impl/src/META-INF/JavaPlugin.xml @@ -531,6 +531,11 @@ groupKey="group.names.code.style.issues" enabledByDefault="true" level="WARNING" implementationClass="com.intellij.codeInspection.OptionalIsPresentInspection" displayName="Replace Optional.isPresent() checks with functional-style expressions"/> + REPORT_NULLABLE_METHODS_RETURNING_NOT_NULL = box.isSelected()); - myReportUncheckedOptionals = createCheckBoxWithHTML( - "Report Optional.get() calls without previous isPresent check", - REPORT_UNCHECKED_OPTIONALS, box -> REPORT_UNCHECKED_OPTIONALS = box.isSelected()); - gc.insets = JBUI.emptyInsets(); gc.gridy = 0; add(mySuggestNullables, gc); @@ -251,9 +246,6 @@ public class DataFlowInspection extends DataFlowInspectionBase { gc.gridy++; add(myReportNullableMethodsReturningNotNull, gc); - - gc.gridy++; - add(myReportUncheckedOptionals, gc); } @Override diff --git a/java/java-impl/src/com/intellij/codeInspection/java18api/OptionalGetWithoutIsPresentInspection.java b/java/java-impl/src/com/intellij/codeInspection/java18api/OptionalGetWithoutIsPresentInspection.java new file mode 100644 index 000000000000..bc4a56f98c70 --- /dev/null +++ b/java/java-impl/src/com/intellij/codeInspection/java18api/OptionalGetWithoutIsPresentInspection.java @@ -0,0 +1,63 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.codeInspection.java18api; + +import com.intellij.codeInsight.PsiEquivalenceUtil; +import com.intellij.codeInspection.AbstractBaseJavaLocalInspectionTool; +import com.intellij.codeInspection.InspectionsBundle; +import com.intellij.codeInspection.ProblemsHolder; +import com.intellij.codeInspection.dataFlow.CommonDataflow; +import com.intellij.codeInspection.dataFlow.DfaFactType; +import com.intellij.codeInspection.dataFlow.DfaOptionalSupport; +import com.intellij.psi.*; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiUtil; +import com.siyeh.ig.psiutils.TypeUtils; +import org.jetbrains.annotations.NotNull; + +public class OptionalGetWithoutIsPresentInspection extends AbstractBaseJavaLocalInspectionTool { + @NotNull + @Override + public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) { + return new JavaElementVisitor() { + @Override + public void visitMethodCallExpression(PsiMethodCallExpression call) { + PsiElement nameElement = call.getMethodExpression().getReferenceNameElement(); + if (nameElement == null) return; + String methodName = nameElement.getText(); + PsiExpression qualifier = PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()); + if (qualifier == null) return; + PsiClass optionalClass = PsiUtil.resolveClassInClassTypeOnly(qualifier.getType()); + if (optionalClass == null) return; + if (DfaOptionalSupport.isOptionalGetMethodName(methodName) && + call.getArgumentList().isEmpty() && + TypeUtils.isOptional(optionalClass)) { + CommonDataflow.DataflowResult result = CommonDataflow.getDataflowResult(qualifier); + if (result != null && + result.expressionWasAnalyzed(qualifier) && + result.getExpressionFact(qualifier, DfaFactType.OPTIONAL_PRESENCE) == null && + !isPresentCallWithSameQualifierExists(qualifier)) { + holder.registerProblem(nameElement, + InspectionsBundle.message("inspection.optional.get.without.is.present.message", optionalClass.getName())); + } + } + } + + public boolean isPresentCallWithSameQualifierExists(PsiExpression qualifier) { + // Conservatively skip the results of method calls if there's an isPresent() call with the same qualifier in the method + if (qualifier instanceof PsiMethodCallExpression) { + PsiElement context = PsiTreeUtil.getParentOfType(qualifier, PsiMember.class, PsiLambdaExpression.class); + if (context != null) { + return !PsiTreeUtil.processElements(context, e -> { + if (e == qualifier || !(e instanceof PsiMethodCallExpression)) return true; + PsiMethodCallExpression call = (PsiMethodCallExpression)e; + if (!"isPresent".equals(call.getMethodExpression().getReferenceName()) || !call.getArgumentList().isEmpty()) return true; + PsiExpression isPresentQualifier = call.getMethodExpression().getQualifierExpression(); + return isPresentQualifier == null || !PsiEquivalenceUtil.areElementsEquivalent(qualifier, isPresentQualifier); + }); + } + } + return false; + } + }; + } +} diff --git a/java/java-impl/src/inspectionDescriptions/OptionalGetWithoutIsPresent.html b/java/java-impl/src/inspectionDescriptions/OptionalGetWithoutIsPresent.html new file mode 100644 index 000000000000..9385b595bf0c --- /dev/null +++ b/java/java-impl/src/inspectionDescriptions/OptionalGetWithoutIsPresent.html @@ -0,0 +1,6 @@ + + +

Reports when Optional.get() method is called without previous checking that optional is definitely not empty.

+ + + \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java index 05a70685824a..522662b6c9a7 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java @@ -16,4 +16,136 @@ class Test { System.out.println(test.get()); } } + + void m(Optional maybe) { + if (!!!maybe.isPresent()) { + maybe = getIntegerOptional(); + } + else { + System.out.println(maybe.get()); + maybe = getIntegerOptional(); + } + if (maybe.isPresent()) { + maybe = Optional.empty(); + System.out.println(maybe.get()); + } + boolean b = ((maybe.isPresent())) && maybe.get() == 1; + boolean c = (!maybe.isPresent()) || maybe.get() == 1; + Integer value = !maybe.isPresent() ? 0 : maybe.get(); + } + + Optional getIntegerOptional() { + return Math.random() > 0.5 ? Optional.of(1) : Optional.empty(); + } + + private static void a() { + Optional optional = Optional.empty(); + final boolean present = optional.isPresent(); + // optional = Optional.empty(); + if (present) { + final String string = optional.get(); + System.out.println(string); + } + } + + private static void b() { + Optional optional = Optional.empty(); + final boolean present = optional.isPresent(); + optional = Optional.empty(); + if (present) { + final String string = optional.get(); + System.out.println(string); + } + } + + public void testMultiVars(Optional opt) { + boolean present = opt.isPresent(); + boolean absent = !present; + boolean otherAbsent = !!absent; + if(otherAbsent) { + System.out.println(opt.get()); + } else { + System.out.println(opt.get()); + } + } + + public static Optional getOptional() { + return Optional.empty(); + } + + private void checkAsserts1() { + Optional o2 = getOptional(); + org.junit.Assert.assertTrue(!o2.isPresent()); + System.out.println(o2.get()); + } + + private void checkAsserts2() { + Optional o3 = Optional.empty(); + org.testng.Assert.assertTrue(o3.isPresent()); + System.out.println(o3.get()); + } + + private void checkOf(boolean b) { + System.out.println(Optional.of("xyz").get()); + Optional test; + if(b) { + test = Optional.empty(); + } else { + test = Optional.empty(); + } + System.out.println(test.get()); + + } + + public static String demo() { + Optional holder = Optional.empty(); + + if (! holder.isPresent()) { + holder = Optional.of("hello world"); + if (!holder.isPresent()) { + return null; + } + } + + return holder.get(); + } + + void order(Optional order, boolean b) { + order.ifPresent(o -> System.out.println(order.get())); + System.out.println(order.orElseGet(() -> order.get().trim())); + } + + void guavaTest(com.google.common.base.Optional opt, String s, String s1) { + System.out.println(opt.get()); + if(opt.isPresent()) { + System.out.println(opt.get()); + } + opt = com.google.common.base.Optional.fromNullable(s); + if(opt.isPresent()) { + System.out.println(opt.get()); + } + opt = com.google.common.base.Optional.of(s); + opt = com.google.common.base.Optional.of(s1); + if(opt.isPresent()) { + System.out.println(opt.get()); + } + opt = com.google.common.base.Optional.absent(); + if(opt.isPresent()) { + System.out.println(opt.get()); + } + } + + void testThrow2(Optional test) { + test.orElseThrow(RuntimeException::new); + if (test.isPresent()) { + System.out.println("Yes"); + } + } + + public void testThrowFail(Optional arg) { + if(!arg.isPresent()) { + System.out.println(arg.orElseThrow(IllegalAccessError::new)); + } + String res = Optional.empty().orElseThrow(RuntimeException::new); + } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java b/java/java-tests/testData/inspection/optionalGet/OptionalGet.java similarity index 58% rename from java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java rename to java/java-tests/testData/inspection/optionalGet/OptionalGet.java index f8e87467cc92..1bb0b2374ab7 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java +++ b/java/java-tests/testData/inspection/optionalGet/OptionalGet.java @@ -1,21 +1,30 @@ -/* - * Copyright 2000-2017 JetBrains s.r.o. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ import java.util.*; +import java.util.stream.Stream; -class OptionalWithoutIsPresent { +class OptionalGet { + private void checkOf(boolean b) { + System.out.println(Optional.of("xyz").get()); + Optional test; + if(b) { + test = Optional.of("x"); + } else { + test = Optional.of("y"); + } + System.out.println(test.get()); + if(b) { + test = Optional.of("x"); + } else { + test = Optional.empty(); + } + System.out.println(test.get()); + + } + + private void checkOfNullable(String value) { + System.out.println(Optional.ofNullable(value).get()); + System.out.println(Optional.ofNullable(value+"a").get()); + System.out.println(Optional.ofNullable("xyz").get()); + } void testSimple(Optional o, OptionalDouble od, OptionalInt oi, OptionalLong ol) { System.out.println(o.get()); @@ -24,6 +33,27 @@ class OptionalWithoutIsPresent { System.out.println(od.getAsDouble()); } + void testParentheses(Optional o, String value, String value2) { + System.out.println((o).get()); + System.out.println((Optional.ofNullable(value)).get()); + System.out.println((Optional.of(value)).get()); + System.out.println((Optional.ofNullable("foo")).get()); + System.out.println((Optional.of("foo")).get()); + } + + void testTernary(Optional foo, Optional bar, boolean b) { + if(bar.isPresent()) { + if(foo.isPresent()) { + System.out.println((b ? foo : bar).get()); + } + System.out.println((b ? foo : bar).get()); + } + if(foo.isPresent()) { + System.out.println((b ? foo : bar).get()); + } + System.out.println((b ? foo : bar).get()); + } + { System.out.println(getIntegerOptional().get()); } @@ -72,32 +102,16 @@ class OptionalWithoutIsPresent { } } - void m(Optional maybe) { - if (!!!maybe.isPresent()) { - maybe = getIntegerOptional(); - } - else { - System.out.println(maybe.get()); - maybe = getIntegerOptional(); - } - if (maybe.isPresent()) { - maybe = Optional.empty(); - System.out.println(maybe.get()); - } - boolean b = ((maybe.isPresent())) && maybe.get() == 1; - boolean c = (!maybe.isPresent()) || maybe.get() == 1; - Integer value = !maybe.isPresent() ? 0 : maybe.get(); - } - Optional getIntegerOptional() { return Math.random() > 0.5 ? Optional.of(1) : Optional.empty(); } private static void a() { Optional optional = Optional.empty(); - final boolean present = optional.isPresent(); + final boolean present = optional.isPresent(); // optional = Optional.empty(); - if (present) { + if (present) { + // do not warn here as the branch is unreachable final String string = optional.get(); System.out.println(string); } @@ -105,25 +119,15 @@ class OptionalWithoutIsPresent { private static void b() { Optional optional = Optional.empty(); - final boolean present = optional.isPresent(); + final boolean present = optional.isPresent(); optional = Optional.empty(); - if (present) { + if (present) { + // do not warn here as the branch is unreachable final String string = optional.get(); System.out.println(string); } } - public void testMultiVars(Optional opt) { - boolean present = opt.isPresent(); - boolean absent = !present; - boolean otherAbsent = !!absent; - if(otherAbsent) { - System.out.println(opt.get()); - } else { - System.out.println(opt.get()); - } - } - private void checkReassign(Optional a, Optional b) { if(a.isPresent()) { b = a; @@ -148,66 +152,13 @@ class OptionalWithoutIsPresent { Optional o3 = getOptional(); org.testng.Assert.assertTrue(o3.isPresent()); System.out.println(o3.get()); - - o2 = getOptional(); - org.junit.Assert.assertTrue(!o2.isPresent()); - System.out.println(o2.get()); } - private void checkAsserts2() { - Optional o3 = Optional.empty(); - org.testng.Assert.assertTrue(o3.isPresent()); - System.out.println(o3.get()); + public Collection findCategories(Long articleId) { + return java.util.stream.Stream.of(Optional.of("asdf")).filter(Optional::isPresent).map(x -> x.get()).collect( + java.util.stream.Collectors.toList()) ; } - private void checkOf(boolean b) { - System.out.println(Optional.of("xyz").get()); - Optional test; - if(b) { - test = Optional.of("x"); - } else { - test = Optional.of("y"); - } - System.out.println(test.get()); - if(b) { - test = Optional.of("x"); - } else { - test = Optional.empty(); - } - System.out.println(test.get()); - if(b) { - test = Optional.empty(); - } else { - test = Optional.empty(); - } - System.out.println(test.get()); - - } - - private void checkOfNullable(String value) { - System.out.println(Optional.ofNullable(value).get()); - System.out.println(Optional.ofNullable(value+"a").get()); - System.out.println(Optional.ofNullable("xyz").get()); - } - - public static String demo() { - Optional holder = Optional.empty(); - - if (! holder.isPresent()) { - holder = Optional.of("hello world"); - if (!holder.isPresent()) { - return null; - } - } - - return holder.get(); - } - - //public Collection findCategories(Long articleId) { - // return java.util.stream.Stream.of(Optional.of("asdf")).filter(Optional::isPresent).map(x -> x.get()).collect( - // java.util.stream.Collectors.toList()) ; - //} - public static void main(String[] args) { Optional stringOpt; @@ -227,8 +178,10 @@ class OptionalWithoutIsPresent { } void order(Optional order, boolean b) { + // here order.get always suceeds order.ifPresent(o -> System.out.println(order.get())); - System.out.println(order.orElseGet(() -> order.get().trim())); + // here order.get always fails: tested in normal DFA + System.out.println(order.orElseGet(() -> order.get().trim())); } public static void two(Optional o1,Optional o2) { @@ -299,7 +252,7 @@ class OptionalWithoutIsPresent { } void shortIf(Optional o) { - if (true || o.isPresent()) { + if (true || o.isPresent()) { o.get(); } } @@ -326,20 +279,20 @@ class OptionalWithoutIsPresent { void guavaTest(com.google.common.base.Optional opt, String s, String s1) { System.out.println(opt.get()); - if(opt.isPresent()) { + if(opt.isPresent()) { System.out.println(opt.get()); } opt = com.google.common.base.Optional.fromNullable(s); if(opt.isPresent()) { System.out.println(opt.get()); } - opt = com.google.common.base.Optional.of(s); + opt = com.google.common.base.Optional.of(s); opt = com.google.common.base.Optional.of(s1); - if(opt.isPresent()) { + if(opt.isPresent()) { System.out.println(opt.get()); } opt = com.google.common.base.Optional.absent(); - if(opt.isPresent()) { + if(opt.isPresent()) { System.out.println(opt.get()); } } @@ -350,13 +303,6 @@ class OptionalWithoutIsPresent { System.out.println(o); } - void testThrow2(Optional test) { - test.orElseThrow(RuntimeException::new); - if (test.isPresent()) { - System.out.println("Yes"); - } - } - void testThrowCatch(Optional opt) { try { opt.orElseThrow(RuntimeException::new); @@ -366,13 +312,6 @@ class OptionalWithoutIsPresent { } } - public void testThrowFail(Optional arg) { - if(!arg.isPresent()) { - System.out.println(arg.orElseThrow(IllegalAccessError::new)); - } - String res = Optional.empty().orElseThrow(RuntimeException::new); - } - void testOrElseGet() { final Optional a = Optional.ofNullable(Math.random() > 0.5 ? null:""); final Optional b = Optional.ofNullable(Math.random() > 0.5 ? null:""); @@ -383,4 +322,32 @@ class OptionalWithoutIsPresent { String result = a.orElseGet(() -> b.get()); System.out.println(result); } + + boolean testBooleanOptional(Optional opt) { + if (opt.isPresent() && !opt.get()) { + return false; + } + return true; + } + + void testArrayStream(int[] arr1, int[] arr2) { + if(arr1.length == 0) return; + System.out.println(Arrays.stream(arr1).map(Math::abs).min().getAsInt()); + System.out.println(Arrays.stream(arr2).map(Math::abs).min().getAsInt()); + } + + public String getFirstItem(Collection data) { + return data.stream().findFirst().get(); + } + + public String getFirstItemChecked(Collection data) { + if(data.isEmpty()) throw new IllegalArgumentException("Data should never be empty"); + // Non-empty stream: get() is fine + return data.stream().findFirst().get(); + } + + public String getMax() { + // Non-empty stream: get() is fine + return Stream.of("foo", "bar", "baz").map(String::toUpperCase).max(Comparator.naturalOrder()).get(); + } } \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java index a09377c4e004..295d72ec2336 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java @@ -64,8 +64,7 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { public void testOptionalOfNullable() { doTest(); } public void testOptionalOrElse() { doTest(); } - public void testOptionalIsPresent() { doTest(); } - public void testOptionalGetWithoutIsPresent() { + public void testOptionalIsPresent() { myFixture.addClass("package org.junit;" + "public class Assert {" + " public static void assertTrue(boolean b) {}" + diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/OptionalGetWithoutIsPresentInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/OptionalGetWithoutIsPresentInspectionTest.java new file mode 100644 index 000000000000..2341f4f3ee64 --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/OptionalGetWithoutIsPresentInspectionTest.java @@ -0,0 +1,62 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.java.codeInspection; + +import com.intellij.JavaTestUtil; +import com.intellij.codeInspection.defUse.DefUseInspection; +import com.intellij.codeInspection.java18api.OptionalGetWithoutIsPresentInspection; +import com.intellij.testFramework.LightProjectDescriptor; +import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase; +import org.jetbrains.annotations.NotNull; + +public class OptionalGetWithoutIsPresentInspectionTest extends LightCodeInsightFixtureTestCase { + @Override + protected String getBasePath() { + return JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/optionalGet"; + } + + public void testOptionalGet() { doTest(); } + + @NotNull + @Override + protected LightProjectDescriptor getProjectDescriptor() { + return JAVA_9; + } + + private void doTest() { + mockClasses(); + myFixture.enableInspections(new OptionalGetWithoutIsPresentInspection()); + myFixture.testHighlighting(getTestName(false) + ".java"); + } + + private void mockClasses() { + myFixture.addClass("package org.junit;" + + "public class Assert {" + + " public static void assertTrue(boolean b) {}" + + "}"); + myFixture.addClass("package org.testng;" + + "public class Assert {" + + " public static void assertTrue(boolean b) {}" + + "}"); + myFixture.addClass("package com.google.common.base;\n" + + "\n" + + "public interface Supplier { T get();}\n"); + myFixture.addClass("package com.google.common.base;\n" + + "\n" + + "public interface Function { T apply(F input);}\n"); + myFixture.addClass("package com.google.common.base;\n" + + "\n" + + "public abstract class Optional {\n" + + " public static Optional absent() {}\n" + + " public static Optional of(T ref) {}\n" + + " public static Optional fromNullable(T ref) {}\n" + + " public abstract T get();\n" + + " public abstract boolean isPresent();\n" + + " public abstract T orNull();\n" + + " public abstract T or(Supplier supplier);\n" + + " public abstract Optional transform(Function fn);\n" + + " public abstract T or(T val);\n" + + " public abstract java.util.Optional toJavaUtil();\n" + + "}"); + + } +} \ No newline at end of file diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index acb54f7a5801..85f778577b8d 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -47,7 +47,6 @@ inspection.data.flow.true.asserts.option=Don't report assertions with condition inspection.data.flow.turn.off.true.asserts.quickfix=Don't report always true assertions inspection.data.flow.turn.off.constant.references.quickfix=Don't report values which are guaranteed to be constant inspection.data.flow.turn.off.nullable.returning.notnull.quickfix=Don't report nullable methods which always return not-null value -inspection.data.flow.turn.off.unchecked.optional.get.quickfix=Don't report Optional.get() calls without previous isPresent check inspection.data.flow.redundant.instanceof.quickfix=Replace with a null check inspection.data.flow.simplify.boolean.expression.quickfix=Simplify boolean expression inspection.data.flow.simplify.to.assignment.quickfix.name=Simplify to normal assignment @@ -89,13 +88,14 @@ dataflow.message.unboxing.method.reference=Use of #ref #loc would n dataflow.too.complex=Method #ref is too complex to analyze by data flow algorithm dataflow.too.complex.class=Class initializer is too complex to analyze by data flow algorithm dataflow.method.fails.with.null.argument=Method will throw an exception when parameter is null -dataflow.message.optional.get.without.is.present={0}.#ref() without ''isPresent()'' check dataflow.message.constant.method.reference=Method reference result is always ''{0}'' dataflow.message.array.index.out.of.bounds=Array index is out of bounds dataflow.message.immutable.modified=Immutable object is modified dataflow.message.immutable.passed=Immutable object is passed where mutable is expected dataflow.message.redundant.assignment=Variable is already assigned to this value +inspection.optional.get.without.is.present.message={0}.#ref() without ''isPresent()'' check + #deprecated inspection.deprecated.display.name=Deprecated API usage inspection.marked.for.removal.display.name=Usage of API marked for removal