diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java index a2d03a365e49..30e276dc5367 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java @@ -473,13 +473,27 @@ public class CFGBuilder { * @return this builder */ public CFGBuilder invokeFunction(int argCount, @Nullable PsiExpression functionalExpression) { + return invokeFunction(argCount, functionalExpression, false); + } + + /** + * Generates instructions to invoke functional expression (inlining it if possible) which + * consumes given amount of stack arguments, assuming that it was previously evaluated + * (see {@link #evaluateFunction(PsiExpression)}). + * + * @param argCount number of stack arguments to consume + * @param functionalExpression a functional expression to invoke + * @param forceNotNullResult if true, function result will be forced to not-null (possibly issuing a warning) + * @return this builder + */ + public CFGBuilder invokeFunction(int argCount, @Nullable PsiExpression functionalExpression, boolean forceNotNullResult) { PsiExpression stripped = PsiUtil.deparenthesizeExpression(functionalExpression); if (stripped instanceof PsiLambdaExpression) { PsiLambdaExpression lambda = (PsiLambdaExpression)stripped; PsiParameter[] parameters = lambda.getParameterList().getParameters(); if (parameters.length == argCount && lambda.getBody() != null) { StreamEx.ofReversed(parameters).forEach(p -> assignTo(p).pop()); - return inlineLambda(lambda); + return inlineLambda(lambda, forceNotNullResult); } } if (stripped instanceof PsiMethodReferenceExpression) { @@ -506,6 +520,9 @@ public class CFGBuilder { myAnalyzer.addBareCall(null, methodRef); myAnalyzer.generateBoxingUnboxingInstructionFor(methodRef, resolveResult.getSubstitutor().substitute(method.getReturnType()), LambdaUtil.getFunctionalInterfaceReturnType(methodRef)); + if (forceNotNullResult) { + checkNotNull(methodRef, NullabilityProblem.nullableFunctionReturn); + } return this; } } @@ -553,8 +570,8 @@ public class CFGBuilder { } } - public CFGBuilder inlineLambda(PsiLambdaExpression lambda) { - myAnalyzer.inlineLambda(lambda); + public CFGBuilder inlineLambda(PsiLambdaExpression lambda, boolean forceNotNullResult) { + myAnalyzer.inlineLambda(lambda, forceNotNullResult); return this; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java index 625c3eb7eff2..b45115b90ea2 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java @@ -62,6 +62,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { private final ExceptionTransfer myError; private final PsiType myAssertionError; private PsiLambdaExpression myLambdaExpression = null; + private boolean myForceNotNullLambdaResult = false; ControlFlowAnalyzer(final DfaValueFactory valueFactory, @NotNull PsiElement codeFragment, boolean ignoreAssertions, boolean inlining) { myInlining = inlining; @@ -671,6 +672,8 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { else { if (returnValue == null) { pushUnknown(); + } else if (myForceNotNullLambdaResult) { + addInstruction(new CheckNotNullInstruction(returnValue, NullabilityProblem.nullableFunctionReturn)); } controlTransfer(new InstructionTransfer(getEndOffset(myLambdaExpression), getVariablesInside(myLambdaExpression)), myTrapStack); } @@ -1704,11 +1707,13 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { @Override public void visitClass(PsiClass aClass) { } - void inlineLambda(PsiLambdaExpression lambda) { + void inlineLambda(PsiLambdaExpression lambda, boolean forceNotNullResult) { PsiLambdaExpression oldLambda = myLambdaExpression; + boolean oldForceNotNullLambdaResult = myForceNotNullLambdaResult; // Transfer value is pushed to avoid emptying stack beyond this point addInstruction(new PushInstruction(myFactory.controlTransfer(ReturnTransfer.INSTANCE, this.myTrapStack), null)); myLambdaExpression = lambda; + myForceNotNullLambdaResult = forceNotNullResult; startElement(lambda); try { PsiElement body = lambda.getBody(); @@ -1719,11 +1724,15 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } else if (body instanceof PsiExpression) { generateBoxingUnboxingInstructionFor((PsiExpression)body, LambdaUtil.getFunctionalInterfaceReturnType(lambda)); + if (myForceNotNullLambdaResult) { + addInstruction(new CheckNotNullInstruction((PsiExpression)body, NullabilityProblem.nullableFunctionReturn)); + } } } finally { finishElement(lambda); myLambdaExpression = oldLambda; + myForceNotNullLambdaResult = oldForceNotNullLambdaResult; // Pop transfer value (which is second value in stack now) addInstruction(new SpliceInstruction(2, 0)); } 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 5a31133b01f6..de9a9dcf6f7d 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 @@ -365,6 +365,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { reportConstantPushes(runner, holder, visitor, reportedAnchors); + reportNullableFunctions(visitor, holder, reportedAnchors); reportNullableArguments(visitor, holder, reportedAnchors); reportNullableAssignments(visitor, holder, reportedAnchors); reportUnboxedNullables(visitor, holder, reportedAnchors); @@ -699,6 +700,13 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { visitor.silenceConstantCondition(psiAnchor); } + private static void reportNullableFunctions(DataFlowInstructionVisitor visitor, ProblemsHolder holder, Set reportedAnchors) { + for (PsiElement expr : visitor.getProblems(NullabilityProblem.nullableFunctionReturn)) { + if (!reportedAnchors.add(expr)) continue; + holder.registerProblem(expr, InspectionsBundle.message("dataflow.message.return.nullable.from.notnull.function")); + } + } + private void reportNullableArguments(DataFlowInstructionVisitor visitor, ProblemsHolder holder, Set reportedAnchors) { for (PsiElement expr : visitor.getProblems(NullabilityProblem.passingNullableToNotNullParameter)) { if (!reportedAnchors.add(expr)) continue; 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 59b9e9f6adb7..7922667774c2 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 @@ -33,7 +33,6 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.*; -import java.util.function.Predicate; public class DataFlowRunner { private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.DataFlowRunner"); @@ -41,9 +40,6 @@ public class DataFlowRunner { private Instruction[] myInstructions; private final MultiMap myNestedClosures = new MultiMap<>(); - // Closures which were registered for previous instruction and can be queried by visitor - // to adjust them somehow - private final Map myStackTopClosures = new HashMap<>(); @NotNull private final DfaValueFactory myValueFactory; private boolean myInlining = true; @@ -91,14 +87,6 @@ public class DataFlowRunner { return Collections.singletonList(createMemoryState()); } - void updateStackTopClosures(Predicate updater) { - myStackTopClosures.forEach((state, element) -> { - if(!updater.test(state)) { - myNestedClosures.remove(element, state); - } - }); - } - @NotNull public final RunnerResult analyzeMethod(@NotNull PsiElement psiBlock, @NotNull InstructionVisitor visitor) { Collection initialStates = createInitialStates(psiBlock, visitor); @@ -118,8 +106,7 @@ public class DataFlowRunner { int endOffset = flow.getInstructionCount(); myInstructions = flow.getInstructions(); myNestedClosures.clear(); - myStackTopClosures.clear(); - + Set joinInstructions = ContainerUtil.newHashSet(); for (int index = 0; index < myInstructions.length; index++) { Instruction instruction = myInstructions[index]; @@ -290,7 +277,6 @@ public class DataFlowRunner { Instruction instruction = instructionState.getInstruction(); DfaInstructionState[] states = instruction.accept(this, instructionState.getMemoryState(), visitor); - myStackTopClosures.clear(); PsiElement closure = DfaUtil.getClosureInside(instruction); if (closure instanceof PsiClass) { registerNestedClosures(instructionState, (PsiClass)closure); @@ -326,9 +312,7 @@ public class DataFlowRunner { } private void createClosureState(PsiElement anchor, DfaMemoryState state) { - DfaMemoryState closureState = state.createClosureState(); - myStackTopClosures.put(closureState, anchor); - myNestedClosures.putValue(anchor, closureState); + myNestedClosures.putValue(anchor, state.createClosureState()); } @NotNull 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 fde262aaebc1..b13b1a33a61b 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 @@ -32,7 +32,7 @@ import org.jetbrains.annotations.Nullable; */ public class DfaOptionalSupport { private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.DfaOptionalSupport"); - private static final String GUAVA_OPTIONAL = "com.google.common.base.Optional"; + public static final String GUAVA_OPTIONAL = "com.google.common.base.Optional"; public static final CallMatcher JDK_OPTIONAL_OF_NULLABLE = CallMatcher.staticCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "ofNullable").parameterCount(1); public static final CallMatcher GUAVA_OPTIONAL_FROM_NULLABLE = CallMatcher.staticCall(GUAVA_OPTIONAL, "fromNullable").parameterCount(1); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblem.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblem.java index 9240f5d3383e..b5c62a649954 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblem.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblem.java @@ -9,6 +9,7 @@ public enum NullabilityProblem { unboxingNullable, assigningToNotNull, nullableReturn, + nullableFunctionReturn, passingNullableToNotNullParameter, passingNullableArgumentToNonAnnotatedParameter } 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 446ef2b1bc1b..5c3f8bcb9602 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 @@ -32,7 +32,6 @@ import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.MultiMap; import com.siyeh.ig.callMatcher.CallMapper; import com.siyeh.ig.callMatcher.CallMatcher; -import com.siyeh.ig.psiutils.TypeUtils; import gnu.trove.THashSet; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; @@ -48,9 +47,6 @@ public class StandardInstructionVisitor extends InstructionVisitor { private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.StandardInstructionVisitor"); private static final Object ANY_VALUE = new Object(); - private static final Set OPTIONAL_METHOD_NAMES = ContainerUtil - .set("of", "ofNullable", "fromNullable", "empty", "absent", "or", "orElse", "orElseGet", "ifPresent", "map", "flatMap", "filter", - "transform"); private static final CallMapper KNOWN_METHOD_RANGES = new CallMapper() .register(CallMatcher.instanceCall("java.time.LocalDateTime", "getHour"), LongRangeSet.range(0, 23)) .register(CallMatcher.instanceCall("java.time.LocalDateTime", "getMinute", "getSecond"), LongRangeSet.range(0, 59)) @@ -297,7 +293,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { @Override public DfaInstructionState[] visitMethodCall(final MethodCallInstruction instruction, final DataFlowRunner runner, final DfaMemoryState memState) { Set finalStates = ContainerUtil.newLinkedHashSet(); - finalStates.addAll(handleOptionalMethods(instruction, runner, memState)); + //finalStates.addAll(handleOptionalMethods(instruction, runner, memState)); finalStates.addAll(handleKnownMethods(instruction, runner, memState)); if (finalStates.isEmpty()) { @@ -352,65 +348,6 @@ public class StandardInstructionVisitor extends InstructionVisitor { return states; } - @NotNull - 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 (methodName == null || !OPTIONAL_METHOD_NAMES.contains(methodName)) return Collections.emptyList(); - PsiMethod method = call.resolveMethod(); - if (method == null || !TypeUtils.isOptional(method.getContainingClass())) return Collections.emptyList(); - DfaCallArguments arguments = popCall(instruction, runner, memState, false); - DfaValue[] argValues = arguments.myArguments; - DfaValue result = null; - DfaValueFactory factory = runner.getFactory(); - switch (methodName) { - case "of": - case "ofNullable": - case "fromNullable": - if ("of".equals(methodName) || (argValues != null && argValues.length == 1 && memState.isNotNull(argValues[0]))) { - result = factory.getOptionalFactory().getOptional(true); - } - break; - case "empty": - case "absent": - result = factory.getOptionalFactory().getOptional(false); - break; - case "orElse": - if (argValues != null && argValues.length == 1) { - DfaMemoryState falseState = memState.createCopy(); - DfaOptionalValue optional = factory.getOptionalFactory().getOptional(true); - DfaValue relation = factory.createCondition(arguments.myQualifier, RelationType.IS, optional); - List states = new ArrayList<>(2); - if (memState.applyCondition(relation)) { - memState.push(factory.createTypeValue(instruction.getResultType(), Nullness.NOT_NULL)); - states.add(memState); - } - if (falseState.applyCondition(relation.createNegated())) { - falseState.push(argValues[0]); - states.add(falseState); - } - return states; - } - break; - case "filter": - case "flatMap": - case "ifPresent": - case "map": - case "or": - case "orElseGet": - case "transform": { - DfaOptionalValue optional = factory.getOptionalFactory().getOptional(!methodName.startsWith("or")); - DfaValue relation = factory.createCondition(arguments.myQualifier, RelationType.IS, optional); - runner.updateStackTopClosures(state -> state.applyCondition(relation)); - break; - } - default: - } - memState.push(result == null ? getMethodResultValue(instruction, arguments.myQualifier, factory) : result); - return Collections.singletonList(memState); - } - @NotNull private DfaCallArguments popCall(MethodCallInstruction instruction, DataFlowRunner runner, @@ -606,7 +543,17 @@ public class StandardInstructionVisitor extends InstructionVisitor { @Override public DfaInstructionState[] visitCheckNotNull(CheckNotNullInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { if (!checkNotNullable(memState, memState.peek(), instruction.getProblem(), instruction.getExpression())) { - forceNotNull(runner, memState, memState.peek()); + DfaValue arg = memState.peek(); + if (arg instanceof DfaVariableValue) { + DfaVariableValue var = (DfaVariableValue)arg; + memState.setVarValue(var, runner.getFactory().createTypeValue(var.getVariableType(), Nullness.NOT_NULL)); + } else if (arg instanceof DfaTypeValue) { + memState.pop(); + memState.push(((DfaTypeValue)arg).withNullness(Nullness.NOT_NULL)); + } else if (memState.isNull(arg) && instruction.getProblem() == NullabilityProblem.nullableFunctionReturn) { + memState.pop(); + memState.push(runner.getFactory().createTypeValue(PsiType.VOID, Nullness.NOT_NULL)); + } } return super.visitCheckNotNull(instruction, runner, memState); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/LambdaInliner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/LambdaInliner.java index 2e3d2add58ef..f83563e264ff 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/LambdaInliner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/LambdaInliner.java @@ -47,7 +47,7 @@ public class LambdaInliner implements CallInliner { .boxUnbox(arg, parameter.getType()) .assign() .pop()); - builder.inlineLambda(lambda); + builder.inlineLambda(lambda, false); return true; } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java index 42a53bd0cebc..976219597c18 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java @@ -21,6 +21,7 @@ import com.intellij.codeInspection.dataFlow.Nullness; import com.intellij.codeInspection.dataFlow.value.DfaOptionalValue; import com.intellij.psi.*; import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; import com.siyeh.ig.callMatcher.CallMapper; import com.siyeh.ig.callMatcher.CallMatcher; @@ -30,60 +31,105 @@ import org.jetbrains.annotations.NotNull; import java.util.function.BiConsumer; +import static com.intellij.codeInspection.dataFlow.DfaOptionalSupport.GUAVA_OPTIONAL; import static com.intellij.psi.CommonClassNames.JAVA_UTIL_OPTIONAL; +import static com.siyeh.ig.callMatcher.CallMatcher.*; /** * An inliner which is capable to inline some Optional chains like * {@code Optional.of(xyz).map(lambda).filter(lambda).flatMap(lambda).orElseGet(lambda)} *

- * TODO support Guava optional * TODO support primitive Optionals */ public class OptionalChainInliner implements CallInliner { - private static final CallMatcher OPTIONAL_OR_ELSE = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "orElse").parameterCount(1); - private static final CallMatcher OPTIONAL_OR_ELSE_GET = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "orElseGet").parameterCount(1); - private static final CallMatcher OPTIONAL_OR = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "or").parameterCount(1); // Java 9 - private static final CallMatcher OPTIONAL_IF_PRESENT = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "ifPresent").parameterCount(1); - private static final CallMatcher OPTIONAL_FILTER = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "filter").parameterCount(1); - private static final CallMatcher OPTIONAL_MAP = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "map").parameterCount(1); - private static final CallMatcher OPTIONAL_FLAT_MAP = CallMatcher.instanceCall(JAVA_UTIL_OPTIONAL, "flatMap").parameterCount(1); - private static final CallMatcher OPTIONAL_OF = CallMatcher.staticCall(JAVA_UTIL_OPTIONAL, "of", "ofNullable").parameterCount(1); - private static final CallMatcher OPTIONAL_EMPTY = CallMatcher.staticCall(JAVA_UTIL_OPTIONAL, "empty").parameterCount(0); + private static final CallMatcher OPTIONAL_OR_ELSE = anyOf( + instanceCall(JAVA_UTIL_OPTIONAL, "orElse").parameterCount(1), + instanceCall(GUAVA_OPTIONAL, "or").parameterTypes("T")); + private static final CallMatcher OPTIONAL_OR_NULL = instanceCall(GUAVA_OPTIONAL, "orNull").parameterCount(0); + private static final CallMatcher OPTIONAL_OR_ELSE_GET = anyOf( + instanceCall(JAVA_UTIL_OPTIONAL, "orElseGet").parameterCount(1), + instanceCall(GUAVA_OPTIONAL, "or").parameterTypes("com.google.common.base.Supplier")); + private static final CallMatcher OPTIONAL_OR = instanceCall(JAVA_UTIL_OPTIONAL, "or").parameterCount(1); // Java 9 + private static final CallMatcher OPTIONAL_IF_PRESENT = instanceCall(JAVA_UTIL_OPTIONAL, "ifPresent").parameterCount(1); + private static final CallMatcher OPTIONAL_FILTER = instanceCall(JAVA_UTIL_OPTIONAL, "filter").parameterCount(1); + private static final CallMatcher OPTIONAL_MAP = instanceCall(JAVA_UTIL_OPTIONAL, "map").parameterCount(1); + // Guava transform() throws if function returns null, so handled separately + private static final CallMatcher GUAVA_TRANSFORM = instanceCall(GUAVA_OPTIONAL, "transform").parameterCount(1); + private static final CallMatcher OPTIONAL_FLAT_MAP = instanceCall(JAVA_UTIL_OPTIONAL, "flatMap").parameterCount(1); + private static final CallMatcher OPTIONAL_OF = anyOf( + staticCall(JAVA_UTIL_OPTIONAL, "of", "ofNullable").parameterCount(1), + staticCall(GUAVA_OPTIONAL, "of", "fromNullable").parameterCount(1)); + private static final CallMatcher OPTIONAL_EMPTY = anyOf( + staticCall(JAVA_UTIL_OPTIONAL, "empty").parameterCount(0), + staticCall(GUAVA_OPTIONAL, "absent").parameterCount(0)); + private static final CallMatcher GUAVA_TO_JAVA = + instanceCall(GUAVA_OPTIONAL, "toJavaUtil").parameterCount(0); private static final CallMapper> TERMINAL_MAPPER = new CallMapper>() - .register(OPTIONAL_OR_ELSE, (builder, call) -> { - PsiExpression argument = call.getArgumentList().getExpressions()[0]; - builder.pushExpression(argument) // stack: .. optValue, elseValue - .boxUnbox(argument, call.getType()) - .splice(2, 0, 1, 1) // stack: .. elseValue, optValue, optValue + .register(OPTIONAL_OR_ELSE, (builder, call) -> { + PsiExpression argument = call.getArgumentList().getExpressions()[0]; + builder.pushExpression(argument) // stack: .. optValue, elseValue + .boxUnbox(argument, call.getType()) + .splice(2, 0, 1, 1) // stack: .. elseValue, optValue, optValue + .ifNotNull() + .swap() // stack: .. optValue, elseValue + .endIf() + .pop(); + }) + .register(OPTIONAL_OR_NULL, (builder, call) -> { + // no op! + }) + .register(OPTIONAL_OR_ELSE_GET, (builder, call) -> { + PsiExpression fn = call.getArgumentList().getExpressions()[0]; + builder + .evaluateFunction(fn) + .dup() + .ifNull() + .pop() + .invokeFunction(0, fn) + .endIf(); + }) + .register(OPTIONAL_IF_PRESENT, (builder, call) -> { + PsiExpression fn = call.getArgumentList().getExpressions()[0]; + builder + .evaluateFunction(fn) + .dup() + .ifNotNull() + .invokeFunction(0, fn) + .elseBranch() + .pop() + .pushUnknown() + .endIf(); + }); + + private static final CallMapper> INTERMEDIATE_MAPPER = + new CallMapper>() + .register(OPTIONAL_MAP, (builder, function) -> inlineMap(builder, function, false)) + .register(GUAVA_TRANSFORM, (builder, function) -> inlineMap(builder, function, true)) + .register(OPTIONAL_FILTER, (builder, function) -> builder + .evaluateFunction(function) + .dup() .ifNotNull() - .swap() // stack: .. optValue, elseValue + .dup() + .invokeFunction(1, function) + .ifConditionIs(false) + .pop() + .pushNull() .endIf() - .pop(); - }) - .register(OPTIONAL_OR_ELSE_GET, (builder, call) -> { - PsiExpression fn = call.getArgumentList().getExpressions()[0]; - builder - .evaluateFunction(fn) + .endIf()) + .register(OPTIONAL_FLAT_MAP, (builder, function) -> builder + .dup() + .ifNotNull() + .chain(b -> invokeAndUnwrapOptional(b, 1, function)) + .endIf()) + .register(OPTIONAL_OR, (builder, function) -> builder .dup() .ifNull() .pop() - .invokeFunction(0, fn) - .endIf(); - }) - .register(OPTIONAL_IF_PRESENT, (builder, call) -> { - PsiExpression fn = call.getArgumentList().getExpressions()[0]; - builder - .evaluateFunction(fn) - .dup() - .ifNotNull() - .invokeFunction(0, fn) - .elseBranch() - .pop() - .pushUnknown() - .endIf(); - }); + .chain(b -> invokeAndUnwrapOptional(b, 0, function)) + .endIf()) + .register(GUAVA_TO_JAVA, (builder, stub) -> {/* no op */}); @Override public boolean tryInlineCall(@NotNull CFGBuilder builder, @NotNull PsiMethodCallExpression call) { @@ -96,21 +142,32 @@ public class OptionalChainInliner implements CallInliner { terminalInliner.accept(builder, call); return true; } + DfaOptionalValue.Factory optionalFactory = builder.getFactory().getOptionalFactory(); if (pushIntermediateOperationValue(builder, call)) { builder.ifNotNull() - .push(builder.getFactory().getOptionalFactory().getOptional(true)) + .push(optionalFactory.getOptional(true)) .elseBranch() - .push(builder.getFactory().getOptionalFactory().getOptional(false)) + .push(optionalFactory.getOptional(false)) .endIf(); return true; } + if (OPTIONAL_EMPTY.test(call)) { + builder.push(optionalFactory.getOptional(false)); + return true; + } return false; } @Contract("null -> null") private static PsiType getOptionalElementType(PsiExpression expression) { if (expression == null) return null; - return PsiUtil.substituteTypeParameter(expression.getType(), JAVA_UTIL_OPTIONAL, 0, false); + PsiClassType type = ObjectUtils.tryCast(expression.getType(), PsiClassType.class); + if (type == null) return null; + String rawName = type.rawType().getCanonicalText(); + if (!rawName.equals(JAVA_UTIL_OPTIONAL) && !rawName.equals(GUAVA_OPTIONAL)) return null; + PsiType[] parameters = type.getParameters(); + if (parameters.length != 1) return null; + return parameters[0]; } private static boolean pushOptionalValue(CFGBuilder builder, PsiExpression expression, @@ -119,11 +176,6 @@ public class OptionalChainInliner implements CallInliner { if (optionalElementType == null) return false; if (expression instanceof PsiMethodCallExpression) { PsiMethodCallExpression qualifierCall = (PsiMethodCallExpression)expression; - if (OPTIONAL_OF.test(qualifierCall)) { - inlineOf(builder, optionalElementType, qualifierCall); - builder.assignTo(builder.createTempVariable(optionalElementType)); - return true; - } if (OPTIONAL_EMPTY.test(qualifierCall)) { builder.pushNull(); return true; @@ -148,26 +200,17 @@ public class OptionalChainInliner implements CallInliner { } private static boolean pushIntermediateOperationValue(CFGBuilder builder, PsiMethodCallExpression call) { - boolean isFilter = OPTIONAL_FILTER.test(call); - boolean isMap = OPTIONAL_MAP.test(call); - boolean isFlatMap = OPTIONAL_FLAT_MAP.test(call); - boolean isOr = OPTIONAL_OR.test(call); - if (!isFilter && !isMap && !isFlatMap && !isOr) return false; - PsiExpression argument = call.getArgumentList().getExpressions()[0]; + if (OPTIONAL_OF.test(call)) { + PsiType optionalElementType = getOptionalElementType(call); + inlineOf(builder, optionalElementType, call); + return true; + } + BiConsumer intermediateInliner = INTERMEDIATE_MAPPER.mapFirst(call); + if (intermediateInliner == null) return false; + PsiExpression argument = ArrayUtil.getFirstElement(call.getArgumentList().getExpressions()); PsiExpression qualifierExpression = call.getMethodExpression().getQualifierExpression(); if (!pushOptionalValue(builder, PsiUtil.skipParenthesizedExprDown(qualifierExpression), call, NullabilityProblem.callNPE)) return false; - if (isFlatMap) { - inlineFlatMap(builder, argument); - } - else if (isFilter) { - inlineFilter(builder, argument); - } - else if (isMap) { - inlineMap(builder, argument); - } - else { - inlineOr(builder, argument); - } + intermediateInliner.accept(builder, argument); return true; } @@ -180,57 +223,26 @@ public class OptionalChainInliner implements CallInliner { PsiExpression lambdaBody = LambdaUtil.extractSingleExpressionFromBody(lambda.getBody()); if (parameters.length == argCount && lambdaBody != null) { StreamEx.ofReversed(parameters).forEach(p -> builder.assignTo(p).pop()); - if (pushOptionalValue(builder, lambdaBody, lambdaBody, NullabilityProblem.callNPE)) { + if (pushOptionalValue(builder, lambdaBody, lambdaBody, NullabilityProblem.nullableFunctionReturn)) { return; } + // Restore stack for common invokeFunction + StreamEx.of(parameters).forEach(p -> builder.push(builder.getFactory().getVarFactory().createVariableValue(p, false))); } } builder - .pushExpression(function) - .checkNotNull(function, NullabilityProblem.passingNullableToNotNullParameter) + .evaluateFunction(function) + .invokeFunction(argCount, function, true) .pop() .pushUnknown(); } - private static void inlineFlatMap(CFGBuilder builder, - PsiExpression function) { - builder - .dup() - .ifNotNull(); - invokeAndUnwrapOptional(builder, 1, function); - builder.endIf(); - } - - private static void inlineOr(CFGBuilder builder, - PsiExpression function) { - builder - .dup() - .ifNull() - .pop(); - invokeAndUnwrapOptional(builder, 0, function); - builder.endIf(); - } - - private static void inlineMap(CFGBuilder builder, PsiExpression function) { + private static void inlineMap(CFGBuilder builder, PsiExpression function, boolean forceNotNullResult) { builder .evaluateFunction(function) .dup() .ifNotNull() - .invokeFunction(1, function) - .endIf(); - } - - private static void inlineFilter(CFGBuilder builder, PsiExpression function) { - builder - .evaluateFunction(function) - .dup() - .ifNotNull() - .dup() - .invokeFunction(1, function) - .ifConditionIs(false) - .pop() - .pushNull() - .endIf() + .invokeFunction(1, function, forceNotNullResult) .endIf(); } @@ -243,11 +255,7 @@ public class OptionalChainInliner implements CallInliner { .invoke(qualifierCall) // ... arg, opt -- keep original call in CFG so some warnings like "ofNullable for null" can work .pop(); // ... arg if ("of".equals(qualifierCall.getMethodExpression().getReferenceName())) { - builder.dup() - .ifNull() - .pop() - .pushUnknown() - .endIf(); + builder.checkNotNull(argument, NullabilityProblem.passingNullableToNotNullParameter); } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckNotNullInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckNotNullInstruction.java index adce98856b58..50c9ca23bffc 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckNotNullInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckNotNullInstruction.java @@ -39,4 +39,9 @@ public class CheckNotNullInstruction extends Instruction { public DfaInstructionState[] accept(DataFlowRunner runner, DfaMemoryState stateBefore, InstructionVisitor visitor) { return visitor.visitCheckNotNull(this, runner, stateBefore); } + + @Override + public String toString() { + return "CHECK_NOT_NULL ["+myProblem+"] "+myExpression.getText(); + } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java index 36299e7c0a6d..f8e87467cc92 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java @@ -324,7 +324,7 @@ class OptionalWithoutIsPresent { } } - void guavaTest(com.google.common.base.Optional opt, String s) { + 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()); @@ -333,7 +333,8 @@ class OptionalWithoutIsPresent { 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()) { System.out.println(opt.get()); } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java index 749117d65d3b..97c7443a85fb 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java @@ -20,6 +20,34 @@ public class OptionalInlining { } } + void testGuavaOr() { + String res = com.google.common.base.Optional.of("xyz").or("foo"); + if(res.equals("xyz")) { + System.out.println("Always"); + } + if(res.equals("foo")) { + System.out.println("Never"); + } + String res2 = com.google.common.base.Optional.absent().or(() -> "foo"); + if(res2.equals("xyz")) { + System.out.println("Never"); + } + if(res2.equals("foo")) { + System.out.println("Always"); + } + } + + void testGuavaOrNull() { + String s = com.google.common.base.Optional.fromNullable("foo").orNull(); + if (s == null) { + System.out.println("Never"); + } + if (s.equals("foo")) { + System.out.println("Always"); + } + } + + void testIsPresent(Optional opt) { if (!opt.isPresent() && opt.orElse("foo").equals("bar")) { @@ -59,6 +87,7 @@ public class OptionalInlining { if (abc.equals("xyz") && opt.isPresent()) { System.out.println("always"); } + opt.filter(x -> x.length() > 5).filter(x -> x.isEmpty()).ifPresent(x -> System.out.println(x)); } @Nullable @@ -69,6 +98,11 @@ public class OptionalInlining { return ""; } + @Nullable + Optional nullableOptionalMethod(String x) { + return x.isEmpty() ? null : Optional.of(x); + } + @Nullable Object getObj(String s) { return new Object(); @@ -91,10 +125,39 @@ public class OptionalInlining { System.out.println(xyz.trim()); } xyz.trim(); + //opt.map(x -> x.isEmpty() ? "foo" : "bar").filter(x -> x.isEmpty()).ifPresent(x -> System.out.println(x)); + } + + void testGuavaTransform(com.google.common.base.Optional opt) { + String trimmed = com.google.common.base.Optional.fromNullable(nullableMethod()).transform(xx -> xx.trim()).or(""); + if(trimmed == null) { + System.out.println("impossible"); + } + if(opt.isPresent()) { + if(opt.transform(x -> x.isEmpty() ? null : x).toJavaUtil().isPresent()) { + System.out.println("Always"); + } + if(opt.toJavaUtil().map(x -> x.isEmpty() ? null : x).isPresent()) { + System.out.println("Sometimes"); + } + } + } + + void testToJavaUtil() { + String xyz = nullableMethod(); + Object n = com.google.common.base.Optional.fromNullable(xyz).transform(String::trim).toJavaUtil().map(this::getObj).orElse(null); + if(n instanceof Integer) { + // n instanceof Integer -> n is not null -> xyz was not null -> safe to dereference + System.out.println(xyz.trim()); + } + xyz.trim(); } void testFlatMap(Optional opt) { opt.flatMap(null); + opt.flatMap(x -> null); + opt.flatMap(this::nullableOptionalMethod); + opt.flatMap(x -> x.isEmpty() ? null : Optional.of(x)); String s = opt.flatMap(str -> Optional.of(str.length() > 10 ? "foo" : "bar")).orElse("baz"); if (s.equals("qux")) { System.out.println("Never"); 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 15c6fe02158d..34781a8755d9 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java @@ -74,6 +74,17 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { "public class Assert {" + " public static void assertTrue(boolean b) {}" + "}"); + addGuava(); + doTest(); + } + + private void addGuava() { + 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" + @@ -82,8 +93,12 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { " 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" + "}"); - doTest(); } public void testPrimitiveInVoidLambda() { doTest(); } @@ -162,7 +177,11 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { } public void testLambdaInlining() { doTest(); } - public void testOptionalInlining() { doTest(); } + + public void testOptionalInlining() { + addGuava(); + doTest(); + } public void testStreamInlining() { doTest(); } public void testMethodVsExpressionTypeAnnotationConflict() { diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 9f4dcb160154..6443ecb8003d 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -91,6 +91,7 @@ dataflow.message.return.null.from.notnull=null is returned by the m dataflow.message.return.nullable.from.notnull=Expression #ref might evaluate to null but is returned by the method declared as @{0} dataflow.message.return.null.from.notnullable=null is returned by the method which is not declared as @{0} dataflow.message.return.nullable.from.notnullable=Expression #ref might evaluate to null but is returned by the method which is not declared as @{0} +dataflow.message.return.nullable.from.notnull.function=Function may return null, but it's not allowed here dataflow.message.unboxing=Unboxing of #ref #loc may produce java.lang.NullPointerException dataflow.message.unboxing.method.reference=Use of #ref #loc would need unboxing which may produce java.lang.NullPointerException dataflow.too.complex=Method #ref is too complex to analyze by data flow algorithm