diff --git a/java/java-analysis-impl/resources/messages/QuickFixBundle.properties b/java/java-analysis-impl/resources/messages/QuickFixBundle.properties index 2d01c738dce4..a1a27c96bbc9 100644 --- a/java/java-analysis-impl/resources/messages/QuickFixBundle.properties +++ b/java/java-analysis-impl/resources/messages/QuickFixBundle.properties @@ -470,4 +470,6 @@ record.delegate.to.canonical.constructor.fix.name=Delegate to canonical construc lift.throw.out.of.switch.expression.fix.name=Lift 'throw' out of 'switch' expression change.to.similar.keyword.fix.family.name=Change to the similar keyword -change.to.similar.keyword.fix.name=Fix the typo ''{0}'' to ''{1}'' \ No newline at end of file +change.to.similar.keyword.fix.name=Fix the typo ''{0}'' to ''{1}'' + +fix.replace.x.with.division=Replace ''{0}'' with division \ No newline at end of file diff --git a/java/java-impl/src/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntention.java b/java/java-impl/src/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntention.java index a2af1edafa66..db6eee24a5df 100644 --- a/java/java-impl/src/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntention.java +++ b/java/java-impl/src/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntention.java @@ -15,15 +15,19 @@ */ package com.siyeh.ipp.shift; +import com.intellij.codeInsight.daemon.QuickFixBundle; import com.intellij.codeInspection.CommonQuickFixBundle; import com.intellij.codeInspection.dataFlow.CommonDataflow; +import com.intellij.modcommand.ActionContext; +import com.intellij.modcommand.ModCommand; +import com.intellij.modcommand.Presentation; +import com.intellij.modcommand.PsiBasedModCommandAction; import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet; import com.intellij.openapi.project.DumbAware; import com.intellij.pom.java.LanguageLevel; import com.intellij.psi.JavaTokenType; import com.intellij.psi.PsiAssignmentExpression; import com.intellij.psi.PsiBinaryExpression; -import com.intellij.psi.PsiElement; import com.intellij.psi.PsiExpression; import com.intellij.psi.PsiJavaToken; import com.intellij.psi.PsiLiteralExpression; @@ -36,12 +40,14 @@ import com.siyeh.IntentionPowerPackBundle; import com.siyeh.ig.PsiReplacementUtil; import com.siyeh.ig.psiutils.CommentTracker; import com.siyeh.ig.psiutils.ParenthesesUtils; -import com.siyeh.ipp.base.MCIntention; -import com.siyeh.ipp.base.PsiElementPredicate; +import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -public final class ReplaceShiftWithMultiplyIntention extends MCIntention implements DumbAware { +public final class ReplaceShiftWithMultiplyIntention extends PsiBasedModCommandAction implements DumbAware { + ReplaceShiftWithMultiplyIntention() { + super(PsiExpression.class); + } @Override public @NotNull String getFamilyName() { @@ -49,54 +55,52 @@ public final class ReplaceShiftWithMultiplyIntention extends MCIntention impleme } @Override - protected @NotNull String getTextForElement(@NotNull PsiElement element) { + protected boolean isElementApplicable(@NotNull PsiExpression element, @NotNull ActionContext context) { + return new ShiftByLiteralPredicate().satisfiedBy(element); + } + + @Override + protected @NotNull Presentation getPresentation(@NotNull ActionContext context, @NotNull PsiExpression element) { if (element instanceof PsiAssignmentExpression exp) { - final PsiJavaToken sign = exp.getOperationSign(); - final IElementType tokenType = sign.getTokenType(); - final String assignString = JavaTokenType.GTGTEQ.equals(tokenType) ? - (isSafelyDivisible(exp.getLExpression()) ? "/=" : "Math.floorDiv") : "*="; - return CommonQuickFixBundle.message("fix.replace.x.with.y", sign.getText(), assignString); + return getPresentation(exp.getOperationSign(), JavaTokenType.GTGTEQ, exp.getLExpression(), "/=", "*="); } else { final PsiBinaryExpression exp = (PsiBinaryExpression) element; - final PsiJavaToken sign = exp.getOperationSign(); - final IElementType tokenType = sign.getTokenType(); - final String operatorString = tokenType.equals(JavaTokenType.GTGT) ? - (isSafelyDivisible(exp.getLOperand()) ? "/" : "Math.floorDiv") : "*"; - return CommonQuickFixBundle.message("fix.replace.x.with.y", sign.getText(), operatorString); + return getPresentation(exp.getOperationSign(), JavaTokenType.GTGT, exp.getLOperand(), "/", "*"); } } - @Override - public @NotNull PsiElementPredicate getElementPredicate() { - return new ShiftByLiteralPredicate() { - @Override - public boolean satisfiedBy(PsiElement element) { - if (element instanceof PsiAssignmentExpression expr - && expr.getOperationTokenType().equals(JavaTokenType.GTGTEQ) - && PsiUtil.getLanguageLevel(element).isLessThan(LanguageLevel.JDK_1_8) - && !isSafelyDivisible(expr.getLExpression()) - ) { - return false; + private static @Nls @NotNull Presentation getPresentation( + @NotNull PsiJavaToken sign, + @NotNull IElementType tokenType, + @NotNull PsiExpression lExpr, + @NotNull String divOperator, + @NotNull String mulOperator + ) { + String message; + if (sign.getTokenType().equals(tokenType)) { + if (PsiUtil.getLanguageLevel(lExpr).isLessThan(LanguageLevel.JDK_1_8)) { + message = CommonQuickFixBundle.message("fix.replace.x.with.y.may.change.semantics", sign.getText(), divOperator); + } else { + if (isSafelyDivisible(lExpr)) { + message = CommonQuickFixBundle.message("fix.replace.x.with.y", sign.getText(), divOperator); + } else { + message = QuickFixBundle.message("fix.replace.x.with.division", sign.getText()); } - if (element instanceof PsiBinaryExpression expr - && expr.getOperationTokenType().equals(JavaTokenType.GTGT) - && PsiUtil.getLanguageLevel(element).isLessThan(LanguageLevel.JDK_1_8) - && !isSafelyDivisible(expr.getLOperand()) - ) { - return false; - } - return super.satisfiedBy(element); } - }; + } else { + message = CommonQuickFixBundle.message("fix.replace.x.with.y", sign.getText(), mulOperator); + } + return Presentation.of(message); } @Override - public void invoke(@NotNull PsiElement element) { + protected @NotNull ModCommand perform(@NotNull ActionContext context, @NotNull PsiExpression element) { if (element instanceof PsiBinaryExpression expr) { - replaceShiftWithMultiplyOrDivide(expr); + return replaceShiftWithMultiplyOrDivide(expr); } - else if (element instanceof PsiAssignmentExpression expr) { - replaceShiftAssignWithMultiplyOrDivideAssign(expr); + else { + PsiAssignmentExpression expr = (PsiAssignmentExpression)element; + return replaceShiftAssignWithMultiplyOrDivideAssign(expr); } } @@ -105,49 +109,84 @@ public final class ReplaceShiftWithMultiplyIntention extends MCIntention impleme return range != null && range.min() >= 0; } - private static void replaceShiftAssignWithMultiplyOrDivideAssign(PsiAssignmentExpression exp) { - final PsiExpression lhsExpr = exp.getLExpression(); - final PsiExpression rhsExpr = PsiUtil.skipParenthesizedExprDown(exp.getRExpression()); - if (!(rhsExpr instanceof PsiLiteralExpression rhsLiteral)) return; - CommentTracker commentTracker = new CommentTracker(); - final String lhsText = commentTracker.text(lhsExpr, ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE); - final String rhsText = rhsReplacement(rhsLiteral, lhsExpr.getType()); - String expString; - if (exp.getOperationTokenType().equals(JavaTokenType.GTGTEQ)) { - if (isSafelyDivisible(lhsExpr)) { - expString = lhsText + "/=" + rhsText; + private static ModCommand replaceShiftAssignWithMultiplyOrDivideAssign(@NotNull PsiAssignmentExpression expr) { + final PsiExpression rhsExpr = PsiUtil.skipParenthesizedExprDown(expr.getRExpression()); + if (!(rhsExpr instanceof PsiLiteralExpression rhsLiteral)) return ModCommand.nop(); + final String rhsText = rhsReplacement(rhsLiteral, expr.getLExpression().getType()); + if (expr.getOperationTokenType().equals(JavaTokenType.GTGTEQ)) { + if (isSafelyDivisible(expr.getLExpression()) || PsiUtil.getLanguageLevel(expr).isLessThan(LanguageLevel.JDK_1_8)) { + return ModCommand.psiUpdate(expr, e -> replaceAssignmentExprOperator(e, "/=", rhsText)); } else { - expString = lhsText + "=" + "Math.floorDiv(" + lhsText + ", " + rhsText + ")"; + return ModCommand.chooseAction( + CommonQuickFixBundle.message("fix.replace.with"), + ModCommand.psiUpdateStep( + expr, + CommonQuickFixBundle.message("fix.replace.x.with.y", ">>=", "Math.floorDiv"), + (e, updater) -> { + CommentTracker commentTracker = new CommentTracker(); + final String lhsText = commentTracker.text(e.getLExpression(), ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE); + PsiReplacementUtil.replaceExpression(e, lhsText + "=" + "Math.floorDiv(" + lhsText + ", " + rhsText + ")"); + }), + ModCommand.psiUpdateStep( + expr, + CommonQuickFixBundle.message("fix.replace.x.with.y.may.change.semantics", ">>=", "/="), + (e, updater) -> replaceAssignmentExprOperator(e, "/=", rhsText) + ) + ); } } else { - expString = lhsText + "*=" + rhsText; + return ModCommand.psiUpdate(expr, e -> replaceAssignmentExprOperator(e, "*=", rhsText)); } - PsiReplacementUtil.replaceExpression(exp, expString, commentTracker); } - private static void replaceShiftWithMultiplyOrDivide(PsiBinaryExpression expression) { - final PsiExpression lhsExpr = expression.getLOperand(); - final PsiExpression rhsExpr = PsiUtil.skipParenthesizedExprDown(expression.getROperand()); - if (!(rhsExpr instanceof PsiLiteralExpression rhsLiteral)) return; + private static void replaceAssignmentExprOperator(PsiAssignmentExpression e, String x, String rhsText) { CommentTracker commentTracker = new CommentTracker(); - final String lhsText = commentTracker.text(lhsExpr, ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE); - final String rhsText = rhsReplacement(rhsLiteral, lhsExpr.getType()); - String expString; - if (expression.getOperationTokenType().equals(JavaTokenType.GTGT)) { - if (isSafelyDivisible(lhsExpr)) { - expString = lhsText + "/" + rhsText; + final String lhsText = commentTracker.text(e.getLExpression(), ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE); + PsiReplacementUtil.replaceExpression(e, lhsText + x + rhsText, commentTracker); + } + + private static ModCommand replaceShiftWithMultiplyOrDivide(@NotNull PsiBinaryExpression expr) { + final PsiExpression rhsExpr = PsiUtil.skipParenthesizedExprDown(expr.getROperand()); + if (!(rhsExpr instanceof PsiLiteralExpression rhsLiteral)) return ModCommand.nop(); + final String rhsText = rhsReplacement(rhsLiteral, expr.getLOperand().getType()); + if (expr.getOperationTokenType().equals(JavaTokenType.GTGT)) { + if (isSafelyDivisible(expr.getLOperand()) || PsiUtil.getLanguageLevel(expr).isLessThan(LanguageLevel.JDK_1_8)) { + return ModCommand.psiUpdate(expr, e -> replaceBinaryExprOperator(e, "/", rhsText)); } else { - expString = "Math.floorDiv(" + lhsText + ", " + rhsText + ")"; + return ModCommand.chooseAction( + CommonQuickFixBundle.message("fix.replace.with"), + ModCommand.psiUpdateStep( + expr, + CommonQuickFixBundle.message("fix.replace.x.with.y", ">>", "Math.floorDiv"), + (e, updater) -> { + CommentTracker commentTracker = new CommentTracker(); + final String lhsText = commentTracker.text(e.getLOperand(), ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE); + PsiReplacementUtil.replaceExpression(e, parenthesizeIfRequired(e, "Math.floorDiv(" + lhsText + ", " + rhsText + ")"), commentTracker); + }), + ModCommand.psiUpdateStep( + expr, + CommonQuickFixBundle.message("fix.replace.x.with.y.may.change.semantics", ">>", "/"), + (e, updater) -> replaceBinaryExprOperator(e, "/", rhsText) + ) + ); } } else { - expString = lhsText + "*" + rhsText; + return ModCommand.psiUpdate(expr, e -> replaceBinaryExprOperator(e, "*", rhsText)); } + } + + private static void replaceBinaryExprOperator(PsiBinaryExpression e, String x, String rhsText) { + CommentTracker commentTracker = new CommentTracker(); + final String lhsText = commentTracker.text(e.getLOperand(), ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE); + PsiReplacementUtil.replaceExpression(e, parenthesizeIfRequired(e, lhsText + x + rhsText), commentTracker); + } + + private static String parenthesizeIfRequired(@NotNull PsiExpression expression, @NotNull String replacement) { if (expression.getParent() instanceof PsiExpression parent && !(parent instanceof PsiParenthesizedExpression) && ParenthesesUtils.getPrecedence(parent) < ParenthesesUtils.MULTIPLICATIVE_PRECEDENCE) { - expString = '(' + expString + ')'; + return '(' + replacement + ')'; } - - PsiReplacementUtil.replaceExpression(expression, expString, commentTracker); + return replacement; } private static String rhsReplacement(@NotNull PsiLiteralExpression rhs, @Nullable PsiType type) { diff --git a/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos.java b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos.java new file mode 100644 index 000000000000..d59c247b9300 --- /dev/null +++ b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos.java @@ -0,0 +1,6 @@ +class Test { + void test() { + int foo = 1; + foo >>= 12; + } +} diff --git a/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos_after.java b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos_after.java new file mode 100644 index 000000000000..0cca3e88556e --- /dev/null +++ b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos_after.java @@ -0,0 +1,6 @@ +class Test { + void test() { + int foo = 1; + foo /= 4096; + } +} diff --git a/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssign_semChange_after.java b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssign_semChange_after.java new file mode 100644 index 000000000000..6bb92646cdbb --- /dev/null +++ b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssign_semChange_after.java @@ -0,0 +1,5 @@ +class Test { + void test(int foo) { + foo /= 4096; + } +} diff --git a/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShift_semChange_after.java b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShift_semChange_after.java new file mode 100644 index 000000000000..176dda8f7f02 --- /dev/null +++ b/java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShift_semChange_after.java @@ -0,0 +1,5 @@ +class Test { + void test(int foo) { + int x = foo / 4096; + } +} diff --git a/java/java-tests/testSrc/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntentionTest.java b/java/java-tests/testSrc/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntentionTest.java index f145442d9c0e..44617f7759cb 100644 --- a/java/java-tests/testSrc/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntentionTest.java +++ b/java/java-tests/testSrc/com/siyeh/ipp/shift/ReplaceShiftWithMultiplyIntentionTest.java @@ -3,6 +3,10 @@ package com.siyeh.ipp.shift; import com.intellij.pom.java.LanguageLevel; import com.intellij.testFramework.IdeaTestUtil; +import com.intellij.testFramework.fixtures.CodeInsightTestUtil; +import com.intellij.ui.ChooserInterceptor; +import java.util.regex.Pattern; +import com.intellij.ui.UiInterceptors; import com.siyeh.ipp.IPPTestCase; public class ReplaceShiftWithMultiplyIntentionTest extends IPPTestCase { @@ -13,22 +17,44 @@ public class ReplaceShiftWithMultiplyIntentionTest extends IPPTestCase { public void testLeftShiftAssign() { doTest("Replace '<<=' with '*='"); } public void testLongShiftAssign() { doTest("Replace '<<=' with '*='"); } public void testParentheses() { doTest("Replace '<<' with '*'"); } - public void testRightShift() { - doTest("Replace '>>' with 'Math.floorDiv'"); - IdeaTestUtil.withLevel(myFixture.getModule(), LanguageLevel.JDK_1_7, () -> { - assertIntentionNotAvailable("Replace '>>' with 'Math.floorDiv'"); - }); + + public void testRightShift() { doTest("Replace '>>' with division"); } + public void testRightShiftNonDefaultChooser() { + doTestWithChooser("RightShift", ">>", "Replace '>>' with '/' (may change semantics)"); } + public void testRightShiftJava7() { doTestJava7("RightShift", "Replace '>>' with '/' (may change semantics)"); } + public void testRightShiftAssign() { - doTest("Replace '>>=' with 'Math.floorDiv'"); - IdeaTestUtil.withLevel(myFixture.getModule(), LanguageLevel.JDK_1_7, () -> { - assertIntentionNotAvailable("Replace '>>' with 'Math.floorDiv'"); - }); + doTest("Replace '>>=' with division"); } + public void testRightShiftAssignNonDefaultChooser() { + doTestWithChooser("RightShiftAssign", ">>=", "Replace '>>=' with '/=' (may change semantics)"); + } + public void testRightShiftAssignJava7() { + doTestJava7("RightShiftAssign", "Replace '>>=' with '/=' (may change semantics)"); + } + public void testRightShiftPos() { doTest("Replace '>>' with '/'"); } + public void testRightShiftAssignPos() { doTest("Replace '>>=' with '/='"); } @Override protected String getRelativePath() { return "shift/replace_shift_with_multiply"; } + + private void doTestJava7(String testName, String intentionName) { + IdeaTestUtil.withLevel(myFixture.getModule(), LanguageLevel.JDK_1_7, () -> { + CodeInsightTestUtil.doIntentionTest(myFixture, intentionName, testName + ".java", testName + "_semChange_after.java"); + }); + } + + private void doTestWithChooser(String testName, String operator, String selectedOption) { + UiInterceptors.register(new ChooserInterceptor(null, Pattern.quote(selectedOption))); + CodeInsightTestUtil.doIntentionTest( + myFixture, + "Replace '" + operator + "' with division", + testName + ".java", + testName + "_semChange_after.java" + ); + } } diff --git a/platform/analysis-api/resources/messages/CommonQuickFixBundle.properties b/platform/analysis-api/resources/messages/CommonQuickFixBundle.properties index 3ea620b02f50..8aa496903985 100644 --- a/platform/analysis-api/resources/messages/CommonQuickFixBundle.properties +++ b/platform/analysis-api/resources/messages/CommonQuickFixBundle.properties @@ -14,10 +14,12 @@ fix.remove.redundant=Remove redundant ''{0}'' fix.remove.statement=Remove ''{0}'' statement fix.remove.guard=Remove guard expression +fix.replace.with=Replace With fix.replace.with.x=Replace with ''{0}'' fix.replace.with.x.call=Replace with ''{0}'' call fix.can.replace.with.x=Can be replaced with ''{0}'' fix.replace.x.with.y=Replace ''{0}'' with ''{1}'' +fix.replace.x.with.y.may.change.semantics=Replace ''{0}'' with ''{1}'' (may change semantics) fix.can.replace.x.with.y=''{0}'' can be replaced with ''{1}'' fix.rename.name="Rename"