IDEA-69573 ConstantConditions - Warn when reading a variable that is guaranteed to be null

This commit is contained in:
peter
2013-07-09 16:12:33 +02:00
parent de046ce8d9
commit e5c829c66b
11 changed files with 208 additions and 31 deletions
@@ -1547,7 +1547,7 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
addInstruction(expression.resolve() instanceof PsiField ? new FieldReferenceInstruction(expression, null) : new PopInstruction());
}
addInstruction(new PushInstruction(getExpressionDfaValue(expression), expression));
addInstruction(new PushInstruction(getExpressionDfaValue(expression), expression, PsiUtil.isAccessedForReading(expression)));
finishElement(expression);
}
@@ -1570,6 +1570,7 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
return dfaValue;
}
@NotNull
private DfaValue createDfaValueForAnotherInstanceMemberAccess(PsiReferenceExpression expression, PsiField field) {
DfaValue dfaValue = null;
if (expression.getQualifierExpression() != null) {
@@ -32,6 +32,7 @@ import com.intellij.codeInsight.daemon.impl.quickfix.SimplifyBooleanExpressionFi
import com.intellij.codeInsight.intention.impl.AddNullableAnnotationFix;
import com.intellij.codeInspection.*;
import com.intellij.codeInspection.dataFlow.instructions.*;
import com.intellij.codeInspection.dataFlow.value.DfaConstValue;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.util.Pair;
@@ -41,6 +42,7 @@ import com.intellij.psi.*;
import com.intellij.psi.util.PsiTreeUtil;
import com.intellij.psi.util.PsiUtil;
import com.intellij.util.ArrayUtil;
import com.intellij.util.ArrayUtilRt;
import com.intellij.util.IncorrectOperationException;
import com.intellij.util.SmartList;
import org.jdom.Element;
@@ -57,6 +59,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
public boolean SUGGEST_NULLABLE_ANNOTATIONS = false;
public boolean DONT_REPORT_TRUE_ASSERT_STATEMENTS = false;
public boolean IGNORE_ASSERT_STATEMENTS = false;
public boolean REPORT_CONSTANT_REFERENCE_VALUES = true;
@Override
public JComponent createOptionsPanel() {
@@ -70,6 +73,9 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
if (IGNORE_ASSERT_STATEMENTS) {
node.addContent(new Element("option").setAttribute("name", "IGNORE_ASSERT_STATEMENTS").setAttribute("value", "true"));
}
if (!REPORT_CONSTANT_REFERENCE_VALUES) {
node.addContent(new Element("option").setAttribute("name", "REPORT_CONSTANT_REFERENCE_VALUES").setAttribute("value", "false"));
}
}
@Override
@@ -99,9 +105,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
final StandardInstructionVisitor visitor = new DataFlowInstructionVisitor(dfaRunner);
final RunnerResult rc = dfaRunner.analyzeMethod(scope, visitor, IGNORE_ASSERT_STATEMENTS);
if (rc == RunnerResult.OK) {
if (dfaRunner.problemsDetected(visitor)) {
createDescription(dfaRunner, holder, visitor);
}
createDescription(dfaRunner, holder, visitor);
}
else if (rc == RunnerResult.TOO_COMPLEX) {
if (scope.getParent() instanceof PsiMethod) {
@@ -170,12 +174,14 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
for (Instruction instruction : allProblems) {
if (instruction instanceof MethodCallInstruction) {
reportCallMayProduceNpe(holder, (MethodCallInstruction)instruction);
reportCallMayProduceNpe(holder, (MethodCallInstruction)instruction, reportedAnchors);
}
else if (instruction instanceof FieldReferenceInstruction) {
else if (instruction instanceof FieldReferenceInstruction &&
reportedAnchors.add(((FieldReferenceInstruction)instruction).getElementToAssert())) {
reportFieldAccessMayProduceNpe(holder, (FieldReferenceInstruction)instruction);
}
else if (instruction instanceof TypeCastInstruction) {
else if (instruction instanceof TypeCastInstruction &&
reportedAnchors.add(((TypeCastInstruction)instruction).getCastExpression().getCastType())) {
reportCastMayFail(holder, (TypeCastInstruction)instruction);
}
else if (instruction instanceof BranchingInstruction) {
@@ -183,23 +189,58 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
}
}
reportNullableArguments(runner, holder);
reportNullableAssignments(runner, holder);
reportUnboxedNullables(runner, holder);
reportNullableReturns(runner, holder);
reportNullableArgumentsPassedToNonAnnotated(runner, holder);
reportNullableArguments(runner, holder, reportedAnchors);
reportNullableAssignments(runner, holder, reportedAnchors);
reportUnboxedNullables(runner, holder, reportedAnchors);
reportNullableReturns(runner, holder, reportedAnchors);
reportNullableArgumentsPassedToNonAnnotated(runner, holder, reportedAnchors);
if (REPORT_CONSTANT_REFERENCE_VALUES) {
reportConstantReferenceValues(holder, visitor, reportedAnchors);
}
}
private void reportNullableArgumentsPassedToNonAnnotated(StandardDataFlowRunner runner, ProblemsHolder holder) {
private static void reportConstantReferenceValues(ProblemsHolder holder, StandardInstructionVisitor visitor, Set<PsiElement> reportedAnchors) {
for (Pair<PsiReferenceExpression, DfaConstValue> pair : visitor.getConstantReferenceValues()) {
PsiReferenceExpression ref = pair.first;
if (!reportedAnchors.add(ref)) {
continue;
}
final Object value = pair.second.getValue();
holder.registerProblem(ref, "Value <code>#ref</code> #loc is always '" + value + "'", new LocalQuickFix() {
@NotNull
@Override
public String getName() {
return "Replace with '" + value + "'";
}
@NotNull
@Override
public String getFamilyName() {
return "Replace with constant value";
}
@Override
public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
descriptor.getPsiElement().replace(JavaPsiFacade.getElementFactory(project).createExpressionFromText(String.valueOf(value), null));
}
});
}
}
private void reportNullableArgumentsPassedToNonAnnotated(StandardDataFlowRunner runner, ProblemsHolder holder, Set<PsiElement> reportedAnchors) {
Set<PsiExpression> exprs = runner.getNullableArgumentsPassedToNonAnnotatedParam();
for (PsiExpression expr : exprs) {
if (reportedAnchors.contains(expr)) continue;
final String text = isNullLiteralExpression(expr)
? "Passing <code>null</code> argument to non annotated parameter"
: "Argument <code>#ref</code> #loc might be null but passed to non annotated parameter";
LocalQuickFix[] fixes = createNPEFixes(expr, expr);
final PsiElement parent = expr.getParent();
if (parent instanceof PsiExpressionList) {
final int idx = ArrayUtil.find(((PsiExpressionList)parent).getExpressions(), expr);
final int idx = ArrayUtilRt.find(((PsiExpressionList)parent).getExpressions(), expr);
if (idx > -1) {
final PsiElement gParent = parent.getParent();
if (gParent instanceof PsiCallExpression) {
@@ -210,6 +251,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
final AddNullableAnnotationFix addNullableAnnotationFix = new AddNullableAnnotationFix(parameters[idx]);
fixes = fixes == null ? new LocalQuickFix[]{addNullableAnnotationFix} : ArrayUtil.append(fixes, addNullableAnnotationFix);
holder.registerProblem(expr, text, fixes);
reportedAnchors.add(expr);
}
}
}
@@ -219,9 +261,11 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
}
}
private void reportCallMayProduceNpe(ProblemsHolder holder, MethodCallInstruction mcInstruction) {
private void reportCallMayProduceNpe(ProblemsHolder holder, MethodCallInstruction mcInstruction, Set<PsiElement> reportedAnchors) {
if (mcInstruction.getCallExpression() instanceof PsiMethodCallExpression) {
PsiMethodCallExpression callExpression = (PsiMethodCallExpression)mcInstruction.getCallExpression();
if (!reportedAnchors.add(callExpression)) return;
LocalQuickFix[] fix = createNPEFixes(callExpression.getMethodExpression().getQualifierExpression(), callExpression);
holder.registerProblem(callExpression,
@@ -241,6 +285,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
}
else {
LocalQuickFix[] fix = createNPEFixes((PsiExpression)elementToAssert, expression);
assert elementToAssert != null;
holder.registerProblem(elementToAssert,
InspectionsBundle.message("dataflow.message.npe.field.access"),
fix);
@@ -303,9 +348,11 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
visitor.silenceConstantCondition(psiAnchor);
}
private void reportNullableArguments(StandardDataFlowRunner runner, ProblemsHolder holder) {
private void reportNullableArguments(StandardDataFlowRunner runner, ProblemsHolder holder, Set<PsiElement> reportedAnchors) {
Set<PsiExpression> exprs = runner.getNullableArguments();
for (PsiExpression expr : exprs) {
if (!reportedAnchors.add(expr)) continue;
final String text = isNullLiteralExpression(expr)
? InspectionsBundle.message("dataflow.message.passing.null.argument")
: InspectionsBundle.message("dataflow.message.passing.nullable.argument");
@@ -314,8 +361,10 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
}
}
private static void reportNullableAssignments(StandardDataFlowRunner runner, ProblemsHolder holder) {
private static void reportNullableAssignments(StandardDataFlowRunner runner, ProblemsHolder holder, Set<PsiElement> reportedAnchors) {
for (PsiExpression expr : runner.getNullableAssignments()) {
if (!reportedAnchors.add(expr)) continue;
final String text = isNullLiteralExpression(expr)
? InspectionsBundle.message("dataflow.message.assigning.null")
: InspectionsBundle.message("dataflow.message.assigning.nullable");
@@ -323,15 +372,19 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
}
}
private static void reportUnboxedNullables(StandardDataFlowRunner runner, ProblemsHolder holder) {
private static void reportUnboxedNullables(StandardDataFlowRunner runner, ProblemsHolder holder, Set<PsiElement> reportedAnchors) {
for (PsiExpression expr : runner.getUnboxedNullables()) {
if (!reportedAnchors.add(expr)) continue;
holder.registerProblem(expr, InspectionsBundle.message("dataflow.message.unboxing"));
}
}
private static void reportNullableReturns(StandardDataFlowRunner runner, ProblemsHolder holder) {
private static void reportNullableReturns(StandardDataFlowRunner runner, ProblemsHolder holder, Set<PsiElement> reportedAnchors) {
for (PsiReturnStatement statement : runner.getNullableReturns()) {
final PsiExpression expr = statement.getReturnValue();
assert expr != null;
if (!reportedAnchors.add(expr)) continue;
if (runner.isInNotNullMethod()) {
final String text = isNullLiteralExpression(expr)
? InspectionsBundle.message("dataflow.message.return.null.from.notnull")
@@ -15,10 +15,12 @@
*/
package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.dataFlow.value.DfaConstValue;
import com.intellij.codeInspection.dataFlow.value.DfaRelationValue;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.codeInspection.dataFlow.value.DfaVariableValue;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
/**
* Created by IntelliJ IDEA.
@@ -57,4 +59,6 @@ public interface DfaMemoryState {
boolean isNotNull(DfaVariableValue dfaVar);
@Nullable
DfaConstValue getConstantValue(DfaVariableValue value);
}
@@ -541,6 +541,19 @@ public class DfaMemoryStateImpl implements DfaMemoryState {
return false;
}
@Override
@Nullable
public DfaConstValue getConstantValue(DfaVariableValue value) {
DfaConstValue result = null;
for (DfaValue equal : getEqClassesFor(value)) {
if (equal == value) continue;
DfaConstValue constValue = asConstantValue(equal);
if (constValue == null) return null;
result = constValue;
}
return result;
}
@Override
public boolean applyInstanceofOrNull(DfaRelationValue dfaCond) {
DfaValue left = dfaCond.getLeftOperand();
@@ -886,13 +899,16 @@ public class DfaMemoryStateImpl implements DfaMemoryState {
myVariableStates.remove(varNegated);
}
@Nullable private static DfaConstValue asConstantValue(DfaValue value) {
if (value instanceof DfaConstValue) return (DfaConstValue)value;
if (value instanceof DfaBoxedValue && ((DfaBoxedValue)value).getWrappedValue() instanceof DfaConstValue) return (DfaConstValue)((DfaBoxedValue)value).getWrappedValue();
return null;
}
private boolean containsConstantsOnly(int id) {
SortedIntSet varClass = myEqClasses.get(id);
for (int i = 0; i < varClass.size(); i++) {
int cl = varClass.get(i);
DfaValue value = myFactory.getValue(cl);
if (!(value instanceof DfaConstValue) &&
!(value instanceof DfaBoxedValue && ((DfaBoxedValue)value).getWrappedValue() instanceof DfaConstValue)) {
if (asConstantValue(myFactory.getValue(varClass.get(i))) == null) {
return false;
}
}
@@ -17,29 +17,32 @@ package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.dataFlow.instructions.*;
import com.intellij.codeInspection.dataFlow.value.*;
import com.intellij.openapi.util.Pair;
import com.intellij.psi.*;
import com.intellij.psi.tree.IElementType;
import com.intellij.psi.util.PsiTreeUtil;
import com.intellij.psi.util.TypeConversionUtil;
import com.intellij.util.containers.ContainerUtil;
import com.intellij.util.containers.FactoryMap;
import com.intellij.util.containers.MultiMap;
import com.intellij.util.containers.MultiMapBasedOnSet;
import gnu.trove.THashSet;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.*;
/**
* @author peter
*/
public class StandardInstructionVisitor extends InstructionVisitor {
private static final Object ANY_VALUE = new Object();
private final Set<BinopInstruction> myReachable = new THashSet<BinopInstruction>();
private final Set<BinopInstruction> myCanBeNullInInstanceof = new THashSet<BinopInstruction>();
private final MultiMap<PushInstruction, Object> myPossibleVariableValues = new MultiMapBasedOnSet<PushInstruction, Object>();
private final Set<PsiElement> myNotToReportReachability = new THashSet<PsiElement>();
private final Set<InstanceofInstruction> myUsefulInstanceofs = new THashSet<InstanceofInstruction>();
@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
private final FactoryMap<MethodCallInstruction, Map<PsiExpression, Nullness>> myParametersNullability = new FactoryMap<MethodCallInstruction, Map<PsiExpression, Nullness>>() {
@Nullable
@Override
@@ -47,6 +50,7 @@ public class StandardInstructionVisitor extends InstructionVisitor {
return calcParameterNullability(key.getCallExpression());
}
};
@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
private final FactoryMap<MethodCallInstruction, Nullness> myReturnTypeNullability = new FactoryMap<MethodCallInstruction, Nullness>() {
@Override
protected Nullness create(MethodCallInstruction key) {
@@ -159,6 +163,32 @@ public class StandardInstructionVisitor extends InstructionVisitor {
return nextInstruction(instruction, runner, memState);
}
@Override
public DfaInstructionState[] visitPush(PushInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) {
if (instruction.isReferenceRead()) {
DfaValue dfaValue = instruction.getValue();
if (dfaValue instanceof DfaVariableValue) {
DfaConstValue constValue = memState.getConstantValue((DfaVariableValue)dfaValue);
myPossibleVariableValues.putValue(instruction, constValue != null ? constValue : ANY_VALUE);
}
}
return super.visitPush(instruction, runner, memState);
}
public List<Pair<PsiReferenceExpression, DfaConstValue>> getConstantReferenceValues() {
List<Pair<PsiReferenceExpression, DfaConstValue>> result = ContainerUtil.newArrayList();
for (PushInstruction instruction : myPossibleVariableValues.keySet()) {
Collection<Object> values = myPossibleVariableValues.get(instruction);
if (values.size() == 1) {
Object singleValue = values.iterator().next();
if (singleValue != ANY_VALUE) {
result.add(Pair.create((PsiReferenceExpression)instruction.getPlace(), (DfaConstValue)singleValue));
}
}
}
return result;
}
@Override
public DfaInstructionState[] visitTypeCast(TypeCastInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) {
final DfaValueFactory factory = runner.getFactory();
@@ -31,18 +31,26 @@ import com.intellij.codeInspection.dataFlow.InstructionVisitor;
import com.intellij.codeInspection.dataFlow.value.DfaUnknownValue;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.psi.PsiExpression;
import com.intellij.psi.PsiField;
import com.intellij.psi.PsiReferenceExpression;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
public class PushInstruction extends Instruction {
private final DfaValue myValue;
private final PsiExpression myPlace;
private final boolean myReferenceRead;
public PushInstruction(@Nullable DfaValue value, PsiExpression place) {
this(value, place, false);
}
public PushInstruction(@Nullable DfaValue value, PsiExpression place, final boolean isReferenceRead) {
myValue = value != null ? value : DfaUnknownValue.getInstance();
myPlace = place;
myReferenceRead = isReferenceRead;
}
public boolean isReferenceRead() {
return myReferenceRead;
}
@NotNull
@@ -47,6 +47,7 @@ public class DataFlowInspection extends DataFlowInspectionBase {
private class OptionsPanel extends JPanel {
private final JCheckBox myIgnoreAssertions;
private final JCheckBox myReportConstantReferences;
private final JCheckBox mySuggestNullables;
private final JCheckBox myDontReportTrueAsserts;
@@ -88,6 +89,15 @@ public class DataFlowInspection extends DataFlowInspectionBase {
}
});
myReportConstantReferences = new JCheckBox("Warn when reading a value guaranteed to be constant");
myReportConstantReferences.setSelected(REPORT_CONSTANT_REFERENCE_VALUES);
myReportConstantReferences.getModel().addChangeListener(new ChangeListener() {
@Override
public void stateChanged(ChangeEvent e) {
REPORT_CONSTANT_REFERENCE_VALUES = myReportConstantReferences.isSelected();
}
});
gc.insets = new Insets(0, 0, 0, 0);
gc.gridy = 0;
add(mySuggestNullables, gc);
@@ -134,6 +144,9 @@ public class DataFlowInspection extends DataFlowInspectionBase {
gc.gridy++;
add(myIgnoreAssertions, gc);
gc.gridy++;
add(myReportConstantReferences, gc);
}
}
@@ -0,0 +1,19 @@
import org.jetbrains.annotations.NotNull;
class Test {
private void test2(@NotNull Object bar) {
}
private Object test(Object foo, Object bar) {
if (foo == null) {
System.out.println(<warning descr="Value 'foo' is always 'null'"><caret>foo</warning>);
System.out.println(<warning descr="Value 'foo' is always 'null'">foo</warning>);
return <warning descr="Expression 'foo' might evaluate to null but is returned by the method which isn't declared as @Nullable">foo</warning>;
}
if (bar == null) {
test2(<warning descr="Argument 'bar' might be null">bar</warning>);
}
return foo;
}
}
@@ -0,0 +1,19 @@
import org.jetbrains.annotations.NotNull;
class Test {
private void test2(@NotNull Object bar) {
}
private Object test(Object foo, Object bar) {
if (foo == null) {
System.out.println(<caret>null);
System.out.println(foo);
return foo;
}
if (bar == null) {
test2(bar);
}
return foo;
}
}
@@ -33,14 +33,18 @@ public class DataFlowInspectionAncientTest extends InspectionTestCase {
doTest(false);
}
private void doTest(boolean lowercase) {
doTest("dataFlow/" + getTestName(lowercase), new DataFlowInspection());
DataFlowInspection inspection = new DataFlowInspection();
inspection.REPORT_CONSTANT_REFERENCE_VALUES = false;
doTest("dataFlow/" + getTestName(lowercase), inspection);
}
private void doTest15() {
doTest15(false);
}
private void doTest15(boolean lowercase) {
doTest("dataFlow/" + getTestName(lowercase), new DataFlowInspection(), "java 1.5");
DataFlowInspection inspection = new DataFlowInspection();
inspection.REPORT_CONSTANT_REFERENCE_VALUES = false;
doTest("dataFlow/" + getTestName(lowercase), inspection, "java 1.5");
}
public void testNpe1() { doTest(true); }
@@ -40,6 +40,7 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase {
private void doTest() {
final DataFlowInspection inspection = new DataFlowInspection();
inspection.SUGGEST_NULLABLE_ANNOTATIONS = true;
inspection.REPORT_CONSTANT_REFERENCE_VALUES = false;
myFixture.enableInspections(inspection);
myFixture.testHighlighting(true, false, true, getTestName(false) + ".java");
}
@@ -140,6 +141,15 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase {
myFixture.testHighlighting(true, false, true, getTestName(false) + ".java");
}
public void testReportConstantReferences() {
DataFlowInspection inspection = new DataFlowInspection();
inspection.SUGGEST_NULLABLE_ANNOTATIONS = true;
myFixture.enableInspections(inspection);
myFixture.testHighlighting(true, false, true, getTestName(false) + ".java");
myFixture.launchAction(myFixture.findSingleIntention("Replace with 'null'"));
myFixture.checkResultByFile(getTestName(false) + "_after.java");
}
public void testCheckFieldInitializers() {
doTest();
}