From eb03b51f6d8484ca222efede27e95050b93e0ee0 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Thu, 29 Mar 2018 16:45:22 +0700 Subject: [PATCH] Initialize constructor entry memory state to initializer final memory state Fixes IDEA-186422 Red code green DFA: reference on non-initialized field --- .../dataFlow/ControlFlowAnalyzer.java | 17 ++-- .../dataFlow/DataFlowInspectionBase.java | 58 +++++++++---- .../dataFlow/DataFlowInstructionVisitor.java | 48 ++++++++++- .../dataFlow/InstructionVisitor.java | 4 + .../EndOfInitializerInstruction.java | 27 ++++++ .../dataFlow/value/DfaValueFactory.java | 4 +- .../MergedInitializerAndConstructor.java | 84 +++++++++++++++++++ .../DataFlowInspectionTest.java | 1 + 8 files changed, 215 insertions(+), 28 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/EndOfInitializerInstruction.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/MergedInitializerAndConstructor.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java index 8a75024c1030..97182f9d5288 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java @@ -17,6 +17,7 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; import com.intellij.codeInsight.ExceptionUtil; +import com.intellij.codeInsight.daemon.impl.UnusedSymbolUtil; import com.intellij.codeInspection.dataFlow.inliner.*; import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet; @@ -79,17 +80,14 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } private void buildClassInitializerFlow(PsiClass psiClass, boolean isStatic) { - pushUnknown(); - ConditionalGotoInstruction conditionalGoto = new ConditionalGotoInstruction(null, false, null); - addInstruction(conditionalGoto); for (PsiElement element : psiClass.getChildren()) { if ((element instanceof PsiField || element instanceof PsiClassInitializer) && ((PsiModifierListOwner)element).hasModifierProperty(PsiModifier.STATIC) == isStatic) { element.accept(this); } } + addInstruction(new EndOfInitializerInstruction(isStatic)); addInstruction(new FlushVariableInstruction(null)); - conditionalGoto.setOffset(getInstructionCount()); } @Nullable @@ -97,9 +95,16 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { myCurrentFlow = new ControlFlow(myFactory); try { if(myCodeFragment instanceof PsiClass) { - // if(unknown) { staticInitializer(); } if(unknown) { instanceInitializer(); } + // if(unknown) { staticInitializer(); } else { instanceInitializer(); } + pushUnknown(); + ConditionalGotoInstruction conditionalGoto = new ConditionalGotoInstruction(null, false, null); + addInstruction(conditionalGoto); buildClassInitializerFlow((PsiClass)myCodeFragment, true); + GotoInstruction unconditionalGoto = new GotoInstruction(null); + addInstruction(unconditionalGoto); + conditionalGoto.setOffset(getInstructionCount()); buildClassInitializerFlow((PsiClass)myCodeFragment, false); + unconditionalGoto.setOffset(getInstructionCount()); } else { myCodeFragment.accept(this); } @@ -280,7 +285,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { if (initializer != null) { initializeVariable(field, initializer); } - else if (!field.hasModifierProperty(PsiModifier.FINAL)) { + else if (!field.hasModifierProperty(PsiModifier.FINAL) && !UnusedSymbolUtil.isImplicitWrite(field)) { // initialize with default value DfaVariableValue dfaVariable = myFactory.getVarFactory().createVariableValue(field); addInstruction(new PushInstruction(dfaVariable, null, true)); 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 39eb3ab70c17..c622faaf70b2 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 @@ -24,6 +24,7 @@ import com.intellij.openapi.util.Pair; import com.intellij.openapi.util.WriteExternalException; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; +import com.intellij.psi.impl.source.resolve.JavaResolveUtil; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiTypesUtil; @@ -88,12 +89,46 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool @Override public void visitClass(PsiClass aClass) { if (aClass instanceof PsiTypeParameter) return; - analyzeCodeBlock(aClass, holder); + if (PsiUtil.isLocalOrAnonymousClass(aClass) && !(aClass instanceof PsiEnumConstantInitializer)) return; + + final StandardDataFlowRunner runner = new StandardDataFlowRunner(TREAT_UNKNOWN_MEMBERS_AS_NULLABLE, aClass); + DataFlowInstructionVisitor visitor = + analyzeDfaWithNestedClosures(aClass, holder, runner, Collections.singletonList(runner.createMemoryState())); + List states = visitor.getEndOfInitializerStates(); + for (PsiMethod method : aClass.getConstructors()) { + List initialStates; + PsiMethodCallExpression call = ConstructorUtil.findThisOrSuperCallInConstructor(method); + if (ConstructorUtil.isChainedConstructorCall(call) || (call == null && hasImplicitImpureSuperCall(aClass, method))) { + initialStates = Collections.singletonList(runner.createMemoryState()); + } else { + initialStates = StreamEx.of(states).map(DfaMemoryState::createCopy).toList(); + } + analyzeMethod(method, runner, initialStates); + } + } + + private boolean hasImplicitImpureSuperCall(PsiClass aClass, PsiMethod constructor) { + PsiClass superClass = aClass.getSuperClass(); + if (superClass == null) return false; + PsiElement superCtor = JavaResolveUtil.resolveImaginarySuperCallInThisPlace(constructor, constructor.getProject(), superClass); + if (!(superCtor instanceof PsiMethod)) return false; + return !ControlFlowAnalyzer.isPure((PsiMethod)superCtor); } @Override public void visitMethod(PsiMethod method) { - analyzeCodeBlock(method.getBody(), holder); + if (method.isConstructor()) return; + final StandardDataFlowRunner runner = new StandardDataFlowRunner(TREAT_UNKNOWN_MEMBERS_AS_NULLABLE, method.getBody()); + analyzeMethod(method, runner, Collections.singletonList(runner.createMemoryState())); + } + + private void analyzeMethod(PsiMethod method, StandardDataFlowRunner runner, List initialStates) { + PsiCodeBlock scope = method.getBody(); + if (scope == null) return; + PsiClass containingClass = PsiTreeUtil.getParentOfType(method, PsiClass.class); + if (containingClass != null && PsiUtil.isLocalOrAnonymousClass(containingClass) && !(containingClass instanceof PsiEnumConstantInitializer)) return; + + analyzeDfaWithNestedClosures(scope, holder, runner, initialStates); analyzeNullLiteralMethodArguments(method, holder); } @@ -161,20 +196,10 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } } - private void analyzeCodeBlock(@Nullable final PsiElement scope, ProblemsHolder holder) { - if (scope == null) return; - - PsiClass containingClass = PsiTreeUtil.getNonStrictParentOfType(scope, PsiClass.class); - if (containingClass != null && PsiUtil.isLocalOrAnonymousClass(containingClass) && !(containingClass instanceof PsiEnumConstantInitializer)) return; - - final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(TREAT_UNKNOWN_MEMBERS_AS_NULLABLE, scope); - analyzeDfaWithNestedClosures(scope, holder, dfaRunner, Collections.singletonList(dfaRunner.createMemoryState())); - } - - private void analyzeDfaWithNestedClosures(PsiElement scope, - ProblemsHolder holder, - StandardDataFlowRunner dfaRunner, - Collection initialStates) { + private DataFlowInstructionVisitor analyzeDfaWithNestedClosures(PsiElement scope, + ProblemsHolder holder, + StandardDataFlowRunner dfaRunner, + Collection initialStates) { final DataFlowInstructionVisitor visitor = new DataFlowInstructionVisitor(); final RunnerResult rc = dfaRunner.analyzeMethod(scope, visitor, IGNORE_ASSERT_STATEMENTS, initialStates); if (rc == RunnerResult.OK) { @@ -195,6 +220,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool holder.registerProblem(name, message, ProblemHighlightType.WEAK_WARNING); } } + return visitor; } @NotNull diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java index ae3aba271676..b5b09983e431 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java @@ -6,6 +6,8 @@ import com.intellij.codeInspection.dataFlow.value.*; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.util.Pair; import com.intellij.psi.*; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiTypesUtil; import com.intellij.util.ObjectUtils; import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; @@ -33,6 +35,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { private final Set myArgumentMutabilityViolation = new HashSet<>(); private final Map mySameValueAssigned = new HashMap<>(); private boolean myAlwaysReturnsNotNull = true; + private final List myEndOfInitializerStates = new ArrayList<>(); @Override public DfaInstructionState[] visitAssign(AssignInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { @@ -43,14 +46,16 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { LOG.debug("Non-physical element in assignment instruction: " + left.getParent().getText(), new Throwable()); } } else { - DfaValue dest = memState.peek(); + DfaValue value = memState.peek(); // Reporting of floating zero is skipped, because this produces false-positives on the code like // if(x == -0.0) x = 0.0; - if (dest instanceof DfaVariableValue || (dest instanceof DfaConstValue && !isFloatingZero(((DfaConstValue)dest).getValue()))) { + if (value instanceof DfaVariableValue || (value instanceof DfaConstValue && !isFloatingZero(((DfaConstValue)value).getValue()))) { DfaMemoryState copy = memState.createCopy(); copy.pop(); - DfaValue src = copy.peek(); - boolean sameValue = !copy.applyCondition(runner.getFactory().createCondition(dest, DfaRelationValue.RelationType.NE, src)); + DfaValue target = copy.peek(); + boolean sameValue = + !isAssignmentToDefaultValueInConstructor(instruction, runner, target) && + !copy.applyCondition(runner.getFactory().createCondition(value, DfaRelationValue.RelationType.NE, target)); mySameValueAssigned.merge(left, sameValue, Boolean::logicalAnd); } else { @@ -61,6 +66,29 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return super.visitAssign(instruction, runner, memState); } + private static boolean isAssignmentToDefaultValueInConstructor(AssignInstruction instruction, DataFlowRunner runner, DfaValue target) { + if (!(target instanceof DfaVariableValue)) return false; + DfaVariableValue var = (DfaVariableValue)target; + if (var.getQualifier() != null || !(var.getPsiVariable() instanceof PsiField)) return false; + + // chained assignment like this.a = this.b = 0; is also supported + PsiExpression rExpression = instruction.getRExpression(); + while (rExpression instanceof PsiAssignmentExpression && + ((PsiAssignmentExpression)rExpression).getOperationTokenType().equals(JavaTokenType.EQ)) { + rExpression = ((PsiAssignmentExpression)rExpression).getRExpression(); + } + if (rExpression == null) return false; + DfaValue dest = runner.getFactory().createValue(rExpression); + if (!(dest instanceof DfaConstValue)) return false; + Object value = ((DfaConstValue)dest).getValue(); + + PsiType type = var.getVariableType(); + boolean isDefaultValue = Objects.equals(PsiTypesUtil.getDefaultValue(type), value) || Long.valueOf(0L).equals(value) && PsiType.INT.equals(type); + if (!isDefaultValue) return false; + PsiMethod method = PsiTreeUtil.getParentOfType(rExpression, PsiMethod.class); + return method != null && method.isConstructor(); + } + private static boolean isFloatingZero(Object value) { if (value instanceof Double) { return ((Double)value).doubleValue() == 0.0; @@ -111,6 +139,10 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return receiver ? myReceiverMutabilityViolation : myArgumentMutabilityViolation; } + public List getEndOfInitializerStates() { + return myEndOfInitializerStates; + } + Stream outOfBoundsArrayAccesses() { return StreamEx.ofKeys(myOutOfBoundsArrayAccesses, ThreeState.YES::equals); } @@ -207,6 +239,14 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return super.visitPush(instruction, runner, memState); } + @Override + public DfaInstructionState[] visitEndOfInitializer(EndOfInitializerInstruction instruction, DataFlowRunner runner, DfaMemoryState state) { + if (!instruction.isStatic()) { + myEndOfInitializerStates.add(state.createCopy()); + } + return super.visitEndOfInitializer(instruction, runner, state); + } + public List> getConstantReferenceValues() { List> result = ContainerUtil.newArrayList(); for (PushInstruction instruction : myPossibleVariableValues.keySet()) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java index 66325aa36133..c115aeb152e1 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java @@ -76,6 +76,10 @@ public abstract class InstructionVisitor { .toArray(DfaInstructionState.EMPTY_ARRAY); } + public DfaInstructionState[] visitEndOfInitializer(EndOfInitializerInstruction instruction, DataFlowRunner runner, DfaMemoryState state) { + return nextInstruction(instruction, runner, state); + } + protected static DfaInstructionState[] nextInstruction(Instruction instruction, DataFlowRunner runner, DfaMemoryState memState) { return new DfaInstructionState[]{new DfaInstructionState(runner.getInstruction(instruction.getIndex() + 1), memState)}; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/EndOfInitializerInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/EndOfInitializerInstruction.java new file mode 100644 index 000000000000..b4ed36c40155 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/EndOfInitializerInstruction.java @@ -0,0 +1,27 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.codeInspection.dataFlow.instructions; + +import com.intellij.codeInspection.dataFlow.DataFlowRunner; +import com.intellij.codeInspection.dataFlow.DfaInstructionState; +import com.intellij.codeInspection.dataFlow.DfaMemoryState; +import com.intellij.codeInspection.dataFlow.InstructionVisitor; + +/** + * Marks end of static or instance initializer + */ +public class EndOfInitializerInstruction extends Instruction { + private final boolean myStatic; + + public EndOfInitializerInstruction(boolean isStatic) { + myStatic = isStatic; + } + + public boolean isStatic() { + return myStatic; + } + + @Override + public DfaInstructionState[] accept(DataFlowRunner runner, DfaMemoryState stateBefore, InstructionVisitor visitor) { + return visitor.visitEndOfInitializer(this, runner, stateBefore); + } +} 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 54b460055f55..911fbb9df084 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 @@ -305,8 +305,8 @@ public class DfaValueFactory { FieldChecker(PsiElement context) { PsiMethod method = context instanceof PsiClass ? null : PsiTreeUtil.getParentOfType(context, PsiMethod.class); - myClass = method == null ? null : method.getContainingClass(); - if (myClass == null) { + myClass = method != null ? method.getContainingClass() : context instanceof PsiClass ? (PsiClass)context : null; + if (method == null || myClass == null) { myTrustDirectFieldInitializers = myTrustFieldInitializersInConstructors = myCanInstantiateItself = false; return; } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/MergedInitializerAndConstructor.java b/java/java-tests/testData/inspection/dataFlow/fixture/MergedInitializerAndConstructor.java new file mode 100644 index 000000000000..25512a8b4788 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/MergedInitializerAndConstructor.java @@ -0,0 +1,84 @@ +import java.util.*; +import org.jetbrains.annotations.*; + +class MergedInitializerAndConstructor { + static class Test1 { + private Collection collection2 = null; + + public Test1() { + collection2.add(""); + } + } + + static class Test2 { + private Collection collection2 = null; + + public Test2() { + collection2.add(""); + } + + { + collection2.add(""); //<- warning here + } + } + + static class Test3 { + private Collection collection2 = null; + + public Test3(String s) { + super(); + collection2.add(s); + } + } + + static class Test4 { + private Collection collection2 = null; + + @Contract(pure = true) + public Test4() { + collection2 = new ArrayList<>(); + } + + public Test4(String s) { + this(); + collection2.add(s); + } + } + + static class Super { + Super() { + init(); + } + + void init() {} + } + + static class Test5 extends Super { + private Collection collection2 = null; + + public Test5() { + // implicit super initializes collection2 + collection2.add("foo"); + } + + public Test5(String s) { + super(); + collection2.add(s); + } + + void init() { + collection2 = new ArrayList<>(); + } + } + + static class DoNotWarnOnDefaultInit { + private int x; + private @Nullable Object val; + + DoNotWarnOnDefaultInit() { + // Do not issue "Variable is already assigned" here + x = 0; + val = null; + } + } +} diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java index 2a703758a682..f6eafc97bb3a 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -591,4 +591,5 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { myFixture.addClass("package org.junit; public class Assume { public static void assumeNotNull(Object... objects) {}}"); doTest(); } + public void testMergedInitializerAndConstructor() { doTest(); } }