From c8b4ce3e872671583831281fba78087543a85891 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 16 May 2018 16:53:16 +0700 Subject: [PATCH] Correctly set possible targets for exit from finally block --- .../dataFlow/ControlFlowAnalyzer.java | 4 +- .../dataFlow/InstructionVisitor.java | 2 +- .../dataFlow/controlTransfer.kt | 56 +++++++++++-------- .../instructions/ReturnInstruction.java | 6 +- .../fixture/TryFinallyInsideFinally.java | 2 +- .../dataFlow/fixture/TryFinallySimple.java | 51 +++++++++++++++++ .../DataFlowInspection8Test.java | 1 + 7 files changed, 94 insertions(+), 28 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/TryFinallySimple.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 46c97e99d329..63b6eddbe973 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 @@ -1049,7 +1049,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { myTrapStack = myTrapStack.prepend(new InsideFinally(finallyBlock)); finallyBlock.accept(this); - addInstruction(new ControlTransferInstruction(null)); // DfaControlTransferValue is on stack + controlTransfer(new ExitFinallyTransfer(finallyDescriptor), FList.emptyList()); popTrap(InsideFinally.class); } @@ -1088,7 +1088,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { myTrapStack = myTrapStack.prepend(new InsideFinally(resourceList)); startElement(resourceList); addThrows(null, closerExceptions.toArray(PsiClassType.EMPTY_ARRAY)); - addInstruction(new ControlTransferInstruction(null)); // DfaControlTransferValue is on stack + controlTransfer(new ExitFinallyTransfer(twrFinallyDescriptor), FList.emptyList()); // DfaControlTransferValue is on stack finishElement(resourceList); popTrap(InsideFinally.class); } 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 9ba4a33ab6e7..a5a26494dc10 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 @@ -67,7 +67,7 @@ public abstract class InstructionVisitor { public DfaInstructionState[] visitControlTransfer(@NotNull ControlTransferInstruction controlTransferInstruction, @NotNull DataFlowRunner runner, @NotNull DfaMemoryState state) { DfaControlTransferValue transferValue = controlTransferInstruction.getTransfer(); - if (transferValue == null) { + if (transferValue.getTarget() instanceof ExitFinallyTransfer) { transferValue = (DfaControlTransferValue)state.pop(); } return new ControlTransferHandler(state, runner, transferValue.getTarget()).iteration(transferValue.getTraps()) diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/controlTransfer.kt b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/controlTransfer.kt index f9354dae6484..d7b7bf4570f0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/controlTransfer.kt +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/controlTransfer.kt @@ -25,6 +25,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaVariableValue import com.intellij.psi.* import com.intellij.util.containers.FList import java.util.* +import kotlin.collections.ArrayList /** * @author peter @@ -35,50 +36,61 @@ class DfaControlTransferValue(factory: DfaValueFactory, override fun toString() = target.toString() + (if (traps.isEmpty()) "" else " $traps") } -interface TransferTarget +interface TransferTarget { + fun getPossibleTargets() : Collection = emptyList() +} data class ExceptionTransfer(val throwable: DfaPsiType?) : TransferTarget { override fun toString() = "Exception($throwable)" } data class InstructionTransfer(val offset: ControlFlow.ControlFlowOffset, val toFlush: List) : TransferTarget { - override fun toString(): String { - return "-> $offset" + (if (toFlush.isEmpty()) "" else "; flushing $toFlush") - } + override fun getPossibleTargets() = listOf(offset.instructionOffset) + + override fun toString() = "-> $offset" + (if (toFlush.isEmpty()) "" else "; flushing $toFlush") +} +data class ExitFinallyTransfer(private val enterFinally: Trap.EnterFinally) : TransferTarget { + override fun getPossibleTargets() = enterFinally.backLinks.asIterable().flatMap { it.getPossibleTargetIndices() } + .filter { index -> index != enterFinally.jumpOffset.instructionOffset }.toSet() + + override fun toString() = "ExitFinally" } object ReturnTransfer : TransferTarget { override fun toString(): String = "Return" } -open class ControlTransferInstruction(val transfer: DfaControlTransferValue?) : Instruction() { +open class ControlTransferInstruction(val transfer: DfaControlTransferValue) : Instruction() { + init { + transfer.traps.forEach { trap -> trap.link(this) } + } + override fun accept(runner: DataFlowRunner, state: DfaMemoryState, visitor: InstructionVisitor): Array { return visitor.visitControlTransfer(this, runner, state) } - fun getPossibleTargetIndices() : List { - if (transfer == null) return emptyList() - - val result = ArrayList(transfer.traps.flatMap(Trap::getPossibleTargets)) - if (transfer.target is InstructionTransfer) { - result.add(transfer.target.offset.instructionOffset) - } - return result - } + fun getPossibleTargetIndices() = transfer.traps.flatMap(Trap::getPossibleTargets) + transfer.target.getPossibleTargets() fun getPossibleTargetInstructions(allInstructions: Array) = getPossibleTargetIndices().map { allInstructions[it] } - override fun toString() = (if (transfer == null) "RET" else "TRANSFER $transfer")+" [TARGETS: "+getPossibleTargetIndices()+"]" + override fun toString() = "TRANSFER $transfer [targets: ${getPossibleTargetIndices()}]" } sealed class Trap(val anchor: PsiElement) { + open fun link(instruction: ControlTransferInstruction) {} + class TryCatch(tryStatement: PsiTryStatement, val clauses: LinkedHashMap) : Trap(tryStatement) { override fun toString() = "TryCatch -> ${clauses.values}" } - class TryFinally(finallyBlock: PsiCodeBlock, val jumpOffset: ControlFlow.ControlFlowOffset): Trap(finallyBlock) { - override fun toString() = "TryFinally -> $jumpOffset" - } - class TwrFinally(resourceList: PsiResourceList, val jumpOffset: ControlFlow.ControlFlowOffset): Trap(resourceList) { - override fun toString() = "TwrFinally -> $jumpOffset" + abstract class EnterFinally(anchor: PsiElement, val jumpOffset: ControlFlow.ControlFlowOffset): Trap(anchor) { + internal val backLinks = ArrayList() + + override fun link(instruction: ControlTransferInstruction) { + backLinks.add(instruction) + } + + override fun toString() = "${javaClass.simpleName} -> $jumpOffset" } + class TryFinally(finallyBlock: PsiCodeBlock, jumpOffset: ControlFlow.ControlFlowOffset): EnterFinally(finallyBlock, jumpOffset) + class TwrFinally(resourceList: PsiResourceList, jumpOffset: ControlFlow.ControlFlowOffset): EnterFinally(resourceList, jumpOffset) class InsideFinally(finallyBlock: PsiElement): Trap(finallyBlock) { override fun toString() = "InsideFinally" } @@ -89,8 +101,7 @@ sealed class Trap(val anchor: PsiElement) { internal fun getPossibleTargets(): Collection { return when (this) { is TryCatch -> clauses.values.map { it.instructionOffset } - is TryFinally -> listOf(jumpOffset.instructionOffset) - is TwrFinally -> listOf(jumpOffset.instructionOffset) + is EnterFinally -> listOf(jumpOffset.instructionOffset) else -> emptyList() } } @@ -112,6 +123,7 @@ private class ControlTransferHandler(val state: DfaMemoryState, val runner: Data assert((state.pop() as DfaControlTransferValue).target === ReturnTransfer) iteration(tail) } + else -> throw InternalError("Impossible") } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ReturnInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ReturnInstruction.java index a1eea93b2e8d..1c6f2dc9e3fd 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ReturnInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ReturnInstruction.java @@ -16,7 +16,9 @@ package com.intellij.codeInspection.dataFlow.instructions; -import com.intellij.codeInspection.dataFlow.*; +import com.intellij.codeInspection.dataFlow.ControlTransferInstruction; +import com.intellij.codeInspection.dataFlow.DfaControlTransferValue; +import com.intellij.codeInspection.dataFlow.ExceptionTransfer; import com.intellij.psi.PsiElement; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -36,7 +38,7 @@ public class ReturnInstruction extends ControlTransferInstruction { public boolean isViaException() { DfaControlTransferValue transfer = getTransfer(); - return transfer != null && transfer.getTarget() instanceof ExceptionTransfer; + return transfer.getTarget() instanceof ExceptionTransfer; } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallyInsideFinally.java b/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallyInsideFinally.java index dfe1c41b2a31..b87492380f6d 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallyInsideFinally.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallyInsideFinally.java @@ -16,7 +16,7 @@ class Test { } if (!rangeMarkersDisposed) { - foo = "dd"; + foo = "dd"; } } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallySimple.java b/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallySimple.java new file mode 100644 index 000000000000..5bbebd43353b --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/TryFinallySimple.java @@ -0,0 +1,51 @@ +// IDEA-191876 +import org.jetbrains.annotations.*; + +class Test { + public static void runXyz() { + String output = null; + + try { + Runnable shutdownHook = () -> { }; + + try { + output = "foo"; + } + finally { + Runtime.getRuntime().addShutdownHook(new Thread(shutdownHook)); + shutdownHook.run(); + } + } + finally { + System.out.println(output != null ? output.trim() : ""); + } + System.out.println(output.trim()); + } + + @NotNull + public static String getString() { + return "foo"; + } + + public static void test() { + String s = null; + try { + s = getString(); + } + finally { + System.out.println(s == null ? null : s.trim()); + } + System.out.println(s.trim()); + } + + public static void test2(@Nullable String s) { + if (s == null) return; + + try { + System.out.println("Hello"); + } + finally { + System.out.println(s.trim()); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java index 582e10b64914..2682709c363c 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java @@ -228,4 +228,5 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { public void testQueuePeek() { doTest(); } public void testForeachCollectionElement() { doTest(); } public void testContractReturnValues() { doTest(); } + public void testTryFinallySimple() { doTest(); } } \ No newline at end of file