From d4ee6c0c2bbdfaed9617a422196c5980b2d9d235 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 14 May 2018 16:55:05 +0700 Subject: [PATCH] ContractChecker: refined processing of 'fail' (IDEA-191776) --- .../dataFlow/ContractChecker.java | 24 +++++++---- .../dataFlow/ContractInspection.java | 3 +- .../contractCheck/FailDelegation.java | 7 +++- .../dataFlow/contractCheck/NewThisParam.java | 40 +++++++++++++++++++ .../contractCheck/TrueInsteadOfFail.java | 2 +- .../codeInspection/ContractCheckTest.java | 1 + 6 files changed, 67 insertions(+), 10 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/NewThisParam.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java index 828fc1ff7da2..0add795dda55 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java +++ b/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 myViolations = ContainerUtil.newHashSet(); private final Set myNonViolations = ContainerUtil.newHashSet(); private final Set 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 checkContractClause(PsiMethod method, StandardMethodContract contract) { + static Map 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(); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractInspection.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractInspection.java index 60eee1bf9e54..6f5ebb2cbf79 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractInspection.java +++ b/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 errors = ContractChecker.checkContractClause(method, contract); + Map errors = ContractChecker.checkContractClause(method, contract, ownContract); for (Map.Entry entry : errors.entrySet()) { PsiElement element = entry.getKey(); holder.registerProblem(element, entry.getValue()); diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java index 0accfd559c8a..703cb0eb3215 100644 --- a/java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java +++ b/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 f.getString(createIfNeeded); + } + + @Contract("!null,false->!null") + String delegationToInstanceOk(@NotNull Foo f, boolean createIfNeeded) { + return f.getString(createIfNeeded); } @Contract("true->fail") diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/NewThisParam.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/NewThisParam.java new file mode 100644 index 000000000000..8f0807dd263d --- /dev/null +++ b/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 list(int size) { + if(size < 0) { + // throwing is not a contract violation + throw new IllegalArgumentException(); + } + ArrayList list = new ArrayList<>(); + for(int i=0; i param1") + public static int test(int x) { + throw new IllegalArgumentException(); + } + + // 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 null; + } + + class Sub extends Test { + // Do not report: contract is inherited + public int testNonStatic(int x) { + throw new UnsupportedOperationException(); + } + + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java index e37328e94cfb..ccc34b687120 100644 --- a/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java +++ b/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) { - throw new RuntimeException(); + throw new RuntimeException(); } return i == 2; } diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/ContractCheckTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/ContractCheckTest.java index 54937b4b2a6a..684d3b090db0 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/ContractCheckTest.java +++ b/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(); } }