improve method return statements inspection
authorMaxim Medvedev <maxim.medvedev@jetbrains.com>
Fri, 13 Nov 2009 17:24:22 +0000 (20:24 +0300)
committerMaxim Medvedev <maxim.medvedev@jetbrains.com>
Fri, 13 Nov 2009 17:24:22 +0000 (20:24 +0300)
plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html [deleted file]
plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/GroovyInspectionProvider.java
plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/assignment/GroovyAssignabilityCheckInspection.java
plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/noReturnMethod/MissingReturnInspection.java
plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/utils/ControlFlowUtils.java
plugins/groovy/src/org/jetbrains/plugins/groovy/codeInspection/validity/GroovyMethodWithInconsistentReturnsInspection.java [deleted file]
plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/statements/expressions/TypesUtil.java

diff --git a/plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html b/plugins/groovy/resources/inspectionDescriptions/GroovyMethodWithInconsistentReturns.html
deleted file mode 100644 (file)
index 9404c2b..0000000
+++ /dev/null
@@ -1,7 +0,0 @@
-<html>
-<body><table> <tr> <td valign="top" height="150">
-<font face="verdana" size="-1">
-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.
-</font></td> </tr> <tr> <td height="20"> <font face="verdana" size="-2">Powered by InspectorGroovy </font> </td> </tr> </table> </body>
-</html>
index 5937c022109baf1ed8b0587743f3d881edbc42b9..13a92bf7135b4d66a3b1bd7e6b36442dfc7d3dda 100644 (file)
@@ -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,
index 0c9c070dc4bb49dbff6756a6a6c74bbdd060fd4a..4acea75b2082260b3d2200637ef4649cb8c0ab16 100644 (file)
@@ -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;
-
-      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 || PsiType.VOID.equals(expectedType)) return;
 
-      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
index be272479ed7adedf3b851864d08b492cec6952c6..9600ce6c0328301f5d430da414aeedb87108ab1b 100644 (file)
@@ -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<Boolean> always = new Ref<Boolean>(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();
   }
 }
index a3f0059bfbe4519a5469a6dc0a5ba97298343e7c..86aa6c0503e419f9a3c728e6c2f67fbb1e28ea86 100644 (file)
@@ -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 (file)
index 9f6d4ba..0000000
+++ /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
index a21546214a7c79f9f027abad2dbf46fdb50bb8ab..b280f86159cf566997d644fa96c148ac40445f0e 100644 (file)
@@ -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) {