OptionalToIfInspection: cr fixes (IDEA-CR-51167)

1. add new line before and after code block braces when wrapping user code
2. add example in inspection description
3. remove OptionalToIfInspectionTest#runSingle
4. remove duplicates from operation names list
5. remove unnecessary whitespaces from strings with converted operations
6. support final variables and local variables with implicit type
7. honor operation precedence when merging two if checks during simplification
8. StringUtil#join instead of Collectors#joining
9. remove code after throw during simplification

GitOrigin-RevId: cd3273072ad3bdba21aa5b28a6fd13dc325e93ad

java/java-impl/src/com/intellij/codeInspection/optionalToIf/Instruction.java

@@ -1,6 +1,7 @@
 // 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.codeInspection.optionalToIf;
 
+import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.psi.*;
 import com.siyeh.ig.psiutils.ControlFlowUtils;
 import org.jetbrains.annotations.Contract;
@@ -47,7 +48,7 @@ interface Instruction {
 
     @Override
     public String generate() {
-      return Arrays.stream(myBlock.getCodeBlock().getStatements()).map(s -> s.getText()).collect(Collectors.joining("\n"));
+      return StringUtil.join(myBlock.getCodeBlock().getStatements(), s -> s.getText(), "\n");
     }
   }
 
@@ -79,7 +80,7 @@ interface Instruction {
 
     @Override
     public String generate() {
-      return myLhs.getName() + " = " + myRhs.getText() + ";\n";
+      return myLhs.getName() + "=" + myRhs.getText() + ";\n";
     }
 
     @Nullable
@@ -106,7 +107,10 @@ interface Instruction {
 
     @Override
     public String generate() {
-      return myLhs.getType().getCanonicalText() + " " + myLhs.getName() + " = " + myRhs.getText() + ";\n";
+      if (myLhs.getInitializer() == myRhs) return myLhs.getText();
+      PsiVariable copy = (PsiVariable)myLhs.copy();
+      copy.setInitializer(myRhs);
+      return copy.getText();
     }
 
     @Nullable
@@ -140,14 +144,14 @@ interface Instruction {
     @Override
     public String generate() {
       if (myInstructions.size() == 1 && !hasElseBranch()) {
-        return "if(" + myCondition.getText() + ") " + myInstructions.get(0).generate();
+        return "if(" + myCondition.getText() + ")" + myInstructions.get(0).generate();
       }
-      String thenBranch = "if(" + myCondition.getText() + ") {\n" +
-                          myInstructions.stream().map(i -> i.generate()).collect(Collectors.joining()) +
+      String thenBranch = "if(" + myCondition.getText() + "){\n" +
+                          StringUtil.join(myInstructions, i -> i.generate(), "") +
                           "}\n";
       if (myElseInstructions == null) return thenBranch;
       return thenBranch +
-             "else {\n" + myElseInstructions.stream().map(i -> i.generate()).collect(Collectors.joining()) + "\n}";
+             "else{\n" + StringUtil.join(myElseInstructions, i -> i.generate(), "") + "\n}";
     }
 
     @Nullable

java/java-impl/src/com/intellij/codeInspection/optionalToIf/IntermediateOperation.java

@@ -155,9 +155,9 @@ abstract class IntermediateOperation implements Operation {
                            @NotNull String code,
                            @NotNull OptionalToIfContext context) {
       String orResult = myRecords.get(myRecords.size() - 1).myOutVar.getName();
-      String orCode = OptionalToIfInspection.wrapCode(context, myRecords, outVar.getName() + " = " + orResult + ";");
+      String orCode = OptionalToIfInspection.wrapCode(context, myRecords, outVar.getName() + "=" + orResult + ";");
       if (orCode == null) return null;
-      return "if (" + outVar.getName() + " == null) {\n" +
+      return "if(" + outVar.getName() + "==null){\n" +
              orCode +
              "\n}" +
              context.generateNotNullCondition(outVar.getName(), code);

java/java-impl/src/com/intellij/codeInspection/optionalToIf/OptionalToIfContext.java

@@ -7,11 +7,14 @@ import com.intellij.psi.*;
 import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.psi.util.PsiUtil;
 import com.siyeh.ig.psiutils.BoolUtils;
-import com.siyeh.ig.psiutils.ComparisonUtils;
+import com.siyeh.ig.psiutils.CommentTracker;
 import com.siyeh.ig.psiutils.ExpressionUtils;
+import com.siyeh.ig.psiutils.VariableAccessUtils;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import java.util.Arrays;
+
 import static com.intellij.util.ObjectUtils.tryCast;
 
 
@@ -43,6 +46,10 @@ class OptionalToIfContext extends ChainContext {
 
   void setInitializer(@NotNull String initializer) {
     LOG.assertTrue(myInitializer == null);
+    if (myModel.needsAdditionalVariable()) {
+      String additionalVarName = registerVarName(Arrays.asList("result", "nonFinalResult", "nonFinal"));
+      myModel.setVarName(additionalVarName);
+    }
     myInitializer = initializer;
   }
 
@@ -53,41 +60,26 @@ class OptionalToIfContext extends ChainContext {
   @NotNull
   String generateNotNullCondition(@NotNull String arg, @NotNull String code) {
     if (myElseBranch == null) {
-      return "if (" + arg + " != null) {\n" +
+      return "if(" + arg + "!=null){\n" +
              code +
              "\n}";
     }
-    return "if(" + arg + " == null) " + myElseBranch +
+    return "if(" + arg + "==null)" + myElseBranch +
            code;
   }
 
   @Nullable
   String generateCondition(@NotNull PsiExpression conditional, @NotNull String code) {
     if (myElseBranch == null) {
-      return "if (" + conditional.getText() + ") {\n" +
+      return "if(" + conditional.getText() + "){\n" +
              code +
              "\n}";
     }
-    String negated = getNegatedConditional(conditional);
-    if (negated == null) return null;
-    return "if (" + negated + ")" + myElseBranch +
+    String negated = BoolUtils.getNegatedExpressionText(conditional, new CommentTracker());
+    return "if(" + negated + ")" + myElseBranch +
            code;
   }
 
-  @Nullable
-  private static String getNegatedConditional(@NotNull PsiExpression conditional) {
-    PsiExpression negated = BoolUtils.getNegated(conditional);
-    if (negated != null) return negated.getText();
-    PsiBinaryExpression condition = tryCast(conditional, PsiBinaryExpression.class);
-    if (condition == null || !ComparisonUtils.isComparison(condition)) return null;
-    PsiExpression lhs = condition.getLOperand();
-    PsiExpression rhs = condition.getROperand();
-    if (rhs == null) return null;
-    String negatedToken = ComparisonUtils.getNegatedComparison(condition.getOperationTokenType());
-    if (negatedToken == null) return null;
-    return lhs.getText() + negatedToken + rhs.getText();
-  }
-
   @Nullable
   static OptionalToIfContext create(@NotNull PsiExpression chainExpression) {
     PsiStatement chainStatement = PsiTreeUtil.getParentOfType(chainExpression, PsiStatement.class);
@@ -108,6 +100,13 @@ class OptionalToIfContext extends ChainContext {
     @NotNull
     abstract String addInitializer(@NotNull String initializer, @NotNull String code);
 
+    boolean needsAdditionalVariable() {
+      return false;
+    }
+
+    public void setVarName(@NotNull String name) {
+    }
+
     @Nullable
     static ChainExpressionModel create(@NotNull PsiStatement chainStatement, @NotNull PsiExpression chainExpression) {
       PsiReturnStatement returnStatement = tryCast(chainStatement, PsiReturnStatement.class);
@@ -200,7 +199,7 @@ class OptionalToIfContext extends ChainContext {
     @NotNull
     @Override
     String createResult(@NotNull String expression) {
-      return myName + " = " + expression + ";";
+      return myName + "=" + expression + ";";
     }
 
     @NotNull
@@ -229,40 +228,67 @@ class OptionalToIfContext extends ChainContext {
 
   private static class ChainDeclaration extends ChainExpressionModel {
 
-    private final String myName;
-    private final PsiVariable myVariable;
+    private final PsiLocalVariable myVariable;
+    private String myName;
 
-    ChainDeclaration(@NotNull String name, @NotNull PsiVariable variable) {
+    ChainDeclaration(@Nullable String name, @NotNull PsiLocalVariable variable) {
       myName = name;
       myVariable = variable;
     }
 
+    @Override
+    boolean needsAdditionalVariable() {
+      return !myVariable.getName().equals(myName);
+    }
+
+    @Override
+    public void setVarName(@NotNull String name) {
+      myName = name;
+    }
+
     @NotNull
     @Override
     String createResult(@NotNull String expression) {
-      return myName + " = " + expression + ";";
+      LOG.assertTrue(myName != null);
+      return myName + "=" + expression + ";";
     }
 
     @NotNull
     @Override
     String createInitializer(@NotNull String expression) {
-      return String.format("%s %s = %s;", myVariable.getType().getCanonicalText(), myName, expression);
+      if (myName == null || !needsAdditionalVariable()) {
+        return declareVariable(expression);
+      }
+      String typeText = myVariable.getType().getCanonicalText();
+      return typeText + " " + myName + "=" + expression + ";";
     }
 
     @NotNull
     @Override
     String addInitializer(@NotNull String initializer, @NotNull String code) {
-      return initializer + code;
+      return needsAdditionalVariable() ? initializer + code + declareVariable(myName) : initializer + code;
+    }
+
+    private String declareVariable(@NotNull String expression) {
+      PsiLocalVariable copy = (PsiLocalVariable)myVariable.copy();
+      PsiElementFactory factory = PsiElementFactory.getInstance(myVariable.getProject());
+      copy.setInitializer(factory.createExpressionFromText(expression, myVariable));
+      if (!needsAdditionalVariable()) {
+        PsiModifierList modifierList = copy.getModifierList();
+        if (modifierList != null) modifierList.setModifierProperty(PsiModifier.FINAL, false);
+      }
+      return copy.getText();
     }
 
     @Nullable
     static ChainDeclaration create(@NotNull PsiDeclarationStatement declaration, @NotNull PsiExpression chainExpression) {
       PsiElement[] elements = declaration.getDeclaredElements();
       if (elements.length != 1) return null;
-      PsiVariable variable = tryCast(elements[0], PsiVariable.class);
+      PsiLocalVariable variable = tryCast(elements[0], PsiLocalVariable.class);
       if (variable == null || PsiUtil.skipParenthesizedExprDown(variable.getInitializer()) != chainExpression) return null;
-      String name = variable.getName();
-      return name == null ? null : new ChainDeclaration(name, variable);
+      boolean needsAdditionalVariable = !VariableAccessUtils.canUseAsNonFinal(variable);
+      String name = needsAdditionalVariable ? null : variable.getName();
+      return new ChainDeclaration(name, variable);
     }
   }
 }

java/java-impl/src/com/intellij/codeInspection/optionalToIf/OptionalToIfInspection.java

@@ -29,7 +29,7 @@ import static com.intellij.util.ObjectUtils.tryCast;
 public class OptionalToIfInspection extends AbstractBaseJavaLocalInspectionTool {
 
   private static final Set<String> SUPPORTED_TERMINALS = ContainerUtil.set(
-    "get", "orElse", "ifPresent", "orElse", "orElseGet", "ifPresentOrElse", "ifPresent", "isPresent", "isEmpty", "stream", "orElseThrow");
+    "get", "orElse", "ifPresent", "orElseGet", "ifPresentOrElse", "isPresent", "isEmpty", "stream", "orElseThrow");
 
   @NotNull
   @Override
@@ -109,7 +109,7 @@ public class OptionalToIfInspection extends AbstractBaseJavaLocalInspectionTool
         context.addBeforeStep(outVar.getDeclaration("null"));
         context.setElseBranch(null);
         List<OperationRecord> rest = records.subList(0, i);
-        String beforeCode = wrapCode(context, rest, outVar.getName() + " = " + inVar.getName() + ";");
+        String beforeCode = wrapCode(context, rest, outVar.getName() + "=" + inVar.getName() + ";");
         if (beforeCode == null) return null;
         return beforeCode + code;
       }
@@ -144,7 +144,7 @@ public class OptionalToIfInspection extends AbstractBaseJavaLocalInspectionTool
   private static PsiStatement @NotNull [] addStatements(@NotNull PsiElementFactory factory,
                                                         @NotNull PsiStatement chainStatement,
                                                         @NotNull String code) {
-    PsiStatement[] statements = ControlFlowUtils.unwrapBlock(factory.createStatementFromText("{" + code + "}", chainStatement));
+    PsiStatement[] statements = ControlFlowUtils.unwrapBlock(factory.createStatementFromText("{\n" + code + "\n}", chainStatement));
     PsiElement parent = chainStatement.getParent();
     return ContainerUtil.map(statements, s -> (PsiStatement)parent.addBefore(s, chainStatement), PsiStatement.EMPTY_ARRAY);
   }

java/java-impl/src/com/intellij/codeInspection/optionalToIf/Simplifier.java

@@ -9,6 +9,7 @@ import com.intellij.psi.PsiReference;
 import com.intellij.psi.PsiVariable;
 import com.intellij.util.containers.ContainerUtil;
 import com.siyeh.ig.psiutils.EquivalenceChecker;
+import com.siyeh.ig.psiutils.ParenthesesUtils;
 import one.util.streamex.StreamEx;
 import org.jetbrains.annotations.Contract;
 import org.jetbrains.annotations.NotNull;
@@ -23,7 +24,7 @@ import static com.intellij.util.ObjectUtils.tryCast;
 interface Simplifier {
 
   Simplifier[] SIMPLIFIERS = {
-    new RemoveChecks(), new MergeChecks(), new RemoveAfterReturn(), new MergeImmediateReturn(), new MergeImmediateAssignment()
+    new RemoveChecks(), new MergeChecks(), new RemoveAfterReturnOrThrow(), new MergeImmediateReturn(), new MergeImmediateAssignment()
   };
 
   List<Instruction> run(@NotNull List<Instruction> instructions);
@@ -99,7 +100,9 @@ interface Simplifier {
       PsiExpression cond1 = c1.myCondition;
       PsiExpression cond2 = c2.myCondition;
       PsiElementFactory factory = PsiElementFactory.getInstance(cond1.getProject());
-      return factory.createExpressionFromText(cond1.getText() + operator + cond2.getText(), cond1);
+      return factory.createExpressionFromText(ParenthesesUtils.getText(cond1, ParenthesesUtils.OR_PRECEDENCE) +
+                                              operator +
+                                              ParenthesesUtils.getText(cond2, ParenthesesUtils.OR_PRECEDENCE), cond1);
     }
 
     @Nullable
@@ -122,7 +125,22 @@ interface Simplifier {
     }
   }
 
-  class RemoveAfterReturn implements Simplifier {
+  /**
+   * Removes all code that appears after throw or return instruction.
+   * This might happen after applying other simplifications, in particular the ones that remove redundant checks.
+   * E.g. for method
+   * String test(String in) {
+   *   if (in == null) return "foo";
+   *   return Optional.ofNullable(in).orElse("bar");
+   * }
+   * We would have two instructions:
+   * - Check(in != null) with Return(in) inside
+   * - Return "bar"
+   *
+   * After simplification of Check that is always true we
+   * end up with two returns in the row, so the second one must be removed.
+   */
+  class RemoveAfterReturnOrThrow implements Simplifier {
 
     @Override
     public List<Instruction> run(@NotNull List<Instruction> instructions) {
@@ -131,7 +149,7 @@ interface Simplifier {
         Check check = tryCast(instruction, Check.class);
         if (check != null && !check.hasElseBranch()) check.myInstructions = run(check.myInstructions);
         simplified.add(instruction);
-        if (instruction instanceof Return) return simplified;
+        if (instruction instanceof Return || instruction instanceof Throw) return simplified;
       }
       return simplified;
     }

java/java-impl/src/com/intellij/codeInspection/optionalToIf/SourceOperation.java

@@ -68,11 +68,11 @@ abstract class SourceOperation implements Operation {
                            @NotNull String code,
                            @NotNull OptionalToIfContext context) {
       if (SourceOperation.getSourceName(myArg) != null) {
-        return "if(" + outVar.getName() + " == null) throw new java.lang.NullPointerException();" +
+        return "if(" + outVar.getName() + "==null)throw new java.lang.NullPointerException();" +
                code;
       }
       return outVar.getDeclaration(myArg.getText()) +
-             "if(" + outVar.getName() + " == null) throw new java.lang.NullPointerException();" +
+             "if(" + outVar.getName() + "==null)throw new java.lang.NullPointerException();" +
              code;
     }
 

java/java-impl/src/com/intellij/codeInspection/optionalToIf/TerminalOperation.java

@@ -107,7 +107,7 @@ abstract class TerminalOperation implements Operation {
                            @NotNull String code,
                            @NotNull OptionalToIfContext context) {
       myFn.transform(context, inVar.getName());
-      return "{\n" + myFn.getStatementText() + "}\n";
+      return "{\n" + myFn.getStatementText() + "\n}\n";
     }
   }
 
@@ -143,13 +143,13 @@ abstract class TerminalOperation implements Operation {
       myElseFn.transform(context);
       myIfPresentFn.transform(context, outVar.getName());
       context.addBeforeStep(outVar.getDeclaration("null"));
-      context.addAfterStep("if (" + outVar.getName() + " == null) {\n" +
-                           "{" + myElseFn.getStatementText() + "}" +
+      context.addAfterStep("if(" + outVar.getName() + "==null){\n" +
+                           "{\n" + myElseFn.getStatementText() + "\n}" +
                            "}" +
-                           "else {\n" +
-                           "{" + myIfPresentFn.getStatementText() + "}" +
+                           "else{\n" +
+                           "{\n" + myIfPresentFn.getStatementText() + "\n}" +
                            "}\n");
-      return outVar.getName() + " = " + inVar.getName() + ";";
+      return outVar.getName() + "=" + inVar.getName() + ";";
     }
   }
 
@@ -203,11 +203,11 @@ abstract class TerminalOperation implements Operation {
         return context.createResult(inVar.getName());
       }
       context.addBeforeStep(outVar.getDeclaration("null"));
-      context.addAfterStep("if (" + outVar.getName() + " == null) {\n" +
-                           outVar.getName() + " = " + myFn.getStatementText() +
-                           "}" +
+      context.addAfterStep("if(" + outVar.getName() + "==null){\n" +
+                           outVar.getName() + "=" + myFn.getStatementText() +
+                           "\n}" +
                            context.createResult(outVar.getName()));
-      return outVar.getName() + " = " + inVar.getName() + ";";
+      return outVar.getName() + "=" + inVar.getName() + ";";
     }
   }
 

java/java-impl/src/inspectionDescriptions/OptionalToIf.html

@@ -1,7 +1,23 @@
 <html>
 <body>
-Replaces optional chain with sequence of if statements.
+Reports <code>Optional</code> call chains which could be replaced with a sequence of 'if' statements.
 <!-- tooltip end -->
+<p>Example:
+<pre><code>
+  return Optional.ofNullable(name)
+    .map(this::extractInitials)
+    .map(initials -> initials.toUpperCase(Locale.ENGLISH))
+    .orElseGet(this::getDefault);
+</code></pre>
+<p>can be replaced with</p>
+<pre><code>
+  if (name != null) {
+    String initials = extractInitials(name);
+    if (initials != null) return initials.toUpperCase(Locale.ENGLISH);
+  }
+  return getDefault();
+</code></pre>
+<p>This inspection only reports if the configured language level is 8 or higher.</p>
 <p><small>New in 2019.3</small></p>
 </body>
 </html>

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/afterFilter.java

@@ -23,6 +23,11 @@ class Test {
       return "foo";
   }
 
+  String twoFiltersInARowPrecedencePreserved(boolean a, boolean b, String in) {
+      if (in != null && (a || b)) return in;
+      return "foo";
+  }
+
   private String getIfTrue(String str, boolean b) {
     return b ? str : null;
   }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/afterGet.java

@@ -9,4 +9,10 @@ class Test {
       return in;
   }
 
+  void ofNullableGetFinalVar(String in) {
+      if (in == null) throw new NoSuchElementException("No value present");
+      final String out = in;
+      Runnable r = () -> java.lang.System.out.println(out);
+  }
+
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/afterIfPresent.java

@@ -10,8 +10,10 @@ class Test {
 
   void lambdaIsNotSimplified(String in, String p1, String p2) {
     if (in == null || p1 == null) throw new IllegalArgumentException();
+      /* comment1 */
+      // comment2
       String tmp = "foo";
-      tmp = "bar";
+      tmp = "bar"; // comment2
   }
 
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/afterIsPresent.java

@@ -11,6 +11,19 @@ class Test {
       return false;
   }
 
+  boolean isPresentFinalVariable(String in) {
+      String result = false;
+      if (in != null) result = true;
+      @Deprecated final String isPresent = result;
+      Runnable r = () -> System.out.println(isPresent);
+  }
+
+  boolean isPresentCanBeNonFinalVariable(String in) {
+      var isPresent = false;
+      if (in != null) isPresent = true;
+      System.out.println(isPresent);
+  }
+
   boolean isEmpty(String in) {
       if (in == null) throw new NullPointerException();
       String s = in.substring(3);

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/beforeFilter.java

@@ -20,6 +20,10 @@ class Test {
     return Optional.ofNullable(in).filter(s -> s.length() > 42).filter(s -> getIfTrue(s, b) != null).orElse("foo");
   }
 
+  String twoFiltersInARowPrecedencePreserved(boolean a, boolean b, String in) {
+    return Optional.ofNullable(in).filter(s -> a || b).orElse("foo");
+  }
+
   private String getIfTrue(String str, boolean b) {
     return b ? str : null;
   }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/beforeGet.java

@@ -8,4 +8,9 @@ class Test {
     return Optional.ofNullable<caret>(in).filter(s -> s.length() > 2).get();
   }
 
+  void ofNullableGetFinalVar(String in) {
+    final String out = Optional<caret>.ofNullable(in).get();
+    Runnable r = () -> java.lang.System.out.println(out);
+  }
+
 }

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/beforeIfPresent.java

@@ -10,9 +10,9 @@ class Test {
 
   void lambdaIsNotSimplified(String in, String p1, String p2) {
     if (in == null || p1 == null) throw new IllegalArgumentException();
-    Optional.ofNullable(in).ifPresent(s -> {
+    Optional./* comment1 */ofNullable(in).ifPresent(s -> {
       String tmp = "foo";
-      tmp = "bar";
+      tmp = "bar"; // comment2
     });
   }
 

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf/beforeIsPresent.java

@@ -8,6 +8,16 @@ class Test {
     return Optional.of<caret>(in).map(in -> in.substring(3)).filter(s -> s.startsWith("1")).isPresent();
   }
 
+  boolean isPresentFinalVariable(String in) {
+    @Deprecated final String isPresent = Optional.<caret>ofNullable(in).isPresent();
+    Runnable r = () -> System.out.println(isPresent);
+  }
+
+  boolean isPresentCanBeNonFinalVariable(String in) {
+    final var isPresent = Optional.<caret>ofNullable(in).isPresent();
+    System.out.println(isPresent);
+  }
+
   boolean isEmpty(String in) {
     return Optional.of(in).map(in -> in.substring(3)).filter(s -> s.startsWith("1")).isEmpty();
   }

java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/OptionalToIfInspectionTest.java

@@ -26,11 +26,6 @@ public class OptionalToIfInspectionTest extends LightQuickFixParameterizedTestCa
     return JAVA_12;
   }
 
-  @Override
-  public void runSingle() throws Throwable {
-    super.runSingle();
-  }
-
   @Override
   protected String getBasePath() {
     return "/codeInsight/daemonCodeAnalyzer/quickFix/optionalToIf";