IG: fix tail recursion quickfix when the value of a parameter is dependent on the previous value of another parameter (IDEA-164124)

2026-09-27 10:03:11 +07:00 · 2016-11-18 16:57:14 +01:00
parent 16d27212c4
commit 4df7574a70
14 changed files with 403 additions and 77 deletions
@@ -1,5 +1,5 @@
 /*
- * Copyright 2003-2011 Dave Griffith, Bas Leijdekkers
+ * Copyright 2003-2016 Dave Griffith, Bas Leijdekkers
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@@ -21,16 +21,23 @@ import com.intellij.psi.*;
 import com.intellij.psi.codeStyle.CodeStyleManager;
 import com.intellij.psi.codeStyle.JavaCodeStyleManager;
 import com.intellij.psi.util.PsiTreeUtil;
-import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.graph.CachingSemiGraph;
+import com.intellij.util.graph.DFSTBuilder;
+import com.intellij.util.graph.Graph;
+import com.intellij.util.graph.GraphGenerator;
 import com.siyeh.InspectionGadgetsBundle;
 import com.siyeh.ig.BaseInspection;
 import com.siyeh.ig.BaseInspectionVisitor;
 import com.siyeh.ig.InspectionGadgetsFix;
 import com.siyeh.ig.psiutils.ControlFlowUtils;
+import com.siyeh.ig.psiutils.ParenthesesUtils;
+import com.siyeh.ig.psiutils.VariableAccessUtils;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

+import java.util.*;
+
 public class TailRecursionInspection extends BaseInspection {

  @Override
@@ -42,8 +49,7 @@ public class TailRecursionInspection extends BaseInspection {
  @Override
  @NotNull
  protected String buildErrorString(Object... infos) {
-    return InspectionGadgetsBundle.message(
-      "tail.recursion.problem.descriptor");
+    return InspectionGadgetsBundle.message("tail.recursion.problem.descriptor");
  }

  @Override
@@ -57,8 +63,10 @@ public class TailRecursionInspection extends BaseInspection {
  }

  private static boolean mayBeReplacedByIterativeMethod(PsiMethod containingMethod) {
-    final PsiParameterList parameterList = containingMethod.getParameterList();
-    final PsiParameter[] parameters = parameterList.getParameters();
+    if (containingMethod.isVarArgs()) {
+      return false;
+    }
+    final PsiParameter[] parameters = containingMethod.getParameterList().getParameters();
    for (final PsiParameter parameter : parameters) {
      if (parameter.hasModifierProperty(PsiModifier.FINAL)) {
        return false;
@@ -72,15 +80,14 @@ public class TailRecursionInspection extends BaseInspection {
    @Override
    @NotNull
    public String getFamilyName() {
-      return InspectionGadgetsBundle.message(
-        "tail.recursion.replace.quickfix");
+      return InspectionGadgetsBundle.message("tail.recursion.replace.quickfix");
    }

    @Override
-    public void doFix(Project project, ProblemDescriptor descriptor)
-      throws IncorrectOperationException {
+    public void doFix(Project project, ProblemDescriptor descriptor) {
      final PsiElement tailCallToken = descriptor.getPsiElement();
-      final PsiMethod method = PsiTreeUtil.getParentOfType(tailCallToken, PsiMethod.class, true, PsiClass.class, PsiLambdaExpression.class);
+      final PsiMethod method =
+        PsiTreeUtil.getParentOfType(tailCallToken, PsiMethod.class, true, PsiClass.class, PsiLambdaExpression.class);
      if (method == null) {
        return;
      }
@@ -99,16 +106,12 @@ public class TailRecursionInspection extends BaseInspection {
      if (methodReturnsContainingClassType(method, containingClass)) {
        builder.append(containingClass.getName());
        thisVariableName = styleManager.suggestUniqueVariableName("result", method, false);
-        builder.append(' ');
-        builder.append(thisVariableName);
-        builder.append(" = this;");
+        builder.append(' ').append(thisVariableName).append(" = this;");
      }
      else if (methodContainsCallOnOtherInstance(method)) {
        builder.append(containingClass.getName());
        thisVariableName = styleManager.suggestUniqueVariableName("other", method, false);
-        builder.append(' ');
-        builder.append(thisVariableName);
-        builder.append(" = this;");
+        builder.append(' ').append(thisVariableName).append(" = this;");
      }
      else {
        thisVariableName = null;
@@ -116,22 +119,17 @@ public class TailRecursionInspection extends BaseInspection {
      final boolean tailCallIsContainedInLoop;
      if (ControlFlowUtils.isInLoop(tailCallToken)) {
        tailCallIsContainedInLoop = true;
-        builder.append(method.getName());
-        builder.append(':');
+        builder.append(method.getName()).append(':');
      }
      else {
        tailCallIsContainedInLoop = false;
      }
      builder.append("while(true)");
-      final CodeStyleManager codeStyleManager = CodeStyleManager.getInstance(project);
      replaceTailCalls(body, method, thisVariableName, tailCallIsContainedInLoop, builder);
      builder.append('}');
-      @NonNls final String replacementText = builder.toString();
-      final JavaPsiFacade psiFacade = JavaPsiFacade.getInstance(project);
-      final PsiElementFactory elementFactory = psiFacade.getElementFactory();
-      final PsiCodeBlock block = elementFactory.createCodeBlockFromText(replacementText, method);
+      final PsiCodeBlock block = JavaPsiFacade.getElementFactory(project).createCodeBlockFromText(builder.toString(), method);
      body.replace(block);
-      codeStyleManager.reformat(method);
+      CodeStyleManager.getInstance(project).reformat(method);
    }

    private static boolean methodReturnsContainingClassType(PsiMethod method, PsiClass containingClass) {
@@ -169,7 +167,7 @@ public class TailRecursionInspection extends BaseInspection {
      private boolean containsCallOnOtherInstance;
      private final PsiClass aClass;

-      private MethodContainsCallOnOtherInstanceVisitor(PsiClass aClass) {
+      MethodContainsCallOnOtherInstanceVisitor(PsiClass aClass) {
        this.aClass = aClass;
      }

@@ -194,25 +192,22 @@ public class TailRecursionInspection extends BaseInspection {
        }
      }

-      private boolean containsCallOnOtherInstance() {
+      boolean containsCallOnOtherInstance() {
        return containsCallOnOtherInstance;
      }
    }

    private static void replaceTailCalls(PsiElement element, PsiMethod method, @Nullable String thisVariableName, 
                                         boolean tailCallIsContainedInLoop, @NonNls StringBuilder out) {
-      final String text = element.getText();
      if (isImplicitCallOnThis(element, method)) {
        if (thisVariableName != null) {
-          out.append(thisVariableName);
-          out.append('.');
+          out.append(thisVariableName).append('.');
        }
-        out.append(text);
+        out.append(element.getText());
      }
-      else if (element instanceof PsiThisExpression ||
-               element instanceof PsiSuperExpression) {
+      else if (element instanceof PsiThisExpression || element instanceof PsiSuperExpression) {
        if (thisVariableName == null) {
-          out.append(text);
+          out.append(element.getText());
        }
        else {
          out.append(thisVariableName);
@@ -220,38 +215,59 @@ public class TailRecursionInspection extends BaseInspection {
      }
      else if (isTailCallReturn(element, method)) {
        final PsiReturnStatement returnStatement = (PsiReturnStatement)element;
-        final PsiMethodCallExpression call = (PsiMethodCallExpression)returnStatement.getReturnValue();
+        final PsiMethodCallExpression call = (PsiMethodCallExpression)ParenthesesUtils.stripParentheses(returnStatement.getReturnValue());
        assert call != null;
-        final PsiExpressionList argumentList = call.getArgumentList();
-        final PsiExpression[] arguments = argumentList.getExpressions();
-        final PsiParameterList parameterList = method.getParameterList();
-        final PsiParameter[] parameters = parameterList.getParameters();
+        final PsiExpression[] arguments = call.getArgumentList().getExpressions();
+        final PsiParameter[] parameters = method.getParameterList().getParameters();
        final boolean isInBlock = returnStatement.getParent() instanceof PsiCodeBlock;
        if (!isInBlock) {
          out.append('{');
        }
-        for (int i = 0; i < parameters.length; i++) {
-          final PsiParameter parameter = parameters[i];
-          final PsiExpression argument = arguments[i];
-          final String parameterName = parameter.getName();
-          if (parameterName == null) {
-            continue;
+        final Graph<Integer> graph = buildGraph(parameters, arguments);
+        final DFSTBuilder<Integer> builder = new DFSTBuilder<>(graph);
+        final Collection<Collection<Integer>> componentOrdering = builder.getComponents();
+        final Set<Integer> seen = new HashSet<>();
+        final Map<PsiElement, String> replacements = new HashMap<>();
+        for (Collection<Integer> component : componentOrdering) {
+          for (Integer index : component) {
+            final Iterator<Integer> in = graph.getIn(index);
+            final PsiParameter parameter = parameters[index];
+            final String parameterName = parameter.getName();
+            assert parameterName != null;
+            final PsiExpression argument = ParenthesesUtils.stripParentheses(arguments[index]);
+            assert argument != null;
+            if (argument instanceof PsiReferenceExpression) {
+              final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)argument;
+              if (parameter.equals(referenceExpression.resolve())) {
+                continue;
+              }
+            }
+            boolean copy = false;
+            while (in.hasNext()) {
+              if (!seen.contains(in.next())) {
+                // if 'in' was some collection instead of an iterator this would have been a nice containsAll expression
+                copy = true;
+                break;
+              }
+            }
+            if (copy) {
+              final String variableName =
+                JavaCodeStyleManager.getInstance(method.getProject()).suggestUniqueVariableName(parameterName, element, false);
+              out.append(parameter.getType().getCanonicalText()).append(' ').append(variableName).append('=');
+              out.append(parameterName).append(';');
+              replacements.put(parameter, variableName);
+            }
+            out.append(parameterName).append('=');
+            buildText(argument, replacements, out);
+            out.append(';');
+            seen.add(index);
          }
-          final String argumentText = argument.getText();
-          if (parameterName.equals(argumentText)) {
-            continue;
-          }
-          out.append(parameterName);
-          out.append(" = ");
-          out.append(argumentText);
-          out.append(';');
        }
        if (thisVariableName != null) {
          final PsiReferenceExpression methodExpression = call.getMethodExpression();
          final PsiExpression qualifier = methodExpression.getQualifierExpression();
          if (qualifier != null) {
-            out.append(thisVariableName);
-            out.append(" = ");
+            out.append(thisVariableName).append('=');
            replaceTailCalls(qualifier, method, thisVariableName, tailCallIsContainedInLoop, out);
            out.append(';');
          }
@@ -262,10 +278,7 @@ public class TailRecursionInspection extends BaseInspection {
          //don't do anything, as the continue is unnecessary
        }
        else if (tailCallIsContainedInLoop) {
-          final String methodName = method.getName();
-          out.append("continue ");
-          out.append(methodName);
-          out.append(';');
+          out.append("continue ").append(method.getName()).append(';');
        }
        else {
          out.append("continue;");
@@ -277,7 +290,7 @@ public class TailRecursionInspection extends BaseInspection {
      else {
        final PsiElement[] children = element.getChildren();
        if (children.length == 0) {
-          out.append(text);
+          out.append(element.getText());
        }
        else {
          for (final PsiElement child : children) {
@@ -287,8 +300,54 @@ public class TailRecursionInspection extends BaseInspection {
      }
    }

-    private static boolean isImplicitCallOnThis(PsiElement element,
-                                                PsiMethod containingMethod) {
+    private static void buildText(PsiElement element, Map<PsiElement, String> replacements, StringBuilder out) {
+      if (element instanceof PsiReferenceExpression) {
+        final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)element;
+        final PsiElement target = referenceExpression.resolve();
+        final String replacement = replacements.get(target);
+        out.append(replacement != null ? replacement : element.getText());
+        return;
+      }
+      final PsiElement[] children = element.getChildren();
+      if (children.length > 0) {
+        for (PsiElement child : children) {
+          buildText(child, replacements, out);
+        }
+      }
+      else {
+        out.append(element.getText());
+      }
+    }
+
+    private static Graph<Integer> buildGraph(PsiParameter[] parameters, PsiExpression[] arguments) {
+      final GraphGenerator.SemiGraph<Integer> graph = new GraphGenerator.SemiGraph<Integer>() {
+
+        @Override
+        public Collection<Integer> getNodes() {
+          final List<Integer> result = new ArrayList<>();
+          for (int i = 0; i < parameters.length; i++) {
+            result.add(i);
+          }
+          return result;
+        }
+
+        @Override
+        public Iterator<Integer> getIn(Integer n) {
+          final List<Integer> result = new ArrayList<>();
+          final PsiParameter target = parameters[n];
+          for (int i = 0, length = arguments.length; i < length; i++) {
+            if (i == n) continue;
+            if (VariableAccessUtils.variableIsUsed(target, arguments[i])) {
+              result.add(i);
+            }
+          }
+          return result.iterator();
+        }
+      };
+      return GraphGenerator.create(CachingSemiGraph.create(graph));
+    }
+
+    private static boolean isImplicitCallOnThis(PsiElement element, PsiMethod containingMethod) {
      if (containingMethod.hasModifierProperty(PsiModifier.STATIC)) {
        return false;
      }
@@ -316,13 +375,12 @@ public class TailRecursionInspection extends BaseInspection {
      }
    }

-    private static boolean isTailCallReturn(PsiElement element,
-                                            PsiMethod containingMethod) {
+    private static boolean isTailCallReturn(PsiElement element, PsiMethod containingMethod) {
      if (!(element instanceof PsiReturnStatement)) {
        return false;
      }
      final PsiReturnStatement returnStatement = (PsiReturnStatement)element;
-      final PsiExpression returnValue = returnStatement.getReturnValue();
+      final PsiExpression returnValue = ParenthesesUtils.stripParentheses(returnStatement.getReturnValue());
      if (!(returnValue instanceof PsiMethodCallExpression)) {
        return false;
      }
@@ -342,25 +400,18 @@ public class TailRecursionInspection extends BaseInspection {
    @Override
    public void visitReturnStatement(@NotNull PsiReturnStatement statement) {
      super.visitReturnStatement(statement);
-      final PsiExpression returnValue = statement.getReturnValue();
+      final PsiExpression returnValue = ParenthesesUtils.stripParentheses(statement.getReturnValue());
      if (!(returnValue instanceof PsiMethodCallExpression)) {
        return;
      }
      final PsiMethodCallExpression returnCall = (PsiMethodCallExpression)returnValue;
-      final PsiMethod containingMethod = PsiTreeUtil.getParentOfType(statement, PsiMethod.class, true, PsiClass.class, PsiLambdaExpression.class);
+      final PsiMethod containingMethod =
+        PsiTreeUtil.getParentOfType(statement, PsiMethod.class, true, PsiClass.class, PsiLambdaExpression.class);
      if (containingMethod == null) {
        return;
      }
-      final PsiReferenceExpression methodExpression = returnCall.getMethodExpression();
-      final String name = containingMethod.getName();
-      if (!name.equals(methodExpression.getReferenceName())) {
-        return;
-      }
-      final PsiMethod method = returnCall.resolveMethod();
-      if (method == null) {
-        return;
-      }
-      if (!method.equals(containingMethod)) {
+      final JavaResolveResult resolveResult = returnCall.resolveMethodGenerics();
+      if (!resolveResult.isValidResult() || !containingMethod.equals(resolveResult.getElement())) {
        return;
      }
      registerMethodCallError(returnCall, containingMethod);
@@ -0,0 +1,30 @@
+class CallOnOtherInstance1 {
+  private boolean duplicate;
+  private Something something;
+  private CallOnOtherInstance1 original;
+
+    public Something getSomething() {
+        CallOnOtherInstance1 other = this;
+        while (true) {
+            if (other.something == null) {
+                if (other.isDuplicate()) {
+                    final CallOnOtherInstance1 recursion = other.getOriginal();
+                    other = recursion;
+                    continue;
+                } else {
+                    other.something = new Something();
+                }
+            }
+            return other.something;
+        }
+    }
+
+  private CallOnOtherInstance1 getOriginal() {
+    return original;
+  }
+  private boolean isDuplicate() {
+    return duplicate;
+  }
+
+  public static class Something {}
+}
@@ -0,0 +1,26 @@
+class CallOnOtherInstance1 {
+  private boolean duplicate;
+  private Something something;
+  private CallOnOtherInstance1 original;
+
+  public Something getSomething() {
+    if (something == null) {
+      if (isDuplicate()) {
+        final CallOnOtherInstance1 recursion = getOriginal();
+        return recursion.<caret>getSomething();
+      } else {
+        something = new Something();
+      }
+    }
+    return something;
+  }
+
+  private CallOnOtherInstance1 getOriginal() {
+    return original;
+  }
+  private boolean isDuplicate() {
+    return duplicate;
+  }
+
+  public static class Something {}
+}
@@ -0,0 +1,25 @@
+class CallOnOtherInstance2 {
+  private boolean duplicate;
+  private Something something;
+  private CallOnOtherInstance2 original;
+
+    public Something foo() {
+        CallOnOtherInstance2 other = this;
+        while (true) {
+            if (!other.duplicate) {
+                return other.something;
+            } else {
+                other = other.getOriginal();
+            }
+        }
+    }
+
+  private CallOnOtherInstance2 getOriginal() {
+    return original;
+  }
+  private boolean isDuplicate() {
+    return duplicate;
+  }
+
+  public static class Something {}
+}
@@ -0,0 +1,22 @@
+class CallOnOtherInstance2 {
+  private boolean duplicate;
+  private Something something;
+  private CallOnOtherInstance2 original;
+
+  public Something foo() {
+    if (!duplicate) {
+      return something;
+    } else {
+      return getOriginal().<caret>foo();
+    }
+  }
+
+  private CallOnOtherInstance2 getOriginal() {
+    return original;
+  }
+  private boolean isDuplicate() {
+    return duplicate;
+  }
+
+  public static class Something {}
+}
@@ -0,0 +1,17 @@
+class Dependency1 {
+
+  public int factorial(int val) {
+    return factorial(val, 1);
+  }
+
+    private int factorial(int val, int runningVal) {
+        while (true) {
+            if (val == 1) {
+                return runningVal;
+            } else {
+                runningVal = runningVal * val;
+                val = val - 1;
+            }
+        }
+    }
+}
@@ -0,0 +1,14 @@
+class Dependency1 {
+
+  public int factorial(int val) {
+    return factorial(val, 1);
+  }
+
+  private int factorial(int val, int runningVal) {
+    if (val == 1) {
+      return runningVal;
+    } else {
+      return (<caret>factorial(val - 1, runningVal * val));
+    }
+  }
+}
@@ -0,0 +1,17 @@
+class Dependency2 {
+
+    private static boolean intersect(@NotNull Set<String> ids1, @NotNull Set<String> ids2) {
+        while (true) {
+            if (ids1.size() > ids2.size()) {
+                Set<String> ids11 = ids1;
+                ids1 = ids2;
+                ids2 = ids11;
+                continue;
+            }
+            for (String id : ids1) {
+                if (ids2.contains(id)) return true;
+            }
+            return false;
+        }
+    }
+}
@@ -0,0 +1,10 @@
+class Dependency2 {
+
+  private static boolean intersect(@NotNull Set<String> ids1, @NotNull Set<String> ids2) {
+    if (ids1.size() > ids2.size()) return intersect<caret>(ids2, ids1);
+    for (String id : ids1) {
+      if (ids2.contains(id)) return true;
+    }
+    return false;
+  }
+}
@@ -0,0 +1,17 @@
+class ThisVariable {
+
+    private ThisVariable getRoot() {
+        ThisVariable result = this;
+        while (true) {
+            if (result.getParent() instanceof ThisVariable) {
+                result = ((ThisVariable) result.getParent());
+                continue;
+            }
+            return result;
+        }
+    }
+
+  public Object getParent() {
+    return null;
+  }
+}
@@ -0,0 +1,14 @@
+class ThisVariable {
+
+  private ThisVariable getRoot() {
+    if (getParent() instanceof ThisVariable)
+    {
+      return ((ThisVariable) getParent()).<caret>getRoot();
+    }
+    return this;
+  }
+
+  public Object getParent() {
+    return null;
+  }
+}
@@ -0,0 +1,22 @@
+import java.awt.*;
+
+class UnmodifiedParameter {
+
+    private static boolean hasAncestor(Container child, Container ancestor) {
+        while (true) {
+            if (child == null) {
+                return false;
+            }
+
+            if (ancestor == null) {
+                return true;
+            }
+
+            if (child.getParent() == ancestor) {
+                return true;
+            }
+
+            child = child.getParent();
+        }
+    }
+}
@@ -0,0 +1,20 @@
+import java.awt.*;
+
+class UnmodifiedParameter {
+
+  private static boolean hasAncestor(Container child, Container ancestor) {
+    if (child == null) {
+      return false;
+    }
+
+    if (ancestor == null) {
+      return true;
+    }
+
+    if (child.getParent() == ancestor) {
+      return true;
+    }
+
+    return <caret>hasAncestor(child.getParent(), ancestor);
+  }
+}
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2000-2016 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.siyeh.ig.fixes.performance;
+
+import com.siyeh.InspectionGadgetsBundle;
+import com.siyeh.ig.IGQuickFixesTestCase;
+import com.siyeh.ig.performance.TailRecursionInspection;
+
+/**
+ * @author Bas Leijdekkers
+ */
+public class RemoveTailRecursionFixTest extends IGQuickFixesTestCase {
+
+  @Override
+  protected void setUp() throws Exception {
+    super.setUp();
+    myFixture.enableInspections(new TailRecursionInspection());
+    myRelativePath = "performance/tail_recursion";
+    myDefaultHint = InspectionGadgetsBundle.message("tail.recursion.replace.quickfix");
+  }
+
+  public void testCallOnOtherInstance1() { doTest(); }
+  public void testCallOnOtherInstance2() { doTest(); }
+  public void testDependency1() { doTest(); }
+  public void testDependency2() { doTest(); }
+  public void testThisVariable() { doTest(); }
+  public void testUnmodifiedParameter() { doTest(); }
+}