From ecf82235b99d6a2d973916ca2ec69de336c71c08 Mon Sep 17 00:00:00 2001 From: Maxim Medvedev Date: Fri, 13 Nov 2009 20:24:22 +0300 Subject: [PATCH] improve method return statements inspection --- .../GroovyMethodWithInconsistentReturns.html | 7 - .../GroovyInspectionProvider.java | 2 - .../GroovyAssignabilityCheckInspection.java | 28 ++-- .../MissingReturnInspection.java | 51 +++---- .../utils/ControlFlowUtils.java | 33 ++++ ...thodWithInconsistentReturnsInspection.java | 141 ------------------ .../statements/expressions/TypesUtil.java | 4 +- 7 files changed, 73 insertions(+), 193 deletions(-) delete mode 100644 plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html delete mode 100644 plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/validity/GroovyMethodWithInconsistentReturnsInspection.java diff --git a/plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html b/plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html deleted file mode 100644 index 9404c2b14251..000000000000 --- a/plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html +++ /dev/null @@ -1,7 +0,0 @@ - -
- -This inspection reports any Groovy methods with both value-returning and non-value-returning return statements. - While theoretically valid, such inconsistency is almost certainly due to coding error. -
Powered by InspectorGroovy
- diff --git a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/GroovyInspectionProvider.java b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/GroovyInspectionProvider.java index 5937c022109b..13a92bf7135b 100644 --- a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/GroovyInspectionProvider.java +++ b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/GroovyInspectionProvider.java @@ -37,7 +37,6 @@ import org.jetbrains.plugins.groovy.codeInspection.untypedUnresolvedAccess.Groov import org.jetbrains.plugins.groovy.codeInspection.untypedUnresolvedAccess.GroovyUntypedAccessInspection; import org.jetbrains.plugins.groovy.codeInspection.unusedDef.UnusedDefInspection; import org.jetbrains.plugins.groovy.codeInspection.validity.GroovyDuplicateSwitchBranchInspection; -import org.jetbrains.plugins.groovy.codeInspection.validity.GroovyMethodWithInconsistentReturnsInspection; import org.jetbrains.plugins.groovy.codeInspection.validity.GroovyUnreachableStatementInspection; /** @@ -129,7 +128,6 @@ public class GroovyInspectionProvider implements InspectionToolProvider, Applica GroovyOctalIntegerInspection.class, GroovyDuplicateSwitchBranchInspection.class, - GroovyMethodWithInconsistentReturnsInspection.class, GroovyNonShortCircuitBooleanInspection.class, GroovyInfiniteLoopStatementInspection.class, diff --git a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/assignment/GroovyAssignabilityCheckInspection.java b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/assignment/GroovyAssignabilityCheckInspection.java index 0c9c070dc4bb..4acea75b2082 100644 --- a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/assignment/GroovyAssignabilityCheckInspection.java +++ b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/assignment/GroovyAssignabilityCheckInspection.java @@ -24,9 +24,9 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.plugins.groovy.GroovyBundle; import org.jetbrains.plugins.groovy.codeInspection.BaseInspection; import org.jetbrains.plugins.groovy.codeInspection.BaseInspectionVisitor; +import org.jetbrains.plugins.groovy.codeInspection.utils.ControlFlowUtils; import org.jetbrains.plugins.groovy.lang.lexer.GroovyTokenTypes; import org.jetbrains.plugins.groovy.lang.psi.GroovyPsiElement; -import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrStatement; import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrVariable; import org.jetbrains.plugins.groovy.lang.psi.api.statements.arguments.GrArgumentLabel; import org.jetbrains.plugins.groovy.lang.psi.api.statements.arguments.GrNamedArgument; @@ -36,6 +36,7 @@ import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrAssign import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrExpression; import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrReferenceExpression; import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.members.GrMethod; +import org.jetbrains.plugins.groovy.lang.psi.controlFlow.Instruction; import org.jetbrains.plugins.groovy.lang.psi.impl.statements.expressions.TypesUtil; import org.jetbrains.plugins.groovy.lang.psi.util.PsiUtil; @@ -72,11 +73,12 @@ public class GroovyAssignabilityCheckInspection extends BaseInspection { return new MyVisitor(); } - private class MyVisitor extends BaseInspectionVisitor { + private static class MyVisitor extends BaseInspectionVisitor { private void checkAssignability(@NotNull PsiType expectedType, @NotNull GrExpression expression, GroovyPsiElement element) { if (PsiUtil.isRawClassMemberAccess(expression)) return; //GRVY-2197 final PsiType rType = expression.getType(); - if (rType != null && !TypesUtil.isAssignable(expectedType, rType, element.getManager(), element.getResolveScope())) { + if (rType == null || rType == PsiType.VOID) return; + if (!TypesUtil.isAssignable(expectedType, rType, element.getManager(), element.getResolveScope())) { registerError(element, rType.getPresentableText(), expectedType.getPresentableText()); } } @@ -85,21 +87,21 @@ public class GroovyAssignabilityCheckInspection extends BaseInspection { @Override public void visitOpenBlock(GrOpenBlock block) { super.visitOpenBlock(block); - final PsiElement element = block.getParent(); if (!(element instanceof GrMethod)) return; GrMethod method = (GrMethod)element; final PsiType expectedType = method.getReturnType(); - if (PsiType.VOID.equals(expectedType)) return; + if (expectedType == null || PsiType.VOID.equals(expectedType)) return; - final GrStatement[] statements = block.getStatements(); - if (statements.length == 0) return; - final GrStatement last = statements[statements.length - 1]; - if (!(last instanceof GrExpression)) return; - final GrExpression expression = (GrExpression)last; - - if (expectedType == null) return; - checkAssignability(expectedType, expression, last); + ControlFlowUtils.visitAllExitPoints(block, new ControlFlowUtils.ExitPointVisitor() { + public boolean visit(Instruction instruction) { + final PsiElement psiElement = instruction.getElement(); + if (psiElement instanceof GrExpression) { + checkAssignability(expectedType, (GrExpression)psiElement, (GrExpression)psiElement); + } + return true; + } + }); } @Override diff --git a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/noReturnMethod/MissingReturnInspection.java b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/noReturnMethod/MissingReturnInspection.java index be272479ed7a..9600ce6c0328 100644 --- a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/noReturnMethod/MissingReturnInspection.java +++ b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/noReturnMethod/MissingReturnInspection.java @@ -16,6 +16,7 @@ package org.jetbrains.plugins.groovy.codeInspection.noReturnMethod; import com.intellij.codeInspection.ProblemsHolder; +import com.intellij.openapi.util.Ref; import com.intellij.openapi.util.TextRange; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiElementVisitor; @@ -25,6 +26,7 @@ import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; import org.jetbrains.plugins.groovy.codeInspection.GroovyInspectionBundle; import org.jetbrains.plugins.groovy.codeInspection.GroovySuppressableInspectionTool; +import org.jetbrains.plugins.groovy.codeInspection.utils.ControlFlowUtils; import org.jetbrains.plugins.groovy.lang.psi.GroovyElementVisitor; import org.jetbrains.plugins.groovy.lang.psi.GroovyPsiElement; import org.jetbrains.plugins.groovy.lang.psi.GroovyPsiElementVisitor; @@ -32,9 +34,9 @@ import org.jetbrains.plugins.groovy.lang.psi.GroovyRecursiveElementVisitor; import org.jetbrains.plugins.groovy.lang.psi.api.statements.blocks.GrClosableBlock; import org.jetbrains.plugins.groovy.lang.psi.api.statements.blocks.GrCodeBlock; import org.jetbrains.plugins.groovy.lang.psi.api.statements.blocks.GrOpenBlock; +import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrAssertStatement; import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrReturnStatement; import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrThrowStatement; -import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrAssertStatement; import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.members.GrMethod; import org.jetbrains.plugins.groovy.lang.psi.controlFlow.Instruction; import org.jetbrains.plugins.groovy.lang.psi.controlFlow.impl.MaybeReturnInstruction; @@ -82,7 +84,7 @@ public class MissingReturnInspection extends GroovySuppressableInspectionTool { } private static void check(GrCodeBlock block, ProblemsHolder holder, boolean mustReturnValue) { - if ((mustReturnValue || hasReturnStatements(block)) && !alwaysReturns(block.getControlFlow())) { + if ((mustReturnValue || hasReturnStatements(block)) && !alwaysReturns(block)) { addNoReturnMessage(block, holder); } } @@ -130,32 +132,25 @@ public class MissingReturnInspection extends GroovySuppressableInspectionTool { return true; } - public static boolean alwaysReturns(Instruction[] flow) { - boolean[] visited = new boolean[flow.length]; - return alwaysReturnsInner(flow[flow.length - 1], flow[0], visited); - } - - private static boolean alwaysReturnsInner(Instruction last, Instruction first, boolean[] visited) { - if (first == last) return false; - if (last instanceof MaybeReturnInstruction) { - return ((MaybeReturnInstruction)last).mayReturnValue(); - } - - final PsiElement element = last.getElement(); - if (element instanceof GrReturnStatement || element instanceof GrThrowStatement || element instanceof GrAssertStatement) { - return true; - } - - if (last.getElement() != null) { - return false; - } - - visited[last.num()] = true; - for (Instruction pred : last.allPred()) { - if (!visited[pred.num()]) { - if (!alwaysReturnsInner(pred, first, visited)) return false; + public static boolean alwaysReturns(GrCodeBlock block) { + final Ref always = new Ref(true); + ControlFlowUtils.visitAllExitPoints(block, new ControlFlowUtils.ExitPointVisitor() { + public boolean visit(Instruction instruction) { + if (instruction instanceof MaybeReturnInstruction) { + if (!((MaybeReturnInstruction)instruction).mayReturnValue()) { + always.set(false); + return false; + } + return true; + } + final PsiElement element = instruction.getElement(); + if (!(element instanceof GrReturnStatement || element instanceof GrThrowStatement || element instanceof GrAssertStatement)) { + always.set(false); + return false; + } + return true; } - } - return true; + }); + return always.get(); } } diff --git a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/utils/ControlFlowUtils.java b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/utils/ControlFlowUtils.java index a3f0059bfbe4..86aa6c0503e4 100644 --- a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/utils/ControlFlowUtils.java +++ b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/utils/ControlFlowUtils.java @@ -15,6 +15,7 @@ */ package org.jetbrains.plugins.groovy.codeInspection.utils; +import com.intellij.psi.PsiElement; import com.intellij.psi.util.PsiTreeUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -31,6 +32,8 @@ import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrReturnState import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrThrowStatement; import org.jetbrains.plugins.groovy.lang.psi.api.statements.clauses.GrCaseSection; import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrExpression; +import org.jetbrains.plugins.groovy.lang.psi.controlFlow.Instruction; +import org.jetbrains.plugins.groovy.lang.psi.controlFlow.impl.MaybeReturnInstruction; @SuppressWarnings({"OverlyComplexClass"}) public class ControlFlowUtils { @@ -538,4 +541,34 @@ public class ControlFlowUtils { return throwStatement != null; } + + public interface ExitPointVisitor { + boolean visit(Instruction instruction); + } + + public static void visitAllExitPoints(@Nullable GrCodeBlock block, ExitPointVisitor visitor) { + if (block == null) return; + final Instruction[] flow = block.getControlFlow(); + boolean[] visited = new boolean[flow.length]; + visitAllExitPointsInner(flow[flow.length - 1], flow[0], visited, visitor); + } + + private static boolean visitAllExitPointsInner(Instruction last, Instruction first, boolean[] visited, ExitPointVisitor visitor) { + if (first == last) return true; + if (last instanceof MaybeReturnInstruction) { + return visitor.visit(last); + } + + final PsiElement element = last.getElement(); + if (element != null) { + return visitor.visit(last); + } + visited[last.num()] = true; + for (Instruction pred : last.allPred()) { + if (!visited[pred.num()]) { + if (!visitAllExitPointsInner(pred, first, visited, visitor)) return false; + } + } + return true; + } } diff --git a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/validity/GroovyMethodWithInconsistentReturnsInspection.java b/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/validity/GroovyMethodWithInconsistentReturnsInspection.java deleted file mode 100644 index 9f6d4babf645..000000000000 --- a/plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/validity/GroovyMethodWithInconsistentReturnsInspection.java +++ /dev/null @@ -1,141 +0,0 @@ -/* - * Copyright 2007-2008 Dave Griffith - * - * 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 org.jetbrains.plugins.groovy.codeInspection.validity; - -import com.intellij.psi.PsiElement; -import com.intellij.psi.util.PsiTreeUtil; -import org.jetbrains.annotations.Nls; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; -import org.jetbrains.plugins.groovy.codeInspection.BaseInspection; -import org.jetbrains.plugins.groovy.codeInspection.BaseInspectionVisitor; -import org.jetbrains.plugins.groovy.codeInspection.utils.ControlFlowUtils; -import org.jetbrains.plugins.groovy.lang.psi.GroovyRecursiveElementVisitor; -import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrBlockStatement; -import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrStatement; -import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrReturnStatement; -import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.members.GrMethod; - -public class GroovyMethodWithInconsistentReturnsInspection extends BaseInspection { - - @Nls - @NotNull - public String getGroupDisplayName() { - return VALIDITY_ISSUES; - } - - @NotNull - public String getDisplayName() { - return "Method with inconsistent returns"; - } - - - public boolean isEnabledByDefault() { - return true; - } - - @Nullable - protected String buildErrorString(Object... args) { - return "Method '#ref' has inconsistent return points #loc"; - } - - public BaseInspectionVisitor buildVisitor() { - return new Visitor(); - } - - private static class Visitor extends BaseInspectionVisitor { - public void visitMethod(GrMethod method) { - super.visitMethod(method); - if (!methodHasReturnValues(method)) { - return; - } - if (!methodHasValuelessReturns(method)) { - return; - } - registerMethodError(method); - } - } - - private static boolean methodHasReturnValues(GrMethod method) { - final ReturnValuesVisitor visitor = new ReturnValuesVisitor(method); - method.accept(visitor); - return visitor.hasReturnValues(); - } - - private static boolean methodHasValuelessReturns(GrMethod method) { - final PsiElement lastChild = method.getLastChild(); - if (lastChild instanceof GrBlockStatement) { - if (ControlFlowUtils.statementMayCompleteNormally((GrStatement) lastChild)) { - return true; - } - } - final ValuelessReturnVisitor visitor = new ValuelessReturnVisitor(method); - method.accept(visitor); - return visitor.hasValuelessReturns(); - } - - private static class ReturnValuesVisitor extends GroovyRecursiveElementVisitor { - private final GrMethod method; - private boolean hasReturnValues = false; - - ReturnValuesVisitor(GrMethod method) { - this.method = method; - } - - public void visitReturnStatement(GrReturnStatement statement) { - super.visitReturnStatement(statement); - if (statement.getReturnValue() != null) { - final GrMethod containingMethod = - PsiTreeUtil.getParentOfType(statement, GrMethod.class); - if (method.equals(containingMethod)) { - hasReturnValues = true; - } - } - } - - public boolean hasReturnValues() { - return hasReturnValues; - } - } - - private static class ValuelessReturnVisitor extends GroovyRecursiveElementVisitor { - private final GrMethod method; - private boolean hasValuelessReturns = false; - - ValuelessReturnVisitor(GrMethod method) { - this.method = method; - } - - public void visitReturnStatement(GrReturnStatement statement) { - super.visitReturnStatement(statement); - if (statement.getReturnValue() == null) { - final GrMethod containingMethod = - PsiTreeUtil.getParentOfType(statement, GrMethod.class); - if (method.equals(containingMethod)) { - hasValuelessReturns = true; - } - } - } - - public void visitGrMethod(GrMethod method) { - // do nothing, so that it doesn't drill into nested methods - } - - public boolean hasValuelessReturns() { - return hasValuelessReturns; - } - } -} \ No newline at end of file diff --git a/plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/statements/expressions/TypesUtil.java b/plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/statements/expressions/TypesUtil.java index a21546214a7c..b280f86159cf 100644 --- a/plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/statements/expressions/TypesUtil.java +++ b/plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/statements/expressions/TypesUtil.java @@ -194,7 +194,7 @@ public class TypesUtil { public static boolean isAssignable(PsiType lType, PsiType rType, PsiManager manager, GlobalSearchScope scope) { //all numeric types are assignable if (isNumericType(lType)) { - return isNumericType(rType) || rType.equalsToText(JAVA_LANG_STRING) || rType.equals(PsiType.NULL); + return isNumericType(rType) || rType.equals(PsiType.NULL); } if (rType instanceof GrTupleType) { final GrTupleType tuple = (GrTupleType)rType; @@ -264,7 +264,7 @@ public class TypesUtil { } public static PsiType boxPrimitiveType(PsiType result, PsiManager manager, GlobalSearchScope resolveScope) { - if (result instanceof PsiPrimitiveType) { + if (result instanceof PsiPrimitiveType && result != PsiType.VOID) { PsiPrimitiveType primitive = (PsiPrimitiveType)result; String boxedTypeName = primitive.getBoxedTypeName(); if (boxedTypeName != null) {