From 265ec5fb377d8911e47c2eb9099876575ead2062 Mon Sep 17 00:00:00 2001 From: Bart van Helvert Date: Fri, 6 Feb 2026 13:31:43 +0100 Subject: [PATCH] [java] Add chooser fix for division that could change semantics Adds a chooser to `ReplaceShiftWithMultiplyIntention` which allows to choose between regular division that changes semantics and floor division. By default floor division is chosen. This changes also makes it so regular division is suggested when the language level is lower than 8. #IDEA-384124 (cherry picked from commit 3e5c5bb4852098ba7b8639c2d887f7cf1298cd93) IJ-MR-189843 GitOrigin-RevId: db19634d66f5a91282536b4c1b41fb0f8ccfcd5e --- .../messages/QuickFixBundle.properties | 4 +- .../ReplaceShiftWithMultiplyIntention.java | 175 +++++++++++------- .../RightShiftAssignPos.java | 6 + .../RightShiftAssignPos_after.java | 6 + .../RightShiftAssign_semChange_after.java | 5 + .../RightShift_semChange_after.java | 5 + ...ReplaceShiftWithMultiplyIntentionTest.java | 44 ++++- .../messages/CommonQuickFixBundle.properties | 2 + 8 files changed, 169 insertions(+), 78 deletions(-) create mode 100644 java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos.java create mode 100644 java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssignPos_after.java create mode 100644 java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShiftAssign_semChange_after.java create mode 100644 java/java-tests/testData/ipp/com/siyeh/ipp/shift/replace_shift_with_multiply/RightShift_semChange_after.java 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"