From 0e3cb89ae2fb8d21d2f1fe3f915ee564e6717bd9 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 13 Feb 2017 17:45:42 +0700 Subject: [PATCH] IDEA-153922 "Optional.get() without isPresent" check when used in ifPresent's consumer --- .../dataFlow/DataFlowRunner.java | 23 ++--- .../dataFlow/DfaMemoryState.java | 5 +- .../dataFlow/DfaMemoryStateImpl.java | 25 +++++- .../dataFlow/StandardInstructionVisitor.java | 88 +++++++++++-------- .../fixture/OptionalGetWithoutIsPresent.java | 7 +- 5 files changed, 87 insertions(+), 61 deletions(-) diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java index 567161a3e08b..aa08cf6ebede 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2015 JetBrains s.r.o. + * 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. @@ -26,7 +26,6 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.value.DfaValueFactory; -import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.progress.ProgressManager; @@ -286,14 +285,14 @@ public class DataFlowRunner { for (PsiMethod method : nestedClass.getMethods()) { PsiCodeBlock body = method.getBody(); if (body != null) { - myNestedClosures.putValue(body, createClosureState(state)); + myNestedClosures.putValue(body, state.createClosureState()); } } for (PsiClassInitializer initializer : nestedClass.getInitializers()) { - myNestedClosures.putValue(initializer.getBody(), createClosureState(state)); + myNestedClosures.putValue(initializer.getBody(), state.createClosureState()); } for (PsiField field : nestedClass.getFields()) { - myNestedClosures.putValue(field, createClosureState(state)); + myNestedClosures.putValue(field, state.createClosureState()); } } @@ -301,7 +300,7 @@ public class DataFlowRunner { DfaMemoryState state = instructionState.getMemoryState(); PsiElement body = expr.getBody(); if (body != null) { - myNestedClosures.putValue(body, createClosureState(state)); + myNestedClosures.putValue(body, state.createClosureState()); } } @@ -359,16 +358,4 @@ public class DataFlowRunner { return Pair.create(trueSet, falseSet); } - - @NotNull - private static DfaMemoryStateImpl createClosureState(@NotNull DfaMemoryState memState) { - DfaMemoryStateImpl copy = (DfaMemoryStateImpl)memState.createCopy(); - copy.flushFields(); - Set vars = new HashSet<>(copy.getVariableStates().keySet()); - for (DfaVariableValue value : vars) { - copy.flushDependencies(value); - } - copy.emptyStack(); - return copy; - } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java index f78f66a45858..e050f2eb9999 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2015 JetBrains s.r.o. + * 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. @@ -34,6 +34,9 @@ public interface DfaMemoryState { @NotNull DfaMemoryState createCopy(); + @NotNull + DfaMemoryStateImpl createClosureState(); + DfaValue pop(); DfaValue peek(); void push(@NotNull DfaValue value); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index e37f3130f3d9..e34694d07798 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -53,6 +53,8 @@ public class DfaMemoryStateImpl implements DfaMemoryState { // dfa value id -> indices in myEqClasses list of the classes which contain the id (or wrapped) private final TIntObjectHashMap myIdToEqClassesIndices; private final Stack myStack; + // Closures which correspond to the stack top (do not track other closures for now) + private final List myStackTopClosures = new ArrayList<>(); private final TLongHashSet myDistinctClasses; private final LinkedHashMap myVariableStates; private final Map myDefaultVariableStates; @@ -106,6 +108,24 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return new DfaMemoryStateImpl(this); } + @NotNull + @Override + public DfaMemoryStateImpl createClosureState() { + DfaMemoryStateImpl copy = createCopy(); + copy.flushFields(); + Set vars = new HashSet<>(copy.getVariableStates().keySet()); + for (DfaVariableValue value : vars) { + copy.flushDependencies(value); + } + copy.emptyStack(); + myStackTopClosures.add(copy); + return copy; + } + + List getStackTopClosures() { + return new ArrayList<>(myStackTopClosures); + } + public boolean equals(Object obj) { if (obj == this) return true; if (!(obj instanceof DfaMemoryStateImpl)) return false; @@ -221,6 +241,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { @Override public DfaValue pop() { myCachedHash = null; + myStackTopClosures.clear(); return myStack.pop(); } @@ -232,12 +253,14 @@ public class DfaMemoryStateImpl implements DfaMemoryState { @Override public void push(@NotNull DfaValue value) { myCachedHash = null; + myStackTopClosures.clear(); myStack.push(value); } @Override public void emptyStack() { myCachedHash = null; + myStackTopClosures.clear(); while (!myStack.isEmpty() && !(myStack.peek() instanceof DfaControlTransferValue)) { myStack.pop(); } @@ -1116,7 +1139,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { @Override public boolean execute(int id, int[] set) { DfaValue value = myFactory.getValue(id); - s.append(value + " -> " + Arrays.toString(set) + ", "); + s.append(value).append(" -> ").append(Arrays.toString(set)).append(", "); return true; } }); 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 5bad45e3d83c..6d0130b73580 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 @@ -1,5 +1,5 @@ /* - * Copyright 2000-2009 JetBrains s.r.o. + * 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. @@ -42,6 +42,9 @@ import static com.intellij.psi.JavaTokenType.*; * @author peter */ public class StandardInstructionVisitor extends InstructionVisitor { + private static final Set OPTIONAL_METHOD_NAMES = + ContainerUtil.set("isPresent", "of", "ofNullable", "fromNullable", "empty", "absent", + "or", "orElseGet", "ifPresent", "map", "flatMap", "filter", "transform"); private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.StandardInstructionVisitor"); private static final Object ANY_VALUE = new Object(); private final Set myReachable = new THashSet<>(); @@ -180,13 +183,13 @@ public class StandardInstructionVisitor extends InstructionVisitor { @Override public DfaInstructionState[] visitMethodCall(final MethodCallInstruction instruction, final DataFlowRunner runner, final DfaMemoryState memState) { - DfaValue[] argValues = popCallArguments(instruction, runner, memState); - final DfaValue qualifier = popQualifier(instruction, runner, memState); - Set finalStates = ContainerUtil.newLinkedHashSet(); - finalStates.addAll(handleOptionalMethods(instruction, runner, memState, qualifier, argValues)); + finalStates.addAll(handleOptionalMethods(instruction, runner, memState)); if (finalStates.isEmpty()) { + DfaValue[] argValues = popCallArguments(instruction, runner, memState); + final DfaValue qualifier = popQualifier(instruction, runner, memState); + LinkedHashSet currentStates = ContainerUtil.newLinkedHashSet(memState); if (argValues != null) { for (MethodContract contract : instruction.getContracts()) { @@ -219,54 +222,63 @@ public class StandardInstructionVisitor extends InstructionVisitor { } @NotNull - private static List handleOptionalMethods(MethodCallInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - DfaValue qualifierValue, @Nullable DfaValue[] argValues) { + private List handleOptionalMethods(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { PsiMethodCallExpression call = ObjectUtils.tryCast(instruction.getCallExpression(), PsiMethodCallExpression.class); if (call == null) return Collections.emptyList(); String methodName = call.getMethodExpression().getReferenceName(); - if ("isPresent".equals(methodName)) { - PsiMethod method = call.resolveMethod(); - if (method != null && TypeUtils.isOptional(method.getContainingClass())) { - ThreeState state = memState.checkOptional(qualifierValue); + if (methodName == null || !OPTIONAL_METHOD_NAMES.contains(methodName)) return Collections.emptyList(); + PsiMethod method = call.resolveMethod(); + if (method == null || !TypeUtils.isOptional(method.getContainingClass())) return Collections.emptyList(); + List closures = ((DfaMemoryStateImpl)memState).getStackTopClosures(); + DfaValue[] argValues = popCallArguments(instruction, runner, memState); + final DfaValue qualifier = popQualifier(instruction, runner, memState); + switch (methodName) { + case "isPresent": { + ThreeState state = memState.checkOptional(qualifier); DfaConstValue.Factory constFactory = runner.getFactory().getConstFactory(); if (state == ThreeState.UNSURE) { DfaMemoryState falseState = memState.createCopy(); memState.push(constFactory.getTrue()); - memState.applyIsPresentCheck(true, qualifierValue); + memState.applyIsPresentCheck(true, qualifier); falseState.push(constFactory.getFalse()); - falseState.applyIsPresentCheck(false, qualifierValue); + falseState.applyIsPresentCheck(false, qualifier); return Arrays.asList(memState, falseState); } else { - DfaValue result = state == ThreeState.YES ? constFactory.getTrue() : constFactory.getFalse(); - memState.push(result); - return Collections.singletonList(memState); + memState.push(state == ThreeState.YES ? constFactory.getTrue() : constFactory.getFalse()); } + break; } - } - if ("of".equals(methodName)) { - PsiMethod method = call.resolveMethod(); - if (method != null && TypeUtils.isOptional(method.getContainingClass())) { - memState.push(runner.getFactory().getOptionalFactory().getOptional(true)); - return Collections.singletonList(memState); - } - } - if (DfaOptionalSupport.resolveOfNullable(call) != null) { - if (argValues != null && argValues.length == 1 && memState.isNotNull(argValues[0])) { - memState.push(runner.getFactory().getOptionalFactory().getOptional(true)); - return Collections.singletonList(memState); - } - } - if ("empty".equals(methodName) || "absent".equals(methodName)) { - PsiMethod method = call.resolveMethod(); - if (method != null && TypeUtils.isOptional(method.getContainingClass())) { + case "of": + case "ofNullable": + case "fromNullable": + if ("of".equals(methodName) || (argValues != null && argValues.length == 1 && memState.isNotNull(argValues[0]))) { + memState.push(runner.getFactory().getOptionalFactory().getOptional(true)); + } else { + memState.push(getMethodResultValue(instruction, qualifier, runner.getFactory())); + } + break; + case "empty": + case "absent": memState.push(runner.getFactory().getOptionalFactory().getOptional(false)); - return Collections.singletonList(memState); - } + break; + case "filter": + case "flatMap": + case "ifPresent": + case "map": + case "or": + case "orElseGet": + case "transform": + for (DfaMemoryState closure : closures) { + closure.applyIsPresentCheck(!methodName.startsWith("or"), qualifier); + } + memState.push(getMethodResultValue(instruction, qualifier, runner.getFactory())); + break; + default: + memState.push(getMethodResultValue(instruction, qualifier, runner.getFactory())); + break; } - return Collections.emptyList(); + return Collections.singletonList(memState); } @Nullable diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java index c864f0de0e39..b57953ada07b 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java @@ -226,9 +226,10 @@ class OptionalWithoutIsPresent { return Optional.empty(); } - //void order(Optional order) { - // order.ifPresent(o -> System.out.println(order.get())); - //} + void order(Optional order, boolean b) { + order.ifPresent(o -> System.out.println(order.get())); + System.out.println(order.orElseGet(() -> order.get().trim())); + } public static void two(Optional o1,Optional o2) { if (!o1.isPresent() && !o2.isPresent()) {