From 38936bdfd440fb0e0696bef5e9e9aa7d002128d3 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 4 Oct 2017 15:06:12 +0700 Subject: [PATCH] BytecodeAnalysisIndex: conservatively handle cases when contract inferred only via exceptional path Fixes IDEA-155544 Wrong inferred contract annotation for Console.readPassword() --- .../bytecodeAnalysis/Analysis.java | 13 ++++- .../BytecodeAnalysisIndex.java | 2 +- .../bytecodeAnalysis/ClassDataIndexer.java | 4 ++ .../bytecodeAnalysis/Contracts.java | 47 ++++++++++--------- .../bytecodeAnalysis/Parameters.java | 45 ++++++++---------- .../bytecodeAnalysis/TooComplexException.java | 14 ++++++ .../annotations/javax/swing/annotations.xml | 3 -- .../org/apache/velocity/util/annotations.xml | 3 -- .../bytecodeAnalysis/data/Test01.java | 34 ++++++++++++++ 9 files changed, 110 insertions(+), 55 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/TooComplexException.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Analysis.java b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Analysis.java index 60a30072c1cb..2cc6ea1d30ad 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Analysis.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Analysis.java @@ -198,12 +198,18 @@ final class State { final boolean taken; final boolean hasCompanions; - State(int index, Conf conf, List history, boolean taken, boolean hasCompanions) { + /** + * Whether this state was reached via an exceptional path (jump to catch block) + */ + final boolean exceptional; + + State(int index, Conf conf, List history, boolean taken, boolean hasCompanions, boolean exceptional) { this.index = index; this.conf = conf; this.history = history; this.taken = taken; this.hasCompanions = hasCompanions; + this.exceptional = exceptional; } } @@ -236,13 +242,16 @@ abstract class Analysis { } final State createStartState() { - return new State(0, new Conf(0, createStartFrame()), new ArrayList<>(), false, false); + return new State(0, new Conf(0, createStartFrame()), new ArrayList<>(), false, false, false); } static boolean stateEquiv(State curr, State prev) { if (curr.taken != prev.taken) { return false; } + if (curr.exceptional != prev.exceptional) { + return false; + } if (curr.conf.fastHashCode != prev.conf.fastHashCode) { return false; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/BytecodeAnalysisIndex.java b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/BytecodeAnalysisIndex.java index 7fc8f7988e2c..82f1591317d1 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/BytecodeAnalysisIndex.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/BytecodeAnalysisIndex.java @@ -52,7 +52,7 @@ public class BytecodeAnalysisIndex extends ScalarIndexExtension { private static final ID NAME = ID.create("bytecodeAnalysis"); private static final HKeyDescriptor KEY_DESCRIPTOR = new HKeyDescriptor(); - private static final int VERSION = 6; // change when inference algorithm changes + private static final int VERSION = 7; // change when inference algorithm changes private static final int VERSION_MODIFIER = HardCodedPurity.AGGRESSIVE_HARDCODED_PURITY ? 1 : 0; private static final int FINAL_VERSION = VERSION * 2 + VERSION_MODIFIER; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/ClassDataIndexer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/ClassDataIndexer.java index a8e2ee05e754..220b6cdf2edf 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/ClassDataIndexer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/ClassDataIndexer.java @@ -252,6 +252,10 @@ public class ClassDataIndexer implements VirtualFileGist.GistCalculator { final Value inValue; private final int generalizeShift; Result internalResult; + boolean exceptionalOnly = true; private int id; private int pendingTop; @@ -81,9 +82,7 @@ abstract class ContractAnalysis extends Analysis { int steps = 0; while (pendingTop > 0 && earlyResult == null) { steps ++; - if (steps >= STEPS_LIMIT) { - throw new AnalyzerException(null, "limit is reached, steps: " + steps + " in method " + method); - } + TooComplexException.check(method, steps); if (steps % 128 == 0) { ProgressManager.checkCanceled(); } @@ -122,6 +121,10 @@ abstract class ContractAnalysis extends Analysis { } if (earlyResult != null) { return mkEquation(earlyResult); + } else if (exceptionalOnly) { + // We are not sure whether exceptional paths were actually taken or not + // probably they handle exceptions which can never be thrown before dereference occurs + return mkEquation(ClassDataIndexer.FINAL_BOT); } else { return mkEquation(internalResult); } @@ -142,46 +145,46 @@ abstract class ContractAnalysis extends Analysis { addComputed(insnIndex, state); int opcode = insnNode.getOpcode(); - if (handleReturn(frame, opcode)) return; + if (handleReturn(frame, opcode, state.exceptional)) return; if (opcode == IFNONNULL && popValue(frame) instanceof ParamValue) { int nextInsnIndex = inValue == Value.Null ? insnIndex + 1 : methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, state.exceptional); pendingPush(nextState); return; } if (opcode == IFNULL && popValue(frame) instanceof ParamValue) { int nextInsnIndex = inValue == Value.NotNull ? insnIndex + 1 : methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, state.exceptional); pendingPush(nextState); return; } if (opcode == IFEQ && popValue(frame) instanceof ParamValue) { int nextInsnIndex = inValue == Value.True ? insnIndex + 1 : methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, state.exceptional); pendingPush(nextState); return; } if (opcode == IFNE && popValue(frame) instanceof ParamValue) { int nextInsnIndex = inValue == Value.False ? insnIndex + 1 : methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, state.exceptional); pendingPush(nextState); return; } if (opcode == IFEQ && popValue(frame) == InstanceOfCheckValue && inValue == Value.Null) { int nextInsnIndex = methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, state.exceptional); pendingPush(nextState); return; } if (opcode == IFNE && popValue(frame) == InstanceOfCheckValue && inValue == Value.Null) { int nextInsnIndex = insnIndex + 1; - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, state.exceptional); pendingPush(nextState); return; } @@ -189,21 +192,21 @@ abstract class ContractAnalysis extends Analysis { // general case for (int nextInsnIndex : controlFlow.transitions[insnIndex]) { Frame nextFrame1 = nextFrame; + boolean exceptional = state.exceptional; if (controlFlow.errors[nextInsnIndex] && controlFlow.errorTransitions.contains(new Edge(insnIndex, nextInsnIndex))) { nextFrame1 = new Frame<>(frame); nextFrame1.clearStack(); nextFrame1.push(ASMUtils.THROWABLE_VALUE); + exceptional = true; } - pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame1), nextHistory, taken, false)); + pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame1), nextHistory, taken, false, exceptional)); } } - abstract boolean handleReturn(Frame frame, int opcode) throws AnalyzerException; + abstract boolean handleReturn(Frame frame, int opcode, boolean exceptional) throws AnalyzerException; - private void pendingPush(State st) throws AnalyzerException { - if (pendingTop >= STEPS_LIMIT) { - throw new AnalyzerException(null, "limit is reached in method " + method); - } + private void pendingPush(State st) { + TooComplexException.check(method, pendingTop); pending[pendingTop++] = st; } @@ -260,7 +263,7 @@ class InOutAnalysis extends ContractAnalysis { super(richControlFlow, direction, resultOrigins, stable, pending); } - boolean handleReturn(Frame frame, int opcode) throws AnalyzerException { + boolean handleReturn(Frame frame, int opcode, boolean exceptional) throws AnalyzerException { if (interpreter.deReferenced) { return true; } @@ -297,7 +300,8 @@ class InOutAnalysis extends ContractAnalysis { return true; } internalResult = checkLimit(resultUtil.join(internalResult, subResult)); - if (internalResult instanceof Final && ((Final)internalResult).value == Value.Top) { + exceptionalOnly &= exceptional; + if (!exceptional && internalResult instanceof Final && ((Final)internalResult).value == Value.Top) { earlyResult = internalResult; } return true; @@ -321,7 +325,7 @@ class InThrowAnalysis extends ContractAnalysis { super(richControlFlow, direction, resultOrigins, stable, pending); } - boolean handleReturn(Frame frame, int opcode) throws AnalyzerException { + boolean handleReturn(Frame frame, int opcode, boolean exceptional) { Result subResult; if (interpreter.deReferenced) { subResult = new Final(Value.Top); @@ -352,7 +356,8 @@ class InThrowAnalysis extends ContractAnalysis { } } internalResult = resultUtil.join(internalResult, subResult); - if (internalResult instanceof Final && ((Final)internalResult).value == Value.Top && myHasNonTrivialReturn) { + exceptionalOnly &= exceptional; + if (!exceptional && internalResult instanceof Final && ((Final)internalResult).value == Value.Top && myHasNonTrivialReturn) { earlyResult = internalResult; } return true; @@ -470,7 +475,7 @@ class InOutInterpreter extends BasicInterpreter { } @Override - public BasicValue ternaryOperation(AbstractInsnNode insn, BasicValue value1, BasicValue value2, BasicValue value3) throws AnalyzerException { + public BasicValue ternaryOperation(AbstractInsnNode insn, BasicValue value1, BasicValue value2, BasicValue value3) { switch (insn.getOpcode()) { case IASTORE: case LASTORE: diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Parameters.java b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Parameters.java index 59021e27a087..f7e1b95cd89b 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Parameters.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Parameters.java @@ -213,9 +213,7 @@ class NonNullInAnalysis extends Analysis { int steps = 0; while (pendingTop > 0 && earlyResult == null) { steps ++; - if (steps >= STEPS_LIMIT) { - throw new AnalyzerException(null, "limit is reached, steps: " + steps + " in method " + method); - } + TooComplexException.check(method, steps); PendingAction action = pendingActions[--pendingTop]; if (action instanceof MakeResult) { MakeResult makeResult = (MakeResult) action; @@ -325,7 +323,7 @@ class NonNullInAnalysis extends Analysis { if (opcode == IFNONNULL && popValue(frame) instanceof ParamValue) { int nextInsnIndex = insnIndex + 1; - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult, state.exceptional); pendingPush(new MakeResult(state, subResult, new int[]{nextState.index})); pendingPush(new ProceedState(nextState)); return; @@ -333,7 +331,7 @@ class NonNullInAnalysis extends Analysis { if (opcode == IFNULL && popValue(frame) instanceof ParamValue) { int nextInsnIndex = methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult, state.exceptional); pendingPush(new MakeResult(state, subResult, new int[]{nextState.index})); pendingPush(new ProceedState(nextState)); return; @@ -341,7 +339,7 @@ class NonNullInAnalysis extends Analysis { if (opcode == IFEQ && popValue(frame) == InstanceOfCheckValue) { int nextInsnIndex = methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult, state.exceptional); pendingPush(new MakeResult(state, subResult, new int[]{nextState.index})); pendingPush(new ProceedState(nextState)); return; @@ -349,7 +347,7 @@ class NonNullInAnalysis extends Analysis { if (opcode == IFNE && popValue(frame) == InstanceOfCheckValue) { int nextInsnIndex = insnIndex + 1; - State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult); + State nextState = new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, hasCompanions || notEmptySubResult, state.exceptional); pendingPush(new MakeResult(state, subResult, new int[]{nextState.index})); pendingPush(new ProceedState(nextState)); return; @@ -365,22 +363,23 @@ class NonNullInAnalysis extends Analysis { for (int i = 0; i < nextInsnIndices.length; i++) { int nextInsnIndex = nextInsnIndices[i]; Frame nextFrame1 = nextFrame; + boolean exceptional = state.exceptional; if (controlFlow.errors[nextInsnIndex] && controlFlow.errorTransitions.contains(new Edge(insnIndex, nextInsnIndex))) { nextFrame1 = new Frame<>(frame); nextFrame1.clearStack(); nextFrame1.push(ASMUtils.THROWABLE_VALUE); + exceptional = true; } - pendingPush(new ProceedState(new State(subIndices[i], new Conf(nextInsnIndex, nextFrame1), nextHistory, taken, hasCompanions || notEmptySubResult))); + pendingPush(new ProceedState(new State(subIndices[i], new Conf(nextInsnIndex, nextFrame1), nextHistory, taken, hasCompanions || notEmptySubResult, + exceptional))); } } private int pendingTop; - private void pendingPush(PendingAction action) throws AnalyzerException { - if (pendingTop >= STEPS_LIMIT) { - throw new AnalyzerException(null, "limit is reached in method " + method); - } + private void pendingPush(PendingAction action) { + TooComplexException.check(method, pendingTop); pendingActions[pendingTop++] = action; } @@ -438,9 +437,7 @@ class NullableInAnalysis extends Analysis { int steps = 0; while (pendingTop > 0 && earlyResult == null) { steps ++; - if (steps >= STEPS_LIMIT) { - throw new AnalyzerException(null, "limit is reached, steps: " + steps + " in method " + method); - } + TooComplexException.check(method, steps); State state = pending[--pendingTop]; int insnIndex = state.conf.insnIndex; Conf conf = state.conf; @@ -527,25 +524,25 @@ class NullableInAnalysis extends Analysis { if (opcode == IFNONNULL && popValue(frame) instanceof ParamValue) { int nextInsnIndex = insnIndex + 1; - pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false)); + pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, false)); return; } if (opcode == IFNULL && popValue(frame) instanceof ParamValue) { int nextInsnIndex = methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false)); + pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, false)); return; } if (opcode == IFEQ && popValue(frame) == InstanceOfCheckValue) { int nextInsnIndex = methodNode.instructions.indexOf(((JumpInsnNode)insnNode).label); - pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false)); + pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, false)); return; } if (opcode == IFNE && popValue(frame) == InstanceOfCheckValue) { int nextInsnIndex = insnIndex + 1; - pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false)); + pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame), nextHistory, true, false, false)); return; } @@ -557,17 +554,15 @@ class NullableInAnalysis extends Analysis { nextFrame1.clearStack(); nextFrame1.push(ASMUtils.THROWABLE_VALUE); } - pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame1), nextHistory, taken, false)); + pendingPush(new State(++id, new Conf(nextInsnIndex, nextFrame1), nextHistory, taken, false, false)); } } private int pendingTop; - private void pendingPush(State state) throws AnalyzerException { - if (pendingTop >= STEPS_LIMIT) { - throw new AnalyzerException(null, "limit is reached in method " + method); - } + private void pendingPush(State state) { + TooComplexException.check(method, pendingTop); pending[pendingTop++] = state; } @@ -668,7 +663,7 @@ abstract class NullityInterpreter extends BasicInterpreter { } @Override - public BasicValue ternaryOperation(AbstractInsnNode insn, BasicValue value1, BasicValue value2, BasicValue value3) throws AnalyzerException { + public BasicValue ternaryOperation(AbstractInsnNode insn, BasicValue value1, BasicValue value2, BasicValue value3) { switch (insn.getOpcode()) { case IASTORE: case LASTORE: diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/TooComplexException.java b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/TooComplexException.java new file mode 100644 index 000000000000..47ee3016c876 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/TooComplexException.java @@ -0,0 +1,14 @@ +// Copyright 2000-2017 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.bytecodeAnalysis; + +class TooComplexException extends RuntimeException { + TooComplexException(Method method, int steps) { + super("limit is reached, steps: " + steps + " in method " + method); + } + + static void check(Method method, int steps) { + if(steps >= Analysis.STEPS_LIMIT) { + throw new TooComplexException(method, steps); + } + } +} diff --git a/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/javax/swing/annotations.xml b/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/javax/swing/annotations.xml index 588eb336ab25..0441a34b00e1 100644 --- a/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/javax/swing/annotations.xml +++ b/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/javax/swing/annotations.xml @@ -1008,9 +1008,6 @@ - - - diff --git a/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/org/apache/velocity/util/annotations.xml b/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/org/apache/velocity/util/annotations.xml index 33b6c499a8ff..3fd51e06281a 100644 --- a/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/org/apache/velocity/util/annotations.xml +++ b/java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/org/apache/velocity/util/annotations.xml @@ -164,9 +164,6 @@ - - - diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/bytecodeAnalysis/data/Test01.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/bytecodeAnalysis/data/Test01.java index 9dd8a1db8c81..267de165debf 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/bytecodeAnalysis/data/Test01.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/bytecodeAnalysis/data/Test01.java @@ -18,6 +18,8 @@ package com.intellij.java.codeInspection.bytecodeAnalysis.data; import com.intellij.java.codeInspection.bytecodeAnalysis.ExpectContract; import com.intellij.java.codeInspection.bytecodeAnalysis.ExpectNotNull; +import java.io.FileReader; +import java.io.IOException; import java.lang.reflect.Array; /** @@ -225,4 +227,36 @@ public class Test01 { if (b) throw new RuntimeException(); return b; } + + @ExpectNotNull + String getStringNoTry(@ExpectNotNull String s) throws IOException { + return String.valueOf(new FileReader(s.trim()).read()); + } + + String getStringTry(String s) { + try { + return String.valueOf(new FileReader(s.trim()).read()); + } + catch (IOException ex) { + return null; + } + } + + @ExpectContract(pure = true) + void testThrow(@ExpectNotNull String s) { + if(s.isEmpty()) { + throw new IllegalArgumentException(); + } + } + + @ExpectContract("!null->!null;null->null") + String testCatchReturn(String s) { + try { + Integer.parseInt(s); + } + catch (NumberFormatException ex) { + System.out.println("exception!"); + } + return s; + } }