BytecodeAnalysisIndex: conservatively handle cases when contract inferred only via exceptional path

Fixes IDEA-155544 Wrong inferred contract annotation for Console.readPassword()

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<Conf> 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<Conf> 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<Res> {
   }
 
   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;
     }

java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/BytecodeAnalysisIndex.java

@@ -52,7 +52,7 @@ public class BytecodeAnalysisIndex extends ScalarIndexExtension<HMethod> {
   private static final ID<HMethod, Void> 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;
 

java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/ClassDataIndexer.java

@@ -252,6 +252,10 @@ public class ClassDataIndexer implements VirtualFileGist.GistCalculator<Map<HMet
         catch (ProcessCanceledException e) {
           throw e;
         }
+        catch (TooComplexException e) {
+          LOG.debug(method + " in " + presentableUrl + " is too complex for bytecode analysis");
+          return topEquations(method, argumentTypes, isReferenceResult, isInterestingResult, stable);
+        }
         catch (Throwable e) {
           // incorrect bytecode may result in Runtime exceptions during analysis
           // so here we suppose that exception is due to incorrect bytecode

java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Contracts.java

@@ -48,6 +48,7 @@ abstract class ContractAnalysis extends Analysis<Result> {
   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<Result> {
     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<Result> {
     }
     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<Result> {
     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<Result> {
     // general case
     for (int nextInsnIndex : controlFlow.transitions[insnIndex]) {
       Frame<BasicValue> 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<BasicValue> frame, int opcode) throws AnalyzerException;
+  abstract boolean handleReturn(Frame<BasicValue> 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<BasicValue> frame, int opcode) throws AnalyzerException {
+  boolean handleReturn(Frame<BasicValue> 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<BasicValue> frame, int opcode) throws AnalyzerException {
+  boolean handleReturn(Frame<BasicValue> 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:

java/java-analysis-impl/src/com/intellij/codeInspection/bytecodeAnalysis/Parameters.java

@@ -213,9 +213,7 @@ class NonNullInAnalysis extends Analysis<PResult> {
     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<PResult> {
 
     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<PResult> {
 
     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<PResult> {
 
     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<PResult> {
 
     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<PResult> {
     for (int i = 0; i < nextInsnIndices.length; i++) {
       int nextInsnIndex = nextInsnIndices[i];
       Frame<BasicValue> 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<PResult> {
     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<PResult> {
 
     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<PResult> {
         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:

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);
+    }
+  }
+}

java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/javax/swing/annotations.xml

@@ -1008,9 +1008,6 @@
         <annotation name='org.jetbrains.annotations.NotNull'/>
     </item>
     <item name='javax.swing.JTable.GenericEditor java.awt.Component getTableCellEditorComponent(javax.swing.JTable, java.lang.Object, boolean, int, int)'>
-        <annotation name='org.jetbrains.annotations.Contract'>
-            <val val="&quot;null,_,_,_,_-&gt;null&quot;"/>
-        </annotation>
         <annotation name='org.jetbrains.annotations.Nullable'/>
     </item>
     <item name='javax.swing.JTable.GenericEditor java.lang.Object getCellEditorValue()'>

java/java-tests/testData/codeInspection/bytecodeAnalysis/annotations/org/apache/velocity/util/annotations.xml

@@ -164,9 +164,6 @@
         </annotation>
     </item>
     <item name='org.apache.velocity.util.StringUtils java.lang.String stackTrace(java.lang.Throwable)'>
-        <annotation name='org.jetbrains.annotations.Contract'>
-            <val val="&quot;null-&gt;null&quot;"/>
-        </annotation>
         <annotation name='org.jetbrains.annotations.Nullable'/>
     </item>
     <item name='org.apache.velocity.util.StringUtils java.lang.String sub(java.lang.String, java.lang.String, java.lang.String)'>

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;
+  }
 }