Java: Simplify side effects check (IDEA-182669)

java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java

@@ -1,6 +1,7 @@
 // Copyright 2000-2017 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
 package com.intellij.codeInspection.intermediaryVariable;
 
+import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil;
 import com.intellij.codeInspection.*;
 import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.openapi.project.Project;
@@ -13,12 +14,12 @@ import com.intellij.psi.util.PsiTypesUtil;
 import com.intellij.psi.util.PsiUtil;
 import com.intellij.refactoring.util.InlineUtil;
 import com.intellij.refactoring.util.RefactoringUtil;
+import com.intellij.util.ObjectUtils;
 import com.intellij.util.Query;
 import com.intellij.util.containers.ContainerUtil;
 import com.siyeh.ig.psiutils.CommentTracker;
 import com.siyeh.ig.psiutils.ExpressionUtils;
 import com.siyeh.ig.psiutils.HighlightUtils;
-import com.siyeh.ig.psiutils.SideEffectChecker;
 import gnu.trove.THashMap;
 import gnu.trove.THashSet;
 import org.jetbrains.annotations.Contract;
@@ -200,20 +201,13 @@ public class ReturnSeparatedFromComputationInspection extends AbstractBaseJavaLo
 
   private static void deleteRedundantVariable(@NotNull ReturnContext context, Highlighter highlighter) {
     PsiExpression value = PsiUtil.skipParenthesizedExprDown(context.returnedVariable.getInitializer());
-    if (value != null && SideEffectChecker.mayHaveSideEffects(value)) {
+
+    boolean isConstant = value instanceof PsiLiteralExpression || value instanceof PsiThisExpression || PsiUtil.isConstantExpression(value);
+    boolean isSimple = isSimpleExpression(value, context.returnScope);
+    if (value != null && !isConstant && !isSimple) {
       return;
     }
-    boolean isConstant = value instanceof PsiLiteralExpression || value instanceof PsiThisExpression || PsiUtil.isConstantExpression(value);
-    boolean isSimple = isSimpleExpression(value);
 
-    if (value != null && !isConstant) {
-      List<PsiReferenceExpression> references = findAnyReferences(value);
-      for (PsiReferenceExpression reference : references) {
-        if (isModifiedInScope(reference, context.variableScope)) {
-          return;
-        }
-      }
-    }
     Query<PsiReference> query = ReferencesSearch.search(context.returnedVariable, new LocalSearchScope(context.variableScope));
     Collection<PsiReference> usages = query.findAll();
     for (PsiReference usage : usages) {
@@ -235,10 +229,12 @@ public class ReturnSeparatedFromComputationInspection extends AbstractBaseJavaLo
     }
   }
 
-  @Contract("null -> false")
-  private static boolean isSimpleExpression(PsiExpression expression) {
+  @Contract("null,_ -> false")
+  private static boolean isSimpleExpression(@Nullable PsiExpression expression, @NotNull PsiElement scope) {
     if (expression instanceof PsiReferenceExpression) {
-      return ((PsiReferenceExpression)expression).resolve() instanceof PsiVariable;
+      PsiVariable variable = ObjectUtils.tryCast(((PsiReferenceExpression)expression).resolve(), PsiVariable.class);
+      return variable != null && (variable.hasModifierProperty(PsiModifier.FINAL) ||
+                                  HighlightControlFlowUtil.isEffectivelyFinal(variable, scope, null));
     }
     if (expression instanceof PsiUnaryExpression) {
       return ((PsiUnaryExpression)expression).getOperand() instanceof PsiLiteralExpression; // "-1" and "!true"
@@ -248,34 +244,6 @@ public class ReturnSeparatedFromComputationInspection extends AbstractBaseJavaLo
            expression instanceof PsiClassObjectAccessExpression;
   }
 
-  @NotNull
-  private static List<PsiReferenceExpression> findAnyReferences(@NotNull PsiElement element) {
-    List<PsiReferenceExpression> references = new ArrayList<>();
-    element.accept(new JavaRecursiveElementWalkingVisitor() {
-      @Override
-      public void visitReferenceExpression(PsiReferenceExpression expression) {
-        references.add(expression);
-        super.visitReferenceExpression(expression);
-      }
-    });
-    return references;
-  }
-
-  private static boolean isModifiedInScope(@NotNull PsiReferenceExpression reference, @NotNull PsiElement scope) {
-    PsiElement resolved = reference.resolve();
-    if (resolved instanceof PsiVariable) {
-      PsiVariable variable = (PsiVariable)resolved;
-      if (variable.hasModifierProperty(PsiModifier.FINAL)) {
-        return false;
-      }
-      if (!(variable instanceof PsiLocalVariable) && !(variable instanceof PsiParameter)) {
-        return true;
-      }
-      return RefactoringUtil.isModifiedInScope(variable, scope);
-    }
-    return false;
-  }
-
   private static void applyChanges(@NotNull Mover mover, @NotNull ReturnContext context, boolean removeReturn, Highlighter highlighter) {
     for (PsiElement anchor : mover.insertBefore) {
       PsiElement added = anchor.getParent().addBefore(context.returnStatement, anchor);

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterArrayInitializer.java

@@ -1,8 +1,9 @@
 // "Move 'return' closer to computation of the value of 'n'" "true"
 class T {
     int[] f(int k) {
+        int[] n = {-1};
         if (k == 1)
             return new int[]{1};
-        return new int[]{-1};
+        return n;
     }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterInitExprOneUsage.java

@@ -3,8 +3,9 @@ import java.util.*;
 
 class T {
     List foo(boolean b) {
+        List n = new ArrayList();
         if (b)
             return null;
-        return new ArrayList();
+        return n;
     }
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/afterInitExprTwoUsages.java

@@ -3,8 +3,9 @@ import java.util.*;
 
 class T {
     List foo(boolean b) {
+        List n = new ArrayList();
         if (b)
             return new ArrayList(1);
-        return new ArrayList();
+        return n;
     }
 }