From 5d8ee45b19f4d1b68ee8272e66d43ed03b9145bc Mon Sep 17 00:00:00 2001 From: peter Date: Sun, 1 Jun 2014 17:18:13 +0200 Subject: [PATCH] IDEA-112222 Validate @Contract annotation is related to the code --- .../dataFlow/ContractChecker.java | 145 +++++++++++++ .../dataFlow/ControlFlowAnalyzer.java | 201 +++++++----------- .../dataFlow/DataFlowInspectionBase.java | 14 +- .../dataFlow/DfaMemoryStateImpl.java | 3 + .../dataFlow/MethodContract.java | 78 ++++++- .../dataFlow/StandardInstructionVisitor.java | 34 +-- .../instructions/ReturnInstruction.java | 11 +- .../DelegationToInstanceMethod.java | 12 ++ .../DelegationWithUnknownArgument.java | 32 +++ .../contractCheck/EqualsUnknownValue.java | 25 +++ .../contractCheck/FailDelegation.java | 17 ++ .../dataFlow/contractCheck/MissingFail.java | 11 + .../contractCheck/NotNullStringLiteral.java | 11 + .../contractCheck/PlainDelegation.java | 16 ++ .../contractCheck/TrueInsteadOfFail.java | 13 ++ .../contractCheck/TrueInsteadOfFalse.java | 10 + .../dataFlow/contractCheck/WrongFail.java | 10 + .../fixture/ContractInLoopNotTooComplex.java | 7 +- .../dataFlow/fixture/ContractVarargs.java | 6 +- .../codeInspection/ContractCheckTest.java | 39 ++++ .../DataFlowInspectionTestSuite.java | 1 + 21 files changed, 547 insertions(+), 149 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationToInstanceMethod.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationWithUnknownArgument.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/EqualsUnknownValue.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/MissingFail.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/NotNullStringLiteral.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/PlainDelegation.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFalse.java create mode 100644 java/java-tests/testData/inspection/dataFlow/contractCheck/WrongFail.java create mode 100644 java/java-tests/testSrc/com/intellij/codeInspection/ContractCheckTest.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 new file mode 100644 index 000000000000..b527b8c51da7 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java @@ -0,0 +1,145 @@ +/* + * Copyright 2000-2014 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.codeInspection.dataFlow; + +import com.intellij.codeInspection.dataFlow.instructions.CheckReturnValueInstruction; +import com.intellij.codeInspection.dataFlow.instructions.Instruction; +import com.intellij.codeInspection.dataFlow.instructions.ReturnInstruction; +import com.intellij.codeInspection.dataFlow.value.DfaConstValue; +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.util.containers.ContainerUtil; + +import java.util.*; + +/** +* @author peter +*/ +class ContractChecker extends DataFlowRunner { + private final PsiMethod myMethod; + private final MethodContract myContract; + private final boolean myOnTheFly; + private final Set myViolations = ContainerUtil.newHashSet(); + private final Set myNonViolations = ContainerUtil.newHashSet(); + private final Set myFailures = ContainerUtil.newHashSet(); + + ContractChecker(PsiMethod method, PsiCodeBlock body, MethodContract contract, final boolean onTheFly) { + super(body); + myMethod = method; + myContract = contract; + myOnTheFly = onTheFly; + } + + static Map checkContractClause(PsiMethod method, + MethodContract contract, + boolean ignoreAssertions, final boolean onTheFly) { + + PsiCodeBlock body = method.getBody(); + if (body == null) return Collections.emptyMap(); + + ContractChecker checker = new ContractChecker(method, body, contract, onTheFly); + + PsiParameter[] parameters = method.getParameterList().getParameters(); + final DfaMemoryState initialState = checker.createMemoryState(); + final DfaValueFactory factory = checker.getFactory(); + for (int i = 0; i < contract.arguments.length; i++) { + MethodContract.ValueConstraint constraint = contract.arguments[i]; + DfaConstValue comparisonValue = constraint.getComparisonValue(factory); + if (comparisonValue != null) { + boolean negated = constraint.shouldUseNonEqComparison(); + DfaVariableValue dfaParam = factory.getVarFactory().createVariableValue(parameters[i], false); + initialState.applyCondition(factory.getRelationFactory().createRelation(dfaParam, comparisonValue, JavaTokenType.EQEQ, negated)); + } + } + + checker.analyzeMethod(body, new StandardInstructionVisitor(), ignoreAssertions, Arrays.asList(initialState)); + return checker.getErrors(); + } + + @Override + protected boolean shouldCheckTimeLimit() { + if (!myOnTheFly) return false; + return super.shouldCheckTimeLimit(); + } + + @Override + protected DfaInstructionState[] acceptInstruction(InstructionVisitor visitor, DfaInstructionState instructionState) { + DfaMemoryState memState = instructionState.getMemoryState(); + if (memState.isEphemeral()) { + return DfaInstructionState.EMPTY_ARRAY; + } + Instruction instruction = instructionState.getInstruction(); + if (instruction instanceof CheckReturnValueInstruction) { + PsiElement anchor = ((CheckReturnValueInstruction)instruction).getReturn(); + DfaValue retValue = memState.pop(); + if (breaksContract(retValue, myContract.returnValue, memState)) { + myViolations.add(anchor); + } else { + myNonViolations.add(anchor); + } + return InstructionVisitor.nextInstruction(instruction, this, memState); + + } + + if (instruction instanceof ReturnInstruction) { + if (((ReturnInstruction)instruction).isViaException()) { + ContainerUtil.addIfNotNull(myFailures, ((ReturnInstruction)instruction).getAnchor()); + } + } + + return super.acceptInstruction(visitor, instructionState); + } + + + private Map getErrors() { + HashMap errors = ContainerUtil.newHashMap(); + for (PsiElement element : myViolations) { + if (!myNonViolations.contains(element)) { + errors.put(element, "Contract clause '" + myContract + "' is violated"); + } + } + + if (myContract.returnValue != MethodContract.ValueConstraint.THROW_EXCEPTION) { + for (PsiElement element : myFailures) { + errors.put(element, "Contract clause '" + myContract + "' is violated: exception might be thrown instead of returning " + myContract.returnValue); + } + } else if (myFailures.isEmpty() && errors.isEmpty()) { + PsiIdentifier nameIdentifier = myMethod.getNameIdentifier(); + errors.put(nameIdentifier != null ? nameIdentifier : myMethod, + "Contract clause '" + myContract + "' is violated: no exception is thrown"); + } + + return errors; + } + + private boolean breaksContract(DfaValue retValue, MethodContract.ValueConstraint constraint, DfaMemoryState state) { + switch (constraint) { + case NULL_VALUE: return state.isNotNull(retValue); + case NOT_NULL_VALUE: return state.isNull(retValue); + case TRUE_VALUE: return isEquivalentTo(retValue, getFactory().getConstFactory().getFalse(), state); + case FALSE_VALUE: return isEquivalentTo(retValue, getFactory().getConstFactory().getTrue(), state); + case THROW_EXCEPTION: return true; + default: return false; + } + } + + private static boolean isEquivalentTo(DfaValue val, DfaConstValue constValue, DfaMemoryState state) { + return val == constValue || val instanceof DfaVariableValue && constValue == state.getConstantValue((DfaVariableValue)val); + } + +} 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 db74b243e329..e6ac2b2d4409 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 @@ -21,7 +21,6 @@ import com.intellij.codeInspection.dataFlow.value.*; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.registry.Registry; -import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; import com.intellij.psi.search.GlobalSearchScope; import com.intellij.psi.tree.IElementType; @@ -114,7 +113,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new CheckReturnValueInstruction(codeFragment)); } - addInstruction(new ReturnInstruction(false)); + addInstruction(new ReturnInstruction(false, null)); return myCurrentFlow; } @@ -239,7 +238,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } initException(myAssertionError); - addThrowCode(false); + addThrowCode(false, statement); } finishElement(statement); } @@ -569,11 +568,11 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new CheckReturnValueInstruction(returnValue)); } - returnCheckingFinally(); + returnCheckingFinally(false, statement); finishElement(statement); } - private void returnCheckingFinally() { + private void returnCheckingFinally(boolean viaException, @NotNull PsiElement anchor) { ControlFlow.ControlFlowOffset finallyOffset = getFinallyOffset(); if (finallyOffset != null) { addInstruction(new PushInstruction(myExceptionHolder, null)); @@ -583,7 +582,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new GotoInstruction(finallyOffset)); } else { - addInstruction(new ReturnInstruction(false)); + addInstruction(new ReturnInstruction(viaException, anchor)); } } @@ -698,7 +697,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { if (exception != null) { exception.accept(this); if (myCatchStack.isEmpty()) { - addInstruction(new ReturnInstruction(true)); + addInstruction(new ReturnInstruction(true, statement)); finishElement(statement); return; } @@ -712,14 +711,14 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new PopInstruction()); initException(myNpe); - addThrowCode(false); + addThrowCode(false, statement); gotoInstruction.setOffset(myCurrentFlow.getInstructionCount()); addInstruction(new PushInstruction(myExceptionHolder, null)); addInstruction(new SwapInstruction()); addInstruction(new AssignInstruction(null)); addInstruction(new PopInstruction()); - addThrowCode(false); + addThrowCode(false, statement); } finishElement(statement); @@ -747,7 +746,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new AssignInstruction(null)); addInstruction(new PopInstruction()); - addThrowCode(false); + addThrowCode(false, null); ifNoException.setOffset(myCurrentFlow.getInstructionCount()); } @@ -765,9 +764,9 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } // the exception object should be in $exception$ variable - private void addThrowCode(boolean catchRethrow) { + private void addThrowCode(boolean catchRethrow, @Nullable PsiElement explicitThrower) { if (myCatchStack.isEmpty()) { - addInstruction(new ReturnInstruction(true)); + addInstruction(new ReturnInstruction(true, explicitThrower)); return; } @@ -780,7 +779,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { i--; } if (i < 0) { - addInstruction(new ReturnInstruction(true)); + addInstruction(new ReturnInstruction(true, explicitThrower)); return; } cd = myCatchStack.get(i); @@ -922,7 +921,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new ConditionalGotoInstruction(getEndOffset(statement), false, null)); // else throw $exception$ - addThrowCode(false); + addThrowCode(false, null); } finishElement(statement); @@ -947,7 +946,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } // not assignable => rethrow - addThrowCode(true); + addThrowCode(true, null); // e = $exception$ addInstruction(new PushInstruction(myFactory.getVarFactory().createVariableValue(section.getParameter(), false), null)); @@ -971,7 +970,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } PsiMethod closer = PsiUtil.getResourceCloserMethod(variable); if (closer != null) { - addMethodThrows(closer); + addMethodThrows(closer, null); } } } @@ -1336,7 +1335,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { finishElement(expression); } - private void addMethodThrows(PsiMethod method) { + private void addMethodThrows(PsiMethod method, @Nullable PsiElement explicitCall) { if (method != null) { PsiClassType[] refs = method.getThrowsList().getReferencedTypes(); for (PsiClassType ref : refs) { @@ -1345,7 +1344,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(cond); addInstruction(new EmptyStackInstruction()); initException(ref); - addThrowCode(false); + addThrowCode(false, explicitCall); cond.setOffset(myCurrentFlow.getInstructionCount()); } } @@ -1397,7 +1396,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } addConditionalRuntimeThrow(); - List contracts = getCallContracts(expression); + List contracts = method instanceof PsiMethod ? getMethodContracts((PsiMethod)method) : Collections.emptyList(); addInstruction(new MethodCallInstruction(expression, createChainedVariableValue(expression), contracts)); if (!contracts.isEmpty()) { // if a contract resulted in 'fail', handle it @@ -1406,12 +1405,12 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new BinopInstruction(JavaTokenType.EQEQ, null, expression.getProject())); ConditionalGotoInstruction ifNotFail = new ConditionalGotoInstruction(null, true, null); addInstruction(ifNotFail); - returnCheckingFinally(); + returnCheckingFinally(true, expression); ifNotFail.setOffset(myCurrentFlow.getInstructionCount()); } if (!myCatchStack.isEmpty()) { - addMethodThrows(expression.resolveMethod()); + addMethodThrows(expression.resolveMethod(), expression); } if (isEqualsCall) { @@ -1432,74 +1431,70 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { finishElement(expression); } - private static List getCallContracts(PsiMethodCallExpression expression) { - final PsiMethod resolved = expression.resolveMethod(); - if (resolved != null) { - final PsiAnnotation contractAnno = findContractAnnotation(resolved); - if (contractAnno != null) { - return CachedValuesManager.getCachedValue(contractAnno, new CachedValueProvider>() { - @Nullable - @Override - public Result> compute() { - String text = AnnotationUtil.getStringAttributeValue(contractAnno, null); - if (text != null) { - try { - List applicable = ContainerUtil.filter(parseContract(text), new Condition() { - @Override - public boolean value(MethodContract contract) { - return contract.arguments.length == resolved.getParameterList().getParametersCount(); - } - }); - return Result.create(applicable, contractAnno); - } - catch (Exception ignored) { - } + static List getMethodContracts(@NotNull final PsiMethod method) { + final PsiAnnotation contractAnno = findContractAnnotation(method); + final int paramCount = method.getParameterList().getParametersCount(); + if (contractAnno != null) { + return CachedValuesManager.getCachedValue(contractAnno, new CachedValueProvider>() { + @Nullable + @Override + public Result> compute() { + String text = AnnotationUtil.getStringAttributeValue(contractAnno, null); + if (text != null) { + try { + List applicable = ContainerUtil.filter(MethodContract.parseContract(text), new Condition() { + @Override + public boolean value(MethodContract contract) { + return contract.arguments.length == paramCount; + } + }); + return Result.create(applicable, contractAnno); + } + catch (Exception ignored) { } - return Result.create(Collections.emptyList(), contractAnno); - } - }); - } - - @NonNls String methodName = resolved.getName(); - - PsiExpression[] params = expression.getArgumentList().getExpressions(); - PsiClass owner = resolved.getContainingClass(); - if (owner != null) { - final String className = owner.getQualifiedName(); - if ("java.lang.System".equals(className)) { - if ("exit".equals(methodName)) { - return Collections.singletonList(new MethodContract(getAnyArgConstraints(params), ValueConstraint.THROW_EXCEPTION)); } + return Result.create(Collections.emptyList(), contractAnno); } - else if ("junit.framework.Assert".equals(className) || "org.junit.Assert".equals(className) || - "junit.framework.TestCase".equals(className) || "org.testng.Assert".equals(className) || "org.testng.AssertJUnit".equals(className)) { - boolean testng = className.startsWith("org.testng."); - if ("fail".equals(methodName)) { - return Collections.singletonList(new MethodContract(getAnyArgConstraints(params), ValueConstraint.THROW_EXCEPTION)); - } + }); + } - int checkedParam = testng ? 0 : params.length - 1; - ValueConstraint[] constraints = getAnyArgConstraints(params); - if ("assertTrue".equals(methodName)) { - constraints[checkedParam] = ValueConstraint.FALSE_VALUE; - return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); - } - if ("assertFalse".equals(methodName)) { - constraints[checkedParam] = ValueConstraint.TRUE_VALUE; - return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); - } - if ("assertNull".equals(methodName)) { - constraints[checkedParam] = ValueConstraint.NOT_NULL_VALUE; - return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); - } - if ("assertNotNull".equals(methodName)) { - constraints[checkedParam] = ValueConstraint.NULL_VALUE; - return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); - } - return Collections.emptyList(); + @NonNls String methodName = method.getName(); + + PsiClass owner = method.getContainingClass(); + if (owner != null) { + final String className = owner.getQualifiedName(); + if ("java.lang.System".equals(className)) { + if ("exit".equals(methodName)) { + return Collections.singletonList(new MethodContract(getAnyArgConstraints(paramCount), ValueConstraint.THROW_EXCEPTION)); } } + else if ("junit.framework.Assert".equals(className) || "org.junit.Assert".equals(className) || + "junit.framework.TestCase".equals(className) || "org.testng.Assert".equals(className) || "org.testng.AssertJUnit".equals(className)) { + boolean testng = className.startsWith("org.testng."); + if ("fail".equals(methodName)) { + return Collections.singletonList(new MethodContract(getAnyArgConstraints(paramCount), ValueConstraint.THROW_EXCEPTION)); + } + int checkedParam = testng ? 0 : paramCount - 1; + ValueConstraint[] constraints = getAnyArgConstraints(paramCount); + if ("assertTrue".equals(methodName)) { + constraints[checkedParam] = ValueConstraint.FALSE_VALUE; + return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); + } + if ("assertFalse".equals(methodName)) { + constraints[checkedParam] = ValueConstraint.TRUE_VALUE; + return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); + } + if ("assertNull".equals(methodName)) { + constraints[checkedParam] = ValueConstraint.NOT_NULL_VALUE; + return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); + } + if ("assertNotNull".equals(methodName)) { + constraints[checkedParam] = ValueConstraint.NULL_VALUE; + return Collections.singletonList(new MethodContract(constraints, ValueConstraint.THROW_EXCEPTION)); + } + return Collections.emptyList(); + } } return Collections.emptyList(); @@ -1510,44 +1505,8 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { return AnnotationUtil.findAnnotation(method, ORG_JETBRAINS_ANNOTATIONS_CONTRACT); } - public static List parseContract(String text) throws ParseException { - List result = ContainerUtil.newArrayList(); - for (String clause : StringUtil.replace(text, " ", "").split(";")) { - String arrow = "->"; - int arrowIndex = clause.indexOf(arrow); - if (arrowIndex < 0) { - throw new ParseException("A contract clause must be in form arg1, ..., argN -> return-value"); - } - - String[] argStrings = clause.substring(0, arrowIndex).split(","); - ValueConstraint[] args = new ValueConstraint[argStrings.length]; - for (int i = 0; i < args.length; i++) { - args[i] = parseConstraint(argStrings[i]); - } - result.add(new MethodContract(args, parseConstraint(clause.substring(arrowIndex + arrow.length())))); - } - return result; - } - - private static ValueConstraint parseConstraint(String name) throws ParseException { - if (StringUtil.isEmpty(name)) throw new ParseException("Constraint should not be empty"); - if ("null".equals(name)) return ValueConstraint.NULL_VALUE; - if ("!null".equals(name)) return ValueConstraint.NOT_NULL_VALUE; - if ("true".equals(name)) return ValueConstraint.TRUE_VALUE; - if ("false".equals(name)) return ValueConstraint.FALSE_VALUE; - if ("fail".equals(name)) return ValueConstraint.THROW_EXCEPTION; - if ("_".equals(name)) return ValueConstraint.ANY_VALUE; - throw new ParseException("Constraint should be one of: null, !null, true, false, fail, _. Found: " + name); - } - - public static class ParseException extends Exception { - private ParseException(String message) { - super(message); - } - } - - private static ValueConstraint[] getAnyArgConstraints(PsiExpression[] params) { - ValueConstraint[] args = new ValueConstraint[params.length]; + private static ValueConstraint[] getAnyArgConstraints(int paramCount) { + ValueConstraint[] args = new ValueConstraint[paramCount]; for (int i = 0; i < args.length; i++) { args[i] = ValueConstraint.ANY_VALUE; } @@ -1610,7 +1569,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new MethodCallInstruction(expression, null, Collections.emptyList())); if (!myCatchStack.isEmpty()) { - addMethodThrows(ctr); + addMethodThrows(ctr, expression); } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java index ec1589d72b34..bf9f44496dee 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java @@ -32,8 +32,7 @@ import com.intellij.codeInsight.daemon.impl.quickfix.SimplifyBooleanExpressionFi import com.intellij.codeInsight.intention.impl.AddNullableAnnotationFix; import com.intellij.codeInspection.*; import com.intellij.codeInspection.dataFlow.instructions.*; -import com.intellij.codeInspection.dataFlow.value.DfaConstValue; -import com.intellij.codeInspection.dataFlow.value.DfaValue; +import com.intellij.codeInspection.dataFlow.value.*; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.Condition; @@ -96,6 +95,13 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { @Override public void visitMethod(PsiMethod method) { analyzeCodeBlock(method.getBody(), holder, isOnTheFly); + + for (MethodContract contract : ControlFlowAnalyzer.getMethodContracts(method)) { + Map errors = ContractChecker.checkContractClause(method, contract, IGNORE_ASSERT_STATEMENTS, isOnTheFly); + for (Map.Entry entry : errors.entrySet()) { + holder.registerProblem(entry.getKey(), entry.getValue()); + } + } } @Override @@ -145,9 +151,9 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { public static String checkContract(PsiMethod method, String text) { List contracts; try { - contracts = ControlFlowAnalyzer.parseContract(text); + contracts = MethodContract.parseContract(text); } - catch (ControlFlowAnalyzer.ParseException e) { + catch (MethodContract.ParseException e) { return e.getMessage(); } int paramCount = method.getParameterList().getParametersCount(); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index c2da3fbf722d..3b44c82f9398 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -474,6 +474,9 @@ public class DfaMemoryStateImpl implements DfaMemoryState { if (dfaVar instanceof DfaConstValue && ((DfaConstValue)dfaVar).getValue() != null) { return true; } + if (dfaVar instanceof DfaTypeValue && ((DfaTypeValue)dfaVar).isNotNull()) { + return true; + } DfaConstValue dfaNull = myFactory.getConstFactory().getNull(); int c1Index = getEqClassIndex(dfaVar); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java index ed88d5cbd71e..f7a5a8b1870a 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java @@ -15,6 +15,15 @@ */ package com.intellij.codeInspection.dataFlow; +import com.intellij.codeInspection.dataFlow.value.DfaConstValue; +import com.intellij.codeInspection.dataFlow.value.DfaValueFactory; +import com.intellij.openapi.util.text.StringUtil; +import com.intellij.util.Function; +import com.intellij.util.containers.ContainerUtil; +import org.jetbrains.annotations.Nullable; + +import java.util.List; + /** * @author peter */ @@ -27,7 +36,72 @@ public class MethodContract { this.returnValue = returnValue; } - public enum ValueConstraint { - ANY_VALUE, NULL_VALUE, NOT_NULL_VALUE, TRUE_VALUE, FALSE_VALUE, THROW_EXCEPTION + @Override + public String toString() { + return StringUtil.join(arguments, new Function() { + @Override + public String fun(ValueConstraint constraint) { + return constraint.toString(); + } + }, ", ") + " -> " + returnValue; } + + public enum ValueConstraint { + ANY_VALUE("_"), NULL_VALUE("null"), NOT_NULL_VALUE("!null"), TRUE_VALUE("true"), FALSE_VALUE("false"), THROW_EXCEPTION("fail"); + private final String myPresentableName; + + ValueConstraint(String presentableName) { + myPresentableName = presentableName; + } + + @Nullable + DfaConstValue getComparisonValue(DfaValueFactory factory) { + if (this == NULL_VALUE || this == NOT_NULL_VALUE) return factory.getConstFactory().getNull(); + if (this == TRUE_VALUE || this == FALSE_VALUE) return factory.getConstFactory().getTrue(); + return null; + } + + boolean shouldUseNonEqComparison() { + return this == NOT_NULL_VALUE || this == FALSE_VALUE; + } + + @Override + public String toString() { + return myPresentableName; + } + } + + public static List parseContract(String text) throws ParseException { + List result = ContainerUtil.newArrayList(); + for (String clause : StringUtil.replace(text, " ", "").split(";")) { + String arrow = "->"; + int arrowIndex = clause.indexOf(arrow); + if (arrowIndex < 0) { + throw new ParseException("A contract clause must be in form arg1, ..., argN -> return-value"); + } + + String[] argStrings = clause.substring(0, arrowIndex).split(","); + ValueConstraint[] args = new ValueConstraint[argStrings.length]; + for (int i = 0; i < args.length; i++) { + args[i] = parseConstraint(argStrings[i]); + } + result.add(new MethodContract(args, parseConstraint(clause.substring(arrowIndex + arrow.length())))); + } + return result; + } + + private static ValueConstraint parseConstraint(String name) throws ParseException { + if (StringUtil.isEmpty(name)) throw new ParseException("Constraint should not be empty"); + for (ValueConstraint constraint : ValueConstraint.values()) { + if (constraint.toString().equals(name)) return constraint; + } + throw new ParseException("Constraint should be one of: null, !null, true, false, fail, _. Found: " + name); + } + + public static class ParseException extends Exception { + private ParseException(String message) { + super(message); + } + } + } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index e66ce1831927..29bcecd0a53f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java @@ -30,12 +30,12 @@ import gnu.trove.THashSet; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; -import static com.intellij.codeInspection.dataFlow.MethodContract.ValueConstraint.*; import static com.intellij.psi.JavaTokenType.*; -import static com.intellij.psi.JavaTokenType.EQEQ; -import static com.intellij.psi.JavaTokenType.NE; /** * @author peter @@ -247,20 +247,17 @@ public class StandardInstructionVisitor extends InstructionVisitor { for (int i = 0; i < argValues.length; i++) { DfaValue argValue = argValues[i]; MethodContract.ValueConstraint constraint = contract.arguments[i]; - DfaConstValue expectedValue = constraint == NULL_VALUE || constraint == NOT_NULL_VALUE ? constFactory.getNull() : - constraint == FALSE_VALUE ? constFactory.getFalse() : - constraint == TRUE_VALUE ? constFactory.getTrue() : - null; + DfaConstValue expectedValue = constraint.getComparisonValue(factory); if (expectedValue == null) continue; - boolean invertCondition = constraint == NOT_NULL_VALUE; + boolean invertCondition = constraint.shouldUseNonEqComparison(); DfaValue condition = factory.getRelationFactory().createRelation(argValue, expectedValue, EQEQ, invertCondition); if (condition == null) { if (!(argValue instanceof DfaConstValue)) { falseStates.addAll(states); continue; } - condition = constFactory.createFromValue(argValue == expectedValue, PsiType.BOOLEAN, null); + condition = constFactory.createFromValue((argValue == expectedValue) != invertCondition, PsiType.BOOLEAN, null); } List nextStates = ContainerUtil.newArrayList(); @@ -469,16 +466,21 @@ public class StandardInstructionVisitor extends InstructionVisitor { return handleConstantComparison(instruction, runner, memState, dfaLeft, dfaRight, DfaRelationValue.getSymmetricOperation(opSign)); } - if (EQEQ != opSign && NE != opSign || - !(dfaLeft instanceof DfaConstValue) || !(dfaRight instanceof DfaConstValue)) { + if (EQEQ != opSign && NE != opSign) { return null; } - boolean negated = (NE == opSign) ^ (DfaMemoryStateImpl.isNaN(dfaLeft) || DfaMemoryStateImpl.isNaN(dfaRight)); - if (dfaLeft == dfaRight ^ negated) { - return alwaysTrue(instruction, runner, memState); + if (dfaLeft instanceof DfaConstValue && dfaRight instanceof DfaConstValue || + dfaLeft == runner.getFactory().getConstFactory().getContractFail() || + dfaRight == runner.getFactory().getConstFactory().getContractFail()) { + boolean negated = (NE == opSign) ^ (DfaMemoryStateImpl.isNaN(dfaLeft) || DfaMemoryStateImpl.isNaN(dfaRight)); + if (dfaLeft == dfaRight ^ negated) { + return alwaysTrue(instruction, runner, memState); + } + return alwaysFalse(instruction, runner, memState); } - return alwaysFalse(instruction, runner, memState); + + return null; } private static DfaInstructionState[] checkTypeRanges(BinopInstruction instruction, 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 55c00001fc01..9c67fbe9f5e1 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 @@ -25,12 +25,21 @@ package com.intellij.codeInspection.dataFlow.instructions; import com.intellij.codeInspection.dataFlow.*; +import com.intellij.psi.PsiElement; +import org.jetbrains.annotations.Nullable; public class ReturnInstruction extends Instruction { private final boolean isViaException; + private final PsiElement myAnchor; - public ReturnInstruction(boolean isViaException) { + public ReturnInstruction(boolean isViaException, @Nullable PsiElement anchor) { this.isViaException = isViaException; + myAnchor = anchor; + } + + @Nullable + public PsiElement getAnchor() { + return myAnchor; } public boolean isViaException() { diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationToInstanceMethod.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationToInstanceMethod.java new file mode 100644 index 000000000000..af1679dd3599 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationToInstanceMethod.java @@ -0,0 +1,12 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Contract("!null,true->!null") + String delegationToInstance(@NotNull Foo f, boolean createIfNeeded) { return f.getString(createIfNeeded); } + + @Contract("true->!null") + String getString(boolean createIfNeeded) { return createIfNeeded ? "" : null; } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationWithUnknownArgument.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationWithUnknownArgument.java new file mode 100644 index 000000000000..98c0c29a1358 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/DelegationWithUnknownArgument.java @@ -0,0 +1,32 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +abstract class Foo { + abstract String getString(); + + @Contract("null -> null;!null -> !null") + public static String delegate(@Nullable String s) { + return s == null ? null : s.substring(1); + } + + @Contract("null -> null;!null -> !null") + public static String callee(@Nullable Foo element) { + return element == null ? null : delegate(element.getString()); + } + + + @Contract("!null -> !null") + @Nullable public static String delegate2(@Nullable String s) { + return s == null ? null : s.substring(1); + } + + @Contract("!null -> !null") + @Nullable public static String callee2(@Nullable Object element) { + if (element instanceof Foo) return delegate2(((Foo)element).getString()); + if (element != null) return element.toString(); + return null; + } + + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/EqualsUnknownValue.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/EqualsUnknownValue.java new file mode 100644 index 000000000000..33c0526125e9 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/EqualsUnknownValue.java @@ -0,0 +1,25 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +abstract class Foo { + public static final String CONSTANT = getSomeString(); + + static native String getSomeString(); + + @Contract("null -> false") + static boolean isConstant(@Nullable String s) { + return s == CONSTANT; + } + + @Contract("null -> false") + static boolean isSomeString(@Nullable String s) { + return s == getSomeString(); + } + + @Contract("null,_ -> false") + static boolean isParameter(@Nullable String s, String param) { + return s == param; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java new file mode 100644 index 000000000000..9f18a6deafdf --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/FailDelegation.java @@ -0,0 +1,17 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Contract("!null,true->!null") + String delegationToInstance(@NotNull Foo f, boolean createIfNeeded) { + return f.getString(createIfNeeded); + } + + @Contract("true->fail") + String getString(boolean fail) { + if (fail) throw new RuntimeException(); + return "a"; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/MissingFail.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/MissingFail.java new file mode 100644 index 000000000000..875fe78257ec --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/MissingFail.java @@ -0,0 +1,11 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +class Foo { + + @Contract("true->fail") + void assertFalse(boolean fail) { + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/NotNullStringLiteral.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/NotNullStringLiteral.java new file mode 100644 index 000000000000..a493436b1744 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/NotNullStringLiteral.java @@ -0,0 +1,11 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Nullable + @Contract ( "_ -> null") + String foo(String s) { + return "42"; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/PlainDelegation.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/PlainDelegation.java new file mode 100644 index 000000000000..80fb846e3b36 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/PlainDelegation.java @@ -0,0 +1,16 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Contract("null->false") + boolean plainDelegation(Object x) { + return bar(2, x); + } + + @Contract("_,null->true") + boolean bar(int i, @Nullable Object foo) { + return foo == null; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java new file mode 100644 index 000000000000..e37328e94cfb --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFail.java @@ -0,0 +1,13 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Contract("null,_->true") + boolean bar(@Nullable Object foo, int i) { + if (foo == null) { + throw new RuntimeException(); + } + return i == 2; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFalse.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFalse.java new file mode 100644 index 000000000000..145496854d6c --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/TrueInsteadOfFalse.java @@ -0,0 +1,10 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Contract("null->true") + boolean bar(@Nullable Object foo) { + return foo != null && foo.hashCode() == 3; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/contractCheck/WrongFail.java b/java/java-tests/testData/inspection/dataFlow/contractCheck/WrongFail.java new file mode 100644 index 000000000000..1e2061e18422 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/contractCheck/WrongFail.java @@ -0,0 +1,10 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +class Foo { + @Contract("_,null->fail") + boolean bar(int i, @Nullable Object foo) { + return foo == null; + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ContractInLoopNotTooComplex.java b/java/java-tests/testData/inspection/dataFlow/fixture/ContractInLoopNotTooComplex.java index 14dfd20e822e..7348d0387229 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ContractInLoopNotTooComplex.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ContractInLoopNotTooComplex.java @@ -2,15 +2,16 @@ import org.jetbrains.annotations.Contract; class Foo { - public void main(String[] args) { + public void main(String s) { for (int i = 0; i < 10; i++) { - assertTrue("str", true); + assertTrue("str", s != null); + s.hashCode(); } } @Contract("_, false->fail") void assertTrue(String msg, boolean value) { - + if (!value) throw new RuntimeException(); } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ContractVarargs.java b/java/java-tests/testData/inspection/dataFlow/fixture/ContractVarargs.java index d8f144af2b41..7d987c762550 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ContractVarargs.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ContractVarargs.java @@ -2,6 +2,8 @@ import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.lang.RuntimeException; + class Contracts { public void simpleFail(@Nullable String message) { @@ -11,12 +13,12 @@ class Contracts { @Contract("_->fail") private void notBlank(@Nullable Object message) { - + throw new RuntimeException(); } @Contract("_,_,_->fail") private void notBlank(@Nullable Object o, String message, Object... args) { - + throw new RuntimeException(); } public void varargFail(@Nullable String message) { diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/ContractCheckTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/ContractCheckTest.java new file mode 100644 index 000000000000..0723577cf57b --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/codeInspection/ContractCheckTest.java @@ -0,0 +1,39 @@ +package com.intellij.codeInspection; + +import com.intellij.JavaTestUtil; +import com.intellij.codeInspection.dataFlow.DataFlowInspection; +import com.intellij.testFramework.LightProjectDescriptor; +import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase; +import org.jetbrains.annotations.NotNull; + +/** + * @author peter + */ +public class ContractCheckTest extends LightCodeInsightFixtureTestCase { + @NotNull + @Override + protected LightProjectDescriptor getProjectDescriptor() { + return JAVA_1_7; + } + + @Override + protected String getTestDataPath() { + return JavaTestUtil.getJavaTestDataPath() + "/inspection/dataFlow/contractCheck/"; + } + + private void doTest() { + myFixture.enableInspections(new DataFlowInspection()); + myFixture.testHighlighting(true, false, true, getTestName(false) + ".java"); + } + + public void testTrueInsteadOfFalse() { doTest(); } + public void testTrueInsteadOfFail() { doTest(); } + public void testWrongFail() { doTest(); } + public void testNotNullStringLiteral() { doTest(); } + public void testPlainDelegation() { doTest(); } + public void testDelegationToInstanceMethod() { doTest(); } + public void testFailDelegation() { doTest(); } + public void testDelegationWithUnknownArgument() { doTest(); } + public void testEqualsUnknownValue() { doTest(); } + public void testMissingFail() { doTest(); } +} diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTestSuite.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTestSuite.java index 22c583e348a8..ff3de591e996 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTestSuite.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTestSuite.java @@ -28,6 +28,7 @@ public class DataFlowInspectionTestSuite { suite.addTestSuite(DataFlowInspectionTest.class); suite.addTestSuite(DataFlowInspection8Test.class); suite.addTestSuite(DataFlowInspectionAncientTest.class); + suite.addTestSuite(ContractCheckTest.class); suite.addTestSuite(SliceTreeTest.class); suite.addTestSuite(SliceBackwardTest.class); suite.addTestSuite(SmartTypeCompletionDfaTest.class);