From 5d245f2dae5ca8038a37b0e217b0d2931bc78714 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 23 Aug 2017 09:58:24 +0700 Subject: [PATCH] MethodCallInstruction cleanup 1. myContext & myCall merged to single field 2. ofNullable stuff moved to DataFlowInspectionBase --- .../dataFlow/DataFlowInspectionBase.java | 57 +++++++++++-------- .../dataFlow/StandardInstructionVisitor.java | 2 +- .../instructions/MethodCallInstruction.java | 55 ++++++------------ .../dataFlow/fixture/OptionalInlining.java | 4 +- 4 files changed, 54 insertions(+), 64 deletions(-) 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 1e660900cab7..cc54027315a0 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 @@ -178,10 +178,11 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { private void analyzeNullLiteralMethodArguments(PsiMethod method, ProblemsHolder holder, boolean isOnTheFly) { if (REPORT_NULLS_PASSED_TO_NOT_NULL_PARAMETER && isOnTheFly) { for (PsiParameter parameter : NullParameterConstraintChecker.checkMethodParameters(method)) { - holder.registerProblem(parameter.getNameIdentifier(), - InspectionsBundle.message("dataflow.method.fails.with.null.argument"), - ProblemHighlightType.GENERIC_ERROR_OR_WARNING, - createNavigateToNullParameterUsagesFix(parameter)); + PsiIdentifier name = parameter.getNameIdentifier(); + if (name != null) { + holder.registerProblem(name, InspectionsBundle.message("dataflow.method.fails.with.null.argument"), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING, createNavigateToNullParameterUsagesFix(parameter)); + } } } } @@ -330,7 +331,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { reportNullableArgumentsPassedToNonAnnotated(visitor, holder, reportedAnchors); } - reportOptionalOfNullableImprovements(holder, reportedAnchors, runner.getInstructions()); + reportOptionalOfNullableImprovements(holder, reportedAnchors, visitor.getOfNullableCalls()); reportUncheckedOptionalGet(holder, visitor.getOptionalCalls(), visitor.getOptionalQualifiers()); @@ -463,27 +464,25 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } } - private static void reportOptionalOfNullableImprovements(ProblemsHolder holder, Set reportedAnchors, Instruction[] instructions) { - for (Instruction instruction : instructions) { - if (instruction instanceof MethodCallInstruction) { - MethodCallInstruction methodCall = (MethodCallInstruction)instruction; - if (methodCall.getArgCount() != 1) continue; - - final PsiElement arg = methodCall.getArgumentAnchor(0); - - if (methodCall.isOptionalAlwaysNullProblem()) { - if (!reportedAnchors.add(arg)) continue; - holder.registerProblem(arg, "Passing null argument to Optional", - DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(arg)); + private static void reportOptionalOfNullableImprovements(ProblemsHolder holder, + Set reportedAnchors, + Map nullArgs) { + nullArgs.forEach((call, nullArg) -> { + PsiElement arg = call.getArgumentAnchor(0); + if (reportedAnchors.add(arg)) { + switch (nullArg) { + case YES: + holder.registerProblem(arg, "Passing null argument to Optional", + DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(arg)); + break; + case NO: + holder.registerProblem(arg, "Passing a non-null argument to Optional", + DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(arg)); + break; + default: } - else if (methodCall.isOptionalAlwaysNotNullProblem()) { - if (!reportedAnchors.add(arg)) continue; - holder.registerProblem(arg, "Passing a non-null argument to Optional", - DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(arg)); - } - } - } + }); } private static void reportConstantReferenceValues(ProblemsHolder holder, StandardInstructionVisitor visitor, Set reportedAnchors) { @@ -932,6 +931,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { 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 myMethodReferenceResults = new HashMap<>(); private final Map myOutOfBoundsArrayAccesses = new HashMap<>(); private final List myOptionalQualifiers = new ArrayList<>(); @@ -956,6 +956,10 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { return myOptionalCalls; } + Map getOfNullableCalls() { + return myOfNullableCalls; + } + Map getBooleanCalls() { return myBooleanCalls; } @@ -1000,6 +1004,11 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } } } + 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; + myOfNullableCalls.merge(instruction, nullArg, ThreeState::merge); + } DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); if (hasNonTrivialFailingContracts(instruction)) { DfaConstValue fail = runner.getFactory().getConstFactory().getContractFail(); 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 daf76c2797d4..9ee4c874c25b 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 @@ -400,7 +400,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { forceNotNull(runner, memState, arg); } } - else if (!instruction.updateOfNullable(memState, arg) && requiredNullability == Nullness.UNKNOWN) { + else if (requiredNullability == Nullness.UNKNOWN) { checkNotNullable(memState, arg, NullabilityProblem.passingNullableArgumentToNonAnnotatedParameter, anchor); } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java index 99106fe42836..fb83bb03a60c 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java @@ -20,6 +20,7 @@ import com.intellij.codeInspection.dataFlow.*; import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.psi.*; import com.intellij.util.ObjectUtils; +import com.siyeh.ig.callMatcher.CallMatcher; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -31,7 +32,6 @@ import java.util.List; public class MethodCallInstruction extends Instruction { private static final Nullness[] EMPTY_NULLNESS_ARRAY = new Nullness[0]; - @Nullable private final PsiCall myCall; @Nullable private final PsiType myType; private final int myArgCount; private final boolean myShouldFlushFields; @@ -40,11 +40,8 @@ public class MethodCallInstruction extends Instruction { private final List myContracts; private final MethodType myMethodType; @Nullable private final DfaValue myPrecalculatedReturnValue; - private final boolean myOfNullable; private final boolean myVarArgCall; private final Nullness[] myArgRequiredNullability; - private boolean myOnlyNullArgs = true; - private boolean myOnlyNotNullArgs = true; private final Nullness myReturnNullability; public enum MethodType { @@ -55,14 +52,12 @@ public class MethodCallInstruction extends Instruction { myContext = context; myContracts = Collections.emptyList(); myMethodType = methodType; - myCall = null; myArgCount = 0; myType = resultType; myShouldFlushFields = false; myPrecalculatedReturnValue = null; myTargetMethod = null; myVarArgCall = false; - myOfNullable = false; myArgRequiredNullability = EMPTY_NULLNESS_ARRAY; myReturnNullability = Nullness.UNKNOWN; } @@ -72,7 +67,6 @@ public class MethodCallInstruction extends Instruction { myMethodType = MethodType.METHOD_REFERENCE_CALL; JavaResolveResult resolveResult = reference.advancedResolve(false); myTargetMethod = ObjectUtils.tryCast(resolveResult.getElement(), PsiMethod.class); - myCall = null; myContracts = Collections.unmodifiableList(contracts); myArgCount = myTargetMethod == null ? 0 : myTargetMethod.getParameterList().getParametersCount(); if (myTargetMethod == null) { @@ -93,7 +87,6 @@ public class MethodCallInstruction extends Instruction { } myVarArgCall = false; // vararg method reference calls are not supported now myPrecalculatedReturnValue = null; - myOfNullable = DfaOptionalSupport.OPTIONAL_OF_NULLABLE.methodReferenceMatches(reference); myArgRequiredNullability = myTargetMethod == null ? EMPTY_NULLNESS_ARRAY : calcArgRequiredNullability(resolveResult.getSubstitutor(), @@ -105,11 +98,10 @@ public class MethodCallInstruction extends Instruction { myContext = call; myContracts = Collections.unmodifiableList(contracts); myMethodType = MethodType.REGULAR_METHOD_CALL; - myCall = call; final PsiExpressionList argList = call.getArgumentList(); PsiExpression[] args = argList != null ? argList.getExpressions() : PsiExpression.EMPTY_ARRAY; myArgCount = args.length; - myType = myCall instanceof PsiCallExpression ? ((PsiCallExpression)myCall).getType() : null; + myType = call instanceof PsiCallExpression ? ((PsiCallExpression)call).getType() : null; JavaResolveResult result = call.resolveMethodGenerics(); myTargetMethod = (PsiMethod)result.getElement(); @@ -126,10 +118,20 @@ public class MethodCallInstruction extends Instruction { myShouldFlushFields = !(call instanceof PsiNewExpression && myType != null && myType.getArrayDimensions() > 0) && !isPureCall(); myPrecalculatedReturnValue = precalculatedReturnValue; - myOfNullable = call instanceof PsiMethodCallExpression && DfaOptionalSupport.OPTIONAL_OF_NULLABLE.test((PsiMethodCallExpression)call); myReturnNullability = call instanceof PsiNewExpression ? Nullness.NOT_NULL : DfaPsiUtil.getElementNullability(myType, myTargetMethod); } + public boolean matches(CallMatcher matcher) { + switch (myMethodType) { + case REGULAR_METHOD_CALL: + return myContext instanceof PsiMethodCallExpression && matcher.test((PsiMethodCallExpression)myContext); + case METHOD_REFERENCE_CALL: + return matcher.methodReferenceMatches((PsiMethodReferenceExpression)myContext); + default: + return false; + } + } + /** * Returns a PsiElement which at best represents an argument with given index * @@ -137,13 +139,13 @@ public class MethodCallInstruction extends Instruction { * @return a PsiElement. Either argument expression or method reference if call is described by method reference */ public PsiElement getArgumentAnchor(int index) { - if (myCall != null) { - PsiExpressionList argumentList = myCall.getArgumentList(); + if (myMethodType == MethodType.REGULAR_METHOD_CALL && myContext instanceof PsiCall) { + PsiExpressionList argumentList = ((PsiCall)myContext).getArgumentList(); if (argumentList != null) { return argumentList.getExpressions()[index]; } } - if (myContext instanceof PsiMethodReferenceExpression) { + if (myMethodType == MethodType.METHOD_REFERENCE_CALL && myContext instanceof PsiMethodReferenceExpression) { return ((PsiMethodReferenceExpression)myContext).getReferenceNameElement(); } return myContext; @@ -243,7 +245,7 @@ public class MethodCallInstruction extends Instruction { @Nullable public PsiCall getCallExpression() { - return myCall; + return myMethodType == MethodType.REGULAR_METHOD_CALL && myContext instanceof PsiCall ? (PsiCall)myContext : null; } @NotNull @@ -272,30 +274,9 @@ public class MethodCallInstruction extends Instruction { case METHOD_REFERENCE_CALL: return "CALL_METHOD_REFERENCE: " + myContext.getText(); case REGULAR_METHOD_CALL: - return "CALL_METHOD: " + (myCall == null ? "null" : myCall.getText()); + return "CALL_METHOD: " + myContext.getText(); default: throw new IllegalStateException("Unexpected method type: " + myMethodType); } } - - public boolean updateOfNullable(DfaMemoryState memState, DfaValue arg) { - if (!myOfNullable) return false; - - if (!memState.isNotNull(arg)) { - myOnlyNotNullArgs = false; - } - if (!memState.isNull(arg)) { - myOnlyNullArgs = false; - } - return true; - } - - public boolean isOptionalAlwaysNullProblem() { - return myOfNullable && myOnlyNullArgs; - } - - public boolean isOptionalAlwaysNotNullProblem() { - return myOfNullable && myOnlyNotNullArgs; - } - } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java index 21d57a61fbf7..eb3491396613 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalInlining.java @@ -129,7 +129,7 @@ public class OptionalInlining { } void testGuavaTransform(com.google.common.base.Optional opt) { - String trimmed = com.google.common.base.Optional.fromNullable(nullableMethod()).transform(xx -> xx.trim()).or(""); + String trimmed = com.google.common.base.Optional.fromNullable(nullableMethod()).transform(xx -> xx.trim()).or(""); if(trimmed == null) { System.out.println("impossible"); } @@ -145,7 +145,7 @@ public class OptionalInlining { void testToJavaUtil() { String xyz = nullableMethod(); - Object n = com.google.common.base.Optional.fromNullable(xyz).transform(String::trim).toJavaUtil().map(this::getObj).orElse(null); + 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());