diff --git a/java/java-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java b/java/java-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java index 405c6f637197..9ec332a8405b 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java @@ -1526,15 +1526,16 @@ class ControlFlowAnalyzer extends JavaElementVisitor { return null; } + boolean isCall = expression instanceof PsiMethodCallExpression; PsiExpression qualifier = refExpr.getQualifierExpression(); if (qualifier == null) { - return myFactory.getVarFactory().createVariableValue(var, false, null); + return myFactory.getVarFactory().createVariableValue(var, false, null, isCall); } if (var instanceof PsiField && var.hasModifierProperty(PsiModifier.FINAL)) { DfaVariableValue qualifierValue = createChainedVariableValue(qualifier); if (qualifierValue != null) { - return myFactory.getVarFactory().createVariableValue(var, false, qualifierValue); + return myFactory.getVarFactory().createVariableValue(var, false, qualifierValue, isCall || qualifierValue.isViaMethods()); } } return null; diff --git a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java index 31fd7d552009..8b49b808dc1e 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java @@ -147,16 +147,13 @@ public class DataFlowInspection extends BaseLocalInspectionTool { Pair, Set> constConditions = runner.getConstConditionalExpressions(); Set trueSet = constConditions.getFirst(); Set falseSet = constConditions.getSecond(); - Set npeSet = runner.getNPEInstructions(); - Set cceSet = runner.getCCEInstructions(); - Set redundantInstanceofs = StandardDataFlowRunner.getRedundantInstanceofs(runner, visitor); ArrayList allProblems = new ArrayList(); allProblems.addAll(trueSet); allProblems.addAll(falseSet); - allProblems.addAll(npeSet); - allProblems.addAll(cceSet); - allProblems.addAll(redundantInstanceofs); + allProblems.addAll(runner.getNPEInstructions()); + allProblems.addAll(runner.getCCEInstructions()); + allProblems.addAll(StandardDataFlowRunner.getRedundantInstanceofs(runner, visitor)); Collections.sort(allProblems, new Comparator() { public int compare(Instruction i1, Instruction i2) { @@ -168,82 +165,105 @@ public class DataFlowInspection extends BaseLocalInspectionTool { for (Instruction instruction : allProblems) { if (instruction instanceof MethodCallInstruction) { - MethodCallInstruction mcInstruction = (MethodCallInstruction)instruction; - if (mcInstruction.getCallExpression() instanceof PsiMethodCallExpression) { - PsiMethodCallExpression callExpression = (PsiMethodCallExpression)mcInstruction.getCallExpression(); - LocalQuickFix[] fix = createNPEFixes(callExpression.getMethodExpression().getQualifierExpression(), callExpression); - - holder.registerProblem(callExpression, - InspectionsBundle.message("dataflow.message.npe.method.invocation"), - fix); - } + reportCallMayProduceNpe(holder, (MethodCallInstruction)instruction); } else if (instruction instanceof FieldReferenceInstruction) { - FieldReferenceInstruction frInstruction = (FieldReferenceInstruction)instruction; - PsiElement elementToAssert = frInstruction.getElementToAssert(); - PsiExpression expression = frInstruction.getExpression(); - if (expression instanceof PsiArrayAccessExpression) { - LocalQuickFix[] fix = createNPEFixes((PsiExpression)elementToAssert, expression); - holder.registerProblem(expression, - InspectionsBundle.message("dataflow.message.npe.array.access"), - fix); - } - else { - LocalQuickFix[] fix = createNPEFixes((PsiExpression)elementToAssert, expression); - holder.registerProblem(elementToAssert, - InspectionsBundle.message("dataflow.message.npe.field.access"), - fix); - } + reportFieldAccessMayProduceNpe(holder, (FieldReferenceInstruction)instruction); } else if (instruction instanceof TypeCastInstruction) { - TypeCastInstruction tcInstruction = (TypeCastInstruction)instruction; - PsiTypeCastExpression typeCast = tcInstruction.getCastExpression(); - holder.registerProblem(typeCast.getCastType(), - InspectionsBundle.message("dataflow.message.cce", typeCast.getOperand().getText())); + reportCastMayFail(holder, (TypeCastInstruction)instruction); } else if (instruction instanceof BranchingInstruction) { - PsiElement psiAnchor = ((BranchingInstruction)instruction).getPsiAnchor(); - boolean underBinary = isAtRHSOfBooleanAnd(psiAnchor); - if (instruction instanceof InstanceofInstruction && visitor.isInstanceofRedundant((InstanceofInstruction)instruction)) { - if (visitor.canBeNull((BinopInstruction)instruction)) { - holder.registerProblem(psiAnchor, - InspectionsBundle.message("dataflow.message.redundant.instanceof"), - new RedundantInstanceofFix()); - } - else { - final LocalQuickFix localQuickFix = createSimplifyBooleanExpressionFix(psiAnchor, true); - holder.registerProblem(psiAnchor, - InspectionsBundle.message(underBinary ? "dataflow.message.constant.condition.whenriched" : "dataflow.message.constant.condition", Boolean.toString(true)), - localQuickFix == null ? null : new LocalQuickFix[]{localQuickFix}); - } - } - else if (psiAnchor instanceof PsiSwitchLabelStatement) { - if (falseSet.contains(instruction)) { - holder.registerProblem(psiAnchor, - InspectionsBundle.message("dataflow.message.unreachable.switch.label")); - } - } - else if (psiAnchor != null && !reportedAnchors.contains(psiAnchor) && !isCompileConstantInIfCondition(psiAnchor)) { - boolean evaluatesToTrue = trueSet.contains(instruction); - if (onTheLeftSideOfConditionalAssignemnt(psiAnchor)) { - holder.registerProblem( - psiAnchor, - InspectionsBundle.message("dataflow.message.pointless.assignment.expression", Boolean.toString(evaluatesToTrue)), - createSimplifyToAssignmentFix() - ); - } - else if (shouldReportConditionAlwaysTrueOrFalse(psiAnchor, evaluatesToTrue)) { - final LocalQuickFix fix = createSimplifyBooleanExpressionFix(psiAnchor, evaluatesToTrue); - String message = InspectionsBundle.message(underBinary ? - "dataflow.message.constant.condition.whenriched" : - "dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); - holder.registerProblem(psiAnchor, message, fix == null ? null : new LocalQuickFix[]{fix}); - } - reportedAnchors.add(psiAnchor); - } + handleBranchingInstruction(holder, visitor, trueSet, falseSet, reportedAnchors, (BranchingInstruction)instruction); } } + reportNullableArguments(runner, holder); + reportNullableAssignments(runner, holder); + reportUnboxedNullables(runner, holder); + reportNullableReturns(runner, holder); + } + + private static void reportCallMayProduceNpe(ProblemsHolder holder, MethodCallInstruction mcInstruction) { + if (mcInstruction.getCallExpression() instanceof PsiMethodCallExpression) { + PsiMethodCallExpression callExpression = (PsiMethodCallExpression)mcInstruction.getCallExpression(); + LocalQuickFix[] fix = createNPEFixes(callExpression.getMethodExpression().getQualifierExpression(), callExpression); + + holder.registerProblem(callExpression, + InspectionsBundle.message("dataflow.message.npe.method.invocation"), + fix); + } + } + + private static void reportFieldAccessMayProduceNpe(ProblemsHolder holder, FieldReferenceInstruction frInstruction) { + PsiElement elementToAssert = frInstruction.getElementToAssert(); + PsiExpression expression = frInstruction.getExpression(); + if (expression instanceof PsiArrayAccessExpression) { + LocalQuickFix[] fix = createNPEFixes((PsiExpression)elementToAssert, expression); + holder.registerProblem(expression, + InspectionsBundle.message("dataflow.message.npe.array.access"), + fix); + } + else { + LocalQuickFix[] fix = createNPEFixes((PsiExpression)elementToAssert, expression); + holder.registerProblem(elementToAssert, + InspectionsBundle.message("dataflow.message.npe.field.access"), + fix); + } + } + + private static void reportCastMayFail(ProblemsHolder holder, TypeCastInstruction instruction) { + PsiTypeCastExpression typeCast = instruction.getCastExpression(); + holder.registerProblem(typeCast.getCastType(), + InspectionsBundle.message("dataflow.message.cce", typeCast.getOperand().getText())); + } + + private void handleBranchingInstruction(ProblemsHolder holder, + StandardInstructionVisitor visitor, + Set trueSet, + Set falseSet, HashSet reportedAnchors, BranchingInstruction instruction) { + PsiElement psiAnchor = instruction.getPsiAnchor(); + boolean underBinary = isAtRHSOfBooleanAnd(psiAnchor); + if (instruction instanceof InstanceofInstruction && visitor.isInstanceofRedundant((InstanceofInstruction)instruction)) { + if (visitor.canBeNull((BinopInstruction)instruction)) { + holder.registerProblem(psiAnchor, + InspectionsBundle.message("dataflow.message.redundant.instanceof"), + new RedundantInstanceofFix()); + } + else { + final LocalQuickFix localQuickFix = createSimplifyBooleanExpressionFix(psiAnchor, true); + holder.registerProblem(psiAnchor, + InspectionsBundle.message(underBinary ? "dataflow.message.constant.condition.when.reached" : "dataflow.message.constant.condition", Boolean.toString(true)), + localQuickFix == null ? null : new LocalQuickFix[]{localQuickFix}); + } + } + else if (psiAnchor instanceof PsiSwitchLabelStatement) { + if (falseSet.contains(instruction)) { + holder.registerProblem(psiAnchor, + InspectionsBundle.message("dataflow.message.unreachable.switch.label")); + } + } + else if (psiAnchor != null && !reportedAnchors.contains(psiAnchor) && !isCompileConstantInIfCondition(psiAnchor)) { + boolean evaluatesToTrue = trueSet.contains(instruction); + if (onTheLeftSideOfConditionalAssignemnt(psiAnchor)) { + holder.registerProblem( + psiAnchor, + InspectionsBundle.message("dataflow.message.pointless.assignment.expression", Boolean.toString(evaluatesToTrue)), + createSimplifyToAssignmentFix() + ); + } + else if (shouldReportConditionAlwaysTrueOrFalse(psiAnchor, evaluatesToTrue) && !visitor.silenceConstantCondition(instruction)) { + final LocalQuickFix fix = createSimplifyBooleanExpressionFix(psiAnchor, evaluatesToTrue); + String message = InspectionsBundle.message(underBinary ? + "dataflow.message.constant.condition.when.reached" : + "dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); + holder.registerProblem(psiAnchor, message, fix == null ? null : new LocalQuickFix[]{fix}); + } + reportedAnchors.add(psiAnchor); + } + } + + private static void reportNullableArguments(StandardDataFlowRunner runner, ProblemsHolder holder) { Set exprs = runner.getNullableArguments(); for (PsiExpression expr : exprs) { final String text = isNullLiteralExpression(expr) @@ -252,22 +272,25 @@ public class DataFlowInspection extends BaseLocalInspectionTool { LocalQuickFix[] fixes = createNPEFixes(expr, expr); holder.registerProblem(expr, text, fixes); } + } - exprs = runner.getNullableAssignments(); - for (PsiExpression expr : exprs) { + private static void reportNullableAssignments(StandardDataFlowRunner runner, ProblemsHolder holder) { + for (PsiExpression expr : runner.getNullableAssignments()) { final String text = isNullLiteralExpression(expr) ? InspectionsBundle.message("dataflow.message.assigning.null") : InspectionsBundle.message("dataflow.message.assigning.nullable"); holder.registerProblem(expr, text); } + } - exprs = runner.getUnboxedNullables(); - for (PsiExpression expr : exprs) { + private static void reportUnboxedNullables(StandardDataFlowRunner runner, ProblemsHolder holder) { + for (PsiExpression expr : runner.getUnboxedNullables()) { holder.registerProblem(expr, InspectionsBundle.message("dataflow.message.unboxing")); } + } - final Set statements = runner.getNullableReturns(); - for (PsiReturnStatement statement : statements) { + private static void reportNullableReturns(StandardDataFlowRunner runner, ProblemsHolder holder) { + for (PsiReturnStatement statement : runner.getNullableReturns()) { final PsiExpression expr = statement.getReturnValue(); if (runner.isInNotNullMethod()) { final String text = isNullLiteralExpression(expr) diff --git a/java/java-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java b/java/java-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index 7d7d19813359..f73bcf4b67ce 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java @@ -36,6 +36,7 @@ import java.util.Set; public class StandardInstructionVisitor extends InstructionVisitor { private final Set myReachable = new THashSet(); private final Set myCanBeNullInInstanceof = new THashSet(); + private final Set myNotToReportReachability = new THashSet(); private final Set myUsefulInstanceofs = new THashSet(); private final FactoryMap myParametersNotNull = new FactoryMap() { @Override @@ -298,7 +299,11 @@ public class StandardInstructionVisitor extends InstructionVisitor { return null; } + if (isViaMethods(dfaLeft) || isViaMethods(dfaRight)) { + myNotToReportReachability.add(instruction); + } myCanBeNullInInstanceof.add(instruction); + ArrayList states = new ArrayList(); final DfaMemoryState trueCopy = memState.createCopy(); @@ -328,6 +333,10 @@ public class StandardInstructionVisitor extends InstructionVisitor { return states.toArray(new DfaInstructionState[states.size()]); } + private static boolean isViaMethods(DfaValue dfa) { + return dfa instanceof DfaVariableValue && ((DfaVariableValue)dfa).isViaMethods(); + } + private void handleInstanceof(InstanceofInstruction instruction, DfaValue dfaRight, DfaValue dfaLeft) { if ((dfaLeft instanceof DfaTypeValue || dfaLeft instanceof DfaNotNullValue) && dfaRight instanceof DfaTypeValue) { final PsiType leftType; @@ -397,4 +406,8 @@ public class StandardInstructionVisitor extends InstructionVisitor { public boolean canBeNull(BinopInstruction instruction) { return myCanBeNullInInstanceof.contains(instruction); } + + public boolean silenceConstantCondition(BranchingInstruction instruction) { + return instruction instanceof BinopInstruction && myNotToReportReachability.contains(instruction); + } } diff --git a/java/java-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java b/java/java-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java index 2a2b9005d819..bddc40d1c32a 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java @@ -33,6 +33,7 @@ import java.util.ArrayList; import java.util.List; public class DfaVariableValue extends DfaValue { + public static class Factory { private final DfaVariableValue mySharedInstance; private final HashMap> myStringToObject; @@ -46,12 +47,13 @@ public class DfaVariableValue extends DfaValue { } public DfaVariableValue createVariableValue(PsiVariable myVariable, boolean isNegated) { - return createVariableValue(myVariable, isNegated, null); + return createVariableValue(myVariable, isNegated, null, false); } - public DfaVariableValue createVariableValue(PsiVariable myVariable, boolean isNegated, @Nullable DfaVariableValue qualifier) { + public DfaVariableValue createVariableValue(PsiVariable myVariable, boolean isNegated, @Nullable DfaVariableValue qualifier, boolean viaMethods) { mySharedInstance.myVariable = myVariable; mySharedInstance.myIsNegated = isNegated; mySharedInstance.myQualifier = qualifier; + mySharedInstance.myViaMethods = viaMethods; String id = mySharedInstance.toString(); ArrayList conditions = myStringToObject.get(id); @@ -65,7 +67,7 @@ public class DfaVariableValue extends DfaValue { } } - DfaVariableValue result = new DfaVariableValue(myVariable, isNegated, myFactory, qualifier); + DfaVariableValue result = new DfaVariableValue(myVariable, isNegated, myFactory, qualifier, viaMethods); if (qualifier != null) { myQualifiersToChainedVariables.putValue(qualifier, result); } @@ -87,12 +89,14 @@ public class DfaVariableValue extends DfaValue { private PsiVariable myVariable; @Nullable private DfaVariableValue myQualifier; private boolean myIsNegated; + private boolean myViaMethods; - private DfaVariableValue(PsiVariable variable, boolean isNegated, DfaValueFactory factory, @Nullable DfaVariableValue qualifier) { + private DfaVariableValue(PsiVariable variable, boolean isNegated, DfaValueFactory factory, @Nullable DfaVariableValue qualifier, boolean viaMethods) { super(factory); myVariable = variable; myIsNegated = isNegated; myQualifier = qualifier; + myViaMethods = viaMethods; } private DfaVariableValue(DfaValueFactory factory) { @@ -111,7 +115,7 @@ public class DfaVariableValue extends DfaValue { } public DfaVariableValue createNegated() { - return myFactory.getVarFactory().createVariableValue(myVariable, !myIsNegated, myQualifier); + return myFactory.getVarFactory().createVariableValue(myVariable, !myIsNegated, myQualifier, myViaMethods); } @SuppressWarnings({"HardCodedStringLiteral"}) @@ -123,6 +127,7 @@ public class DfaVariableValue extends DfaValue { private boolean hardEquals(DfaVariableValue aVar) { return aVar.myVariable == myVariable && aVar.myIsNegated == myIsNegated && + aVar.myViaMethods == myViaMethods && (myQualifier == null ? aVar.myQualifier == null : myQualifier.hardEquals(aVar.myQualifier)); } @@ -130,4 +135,8 @@ public class DfaVariableValue extends DfaValue { public DfaVariableValue getQualifier() { return myQualifier; } + + public boolean isViaMethods() { + return myViaMethods; + } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ChainedFinalFieldAccessorsDfa.java b/java/java-tests/testData/inspection/dataFlow/fixture/ChainedFinalFieldAccessorsDfa.java index 5eb2ef10dcc4..e166ce3f8cd7 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ChainedFinalFieldAccessorsDfa.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ChainedFinalFieldAccessorsDfa.java @@ -15,11 +15,9 @@ public class BrokenAlignment { if (data.inner() != null) { System.out.println(data.inner().hashCode()); System.out.println(data.inner().getText().hashCode()); - /* if (data.inner() != null) { System.out.println(data.inner().hashCode()); } - */ data = new Data(null, null); System.out.println(data.inner().hashCode()); diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 771d65997918..4100c767f268 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -59,7 +59,7 @@ dataflow.message.npe.field.access=Dereference of #ref #loc may prod dataflow.message.cce=Casting {0} to #ref #loc may produce java.lang.ClassCastException dataflow.message.redundant.instanceof=Condition #ref #loc is redundant and can be replaced with != null dataflow.message.constant.condition=Condition #ref #loc is always {0} -dataflow.message.constant.condition.whenriched=Condition #ref #loc is always {0} when reached +dataflow.message.constant.condition.when.reached=Condition #ref #loc is always {0} when reached dataflow.message.unreachable.switch.label=Switch label#ref #loc is unreachable dataflow.message.pointless.assignment.expression=Condition #ref #loc at the left side of assignment expression is always {0}. Can be simplified to normal assignment dataflow.message.passing.null.argument=Passing null argument to parameter annotated as @NotNull