IDEA-211473 Inline method: process some boolean methods with multiple exits specially

GitOrigin-RevId: 5e3bc7e8577a346059192cb7d68bef7eef818ffd
2026-09-27 10:03:11 +07:00 · 2019-04-23 13:20:13 +03:00
parent 8f4ed82473
commit 92507ef9b6
18 changed files with 426 additions and 41 deletions
@@ -24,6 +24,7 @@ import org.jetbrains.annotations.Nullable;

 import java.util.Arrays;
 import java.util.List;
+import java.util.Objects;

 import static com.intellij.util.ObjectUtils.tryCast;

@@ -59,7 +60,12 @@ public class ConvertToSingleReturnAction extends PsiElementBaseIntentionAction {
    PsiCodeBlock copy = (PsiCodeBlock)block.copy();
    indicator.checkCanceled();
    indicator.setFraction(0.3);
-    convertReturns(project, copy, returnType, marker, returns.size(), indicator);
+    PsiLocalVariable variable = convertReturns(project, copy, returnType, marker, returns.size(), indicator);
+    if (variable != null) {
+      PsiJavaToken end = Objects.requireNonNull(copy.getRBrace());
+      copy.addBefore(JavaPsiFacade.getElementFactory(project).createStatementFromText("return " + variable.getName() + ";", copy), end);
+    }
+
    return copy;
  }

@@ -122,8 +122,6 @@ class ExitContext {
        // Keep final when possible to respect code style setting "generate local variables as 'final'"
        requireNonNull(var.getModifierList()).setModifierProperty(PsiModifier.FINAL, false);
      }
-      PsiJavaToken end = requireNonNull(myBlock.getRBrace());
-      myBlock.addBefore(myFactory.createStatementFromText("return " + myReturnVariable + ";", myBlock), end);
      return var;
    }
    return null;
@@ -0,0 +1,177 @@
+// Copyright 2000-2019 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.refactoring.inline;
+
+import com.intellij.psi.*;
+import com.intellij.psi.util.PsiUtil;
+import com.intellij.util.ObjectUtils;
+import com.siyeh.ig.psiutils.BoolUtils;
+import com.siyeh.ig.psiutils.CommentTracker;
+import com.siyeh.ig.psiutils.ControlFlowUtils;
+import com.siyeh.ig.psiutils.ExpressionUtils;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+class BooleanReturnModel {
+  private final boolean myEarlyReturnValue;
+  private final int myEarlyReturnValueCount;
+  private final int myTerminalNonRemovableValueCount;
+  private final boolean myHasReturnInLoopOrSwitch;
+
+  BooleanReturnModel(boolean value, int count, int terminalNonRemovableValueCount, boolean hasReturnInLoopOrSwitch) {
+    myEarlyReturnValue = value;
+    myEarlyReturnValueCount = count;
+    myTerminalNonRemovableValueCount = terminalNonRemovableValueCount;
+    myHasReturnInLoopOrSwitch = hasReturnInLoopOrSwitch;
+  }
+
+  @Nullable
+  InlineTransformer getTransformer(PsiReference ref) {
+    if (!(ref instanceof PsiReferenceExpression)) return null;
+    PsiMethodCallExpression call = ObjectUtils.tryCast(((PsiReferenceExpression)ref).getParent(), PsiMethodCallExpression.class);
+    if (call == null) return null;
+    boolean wantedValue = true;
+    PsiElement parent;
+    PsiExpression expression = call;
+    while (true) {
+      parent = PsiUtil.skipParenthesizedExprUp(expression.getParent());
+      if (parent instanceof PsiPrefixExpression && JavaTokenType.EXCL.equals(((PsiPrefixExpression)parent).getOperationTokenType())) {
+        wantedValue = !wantedValue;
+        expression = (PsiPrefixExpression)parent;
+      }
+      else {
+        break;
+      }
+    }
+    if (parent instanceof PsiIfStatement) {
+      PsiIfStatement ifStatement = (PsiIfStatement)parent;
+      PsiStatement thenStatement = ControlFlowUtils.stripBraces(ifStatement.getThenBranch());
+      PsiStatement elseStatement = ControlFlowUtils.stripBraces(ifStatement.getElseBranch());
+      if (elseStatement == null && wantedValue == myEarlyReturnValue) {
+        boolean allowReplacement;
+        if (myEarlyReturnValueCount == 1 && myTerminalNonRemovableValueCount == 0) {
+          allowReplacement = isJumpOut(thenStatement);
+        }
+        else {
+          allowReplacement = isCopyableJumpOut(thenStatement);
+        }
+        if (allowReplacement) {
+          return getTransformer(thenStatement, null);
+        }
+      }
+      if (isJumpOut(thenStatement) && isJumpOut(elseStatement) && myTerminalNonRemovableValueCount == 0 && myEarlyReturnValueCount == 1) {
+        PsiStatement earlyStatement = myEarlyReturnValue == wantedValue ? thenStatement : elseStatement;
+        PsiStatement finalStatement = myEarlyReturnValue != wantedValue ? thenStatement : elseStatement;
+        return getTransformer(earlyStatement, finalStatement);
+      }
+    }
+    return null;
+  }
+
+  private boolean isJumpOut(PsiStatement statement) {
+    return statement instanceof PsiReturnStatement || statement instanceof PsiThrowStatement ||
+           (!myHasReturnInLoopOrSwitch && (statement instanceof PsiBreakStatement || statement instanceof PsiContinueStatement));
+  }
+
+  /**
+   * @param statement statement to check
+   * @return true if given statement could be copied to several return sites. We don't copy {@code throw} or {@code return} 
+   * with non-trivial return value as this causes code duplication.
+   */
+  private boolean isCopyableJumpOut(PsiStatement statement) {
+    if (!myHasReturnInLoopOrSwitch && (statement instanceof PsiBreakStatement || statement instanceof PsiContinueStatement)) {
+      return true;
+    }
+    if (statement instanceof PsiReturnStatement) {
+      PsiReturnStatement thenReturn = (PsiReturnStatement)statement;
+      return thenReturn.getReturnValue() == null ||
+             ExpressionUtils.isSafelyRecomputableExpression(thenReturn.getReturnValue());
+    }
+    return false;
+  }
+
+  @NotNull
+  private InlineTransformer getTransformer(PsiStatement earlyStatement, PsiStatement finalStatement) {
+    return (methodCopy, callSite, returnType) -> {
+      PsiCodeBlock block = Objects.requireNonNull(methodCopy.getBody());
+      PsiReturnStatement[] returns = PsiUtil.findReturnStatements(methodCopy);
+      for (PsiReturnStatement returnStatement : returns) {
+        PsiExpression returnValue = Objects.requireNonNull(returnStatement.getReturnValue()); // null-checked in "from" method
+        PsiLiteralExpression literal = ExpressionUtils.getLiteral(returnValue);
+        Boolean value = literal == null ? null : ObjectUtils.tryCast(literal.getValue(), Boolean.class);
+        if (value == null) {
+          CommentTracker tracker = new CommentTracker();
+          String condition = myEarlyReturnValue ? tracker.text(returnValue) : BoolUtils.getNegatedExpressionText(returnValue, tracker);
+          tracker.replaceAndRestoreComments(returnStatement, "if(" + condition + ") {" + earlyStatement.getText() + "}");
+        }
+        else if (value == myEarlyReturnValue) {
+          new CommentTracker().replaceAndRestoreComments(returnStatement, earlyStatement);
+        }
+        else {
+          new CommentTracker().delete(returnStatement);
+        }
+      }
+      if (finalStatement != null) {
+        block.addBefore(finalStatement, block.getRBrace());
+      }
+      return null;
+    };
+  }
+
+  @Nullable
+  static BooleanReturnModel from(@NotNull PsiCodeBlock body, @NotNull PsiReturnStatement[] returns) {
+    List<PsiExpression> terminal = new ArrayList<>();
+    boolean earlyValue = false;
+    int earlyCount = 0;
+    boolean hasReturnInLoopOrSwitch = false;
+    for (PsiReturnStatement returnStatement : returns) {
+      if (!hasReturnInLoopOrSwitch) {
+        hasReturnInLoopOrSwitch = isInLoopOrSwitch(body, returnStatement);
+      }
+      PsiExpression returnValue = returnStatement.getReturnValue();
+      if (returnValue == null || !PsiType.BOOLEAN.equals(returnValue.getType())) return null;
+      if (ControlFlowUtils.blockCompletesWithStatement(body, returnStatement)) {
+        terminal.add(returnValue);
+      }
+      else {
+        PsiLiteralExpression literal = ExpressionUtils.getLiteral(returnValue);
+        if (literal == null) return null;
+        Boolean literalValue = ObjectUtils.tryCast(literal.getValue(), Boolean.class);
+        if (literalValue == null) return null;
+        if (earlyCount == 0) {
+          earlyValue = literalValue;
+        }
+        else if (earlyValue != literalValue) {
+          return null;
+        }
+        earlyCount++;
+      }
+    }
+    if (earlyCount == 0) return null;
+    int terminalCount = 0;
+    for (PsiExpression value : terminal) {
+      PsiLiteralExpression literal = ExpressionUtils.getLiteral(value);
+      if (literal == null || !(literal.getValue() instanceof Boolean)) {
+        terminalCount++;
+      }
+      else if (literal.getValue().equals(earlyValue)) {
+        earlyCount++;
+      }
+    }
+    return new BooleanReturnModel(earlyValue, earlyCount, terminalCount, hasReturnInLoopOrSwitch);
+  }
+
+  private static boolean isInLoopOrSwitch(@NotNull PsiCodeBlock body, @NotNull PsiReturnStatement returnStatement) {
+    PsiElement parent = returnStatement.getParent();
+    while (parent != body) {
+      if (parent instanceof PsiLoopStatement || parent instanceof PsiSwitchStatement) {
+        return true;
+      }
+      parent = parent.getParent();
+    }
+    return false;
+  }
+}
@@ -611,7 +611,6 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor {
  }

  public void inlineMethodCall(PsiReferenceExpression ref) throws IncorrectOperationException {
-    InlineUtil.TailCallType tailCall = InlineUtil.getTailCallType(ref);
    ChangeContextUtil.encodeContextInfo(myMethod, false);
    myMethodCopy = (PsiMethod)myMethod.copy();
    ChangeContextUtil.clearContextInfo(myMethod);
@@ -667,11 +666,6 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor {
    if (firstBodyElement != null && firstBodyElement != blockData.block.getRBrace()) {
      int last = statements.length - 1;

-      if (last > 0 && statements[last] instanceof PsiReturnStatement &&
-          tailCall != InlineUtil.TailCallType.Return) {
-        last--;
-      }
-
      final PsiElement rBraceOrReturnStatement =
        last >= 0 ? PsiTreeUtil.skipWhitespacesAndCommentsForward(statements[last]) : blockData.block.getLastBodyElement();
      LOG.assertTrue(rBraceOrReturnStatement != null);
@@ -703,18 +697,6 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor {
          }
        }
      }
-
-      if (statements.length > 0) {
-        final PsiStatement lastStatement = statements[statements.length - 1];
-        if (lastStatement instanceof PsiReturnStatement && tailCall != InlineUtil.TailCallType.Return) {
-          final PsiExpression returnValue = ((PsiReturnStatement)lastStatement).getReturnValue();
-          if (returnValue != null && PsiUtil.isStatement(returnValue)) {
-            PsiExpressionStatement exprStatement = (PsiExpressionStatement)myFactory.createStatementFromText("a;", null);
-            exprStatement.getExpression().replace(returnValue);
-            anchorParent.addBefore(exprStatement, anchor);
-          }
-        }
-      }
    }


@@ -733,25 +715,20 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor {
    }
    ChangeContextUtil.decodeContextInfo(anchorParent, thisClass, thisAccessExpr);

-    PsiElement callParent = methodCall.getParent();
    PsiReferenceExpression resultUsage = null;
-    if (callParent instanceof PsiLambdaExpression) {
-      methodCall.delete();
-    }
-    else if (callParent instanceof PsiExpressionStatement || tailCall == InlineUtil.TailCallType.Return) {
-      CommentTracker tracker = new CommentTracker();
-      tracker.delete(callParent);
-      if (firstAdded != null) {
-        tracker.insertCommentsBefore(firstAdded);
-      }
+    if (blockData.resultVar != null) {
+      PsiExpression expr = myFactory.createExpressionFromText(Objects.requireNonNull(blockData.resultVar.getName()), null);
+      resultUsage = (PsiReferenceExpression)new CommentTracker().replaceAndRestoreComments(methodCall, expr);
    }
    else {
-      if (blockData.resultVar != null) {
-        PsiExpression expr = myFactory.createExpressionFromText(Objects.requireNonNull(blockData.resultVar.getName()), null);
-        resultUsage = (PsiReferenceExpression)new CommentTracker().replaceAndRestoreComments(methodCall, expr);
-      }
-      else {
-        //??
+      // If return var is not specified, we trust that InlineTransformer fully processed the original anchor statement,
+      // and we can delete it.
+      CommentTracker tracker = new CommentTracker();
+      if (firstAdded != null) {
+        tracker.delete(anchor);
+        tracker.insertCommentsBefore(firstAdded);
+      } else {
+        tracker.deleteAndRestoreComments(anchor);
      }
    }

@@ -44,8 +44,8 @@ public interface InlineTransformer {

    @Override
    public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) {
-      if (returnType == null || PsiType.VOID.equals(returnType)) return null;
-      if (callSite.getParent() instanceof PsiMethodCallExpression && ExpressionUtils.isVoidContext((PsiExpression)callSite.getParent())) {
+      if (returnType == null || PsiType.VOID.equals(returnType) ||
+          callSite.getParent() instanceof PsiMethodCallExpression && ExpressionUtils.isVoidContext((PsiExpression)callSite.getParent())) {
        InlineUtil.extractReturnValues(methodCopy, false);
        return null;
      }
@@ -101,7 +101,7 @@ public interface InlineTransformer {
  @NotNull
  static Function<PsiReference, InlineTransformer> getSuitableTransformer(PsiMethod method) {
    PsiReturnStatement[] returns = PsiUtil.findReturnStatements(method);
-    PsiCodeBlock body = method.getBody();
+    PsiCodeBlock body = Objects.requireNonNull(method.getBody());
    if (!InlineMethodProcessor.checkBadReturns(returns, body)) {
      return ref -> {
        InlineUtil.TailCallType type = InlineUtil.getTailCallType(ref);
@@ -116,13 +116,23 @@ public interface InlineTransformer {
      // Introducing a label is ugly, so let's move to fallback transformer 
      return PsiTreeUtil.getParentOfType(statement, PsiLoopStatement.class, true, PsiMethod.class) == null;
    });
+    BooleanReturnModel model = BooleanReturnModel.from(body, returns);
    return ref -> {
      InlineUtil.TailCallType type = InlineUtil.getTailCallType(ref);
      if (type == InlineUtil.TailCallType.Continue && !canUseContinue) {
        type = InlineUtil.TailCallType.None;
      }
      InlineTransformer fromTailCall = type.getTransformer();
-      return fromTailCall != null ? fromTailCall : new ConvertToSingleReturnTransformer();
+      if (fromTailCall != null) {
+        return fromTailCall;
+      }
+      if (model != null) {
+        InlineTransformer fromBooleanModel = model.getTransformer(ref);
+        if (fromBooleanModel != null) {
+          return fromBooleanModel;
+        }
+      }
+      return new ConvertToSingleReturnTransformer();
    };
  }
 }
@@ -0,0 +1,20 @@
+
+class Test {
+
+  private boolean test(String s) {
+    if(s != null) {
+      s = s.trim();
+      if (s.isEmpty()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  String use(String[] list) {
+    for(String str : list) {
+      if (<caret>test(str)) continue;
+      System.out.println("Ok string: "+str);
+    }
+  }
+}
@@ -0,0 +1,16 @@
+
+class Test {
+
+    String use(String[] list) {
+    for(String str : list) {
+        String s = str;
+        if(s != null) {
+          s = s.trim();
+          if (s.isEmpty()) {
+              continue;
+          }
+        }
+        System.out.println("Ok string: "+str);
+    }
+  }
+}
@@ -0,0 +1,24 @@
+
+class Test {
+
+  private boolean test(String s) {
+    if (s == null) return false;
+    s = s.trim();
+    if (s.isEmpty()) return false;
+    int i;
+    try {
+      i = Integer.parseInt(s);
+    }
+    catch (NumberFormatException ex) {
+      return false;
+    }
+    return i > 0;
+  }
+
+  String use(String[] list) {
+    for(String str : list) {
+      if (!<caret>test(str)) break;
+      System.out.println("Ok string: "+str);
+    }
+  }
+}
@@ -0,0 +1,23 @@
+
+class Test {
+
+    String use(String[] list) {
+    for(String str : list) {
+        String s = str;
+        if (s == null) break;
+        s = s.trim();
+        if (s.isEmpty()) break;
+        int i;
+        try {
+          i = Integer.parseInt(s);
+        }
+        catch (NumberFormatException ex) {
+            break;
+        }
+        if (i <= 0) {
+            break;
+        }
+        System.out.println("Ok string: "+str);
+    }
+  }
+}
@@ -0,0 +1,17 @@
+class Test {
+
+  private boolean test(String s) {
+    if(s != null) {
+      s = s.trim();
+      if (s.isEmpty()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  String use(String s) {
+    if(<caret>test(s+s)) throw new IllegalArgumentException();
+    else return "nice";
+  }
+}
@@ -0,0 +1,13 @@
+class Test {
+
+    String use(String s) {
+        String s1 = s+s;
+        if(s1 != null) {
+          s1 = s1.trim();
+          if (s1.isEmpty()) {
+              throw new IllegalArgumentException();
+          }
+        }
+        return "nice";
+    }
+}
@@ -0,0 +1,17 @@
+class Test {
+
+  private boolean test() {
+    for(int i=0; i<10; i++) {
+      if (Math.random() > 0.5) return true;
+    }
+    return false;
+  }
+
+  String useTest() {
+    if(<caret>test()) {
+      return "foo".trim();
+    } else {
+      return "bar".trim();
+    }
+  }
+}
@@ -0,0 +1,9 @@
+class Test {
+
+    String useTest() {
+        for(int i=0; i<10; i++) {
+          if (Math.random() > 0.5) return "foo".trim();
+        }
+        return "bar".trim();
+    }
+}
@@ -0,0 +1,14 @@
+class Test {
+
+  private boolean test(String s) {
+    if (s == null) return false;
+    s = s.trim();
+    if (s.isEmpty()) return false;
+    return true;
+  }
+
+  void use(String s) {
+    if(!<caret>test(s+s)) return;
+    System.out.println("woohoo");
+  }
+}
@@ -0,0 +1,10 @@
+class Test {
+
+    void use(String s) {
+        String s1 = s+s;
+        if (s1 == null) return;
+        s1 = s1.trim();
+        if (s1.isEmpty()) return;
+        System.out.println("woohoo");
+  }
+}
@@ -0,0 +1,17 @@
+class Test {
+
+  private boolean test(String s) {
+    if(s != null) {
+      s = s.trim();
+      if (s.isEmpty()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  void use(String s) {
+    if(<caret>test(s+s)) throw new IllegalArgumentException();
+    System.out.println("woohoo");
+  }
+}
@@ -0,0 +1,13 @@
+class Test {
+
+    void use(String s) {
+        String s1 = s+s;
+        if(s1 != null) {
+          s1 = s1.trim();
+          if (s1.isEmpty()) {
+              throw new IllegalArgumentException();
+          }
+        }
+        System.out.println("woohoo");
+  }
+}
@@ -490,6 +490,30 @@ public class InlineMethodTest extends LightRefactoringTestCase {
  public void testSpecializeClassGetName() {
    doTest();
  }
+  
+  public void testBooleanModelSimple() {
+    doTestAssertBadReturn();
+  }
+  
+  public void testBooleanModelMultiReturns() {
+    doTestAssertBadReturn();
+  }
+  
+  public void testBooleanModelIfElse() {
+    doTestAssertBadReturn();
+  }
+
+  public void testBooleanModelIfElse2() {
+    doTestAssertBadReturn();
+  }
+
+  public void testBooleanModelContinue() {
+    doTestAssertBadReturn();
+  }
+
+  public void testBooleanModelFinalCondition() {
+    doTestAssertBadReturn();
+  }

  @Override
  protected Sdk getProjectJDK() {