ContractChecker: refined processing of 'fail' (IDEA-191776)

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java

@@ -13,7 +13,9 @@ 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.*;
+import com.intellij.psi.util.PsiUtil;
 import com.intellij.util.containers.ContainerUtil;
+import com.siyeh.ig.psiutils.ControlFlowUtils;
 import org.jetbrains.annotations.NotNull;
 
 import java.util.Collections;
@@ -27,22 +29,25 @@ import java.util.Set;
 class ContractChecker extends DataFlowRunner {
   private final PsiMethod myMethod;
   private final StandardMethodContract myContract;
+  private final boolean myOwnContract;
   private final Set<PsiElement> myViolations = ContainerUtil.newHashSet();
   private final Set<PsiElement> myNonViolations = ContainerUtil.newHashSet();
   private final Set<PsiElement> myFailures = ContainerUtil.newHashSet();
+  private boolean myMayReturnNormally = false;
 
-  private ContractChecker(PsiMethod method, StandardMethodContract contract) {
+  private ContractChecker(PsiMethod method, StandardMethodContract contract, boolean ownContract) {
     super(false, null);
     myMethod = method;
     myContract = contract;
+    myOwnContract = ownContract;
   }
 
-  static Map<PsiElement, String> checkContractClause(PsiMethod method, StandardMethodContract contract) {
+  static Map<PsiElement, String> checkContractClause(PsiMethod method, StandardMethodContract contract, boolean ownContract) {
 
     PsiCodeBlock body = method.getBody();
     if (body == null) return Collections.emptyMap();
 
-    ContractChecker checker = new ContractChecker(method, contract);
+    ContractChecker checker = new ContractChecker(method, contract, ownContract);
 
     PsiParameter[] parameters = method.getParameterList().getParameters();
     final DfaMemoryState initialState = checker.createMemoryState();
@@ -82,8 +87,10 @@ class ContractChecker extends DataFlowRunner {
     }
 
     if (instruction instanceof ReturnInstruction) {
-      if (((ReturnInstruction)instruction).isViaException() && !myContract.getReturnValue().isNotNull()) {
+      if (((ReturnInstruction)instruction).isViaException()) {
         ContainerUtil.addIfNotNull(myFailures, ((ReturnInstruction)instruction).getAnchor());
+      } else {
+        myMayReturnNormally = true;
       }
     }
 
@@ -125,9 +132,12 @@ class ContractChecker extends DataFlowRunner {
     }
 
     if (!myContract.getReturnValue().isFail()) {
-      for (PsiElement element : myFailures) {
-        errors.put(element, "Contract clause '" + myContract + "' is violated: exception might be thrown instead of returning " +
-                            myContract.getReturnValue());
+      if (myOwnContract && !myMayReturnNormally &&
+          !(PsiUtil.canBeOverridden(myMethod) && ControlFlowUtils.methodAlwaysThrowsException(myMethod))) {
+        for (PsiElement element : myFailures) {
+          errors.put(element, "Return value of clause '" + myContract + "' could be replaced with 'fail' as method always fails"+
+                              (myContract.isTrivial() ? "" : " in this case"));
+        }
       }
     } else if (myFailures.isEmpty() && errors.isEmpty()) {
       PsiIdentifier nameIdentifier = myMethod.getNameIdentifier();

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractInspection.java

@@ -27,8 +27,9 @@ public class ContractInspection extends AbstractBaseJavaLocalInspectionTool {
       public void visitMethod(PsiMethod method) {
         PsiAnnotation annotation = JavaMethodContractUtil.findContractAnnotation(method);
         if (annotation == null || AnnotationUtil.isInferredAnnotation(annotation)) return;
+        boolean ownContract = annotation.getOwner() == method.getModifierList();
         for (StandardMethodContract contract : JavaMethodContractUtil.getMethodContracts(method)) {
-          Map<PsiElement, String> errors = ContractChecker.checkContractClause(method, contract);
+          Map<PsiElement, String> errors = ContractChecker.checkContractClause(method, contract, ownContract);
           for (Map.Entry<PsiElement, String> entry : errors.entrySet()) {
             PsiElement element = entry.getKey();
             holder.registerProblem(element, entry.getValue());

java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java

@@ -5,7 +5,12 @@ import org.jetbrains.annotations.Nullable;
 class Foo {
   @Contract("!null,true->!null")
   String delegationToInstance(@NotNull Foo f, boolean createIfNeeded) {
-    return f.getString(createIfNeeded); // not smart enough to check this 
+    return <warning descr="Return value of clause '!null, true -> !null' could be replaced with 'fail' as method always fails in this case">f.getString(createIfNeeded)</warning>;
+  }
+
+  @Contract("!null,false->!null")
+  String delegationToInstanceOk(@NotNull Foo f, boolean createIfNeeded) {
+    return f.getString(createIfNeeded);
   }
 
   @Contract("true->fail")

java/java-tests/testData/inspection/dataFlow/contractCheck/NewThisParam.java

@@ -0,0 +1,40 @@
+package org.jetbrains.annotations;
+
+import java.util.*;
+
+class Test {
+  @Contract("_ -> new")
+  public List<String> list(int size) {
+    if(size < 0) {
+      // throwing is not a contract violation
+      throw new IllegalArgumentException();
+    }
+    ArrayList<String> list = new ArrayList<>();
+    for(int i=0; i<size; i++) list.add("");
+    return list;
+  }
+
+  @Contract("_ -> param1")
+  public static int test(int x) {
+    <warning descr="Return value of clause '_ -> param1' could be replaced with 'fail' as method always fails">throw new IllegalArgumentException();</warning>
+  }
+
+  // Do not report: could be intended to override in subclasses
+  @Contract("_ -> param1")
+  public int testNonStatic(int x) {
+    throw new UnsupportedOperationException();
+  }
+
+  @Contract("-> this")
+  public Test returnThis() {
+    return <warning descr="Contract clause ' -> this' is violated">null</warning>;
+  }
+
+  class Sub extends Test {
+    // Do not report: contract is inherited
+    public int testNonStatic(int x) {
+      throw new UnsupportedOperationException();
+    }
+
+  }
+}

java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java

@@ -5,7 +5,7 @@ class Foo {
   @Contract("null,_->true")
   boolean bar(@Nullable Object foo, int i) {
     if (foo == null) {
-      <warning descr="Contract clause 'null, _ -> true' is violated: exception might be thrown instead of returning true">throw new RuntimeException();</warning>
+      <warning descr="Return value of clause 'null, _ -> true' could be replaced with 'fail' as method always fails in this case">throw new RuntimeException();</warning>
     }
     return i == 2;
   }

java/java-tests/testSrc/com/intellij/java/codeInspection/ContractCheckTest.java

@@ -51,4 +51,5 @@ public class ContractCheckTest extends LightCodeInsightFixtureTestCase {
   public void testUnknownIfCondition() { doTest(); }
   public void testCallingNotNullMethod() { doTest(); }
   public void testMutationSignatureProblems() { doTest(); }
+  public void testNewThisParam() { doTest(); }
 }