returns without value aren't returning a value; ignored allocation result false positive (IDEA-51880)

2026-09-27 10:03:11 +07:00 · 2010-02-03 17:44:26 +00:00
parent ff07a64362
commit 995261dba6
19 changed files with 78 additions and 141 deletions
@@ -1,8 +0,0 @@
-<html>
-<body><table> <tr> <td valign="top" height="150">
-<font face="verdana" size="-1">
-This inspection reports any instances of Groovy array allocation where the array allocated ignored.
-Such allocation expressions are legal Groovy, but are usually either inadvertant, or
-evidence of a very odd object initialization strategy.
-</font></td> </tr> <tr> <td height="20"> <font face="verdana" size="-2">Powered by InspectorGroovy </font> </td> </tr> </table> </body>
-</html>
@@ -133,7 +133,6 @@ public class GroovyInspectionProvider implements InspectionToolProvider, Applica
        GroovyInfiniteLoopStatementInspection.class,
        GroovyInfiniteRecursionInspection.class,
        GroovyDivideByZeroInspection.class,
-        GroovyResultOfArrayAllocationIgnoredInspection.class,
        GroovyResultOfObjectAllocationIgnoredInspection.class,

        GroovyClassNamingConventionInspection.class,
@@ -1,88 +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.bugs;
-
-import com.intellij.psi.PsiElement;
-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.api.statements.blocks.GrCodeBlock;
-import org.jetbrains.plugins.groovy.lang.psi.api.statements.blocks.GrOpenBlock;
-import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrNewExpression;
-import org.jetbrains.plugins.groovy.lang.psi.api.types.GrCodeReferenceElement;
-
-public class GroovyResultOfArrayAllocationIgnoredInspection extends BaseInspection {
-
-  @Nls
-  @NotNull
-  public String getGroupDisplayName() {
-    return PROBABLE_BUGS;
-  }
-
-  @Nls
-  @NotNull
-  public String getDisplayName() {
-    return "Result of array allocation ignored";
-  }
-
-  @Nullable
-  protected String buildErrorString(Object... args) {
-    final Boolean isCompleteExpression = (Boolean) args[0];
-    if (isCompleteExpression.booleanValue()) {
-      return "Result of <code>#ref</code> is ignored #loc";
-    } else {
-      return "Result of <code>new #ref[]</code> is ignored #loc";
-    }
-  }
-
-  public boolean isEnabledByDefault() {
-    return true;
-  }
-
-  public BaseInspectionVisitor buildVisitor() {
-    return new Visitor();
-  }
-
-  private static class Visitor extends BaseInspectionVisitor {
-
-    public void visitNewExpression(GrNewExpression newExpression) {
-      super.visitNewExpression(newExpression);
-      final PsiElement parent = newExpression.getParent();
-      if (!(parent instanceof GrCodeBlock)) {
-        return;
-      }
-      if (newExpression.getArrayCount() == 0) {
-        return;
-      }
-      if (parent instanceof GrOpenBlock) {
-        final GrOpenBlock openBlock = (GrOpenBlock) parent;
-        if (ControlFlowUtils.openBlockCompletesWithStatement(openBlock, newExpression)) {
-          return;
-        }
-      }
-
-      final GrCodeReferenceElement referenceElement = newExpression.getReferenceElement();
-      if (referenceElement != null) {
-        registerError(referenceElement, Boolean.FALSE);
-      } else {
-        registerError(newExpression, Boolean.TRUE);
-      }
-    }
-  }
-}
@@ -22,12 +22,11 @@ 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.GroovyFile;
+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.blocks.GrClosableBlock;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrNewExpression;
-import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrReferenceExpression;
-import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.path.GrMethodCallExpression;

 public class GroovyResultOfObjectAllocationIgnoredInspection extends BaseInspection {

@@ -45,7 +44,7 @@ public class GroovyResultOfObjectAllocationIgnoredInspection extends BaseInspect

  @Nullable
  protected String buildErrorString(Object... args) {
-    return "Result of <code>new #ref()</code> is ignored #loc";
+    return "Result of <code>new #ref" + (args[0].equals(new Integer(0)) ? "()" : "[]") + "</code> is ignored #loc";

  }

@@ -62,29 +61,19 @@ public class GroovyResultOfObjectAllocationIgnoredInspection extends BaseInspect
    public void visitNewExpression(GrNewExpression newExpression) {
      super.visitNewExpression(newExpression);
      final PsiElement parent = newExpression.getParent();
-      if (!(parent instanceof GrCodeBlock)) {
+      if (parent instanceof GrClosableBlock) {
        return;
      }
-      if (parent instanceof GrOpenBlock) {
-        final GrOpenBlock openBlock = (GrOpenBlock) parent;
-        if (ControlFlowUtils.openBlockCompletesWithStatement(openBlock, newExpression)) {
-          return;
-        }
-      } else if (parent instanceof GrClosableBlock) {
-        final PsiElement grandParent = parent.getParent();
-        if (grandParent instanceof GrMethodCallExpression) {
-          return;
-        } else if (grandParent instanceof GrReferenceExpression) {
-          final PsiElement greatGrandParent = grandParent.getParent();
-          if (greatGrandParent instanceof GrMethodCallExpression) {
+
+      if (parent instanceof GrCodeBlock || parent instanceof GroovyFile) {
+        if (parent instanceof GrOpenBlock) {
+          final GrOpenBlock openBlock = (GrOpenBlock)parent;
+          if (ControlFlowUtils.openBlockCompletesWithStatement(openBlock, newExpression)) {
            return;
          }
        }
+        registerError(newExpression.getReferenceElement(), newExpression.getArrayCount());
      }
-      if (newExpression.getArrayCount() != 0) {
-        return;
-      }
-      registerError(newExpression.getReferenceElement());
    }
  }
 }
@@ -99,7 +99,9 @@ public class MissingReturnInspection extends GroovySuppressableInspectionTool {
        final PsiElement element = instruction.getElement();
        if (element instanceof GrReturnStatement) {
          sometimes.set(true);
-          hasExplicitReturn.set(true);
+          if (((GrReturnStatement)element).getReturnValue() != null) {
+            hasExplicitReturn.set(true);
+          }
        }
        else if (element instanceof GrThrowStatement || element instanceof GrAssertStatement) {
          sometimes.set(true);
@@ -18,10 +18,10 @@ import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.plugins.groovy.codeInspection.assignment.GroovyAssignabilityCheckInspection;
 import org.jetbrains.plugins.groovy.codeInspection.assignment.GroovyUncheckedAssignmentOfMemberOfRawTypeInspection;
+import org.jetbrains.plugins.groovy.codeInspection.bugs.GroovyResultOfObjectAllocationIgnoredInspection;
 import org.jetbrains.plugins.groovy.codeInspection.control.GroovyTrivialConditionalInspection;
 import org.jetbrains.plugins.groovy.codeInspection.control.GroovyTrivialIfInspection;
 import org.jetbrains.plugins.groovy.codeInspection.metrics.GroovyOverlyLongMethodInspection;
-import org.jetbrains.plugins.groovy.codeInspection.noReturnMethod.MissingReturnInspection;
 import org.jetbrains.plugins.groovy.codeInspection.unassignedVariable.UnassignedVariableAccessInspection;
 import org.jetbrains.plugins.groovy.codeInspection.untypedUnresolvedAccess.GroovyUnresolvedAccessInspection;
 import org.jetbrains.plugins.groovy.codeInspection.untypedUnresolvedAccess.GroovyUntypedAccessInspection;
@@ -34,6 +34,17 @@ import java.io.IOException;
 * @author peter
 */
 public class GroovyHighlightingTest extends LightCodeInsightFixtureTestCase {
+  public static final DefaultLightProjectDescriptor GROOVY_17_PROJECT_DESCRIPTOR = new DefaultLightProjectDescriptor() {
+    @Override
+    public void configureModule(Module module, ModifiableRootModel model, ContentEntry contentEntry) {
+      final Library.ModifiableModel modifiableModel = model.getModuleLibraryTable().createLibrary("GROOVY").getModifiableModel();
+      final VirtualFile groovyJar =
+        JarFileSystem.getInstance().refreshAndFindFileByPath(TestUtils.getMockGroovy1_7LibraryName()+"!/");
+      modifiableModel.addRoot(groovyJar, OrderRootType.CLASSES);
+      modifiableModel.commit();
+    }
+  };
+
  @Override
  protected String getBasePath() {
    return TestUtils.getTestDataPath() + "highlighting/";
@@ -42,16 +53,7 @@ public class GroovyHighlightingTest extends LightCodeInsightFixtureTestCase {
  @NotNull
  @Override
  protected LightProjectDescriptor getProjectDescriptor() {
-    return new DefaultLightProjectDescriptor() {
-      @Override
-      public void configureModule(Module module, ModifiableRootModel model, ContentEntry contentEntry) {
-        final Library.ModifiableModel modifiableModel = model.getModuleLibraryTable().createLibrary("GROOVY").getModifiableModel();
-        final VirtualFile groovyJar =
-          JarFileSystem.getInstance().refreshAndFindFileByPath(TestUtils.getMockGroovy1_7LibraryName()+"!/");
-        modifiableModel.addRoot(groovyJar, OrderRootType.CLASSES);
-        modifiableModel.commit();
-      }
-    };
+    return GROOVY_17_PROJECT_DESCRIPTOR;
  }

  public void testDuplicateClosurePrivateVariable() throws Throwable {
@@ -139,18 +141,9 @@ public class GroovyHighlightingTest extends LightCodeInsightFixtureTestCase {
  public void testDefaultMapConstructorNamedArgsError() throws Throwable {doTest();}
  public void testDefaultMapConstructorWhenDefConstructorExists() throws Throwable {doTest();}

-  public void testUnresolvedLhsAssignment() throws Throwable { doTest(new GroovyUnresolvedAccessInspection()); }
+  public void testSingleAllocationInClosure() throws Throwable {doTest(new GroovyResultOfObjectAllocationIgnoredInspection());}

-  public void testMissingReturnWithLastLoop() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnWithUnknownCall() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnWithIf() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnWithAssertion() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnThrowException() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnTryCatch() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnLastNull() throws Throwable { doTest(new MissingReturnInspection()); }
-  public void testMissingReturnImplicitReturns() throws Throwable {doTest(new MissingReturnInspection());}
-  public void testMissingReturnOvertReturnType() throws Throwable {doTest(new MissingReturnInspection());}
-  public void testMissingReturnFromClosure() throws Throwable {doTest(new MissingReturnInspection());}
+  public void testUnresolvedLhsAssignment() throws Throwable { doTest(new GroovyUnresolvedAccessInspection()); }

  public void testUnresolvedMethodCallWithTwoDeclarations() throws Throwable{
    doTest();
@@ -0,0 +1,42 @@
+package org.jetbrains.plugins.groovy.lang;
+
+import com.intellij.testFramework.LightProjectDescriptor;
+import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.plugins.groovy.codeInspection.noReturnMethod.MissingReturnInspection;
+import org.jetbrains.plugins.groovy.util.TestUtils;
+
+/**
+ * @author peter
+ */
+public class MissingReturnTest extends LightCodeInsightFixtureTestCase {
+
+  @Override
+  protected String getBasePath() {
+    return TestUtils.getTestDataPath() + "highlighting/missingReturn";
+  }
+
+  @NotNull
+  @Override
+  protected LightProjectDescriptor getProjectDescriptor() {
+    return GroovyHighlightingTest.GROOVY_17_PROJECT_DESCRIPTOR;
+  }
+
+  public void testMissingReturnWithLastLoop() throws Throwable { doTest(); }
+  public void testMissingReturnWithUnknownCall() throws Throwable { doTest(); }
+  public void testMissingReturnWithIf() throws Throwable { doTest(); }
+  public void testMissingReturnWithAssertion() throws Throwable { doTest(); }
+  public void testMissingReturnThrowException() throws Throwable { doTest(); }
+  public void testMissingReturnTryCatch() throws Throwable { doTest(); }
+  public void testMissingReturnLastNull() throws Throwable { doTest(); }
+  public void testMissingReturnImplicitReturns() throws Throwable {doTest();}
+  public void testMissingReturnOvertReturnType() throws Throwable {doTest();}
+  public void testMissingReturnFromClosure() throws Throwable {doTest();}
+  public void testReturnsWithoutValue() throws Throwable {doTest();}
+
+  private void doTest() throws Exception {
+    myFixture.enableInspections(new MissingReturnInspection());
+    myFixture.testHighlighting(true, false, false, getTestName(false) + ".groovy");
+  }
+
+}
@@ -0,0 +1,3 @@
+def c = { new String() }
+def d = { new String[0] }
+new <warning descr="Result of 'new String()' is ignored">String</warning>()
@@ -0,0 +1,5 @@
+list.each {c ->
+  if (some) {
+    return
+  }
+}