From 99e97f3207ce63c33b4c950de8cf564040309cdc Mon Sep 17 00:00:00 2001 From: peter Date: Wed, 16 Oct 2013 14:29:30 +0200 Subject: [PATCH] build constructor dfa to check final field not-nullability (IDEA-114828) --- .../guess/impl/GuessManagerImpl.java | 2 +- .../dataFlow/DataFlowInspectionBase.java | 2 +- .../dataFlow/DataFlowRunner.java | 14 +++- .../codeInspection/dataFlow/DfaPsiUtil.java | 72 +++++++++++++++++-- .../codeInspection/dataFlow/DfaUtil.java | 4 +- .../dataFlow/StandardDataFlowRunner.java | 13 ++-- .../dataFlow/ValuableDataFlowRunner.java | 5 ++ .../dataFlow/value/DfaValueFactory.java | 8 ++- .../dataFlow/value/DfaVariableValue.java | 18 +++-- .../extractMethod/ExtractMethodProcessor.java | 22 +++--- .../FinalFieldsInitializedNotNull.java | 35 +++++++++ .../DataFlowInspectionTest.java | 1 + 12 files changed, 159 insertions(+), 37 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldsInitializedNotNull.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java b/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java index 39d7520852c3..d9bfebce70fc 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java @@ -151,7 +151,7 @@ public class GuessManagerImpl extends GuessManager { scope = file; } - DataFlowRunner runner = new DataFlowRunner() { + DataFlowRunner runner = new DataFlowRunner(scope) { @Override protected DfaMemoryState createMemoryState() { return new ExpressionTypeMemoryState(getFactory()); 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 e2466f081291..1987383d310f 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 @@ -148,7 +148,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { PsiClass containingClass = PsiTreeUtil.getParentOfType(scope, PsiClass.class); if (containingClass != null && PsiUtil.isLocalOrAnonymousClass(containingClass)) return; - final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner() { + final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(scope) { @Override protected boolean shouldCheckTimeLimit() { if (!onTheFly) return false; 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 da760c4bda2b..09c4612b0e6e 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 @@ -30,6 +30,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.progress.ProgressManager; +import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.Key; import com.intellij.openapi.util.Pair; import com.intellij.psi.*; @@ -51,13 +52,20 @@ public class DataFlowRunner { private Instruction[] myInstructions; private final MultiMap myNestedClosures = new MultiMap(); private DfaVariableValue[] myFields; - private final DfaValueFactory myValueFactory = new DfaValueFactory(); + private final DfaValueFactory myValueFactory; // Maximum allowed attempts to process instruction. Fail as too complex to process if certain instruction // is executed more than this limit times. public static final int MAX_STATES_PER_BRANCH = 300; - protected DataFlowRunner() { + protected DataFlowRunner(PsiElement block) { + PsiElement parentConstructor = PsiTreeUtil.findFirstParent(block, new Condition() { + @Override + public boolean value(PsiElement psiElement) { + return psiElement instanceof PsiMethod && ((PsiMethod)psiElement).isConstructor(); + } + }); + myValueFactory = new DfaValueFactory(parentConstructor == null); } public DfaValueFactory getFactory() { @@ -207,7 +215,7 @@ public class DataFlowRunner { return !ApplicationManager.getApplication().isUnitTestMode(); } - private DfaInstructionState[] acceptInstruction(InstructionVisitor visitor, DfaInstructionState instructionState) { + protected DfaInstructionState[] acceptInstruction(InstructionVisitor visitor, DfaInstructionState instructionState) { Instruction instruction = instructionState.getInstruction(); if (instruction instanceof MethodCallInstruction) { PsiCallExpression anchor = ((MethodCallInstruction)instruction).getCallExpression(); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java index 8ed7836f0dfd..1f27148d1873 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java @@ -16,6 +16,8 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.NullableNotNullManager; +import com.intellij.codeInspection.dataFlow.instructions.Instruction; +import com.intellij.codeInspection.dataFlow.instructions.ReturnInstruction; import com.intellij.openapi.util.Ref; import com.intellij.psi.*; import com.intellij.psi.search.LocalSearchScope; @@ -23,6 +25,7 @@ import com.intellij.psi.search.searches.ReferencesSearch; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.CachedValueProvider; import com.intellij.psi.util.CachedValuesManager; +import com.intellij.psi.util.PsiModificationTracker; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.util.NullableFunction; import com.intellij.util.containers.ContainerUtil; @@ -31,10 +34,7 @@ import com.intellij.util.containers.Stack; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.Collection; -import java.util.Collections; -import java.util.List; -import java.util.Set; +import java.util.*; public class DfaPsiUtil { @@ -93,6 +93,70 @@ public class DfaPsiUtil { return Nullness.UNKNOWN; } + public static boolean isInitializedNotNull(PsiField field) { + PsiClass containingClass = field.getContainingClass(); + if (containingClass == null) return false; + + PsiMethod[] constructors = containingClass.getConstructors(); + if (constructors.length == 0) return false; + + for (PsiMethod method : constructors) { + if (!getNotNullInitializedFields(method, containingClass).contains(field)) { + return false; + } + } + return true; + } + + private static Set getNotNullInitializedFields(final PsiMethod constructor, PsiClass containingClass) { + final PsiCodeBlock body = constructor.getBody(); + if (body == null) return Collections.emptySet(); + final PsiField[] fields = containingClass.getFields(); + return CachedValuesManager.getCachedValue(constructor, new CachedValueProvider>() { + @Nullable + @Override + public Result> compute() { + final Map map = ContainerUtil.newHashMap(); + final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(body) { + boolean shouldCheck; + + @Override + protected void prepareAnalysis(@NotNull PsiElement psiBlock, Iterable initialStates) { + super.prepareAnalysis(psiBlock, initialStates); + shouldCheck = psiBlock == body; + } + + @Override + protected DfaInstructionState[] acceptInstruction(InstructionVisitor visitor, DfaInstructionState instructionState) { + if (shouldCheck) { + Instruction instruction = instructionState.getInstruction(); + if (instruction instanceof ReturnInstruction && !((ReturnInstruction)instruction).isViaException()) { + for (PsiField field : fields) { + if (!instructionState.getMemoryState().isNotNull(getFactory().getVarFactory().createVariableValue(field, false))) { + map.put(field, false); + } else if (!map.containsKey(field)) { + map.put(field, true); + } + } + } + } + return super.acceptInstruction(visitor, instructionState); + } + }; + final RunnerResult rc = dfaRunner.analyzeMethod(body, new StandardInstructionVisitor()); + Set notNullFields = ContainerUtil.newHashSet(); + if (rc == RunnerResult.OK) { + for (PsiField field : map.keySet()) { + if (map.get(field)) { + notNullFields.add(field); + } + } + } + return Result.create(notNullFields, constructor, PsiModificationTracker.JAVA_STRUCTURE_MODIFICATION_COUNT); + } + }); + } + public static List findAllConstructorInitializers(PsiField field) { final List result = ContainerUtil.createLockFreeCopyOnWriteList(); ContainerUtil.addIfNotNull(result, field.getInitializer()); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java index 16a2a6d8e3a7..c99be10dc11d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java @@ -62,7 +62,7 @@ public class DfaUtil { } else { final ValuableInstructionVisitor visitor = new ValuableInstructionVisitor(context); - RunnerResult runnerResult = new ValuableDataFlowRunner().analyzeMethod(codeBlock, visitor); + RunnerResult runnerResult = new ValuableDataFlowRunner(codeBlock).analyzeMethod(codeBlock, visitor); if (runnerResult == RunnerResult.OK) { result = visitor.myValues; } @@ -90,7 +90,7 @@ public class DfaUtil { return Nullness.UNKNOWN; } final ValuableInstructionVisitor visitor = new ValuableInstructionVisitor(context); - RunnerResult result = new ValuableDataFlowRunner().analyzeMethod(codeBlock, visitor); + RunnerResult result = new ValuableDataFlowRunner(codeBlock).analyzeMethod(codeBlock, visitor); if (result != RunnerResult.OK) { return Nullness.UNKNOWN; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardDataFlowRunner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardDataFlowRunner.java index 2d6ee1b71232..3ada3e849647 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardDataFlowRunner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardDataFlowRunner.java @@ -27,7 +27,6 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.NullableNotNullManager; import com.intellij.codeInspection.dataFlow.instructions.InstanceofInstruction; import com.intellij.codeInspection.dataFlow.instructions.Instruction; -import com.intellij.openapi.util.Pair; import com.intellij.psi.CommonClassNames; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiMethod; @@ -44,6 +43,10 @@ public class StandardDataFlowRunner extends DataFlowRunner { private boolean myInNotNullMethod = false; private boolean myIsInMethod = false; + public StandardDataFlowRunner(PsiElement block) { + super(block); + } + @Override protected void prepareAnalysis(@NotNull PsiElement psiBlock, Iterable initialStates) { myIsInMethod = psiBlock.getParent() instanceof PsiMethod; @@ -78,14 +81,6 @@ public class StandardDataFlowRunner extends DataFlowRunner { return myIsInMethod; } - public boolean problemsDetected(StandardInstructionVisitor visitor) { - final Pair, Set> constConditions = getConstConditionalExpressions(); - return !constConditions.getFirst().isEmpty() - || !constConditions.getSecond().isEmpty() - || !myCCEInstructions.isEmpty() - || !getRedundantInstanceofs(this, visitor).isEmpty(); - } - @NotNull public static Set getRedundantInstanceofs(final DataFlowRunner runner, StandardInstructionVisitor visitor) { HashSet result = new HashSet(1); for (Instruction instruction : runner.getInstructions()) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java index 87aa89759798..efbbbc458254 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java @@ -20,6 +20,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaPsiType; import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.codeInspection.dataFlow.value.DfaValueFactory; import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; +import com.intellij.psi.PsiElement; import com.intellij.psi.PsiExpression; import org.jetbrains.annotations.Nullable; @@ -30,6 +31,10 @@ import java.util.Set; */ public class ValuableDataFlowRunner extends DataFlowRunner { + protected ValuableDataFlowRunner(PsiElement block) { + super(block); + } + @Override protected DfaMemoryState createMemoryState() { return new MyDfaMemoryState(getFactory()); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java index 70cb578bd4fa..d18d27643ddc 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java @@ -42,8 +42,10 @@ public class DfaValueFactory { private final Map, Boolean> myAssignableCache = ContainerUtil.newHashMap(); private final Map, Boolean> myConvertibleCache = ContainerUtil.newHashMap(); private final Map myDfaTypes = ContainerUtil.newHashMap(); + private final boolean myHonorFieldInitializers; - public DfaValueFactory() { + public DfaValueFactory(boolean honorFieldInitializers) { + myHonorFieldInitializers = honorFieldInitializers; myValues.add(null); myVarFactory = new DfaVariableValue.Factory(this); myConstFactory = new DfaConstValue.Factory(this); @@ -52,6 +54,10 @@ public class DfaValueFactory { myRelationFactory = new DfaRelationValue.Factory(this); } + public boolean isHonorFieldInitializers() { + return myHonorFieldInitializers; + } + public DfaValue createTypeValue(@Nullable PsiType type, Nullness nullability) { if (type == null) return DfaUnknownValue.getInstance(); return getTypeFactory().createTypeValue(internType(type), nullability); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java index fa2217a2918b..70f2c483a1d4 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java @@ -159,7 +159,7 @@ public class DfaVariableValue extends DfaValue { return nullability; } - if (var instanceof PsiField && DfaPsiUtil.isFinalField((PsiVariable)var)) { + if (var instanceof PsiField && DfaPsiUtil.isFinalField((PsiVariable)var) && myFactory.isHonorFieldInitializers()) { List initializers = DfaPsiUtil.findAllConstructorInitializers((PsiField)var); if (initializers.isEmpty()) { return Nullness.UNKNOWN; @@ -168,11 +168,13 @@ public class DfaVariableValue extends DfaValue { boolean hasUnknowns = false; for (PsiExpression expression : initializers) { if (!(expression instanceof PsiReferenceExpression)) { - return Nullness.UNKNOWN; + hasUnknowns = true; + continue; } PsiElement target = ((PsiReferenceExpression)expression).resolve(); if (!(target instanceof PsiParameter)) { - return Nullness.UNKNOWN; + hasUnknowns = true; + continue; } if (NullableNotNullManager.isNullable((PsiParameter)target)) { return Nullness.NULLABLE; @@ -181,7 +183,15 @@ public class DfaVariableValue extends DfaValue { hasUnknowns = true; } } - return hasUnknowns ? Nullness.UNKNOWN : Nullness.NOT_NULL; + + if (hasUnknowns) { + if (DfaPsiUtil.isInitializedNotNull((PsiField)var)) { + return Nullness.NOT_NULL; + } + return Nullness.UNKNOWN; + } + + return Nullness.NOT_NULL; } return Nullness.UNKNOWN; diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java index cf428c752d63..701d6bf49970 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java @@ -40,14 +40,16 @@ import com.intellij.openapi.editor.markup.TextAttributes; import com.intellij.openapi.progress.ProgressManager; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.Comparing; -import com.intellij.openapi.util.Pair; import com.intellij.openapi.util.Pass; import com.intellij.openapi.util.TextRange; import com.intellij.openapi.util.text.StringUtil; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.openapi.wm.WindowManager; import com.intellij.psi.*; -import com.intellij.psi.codeStyle.*; +import com.intellij.psi.codeStyle.CodeStyleManager; +import com.intellij.psi.codeStyle.CodeStyleSettingsManager; +import com.intellij.psi.codeStyle.JavaCodeStyleManager; +import com.intellij.psi.codeStyle.VariableKind; import com.intellij.psi.controlFlow.ControlFlowUtil; import com.intellij.psi.impl.source.codeStyle.JavaCodeStyleManagerImpl; import com.intellij.psi.search.GlobalSearchScope; @@ -327,7 +329,6 @@ public class ExtractMethodProcessor implements MatchProvider { } private boolean isNotNull(PsiVariable outputVariable) { - final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(); final PsiCodeBlock block = myElementFactory.createCodeBlock(); for (PsiElement element : myElements) { block.add(element); @@ -335,18 +336,15 @@ public class ExtractMethodProcessor implements MatchProvider { final PsiIfStatement statementFromText = (PsiIfStatement)myElementFactory.createStatementFromText("if (" + outputVariable.getName() + " == null);", null); block.add(statementFromText); + final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(block); final StandardInstructionVisitor visitor = new StandardInstructionVisitor(); final RunnerResult rc = dfaRunner.analyzeMethod(block, visitor); if (rc == RunnerResult.OK) { - if (dfaRunner.problemsDetected(visitor)) { - final Pair,Set> - conditionalExpressions = dfaRunner.getConstConditionalExpressions(); - final Set falseSet = conditionalExpressions.getSecond(); - for (Instruction instruction : falseSet) { - if (instruction instanceof BranchingInstruction) { - if (((BranchingInstruction)instruction).getPsiAnchor().getText().equals(statementFromText.getCondition().getText())) { - return true; - } + final Set falseSet = dfaRunner.getConstConditionalExpressions().getSecond(); + for (Instruction instruction : falseSet) { + if (instruction instanceof BranchingInstruction) { + if (((BranchingInstruction)instruction).getPsiAnchor().getText().equals(statementFromText.getCondition().getText())) { + return true; } } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldsInitializedNotNull.java b/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldsInitializedNotNull.java new file mode 100644 index 000000000000..c940feab8f73 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldsInitializedNotNull.java @@ -0,0 +1,35 @@ +import java.lang.IllegalArgumentException; +import java.lang.Object; + +public class Doo { + private final Object myA; + private final Object myB; + private final Object myC; + + public Doo(Object myA, Object myB, Object c) { + if (myB == null) { +// assert myA != null; + throw new IllegalArgumentException(); + } + assert c != null; + this.myA = myA; + this.myB = myB; + myC = c; + } + + int bar() { + return myC.hashCode(); + } + + + int foo() { + if (myB == null) { + return 2; + } + if (myC != null) { + return 3; + } + + return myA.hashCode(); + } +} diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index f4da7ba94d35..cce1a80092a1 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -91,6 +91,7 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase { public void testNotNullPrimitive() throws Throwable { doTest(); } public void testBoxing128() throws Throwable { doTest(); } public void testFinalFieldsInitializedByAnnotatedParameters() throws Throwable { doTest(); } + public void testFinalFieldsInitializedNotNull() throws Throwable { doTest(); } public void testMultiCatch() throws Throwable { doTest(); } public void testContinueFlushesLoopVariable() throws Throwable { doTest(); }