HighlightUtil: suggest fix for cases when method has void return type and return statement value type is unknown (IDEA-216279)

GitOrigin-RevId: 2af7ce37a427c8ec6f5175fc820ec0eecbf719c9

java/java-analysis-api/src/com/intellij/codeInsight/intention/QuickFixFactory.java

@@ -60,6 +60,13 @@ public abstract class QuickFixFactory {
                                                                                     @NotNull PsiType toReturn,
                                                                                     boolean fixWholeHierarchy);
 
+  /**
+   * Provides fix for changing method return type when it's not clear with which type method return type should be replaced.<p>
+   * For example, in case when <code>void</code> method returns <code>null</code> this fix should be used.
+   */
+  @NotNull
+  public abstract LocalQuickFixAndIntentionActionOnPsiElement createMethodReturnUnknownTypeFix(@NotNull PsiMethod method);
+
   @NotNull
   public abstract LocalQuickFixAndIntentionActionOnPsiElement createAddMethodFix(@NotNull PsiMethod method, @NotNull PsiClass toClass);
 
@@ -174,6 +181,16 @@ public abstract class QuickFixFactory {
   @NotNull
   public abstract IntentionAction createConvertToStringLiteralAction();
 
+  /**
+   * Provides fix to remove return statement or return value in case when return statement is not last statement in block.
+   * This fix won't be available if return value may have side effects.
+   *
+   * @param codeBlock code block with return statement
+   * @param statement return statement to remove
+   */
+  @NotNull
+  public abstract IntentionAction createDeleteReturnFix(@NotNull PsiCodeBlock codeBlock, @NotNull PsiReturnStatement statement);
+
   @NotNull
   public abstract IntentionAction createDeleteCatchFix(@NotNull PsiParameter parameter);
 

java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightUtil.java

@@ -559,11 +559,7 @@ public class HighlightUtil extends HighlightUtilBase {
       if (returnValue != null) {
         PsiType valueType = RefactoringChangeUtil.getTypeByExpression(returnValue);
         if (isMethodVoid) {
-          description = JavaErrorMessages.message("return.from.void.method");
-          errorResult = HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(statement).descriptionAndTooltip(description).create();
-          if (valueType != null && method != null) {
-            QuickFixAction.registerQuickFixAction(errorResult, QUICK_FIX_FACTORY.createMethodReturnFix(method, valueType, true));
-          }
+          return generateReturnValueFromVoidMethodInfo(method, statement, valueType);
         }
         else {
           TextRange textRange = statement.getTextRange();
@@ -593,6 +589,36 @@ public class HighlightUtil extends HighlightUtilBase {
     return errorResult;
   }
 
+  private static boolean isValidConstructor(@NotNull PsiMethod method) {
+    PsiClass aClass = method.getContainingClass();
+    if (aClass == null) return false;
+    return method.getName().equals(aClass.getName());
+  }
+
+  private static HighlightInfo generateReturnValueFromVoidMethodInfo(@Nullable PsiMethod method,
+                                                                     @NotNull PsiReturnStatement returnStatement,
+                                                                     @Nullable PsiType returnValueType) {
+    String description = JavaErrorMessages.message("return.from.void.method");
+    HighlightInfo info =
+      HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(returnStatement).descriptionAndTooltip(description).create();
+
+    boolean canCreateFixes = method != null && returnValueType != null && (!method.isConstructor() || isValidConstructor(method));
+    if (!canCreateFixes) return info;
+
+    PsiCodeBlock codeBlock = method.getBody();
+    if (codeBlock == null) return info;
+    QuickFixAction.registerQuickFixAction(info, QUICK_FIX_FACTORY.createDeleteReturnFix(codeBlock, returnStatement));
+    if (method.isConstructor()) return info;
+    boolean isKnownReturnType = returnValueType.isValid() && !TypeConversionUtil.isNullType(returnValueType);
+    if (isKnownReturnType) {
+      QuickFixAction.registerQuickFixAction(info, QUICK_FIX_FACTORY.createMethodReturnFix(method, returnValueType, true));
+    }
+    else {
+      QuickFixAction.registerQuickFixAction(info, QUICK_FIX_FACTORY.createMethodReturnUnknownTypeFix(method));
+    }
+    return info;
+  }
+
   @NotNull
   public static String getUnhandledExceptionsDescriptor(@NotNull final Collection<? extends PsiClassType> unhandled) {
     return getUnhandledExceptionsDescriptor(unhandled, null);

java/java-analysis-impl/src/messages/QuickFixBundle.properties

@@ -146,6 +146,7 @@ fix.parameter.type.family=Fix Parameter Type
 fix.parameter.type.text=Make ''{0}'' take parameter of type ''{1}'' here
 fix.return.type.family=Fix return type
 fix.return.type.text=Make ''{0}'' return ''{1}''
+fix.return.type.change.type.text=Change return type for method ''{0}''
 fix.throws.list.family=Fix throws list
 fix.throws.list.add.exception=Add ''{0}'' to ''{1}'' throws list
 fix.throws.list.remove.exception=Remove ''{0}'' from ''{1}'' throws list
@@ -311,6 +312,9 @@ wrap.with.optional.parameter.text=Wrap {0, choice, 1#1st|2#2nd|3#3rd|4#{0,number
 wrap.with.optional.single.parameter.text=Wrap using 'java.util.Optional'
 
 move.file.to.source.root.text=Move file to a source root
+delete.return.fix.family=Delete return
+delete.return.fix.statement.text=Delete return statement
+delete.return.fix.value.text=Delete return value ''{0}''
 
 delete.element.fix.text=Delete element
 delete.reference.fix.text=Delete reference

java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/AddReturnFix.java

@@ -81,7 +81,7 @@ public class AddReturnFix implements IntentionAction {
     assert body != null;
     returnStatement = (PsiReturnStatement) body.addBefore(returnStatement, body.getRBrace());
 
-    MethodReturnTypeFix.selectReturnValueInEditor(returnStatement, editor);
+    MethodReturnTypeFix.selectInEditor(returnStatement.getReturnValue(), editor);
   }
 
   private String suggestReturnValue() {

java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/DeleteReturnFix.java

@@ -0,0 +1,69 @@
+// 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.codeInsight.daemon.impl.quickfix;
+
+import com.intellij.codeInsight.daemon.QuickFixBundle;
+import com.intellij.codeInsight.intention.IntentionAction;
+import com.intellij.codeInsight.intention.impl.BaseIntentionAction;
+import com.intellij.openapi.editor.Editor;
+import com.intellij.openapi.project.Project;
+import com.intellij.psi.*;
+import com.intellij.util.IncorrectOperationException;
+import com.siyeh.ig.psiutils.CommentTracker;
+import com.siyeh.ig.psiutils.ControlFlowUtils;
+import com.siyeh.ig.psiutils.SideEffectChecker;
+import org.jetbrains.annotations.Contract;
+import org.jetbrains.annotations.Nls;
+import org.jetbrains.annotations.NotNull;
+
+public class DeleteReturnFix implements IntentionAction {
+
+  private final PsiReturnStatement myReturnStatement;
+  private final boolean myIsLastStatement;
+
+  @Contract(pure = true)
+  public DeleteReturnFix(@NotNull PsiCodeBlock codeBlock, @NotNull PsiReturnStatement statement) {
+    myReturnStatement = statement;
+    myIsLastStatement = ControlFlowUtils.blockCompletesWithStatement(codeBlock, statement);
+  }
+
+  @Nls(capitalization = Nls.Capitalization.Sentence)
+  @NotNull
+  @Override
+  public String getText() {
+    if (!myIsLastStatement) {
+      PsiExpression value = myReturnStatement.getReturnValue();
+      if (value != null) return QuickFixBundle.message("delete.return.fix.value.text", value.getText());
+    }
+    return QuickFixBundle.message("delete.return.fix.statement.text");
+  }
+
+  @Nls(capitalization = Nls.Capitalization.Sentence)
+  @NotNull
+  @Override
+  public String getFamilyName() {
+    return QuickFixBundle.message("delete.return.fix.family");
+  }
+
+  @Override
+  public boolean isAvailable(@NotNull Project project, Editor editor, PsiFile file) {
+    if (!myReturnStatement.isValid() || !BaseIntentionAction.canModify(myReturnStatement)) return false;
+    PsiExpression returnValue = myReturnStatement.getReturnValue();
+    return returnValue == null || !SideEffectChecker.mayHaveSideEffects(returnValue);
+  }
+
+  @Override
+  public void invoke(@NotNull Project project, Editor editor, PsiFile file) throws IncorrectOperationException {
+    if (myIsLastStatement) {
+      new CommentTracker().deleteAndRestoreComments(myReturnStatement);
+    }
+    else {
+      PsiElement toDelete = myReturnStatement.getReturnValue();
+      new CommentTracker().deleteAndRestoreComments(toDelete == null ? myReturnStatement : toDelete);
+    }
+  }
+
+  @Override
+  public boolean startInWriteAction() {
+    return true;
+  }
+}

java/java-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/MethodReturnTypeFix.java

@@ -49,6 +49,14 @@ public class MethodReturnTypeFix extends LocalQuickFixAndIntentionActionOnPsiEle
   private final String myName;
   private final String myCanonicalText;
 
+  public MethodReturnTypeFix(@NotNull PsiMethod method) {
+    super(method);
+    myReturnTypePointer = null;
+    myFixWholeHierarchy = false;
+    myName = method.getName();
+    myCanonicalText = null;
+  }
+
   public MethodReturnTypeFix(@NotNull PsiMethod method, @NotNull PsiType returnType, boolean fixWholeHierarchy) {
     super(method);
     myReturnTypePointer = SmartTypePointerManager.getInstance(method.getProject()).createSmartTypePointer(returnType);
@@ -67,7 +75,8 @@ public class MethodReturnTypeFix extends LocalQuickFixAndIntentionActionOnPsiEle
   @NotNull
   @Override
   public String getText() {
-    return QuickFixBundle.message("fix.return.type.text", myName, myCanonicalText);
+    return myCanonicalText == null ? QuickFixBundle.message("fix.return.type.change.type.text", myName) :
+           QuickFixBundle.message("fix.return.type.text", myName, myCanonicalText);
   }
 
   @Override
@@ -82,10 +91,10 @@ public class MethodReturnTypeFix extends LocalQuickFixAndIntentionActionOnPsiEle
                              @NotNull PsiElement startElement,
                              @NotNull PsiElement endElement) {
     final PsiMethod myMethod = (PsiMethod)startElement;
-
+    if (!BaseIntentionAction.canModify(myMethod)) return false;
+    if (myReturnTypePointer == null) return myMethod.getReturnTypeElement() != null;
     final PsiType myReturnType = myReturnTypePointer.getType();
-    if (BaseIntentionAction.canModify(myMethod) &&
-        myReturnType != null &&
+    if (myReturnType != null &&
         myReturnType.isValid() &&
         !TypeConversionUtil.isNullType(myReturnType)) {
       final PsiType returnType = myMethod.getReturnType();
@@ -105,6 +114,11 @@ public class MethodReturnTypeFix extends LocalQuickFixAndIntentionActionOnPsiEle
     final PsiMethod myMethod = (PsiMethod)startElement;
 
     if (!FileModificationService.getInstance().prepareFileForWrite(myMethod.getContainingFile())) return;
+    if (myReturnTypePointer == null) {
+      Editor editorForMethod = getEditorForMethod(myMethod, project, editor, file);
+      if (editorForMethod != null) selectInEditor(myMethod.getReturnTypeElement(), editorForMethod);
+      return;
+    }
     final PsiType myReturnType = myReturnTypePointer.getType();
     if (myReturnType == null) return;
     if (myFixWholeHierarchy) {
@@ -135,7 +149,7 @@ public class MethodReturnTypeFix extends LocalQuickFixAndIntentionActionOnPsiEle
     if (statementToSelect != null) {
       Editor editorForMethod = getEditorForMethod(myMethod, project, editor, file);
       if (editorForMethod != null) {
-        selectReturnValueInEditor(statementToSelect, editorForMethod);
+        selectInEditor(statementToSelect.getReturnValue(), editorForMethod);
       }
     }
   }
@@ -308,10 +322,9 @@ public class MethodReturnTypeFix extends LocalQuickFixAndIntentionActionOnPsiEle
     }
   }
 
-  static void selectReturnValueInEditor(final PsiReturnStatement returnStatement, final Editor editor) {
-    final PsiExpression returnValue = returnStatement.getReturnValue();
-    LOG.assertTrue(returnValue != null, returnStatement);
-    TextRange range = returnValue.getTextRange();
+  static void selectInEditor(@Nullable PsiElement element, Editor editor) {
+    LOG.assertTrue(element != null);
+    TextRange range = element.getTextRange();
     int offset = range.getStartOffset();
 
     editor.getCaretModel().moveToOffset(offset);

java/java-impl/src/com/intellij/codeInsight/intention/impl/config/QuickFixFactoryImpl.java

@@ -84,6 +84,12 @@ public class QuickFixFactoryImpl extends QuickFixFactory {
     return new MethodReturnTypeFix(method, toReturn, fixWholeHierarchy);
   }
 
+  @NotNull
+  @Override
+  public LocalQuickFixAndIntentionActionOnPsiElement createMethodReturnUnknownTypeFix(@NotNull PsiMethod method) {
+    return new MethodReturnTypeFix(method);
+  }
+
   @NotNull
   @Override
   public LocalQuickFixAndIntentionActionOnPsiElement createAddMethodFix(@NotNull PsiMethod method, @NotNull PsiClass toClass) {
@@ -269,6 +275,12 @@ public class QuickFixFactoryImpl extends QuickFixFactory {
     return new ConvertToStringLiteralAction();
   }
 
+  @NotNull
+  @Override
+  public IntentionAction createDeleteReturnFix(@NotNull PsiCodeBlock codeBlock, @NotNull PsiReturnStatement statement) {
+    return new DeleteReturnFix(codeBlock, statement);
+  }
+
   @NotNull
   @Override
   public IntentionAction createDeleteCatchFix(@NotNull PsiParameter parameter) {

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/afterKnownTypeLastStatement.java

@@ -0,0 +1,7 @@
+// "Delete return statement" "true"
+
+class Test {
+
+  void foo(boolean b) {
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/afterKnownTypeUsefulReturn.java

@@ -0,0 +1,9 @@
+// "Delete return value '"foo"'" "true"
+
+class Test {
+
+  void foo(boolean b) {
+    if (b) return;
+    System.out.println("bar");
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/afterUnknownTypeLastStatement.java

@@ -0,0 +1,7 @@
+// "Delete return statement" "true"
+
+class Test {
+
+  void foo(boolean b) {
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/afterUnknownTypeUsefulReturn.java

@@ -0,0 +1,9 @@
+// "Delete return value 'null'" "true"
+
+class Test {
+
+  void foo(boolean b) {
+    if (b) return;
+    System.out.println("bar");
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/beforeKnownTypeLastStatement.java

@@ -0,0 +1,8 @@
+// "Delete return statement" "true"
+
+class Test {
+
+  void foo(boolean b) {
+    return<caret> "foo";
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/beforeKnownTypeLastStatementSideEffects.java

@@ -0,0 +1,12 @@
+// "Delete return statement" "false"
+
+class Test {
+
+  void foo(boolean b) {
+    return<caret> getWithSideEffects();
+  }
+
+  private String getWithSideEffects() {
+    System.out.println("baz");
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/beforeKnownTypeUsefulReturn.java

@@ -0,0 +1,9 @@
+// "Delete return value '"foo"'" "true"
+
+class Test {
+
+  void foo(boolean b) {
+    if (b) return<caret> "foo";
+    System.out.println("bar");
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/beforeKnownTypeUsefulReturnSideEffects.java

@@ -0,0 +1,13 @@
+// "Delete return statement" "false"
+
+class Test {
+
+  void foo(boolean b) {
+      if (b) return<caret> getWithSideEffects();
+      System.out.println("bar");
+  }
+
+  private String getWithSideEffects() {
+    System.out.println("baz");
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/beforeUnknownTypeLastStatement.java

@@ -0,0 +1,8 @@
+// "Delete return statement" "true"
+
+class Test {
+
+  void foo(boolean b) {
+    return<caret> null;
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn/beforeUnknownTypeUsefulReturn.java

@@ -0,0 +1,9 @@
+// "Delete return value 'null'" "true"
+
+class Test {
+
+  void foo(boolean b) {
+    if (b) return<caret> null;
+    System.out.println("bar");
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/methodReturn/afterUknownTypeReturnFromVoid.java

@@ -0,0 +1,8 @@
+// "Change return type for method 'foo'" "true"
+
+class Test {
+
+  <caret><selection>void</selection> foo() {
+    return null;
+  }
+}

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/methodReturn/beforeUknownTypeReturnFromVoid.java

@@ -0,0 +1,8 @@
+// "Change return type for method 'foo'" "true"
+
+class Test {
+
+  void foo() {
+    return <caret>null;
+  }
+}

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

@@ -0,0 +1,11 @@
+// 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.java.codeInsight.daemon.quickFix;
+
+import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase;
+
+public class DeleteReturnFixTest extends LightQuickFixParameterizedTestCase {
+  @Override
+  protected String getBasePath() {
+    return "/codeInsight/daemonCodeAnalyzer/quickFix/deleteReturn";
+  }
+}