From 20efa22b743ea11a9fef740947f8390d39587d9d Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 26 Nov 2018 18:03:05 +0700 Subject: [PATCH] DataFlowInspection: ProblemReporter to avoid duplicate reports on the same anchor --- .../dataFlow/DataFlowInspectionBase.java | 296 +++++++++--------- .../dataFlow/DataFlowInspection.java | 4 +- 2 files changed, 152 insertions(+), 148 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 58532139130f..47e8807c2669 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 @@ -262,29 +262,29 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool allProblems.addAll(falseSet); StreamEx.of(runner.getInstructions()).select(InstanceofInstruction.class).filter(visitor::isInstanceofRedundant).into(allProblems); - HashSet reportedAnchors = new HashSet<>(); + ProblemReporter reporter = new ProblemReporter(holder); - reportFailingCasts(holder, visitor, reportedAnchors); + reportFailingCasts(reporter, visitor); reportUnreachableSwitchBranches(trueSet, falseSet, holder); for (Instruction instruction : allProblems) { if (instruction instanceof BranchingInstruction) { - handleBranchingInstruction(holder, visitor, trueSet, reportedAnchors, (BranchingInstruction)instruction); + handleBranchingInstruction(reporter, visitor, trueSet, (BranchingInstruction)instruction); } } - reportAlwaysFailingCalls(holder, visitor, reportedAnchors); + reportAlwaysFailingCalls(reporter, visitor); - reportNullabilityProblems(holder, visitor, reportedAnchors); - reportNullableReturns(visitor, holder, reportedAnchors, scope); + reportNullabilityProblems(reporter, visitor); + reportNullableReturns(visitor, reporter, scope); if (SUGGEST_NULLABLE_ANNOTATIONS) { - reportNullableArgumentsPassedToNonAnnotated(visitor, holder, reportedAnchors); - reportNullableAssignedToNonAnnotatedFields(visitor, holder, reportedAnchors); + reportNullableArgumentsPassedToNonAnnotated(visitor, reporter); + reportNullableAssignedToNonAnnotatedFields(visitor, reporter); } - reportOptionalOfNullableImprovements(holder, reportedAnchors, visitor.getOfNullableCalls()); + reportOptionalOfNullableImprovements(reporter, visitor.getOfNullableCalls()); - reportConstants(holder, visitor, reportedAnchors); + reportConstants(reporter, visitor); reportMethodReferenceProblems(holder, visitor); @@ -296,13 +296,13 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool reportAlwaysReturnsNotNull(holder, scope); } - reportMutabilityViolations(holder, reportedAnchors, visitor.getMutabilityViolations(true), + reportMutabilityViolations(holder, visitor.getMutabilityViolations(true), InspectionsBundle.message("dataflow.message.immutable.modified")); - reportMutabilityViolations(holder, reportedAnchors, visitor.getMutabilityViolations(false), + reportMutabilityViolations(holder, visitor.getMutabilityViolations(false), InspectionsBundle.message("dataflow.message.immutable.passed")); - reportDuplicateAssignments(holder, reportedAnchors, visitor); - reportPointlessSameArguments(holder, reportedAnchors, visitor); + reportDuplicateAssignments(reporter, visitor); + reportPointlessSameArguments(reporter, visitor); } private void reportUnreachableSwitchBranches(Set trueSet, Set falseSet, ProblemsHolder holder) { @@ -336,16 +336,16 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } } - private void reportConstants(ProblemsHolder holder, DataFlowInstructionVisitor visitor, HashSet reportedAnchors) { + private void reportConstants(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { visitor.getConstantExpressions().forEach((expression, result) -> { if (result == ConstantResult.UNKNOWN) return; if (isCondition(expression)) { if (result.value() instanceof Boolean) { - reportConstantBoolean(holder, expression, reportedAnchors, (Boolean)result.value()); + reportConstantBoolean(reporter, expression, (Boolean)result.value()); } } else { - reportConstantReferenceValue(holder, reportedAnchors, expression, result); + reportConstantReferenceValue(reporter, expression, result); } }); } @@ -366,11 +366,9 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return false; } - private void reportConstantReferenceValue(ProblemsHolder holder, Set reportedAnchors, - PsiExpression ref, ConstantResult constant) { + private void reportConstantReferenceValue(ProblemReporter reporter, PsiExpression ref, ConstantResult constant) { if (!REPORT_CONSTANT_REFERENCE_VALUES && ref instanceof PsiReferenceExpression) return; - if (shouldBeSuppressed(ref)) return; - if (constant == ConstantResult.UNKNOWN || !reportedAnchors.add(ref)) return; + if (shouldBeSuppressed(ref) || constant == ConstantResult.UNKNOWN) return; List fixes = new SmartList<>(); String presentableName = constant.toString(); fixes.add(new ReplaceWithConstantValueFix(presentableName, presentableName)); @@ -380,7 +378,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool if (value instanceof Boolean) { ContainerUtil.addIfNotNull(fixes, createReplaceWithNullCheckFix(ref, (Boolean)value)); } - if (holder.isOnTheFly()) { + if (reporter.isOnTheFly()) { if (ref instanceof PsiReferenceExpression) { fixes.add(new SetInspectionOptionFix(this, "REPORT_CONSTANT_REFERENCE_VALUES", InspectionsBundle.message("inspection.data.flow.turn.off.constant.references.quickfix"), @@ -402,27 +400,23 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool type = ProblemHighlightType.WEAK_WARNING; valueText = "Value"; } - holder.registerProblem(ref, MessageFormat.format("{0} #ref #loc is always ''{1}''", valueText, presentableName), - type, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + reporter.registerProblem(ref, MessageFormat.format("{0} #ref #loc is always ''{1}''", valueText, presentableName), + type, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } - private static void reportPointlessSameArguments(ProblemsHolder holder, - HashSet reportedAnchors, - DataFlowInstructionVisitor visitor) { + private static void reportPointlessSameArguments(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { visitor.pointlessSameArguments().forEach(expr -> { PsiElement name = expr.getReferenceNameElement(); - if (name != null && reportedAnchors.add(name)) { - holder.registerProblem(name, InspectionsBundle.message("dataflow.message.pointless.same.arguments")); + if (name != null) { + reporter.registerProblem(name, InspectionsBundle.message("dataflow.message.pointless.same.arguments")); } }); } - private void reportDuplicateAssignments(ProblemsHolder holder, - HashSet reportedAnchors, - DataFlowInstructionVisitor visitor) { + private void reportDuplicateAssignments(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { visitor.sameValueAssignments().forEach(expr -> { expr = PsiUtil.skipParenthesizedExprDown(expr); - if(expr == null || !reportedAnchors.add(expr)) return; + if (expr == null) return; PsiAssignmentExpression assignment = PsiTreeUtil.getParentOfType(expr, PsiAssignmentExpression.class); PsiElement context = PsiTreeUtil.getParentOfType(expr, PsiForStatement.class, PsiClassInitializer.class); if (context instanceof PsiForStatement && PsiTreeUtil.isAncestor(((PsiForStatement)context).getInitialization(), expr, true)) { @@ -442,28 +436,21 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } } } - holder.registerProblem(expr, InspectionsBundle.message("dataflow.message.redundant.assignment"), createRemoveAssignmentFix(assignment)); + reporter.registerProblem(expr, InspectionsBundle.message("dataflow.message.redundant.assignment"), createRemoveAssignmentFix(assignment)); }); } - private void reportMutabilityViolations(ProblemsHolder holder, - Set reportedAnchors, - Set violations, - String message) { + private void reportMutabilityViolations(ProblemsHolder holder, Set violations, String message) { for (PsiElement violation : violations) { - if (reportedAnchors.add(violation)) { - holder.registerProblem(violation, message, createMutabilityViolationFix(holder, violation)); - } + holder.registerProblem(violation, message, createMutabilityViolationFix(violation, holder.isOnTheFly())); } } - protected LocalQuickFix createMutabilityViolationFix(ProblemsHolder holder, PsiElement violation) { + protected LocalQuickFix createMutabilityViolationFix(PsiElement violation, boolean onTheFly) { return null; } - private void reportNullabilityProblems(ProblemsHolder holder, - DataFlowInstructionVisitor visitor, - HashSet reportedAnchors) { + private void reportNullabilityProblems(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { Map expressions = visitor.getConstantExpressions(); visitor.problems().forEach(problem -> { if (NullabilityProblemKind.passingNullableArgumentToNonAnnotatedParameter.isMyProblem(problem) || @@ -472,47 +459,44 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool // these kinds are still reported separately return; } - if (!reportedAnchors.add(problem.getAnchor())) return; - if (problem.getAnchor() instanceof PsiParenthesizedExpression) { - reportedAnchors.add(PsiUtil.skipParenthesizedExprDown((PsiExpression)problem.getAnchor())); - } NullabilityProblemKind.innerClassNPE.ifMyProblem(problem, newExpression -> { - List fixes = createNPEFixes(newExpression.getQualifier(), newExpression, holder.isOnTheFly()); - holder.registerProblem(getElementToHighlight(newExpression), problem.getMessage(expressions), fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + List fixes = createNPEFixes(newExpression.getQualifier(), newExpression, reporter.isOnTheFly()); + reporter.registerProblem(getElementToHighlight(newExpression), problem.getMessage(expressions), fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); }); NullabilityProblemKind.callMethodRefNPE.ifMyProblem(problem, methodRef -> - holder.registerProblem(methodRef, InspectionsBundle.message("dataflow.message.npe.methodref.invocation"), - createMethodReferenceNPEFixes(methodRef, holder.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY))); - NullabilityProblemKind.callNPE.ifMyProblem(problem, call -> reportCallMayProduceNpe(holder, problem.getMessage(expressions), call)); - NullabilityProblemKind.passingNullableToNotNullParameter.ifMyProblem(problem, expr -> reportNullableArgument(holder, expr, expressions)); + reporter.registerProblem(methodRef, InspectionsBundle.message("dataflow.message.npe.methodref.invocation"), + createMethodReferenceNPEFixes(methodRef, reporter.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY))); + NullabilityProblemKind.callNPE.ifMyProblem(problem, call -> reportCallMayProduceNpe(reporter, problem.getMessage(expressions), call)); + NullabilityProblemKind.passingNullableToNotNullParameter.ifMyProblem(problem, expr -> reportNullableArgument(reporter, expr, expressions)); NullabilityProblemKind.arrayAccessNPE.ifMyProblem(problem, expression -> { LocalQuickFix[] fix = - createNPEFixes(expression.getArrayExpression(), expression, holder.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY); - holder.registerProblem(expression, problem.getMessage(expressions), fix); + createNPEFixes(expression.getArrayExpression(), expression, reporter.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY); + reporter.registerProblem(expression, problem.getMessage(expressions), fix); }); NullabilityProblemKind.fieldAccessNPE.ifMyProblem(problem, element -> { PsiElement parent = element.getParent(); PsiExpression fieldAccess = parent instanceof PsiReferenceExpression ? (PsiExpression)parent : element; - LocalQuickFix[] fix = createNPEFixes(element, fieldAccess, holder.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY); - holder.registerProblem(element, problem.getMessage(expressions), fix); + LocalQuickFix[] fix = createNPEFixes(element, fieldAccess, reporter.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY); + reporter.registerProblem(element, problem.getMessage(expressions), fix); }); NullabilityProblemKind.unboxingNullable.ifMyProblem(problem, element -> { if (element instanceof PsiTypeCastExpression && ((PsiTypeCastExpression)element).getType() instanceof PsiPrimitiveType) { element = Objects.requireNonNull(((PsiTypeCastExpression)element).getOperand()); } - holder.registerProblem(element, problem.getMessage(expressions)); + reporter.registerProblem(element, problem.getMessage(expressions)); }); - NullabilityProblemKind.nullableFunctionReturn.ifMyProblem(problem, expr -> holder.registerProblem(expr, problem.getMessage(expressions))); - NullabilityProblemKind.assigningToNotNull.ifMyProblem(problem, expr -> reportNullabilityProblem(holder, problem, expr, expressions)); - NullabilityProblemKind.storingToNotNullArray.ifMyProblem(problem, expr -> reportNullabilityProblem(holder, problem, expr, expressions)); + NullabilityProblemKind.nullableFunctionReturn.ifMyProblem(problem, expr -> reporter.registerProblem(expr, problem.getMessage(expressions))); + NullabilityProblemKind.assigningToNotNull.ifMyProblem(problem, expr -> reportNullabilityProblem(reporter, problem, expr, expressions)); + NullabilityProblemKind.storingToNotNullArray.ifMyProblem(problem, expr -> reportNullabilityProblem(reporter, problem, expr, expressions)); }); } - private void reportNullabilityProblem(ProblemsHolder holder, + private void reportNullabilityProblem(ProblemReporter reporter, NullabilityProblem problem, PsiExpression expr, Map expressions) { - holder.registerProblem(expr, problem.getMessage(expressions), createNPEFixes(expr, expr, holder.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY)); + LocalQuickFix[] fixes = createNPEFixes(expr, expr, reporter.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY); + reporter.registerProblem(expr, problem.getMessage(expressions), fixes); } private static void reportArrayAccessProblems(ProblemsHolder holder, DataFlowInstructionVisitor visitor) { @@ -566,13 +550,10 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool holder.registerProblem(annoName, msg, fixes); } - private static void reportAlwaysFailingCalls(ProblemsHolder holder, - DataFlowInstructionVisitor visitor, - HashSet reportedAnchors) { + private static void reportAlwaysFailingCalls(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { visitor.alwaysFailingCalls().remove(TestUtils::isExceptionExpected).forEach(call -> { - if (reportedAnchors.add(call)) { - holder.registerProblem(getElementToHighlight(call), getContractMessage(JavaMethodContractUtil.getMethodCallContracts(call))); - } + String message = getContractMessage(JavaMethodContractUtil.getMethodCallContracts(call)); + reporter.registerProblem(getElementToHighlight(call), message); }); } @@ -602,31 +583,27 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return call; } - private static void reportOptionalOfNullableImprovements(ProblemsHolder holder, - Set reportedAnchors, - Map nullArgs) { + private static void reportOptionalOfNullableImprovements(ProblemReporter reporter, Map nullArgs) { nullArgs.forEach((anchor, alwaysPresent) -> { if (alwaysPresent == ThreeState.UNSURE) return; - if (reportedAnchors.add(anchor)) { - if (alwaysPresent.toBoolean()) { - holder.registerProblem(anchor, "Passing a non-null argument to Optional", + if (alwaysPresent.toBoolean()) { + reporter.registerProblem(anchor, "Passing a non-null argument to Optional", DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(anchor)); - } else { - holder.registerProblem(anchor, "Passing null argument to Optional", + } + else { + reporter.registerProblem(anchor, "Passing null argument to Optional", DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(anchor)); - } } }); } - private void reportNullableArgumentsPassedToNonAnnotated(DataFlowInstructionVisitor visitor, ProblemsHolder holder, Set reportedAnchors) { + private void reportNullableArgumentsPassedToNonAnnotated(DataFlowInstructionVisitor visitor, ProblemReporter reporter) { for (PsiElement anchor : visitor.problems() .map(NullabilityProblemKind.passingNullableArgumentToNonAnnotatedParameter::asMyProblem).nonNull() .map(NullabilityProblem::getAnchor)) { - if (reportedAnchors.contains(anchor)) continue; if (anchor.getParent() instanceof PsiMethodReferenceExpression) { - holder.registerProblem(anchor.getParent(), "Method reference argument might be null but passed to non-annotated parameter"); + reporter.registerProblem(anchor.getParent(), "Method reference argument might be null but passed to non-annotated parameter"); continue; } @@ -634,7 +611,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool final String text = isNullLiteralExpression(expression) || visitor.getConstantExpressions().get(expression) == ConstantResult.NULL ? "Passing null argument to non-annotated parameter" : "Argument #ref #loc might be null but passed to non-annotated parameter"; - List fixes = createNPEFixes(expression, expression, holder.isOnTheFly()); + List fixes = createNPEFixes(expression, expression, reporter.isOnTheFly()); final PsiElement parent = anchor.getParent(); if (parent instanceof PsiExpressionList) { final int idx = ArrayUtilRt.find(((PsiExpressionList)parent).getExpressions(), anchor); @@ -646,35 +623,30 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool final PsiParameter[] parameters = psiMethod.getParameterList().getParameters(); if (idx < parameters.length) { fixes.add(AddAnnotationPsiFix.createAddNullableFix(parameters[idx])); - holder.registerProblem(anchor, text, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); - reportedAnchors.add(anchor); - reportedAnchors.add(expression); + reporter.registerProblem(anchor, text, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } } } } } - } } - - private void reportNullableAssignedToNonAnnotatedFields(DataFlowInstructionVisitor visitor, ProblemsHolder holder, Set reportedAnchors) { + + private void reportNullableAssignedToNonAnnotatedFields(DataFlowInstructionVisitor visitor, ProblemReporter reporter) { for (PsiElement anchor : visitor.problems() .map(NullabilityProblemKind.assigningNullableValueToNonAnnotatedField::asMyProblem).nonNull() .map(NullabilityProblem::getAnchor)) { - if (reportedAnchors.contains(anchor)) continue; PsiExpression expression = (PsiExpression)anchor; String text = isNullLiteralExpression(expression) || visitor.getConstantExpressions().get(expression) == ConstantResult.NULL ? "Assigning null value to non-annotated field" : "Expression #ref #loc might be null but is assigned to non-annotated field"; - List fixes = createNPEFixes(expression, expression, holder.isOnTheFly()); + List fixes = createNPEFixes(expression, expression, reporter.isOnTheFly()); PsiField field = getAssignedField(anchor); if (field != null) { fixes.add(AddAnnotationPsiFix.createAddNullableFix(field)); - holder.registerProblem(anchor, text, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); - reportedAnchors.add(anchor); + reporter.registerProblem(anchor, text, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } } } @@ -690,44 +662,39 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return null; } - private void reportCallMayProduceNpe(ProblemsHolder holder, - String message, - PsiMethodCallExpression callExpression) { + private void reportCallMayProduceNpe(ProblemReporter reporter, String message, PsiMethodCallExpression callExpression) { PsiReferenceExpression methodExpression = callExpression.getMethodExpression(); - List fixes = createNPEFixes(methodExpression.getQualifierExpression(), callExpression, holder.isOnTheFly()); + List fixes = createNPEFixes(methodExpression.getQualifierExpression(), callExpression, reporter.isOnTheFly()); ContainerUtil.addIfNotNull(fixes, ReplaceWithObjectsEqualsFix.createFix(callExpression, methodExpression)); PsiElement toHighlight = getElementToHighlight(callExpression); - holder.registerProblem(toHighlight, message, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + reporter.registerProblem(toHighlight, message, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } - private static void reportFailingCasts(ProblemsHolder holder, DataFlowInstructionVisitor visitor, HashSet reportedAnchors) { + private static void reportFailingCasts(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { for (TypeCastInstruction instruction : visitor.getClassCastExceptionInstructions()) { - if (reportedAnchors.add(instruction.getExpression().getCastType())) { - PsiTypeCastExpression typeCast = instruction.getExpression(); - PsiExpression operand = typeCast.getOperand(); - PsiTypeElement castType = typeCast.getCastType(); - assert castType != null; - assert operand != null; - holder.registerProblem(castType, InspectionsBundle.message("dataflow.message.cce", operand.getText())); - } + PsiTypeCastExpression typeCast = instruction.getExpression(); + PsiExpression operand = typeCast.getOperand(); + PsiTypeElement castType = typeCast.getCastType(); + assert castType != null; + assert operand != null; + reporter.registerProblem(castType, InspectionsBundle.message("dataflow.message.cce", operand.getText())); } } - private void handleBranchingInstruction(ProblemsHolder holder, + private void handleBranchingInstruction(ProblemReporter reporter, StandardInstructionVisitor visitor, Set trueSet, - HashSet reportedAnchors, BranchingInstruction instruction) { PsiElement psiAnchor = instruction.getPsiAnchor(); if (instruction instanceof InstanceofInstruction && visitor.isInstanceofRedundant((InstanceofInstruction)instruction)) { if (visitor.canBeNull((InstanceofInstruction)instruction)) { - holder.registerProblem(psiAnchor, - InspectionsBundle.message("dataflow.message.redundant.instanceof"), - new RedundantInstanceofFix()); + reporter.registerProblem(psiAnchor, + InspectionsBundle.message("dataflow.message.redundant.instanceof"), + new RedundantInstanceofFix()); } else { - reportConstantBoolean(holder, psiAnchor, reportedAnchors, true); + reportConstantBoolean(reporter, psiAnchor, true); } } else if (psiAnchor != null && @@ -735,13 +702,11 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool !isFlagCheck(psiAnchor)) { boolean evaluatesToTrue = trueSet.contains(instruction); final PsiElement parent = psiAnchor.getParent(); - if (parent instanceof PsiAssignmentExpression && - ((PsiAssignmentExpression)parent).getLExpression() == psiAnchor && - reportedAnchors.add(psiAnchor)) { - holder.registerProblem( + if (parent instanceof PsiAssignmentExpression && ((PsiAssignmentExpression)parent).getLExpression() == psiAnchor) { + reporter.registerProblem( psiAnchor, InspectionsBundle.message("dataflow.message.pointless.assignment.expression", Boolean.toString(evaluatesToTrue)), - createConditionalAssignmentFixes(evaluatesToTrue, (PsiAssignmentExpression)parent, holder.isOnTheFly()) + createConditionalAssignmentFixes(evaluatesToTrue, (PsiAssignmentExpression)parent, reporter.isOnTheFly()) ); } else { @@ -750,43 +715,39 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool if (range != null) { // report rare cases like a == b == c where "a == b" part is constant String message = InspectionsBundle.message("dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); - holder.registerProblem(psiAnchor, range, message); + reporter.registerProblem(psiAnchor, range, message); // do not add to reported anchors if only part of expression was reported } else if (PsiUtil.skipParenthesizedExprUp(psiAnchor.getParent()) instanceof PsiForeachStatement) { // highlighted for-each iterated value means evaluatesToTrue == "collection is always empty" - if (!evaluatesToTrue || !reportedAnchors.add(psiAnchor)) { + if (!evaluatesToTrue) { // loop on always non-empty collection -- nothing to report return; } boolean array = psiAnchor instanceof PsiExpression && ((PsiExpression)psiAnchor).getType() instanceof PsiArrayType; - holder.registerProblem(psiAnchor, array ? - InspectionsBundle.message("dataflow.message.loop.on.empty.array") : - InspectionsBundle.message("dataflow.message.loop.on.empty.collection")); + reporter.registerProblem(psiAnchor, array ? + InspectionsBundle.message("dataflow.message.loop.on.empty.array") : + InspectionsBundle.message("dataflow.message.loop.on.empty.collection")); } else if (!(psiAnchor instanceof PsiMethodReferenceExpression)) { - reportConstantBoolean(holder, psiAnchor, reportedAnchors, evaluatesToTrue); + reportConstantBoolean(reporter, psiAnchor, evaluatesToTrue); } } } } - private void reportConstantBoolean(ProblemsHolder holder, - PsiElement psiAnchor, - HashSet reportedAnchors, - boolean evaluatesToTrue) { + private void reportConstantBoolean(ProblemReporter reporter, PsiElement psiAnchor, boolean evaluatesToTrue) { while (psiAnchor instanceof PsiParenthesizedExpression) { psiAnchor = ((PsiParenthesizedExpression)psiAnchor).getExpression(); } if (psiAnchor == null || shouldBeSuppressed(psiAnchor)) return; boolean isAssertion = isAssertionEffectively(psiAnchor, evaluatesToTrue); if (DONT_REPORT_TRUE_ASSERT_STATEMENTS && isAssertion) return; - if (!reportedAnchors.add(psiAnchor)) return; List fixes = new ArrayList<>(); if (!isCoveredBySurroundingFix(psiAnchor, evaluatesToTrue)) { ContainerUtil.addIfNotNull(fixes, createSimplifyBooleanExpressionFix(psiAnchor, evaluatesToTrue)); - if (isAssertion && holder.isOnTheFly()) { + if (isAssertion && reporter.isOnTheFly()) { fixes.add(new SetInspectionOptionFix(this, "DONT_REPORT_TRUE_ASSERT_STATEMENTS", InspectionsBundle.message("inspection.data.flow.turn.off.true.asserts.quickfix"), true)); } @@ -795,7 +756,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool String message = InspectionsBundle.message(isAtRHSOfBooleanAnd(psiAnchor) ? "dataflow.message.constant.condition.when.reached" : "dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); - holder.registerProblem(psiAnchor, message, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + reporter.registerProblem(psiAnchor, message, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } private static boolean isCoveredBySurroundingFix(PsiElement anchor, boolean evaluatesToTrue) { @@ -875,21 +836,21 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return LocalQuickFix.EMPTY_ARRAY; } - private void reportNullableArgument(ProblemsHolder holder, + private void reportNullableArgument(ProblemReporter reporter, PsiElement anchor, Map expressions) { if (anchor.getParent() instanceof PsiMethodReferenceExpression) { PsiMethodReferenceExpression methodRef = (PsiMethodReferenceExpression)anchor.getParent(); - holder.registerProblem(methodRef, InspectionsBundle.message("dataflow.message.passing.nullable.argument.methodref"), - createMethodReferenceNPEFixes(methodRef, holder.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY)); + reporter.registerProblem(methodRef, InspectionsBundle.message("dataflow.message.passing.nullable.argument.methodref"), + createMethodReferenceNPEFixes(methodRef, reporter.isOnTheFly()).toArray(LocalQuickFix.EMPTY_ARRAY)); } else { PsiExpression expression = PsiUtil.skipParenthesizedExprDown((PsiExpression)anchor); final String text = isNullLiteralExpression(expression) || expressions.get(expression) == ConstantResult.NULL ? InspectionsBundle.message("dataflow.message.passing.null.argument") : InspectionsBundle.message("dataflow.message.passing.nullable.argument"); - List fixes = createNPEFixes(expression, expression, holder.isOnTheFly()); - holder.registerProblem(anchor, text, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + List fixes = createNPEFixes(expression, expression, reporter.isOnTheFly()); + reporter.registerProblem(anchor, text, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } } @@ -901,13 +862,10 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return null; } - private void reportNullableReturns(DataFlowInstructionVisitor visitor, - ProblemsHolder holder, - Set reportedAnchors, - @NotNull PsiElement block) { + private void reportNullableReturns(DataFlowInstructionVisitor visitor, ProblemReporter reporter, @NotNull PsiElement block) { final PsiMethod method = getScopeMethod(block); if (method == null) return; - NullableNotNullManager manager = NullableNotNullManager.getInstance(holder.getProject()); + NullableNotNullManager manager = NullableNotNullManager.getInstance(method.getProject()); NullabilityAnnotationInfo info = manager.findEffectiveNullabilityInfo(method); PsiAnnotation anno = info == null ? null : info.getAnnotation(); Nullability nullability = info == null ? Nullability.UNKNOWN : info.getNullability(); @@ -927,16 +885,14 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool for (NullabilityProblem problem : visitor.problems().map(NullabilityProblemKind.nullableReturn::asMyProblem).nonNull()) { final PsiExpression anchor = problem.getAnchor(); - if (!reportedAnchors.add(anchor)) continue; PsiExpression expr = PsiUtil.skipParenthesizedExprDown(anchor); - reportedAnchors.add(expr); if (nullability == Nullability.NOT_NULL) { String presentable = NullableStuffInspectionBase.getPresentableAnnoName(anno); final String text = isNullLiteralExpression(expr) || visitor.getConstantExpressions().get(expr) == ConstantResult.NULL ? InspectionsBundle.message("dataflow.message.return.null.from.notnull", presentable) : InspectionsBundle.message("dataflow.message.return.nullable.from.notnull", presentable); - holder.registerProblem(anchor, text); + reporter.registerProblem(anchor, text); } else if (AnnotationUtil.isAnnotatingApplicable(anchor)) { final String defaultNullable = manager.getDefaultNullable(); @@ -948,7 +904,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool PsiTreeUtil.getParentOfType(anchor, PsiMethod.class, PsiLambdaExpression.class) instanceof PsiLambdaExpression ? LocalQuickFix.EMPTY_ARRAY : new LocalQuickFix[]{ new AnnotateMethodFix(defaultNullable, ArrayUtil.toStringArray(manager.getNotNulls()))}; - holder.registerProblem(anchor, text, fixes); + reporter.registerProblem(anchor, text, fixes); } } } @@ -1159,4 +1115,52 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool public String getShortName() { return SHORT_NAME; } + + /** + * {@link ProblemsHolder} wrapper to avoid reporting two problems on the same anchor + */ + static class ProblemReporter { + private final Set myReportedAnchors = new HashSet<>(); + private final ProblemsHolder myHolder; + + ProblemReporter(ProblemsHolder holder) { + myHolder = holder; + } + + void registerProblem(PsiElement element, String message, LocalQuickFix... fixes) { + if (register(element)) { + myHolder.registerProblem(element, message, fixes); + } + } + + void registerProblem(PsiElement element, String message, ProblemHighlightType type, LocalQuickFix... fixes) { + if (register(element)) { + myHolder.registerProblem(element, message, type, fixes); + } + } + + void registerProblem(PsiElement element, TextRange range, String message, LocalQuickFix... fixes) { + if (range == null) { + registerProblem(element, message, fixes); + } + else { + myHolder.registerProblem(element, range, message, fixes); + } + } + + private boolean register(PsiElement element) { + if (!myReportedAnchors.add(element)) return false; + if (element instanceof PsiParenthesizedExpression) { + PsiExpression deparenthesized = PsiUtil.skipParenthesizedExprDown((PsiExpression)element); + if (deparenthesized != null) { + myReportedAnchors.add(deparenthesized); + } + } + return true; + } + + boolean isOnTheFly() { + return myHolder.isOnTheFly(); + } + } } 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 1f7767b08124..1deaa5c28205 100644 --- a/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspection.java @@ -72,8 +72,8 @@ public class DataFlowInspection extends DataFlowInspectionBase { } @Override - protected LocalQuickFix createMutabilityViolationFix(ProblemsHolder holder, PsiElement violation) { - return WrapWithMutableCollectionFix.createFix(violation, holder.isOnTheFly()); + protected LocalQuickFix createMutabilityViolationFix(PsiElement violation, boolean onTheFly) { + return WrapWithMutableCollectionFix.createFix(violation, onTheFly); } @Nullable