From 4fbe223f67904bef06daac97de1f253ae974b4af Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Mon, 25 Nov 2024 18:09:52 +0100 Subject: [PATCH] QuickFix for S7158 (String.isEmpty() instead of length check). --- .../java/checks/StringIsEmptyCheckSample.java | 81 ++++++++++--- .../sonar/java/checks/StringIsEmptyCheck.java | 111 ++++++++++++++++-- 2 files changed, 166 insertions(+), 26 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/StringIsEmptyCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/StringIsEmptyCheckSample.java index 068a002020..b8932c8bc2 100644 --- a/java-checks-test-sources/default/src/main/java/checks/StringIsEmptyCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/StringIsEmptyCheckSample.java @@ -5,31 +5,82 @@ public boolean sample(String s, String t) { boolean b; // test `length() == 0` and equivalent code - b = s.length() == 0; // Noncompliant - b = s.length() <= 0; // Noncompliant - b = s.length() < 1; // Noncompliant + + b = s.length() == 0; // Noncompliant [[quickfixes=qf1]] + // fix@qf1 {{Replace with "isEmpty()"}} + // edit@qf1 [[sc=11;ec=24]]{{isEmpty()}} + + b = s.length() <= 0; // Noncompliant [[quickfixes=qf2]] + // fix@qf2 {{Replace with "isEmpty()"}} + // edit@qf2 [[sc=11;ec=24]]{{isEmpty()}} + + b = s.length() < 1; // Noncompliant [[quickfixes=qf3]] + // fix@qf3 {{Replace with "isEmpty()"}} + // edit@qf3 [[sc=11;ec=23]]{{isEmpty()}} // test `length() != 0` and equivalent code - b = s.length() != 0; // Noncompliant - b = s.length() > 0; // Noncompliant - b = s.length() >= 1; // Noncompliant + b = s.length() != 0; // Noncompliant [[quickfixes=qf4]] + // fix@qf4 {{Replace with "isEmpty()"}} + // edit@qf4 [[sc=9;ec=9]]{{!}} + // edit@qf4 [[sc=11;ec=24]]{{isEmpty()}} + + b = s.length() > 0; // Noncompliant [[quickfixes=qf5]] + // fix@qf5 {{Replace with "isEmpty()"}} + // edit@qf5 [[sc=9;ec=9]]{{!}} + // edit@qf5 [[sc=11;ec=23]]{{isEmpty()}} + + b = s.length() >= 1; // Noncompliant [[quickfixes=qf6]] + // fix@qf6 {{Replace with "isEmpty()"}} + // edit@qf6 [[sc=9;ec=9]]{{!}} + // edit@qf6 [[sc=11;ec=24]]{{isEmpty()}} // reversed order - b = 0 == s.length(); // Noncompliant - b = 0 >= s.length(); // Noncompliant - b = 1 > s.length(); // Noncompliant - b = 0 != s.length(); // Noncompliant - b = 0 < s.length(); // Noncompliant - b = 1 <= s.length(); // Noncompliant + + b = 0 == s.length(); // Noncompliant [[quickfixes=qf7]] + // fix@qf7 {{Replace with "isEmpty()"}} + // edit@qf7 [[sc=9;ec=14]]{{}} + // edit@qf7 [[sc=16;ec=24]]{{isEmpty()}} + + b = 0 >= s.length(); // Noncompliant [[quickfixes=qf8]] + // fix@qf8 {{Replace with "isEmpty()"}} + // edit@qf8 [[sc=9;ec=14]]{{}} + // edit@qf8 [[sc=16;ec=24]]{{isEmpty()}} + + b = 1 > s.length(); // Noncompliant [[quickfixes=qf9]] + // fix@qf9 {{Replace with "isEmpty()"}} + // edit@qf9 [[sc=9;ec=13]]{{}} + // edit@qf9 [[sc=15;ec=23]]{{isEmpty()}} + + b = 0 != s.length(); // Noncompliant [[quickfixes=qf10]] + // fix@qf10 {{Replace with "isEmpty()"}} + // edit@qf10 [[sc=9;ec=14]]{{!}} + // edit@qf10 [[sc=16;ec=24]]{{isEmpty()}} + + b = 0 < s.length(); // Noncompliant [[quickfixes=qf11]] + // fix@qf11 {{Replace with "isEmpty()"}} + // edit@qf11 [[sc=9;ec=13]]{{!}} + // edit@qf11 [[sc=15;ec=23]]{{isEmpty()}} + + b = 1 <= s.length(); // Noncompliant [[quickfixes=qf12]] + // fix@qf12 {{Replace with "isEmpty()"}} + // edit@qf12 [[sc=9;ec=14]]{{!}} + // edit@qf12 [[sc=16;ec=24]]{{isEmpty()}} // extra parentheses - b = (s.length()) == 0; // Noncompliant + b = (s.length()) == 0; // Noncompliant [[quickfixes=qf13]] + // fix@qf13 {{Replace with "isEmpty()"}} + // edit@qf13 [[sc=9;ec=10]]{{}} + // edit@qf13 [[sc=12;ec=26]]{{isEmpty()}} // chained method calls - b = s.toUpperCase().length() == 0; // Noncompliant + b = s.toUpperCase().length() == 0; // Noncompliant [[quickfixes=qf14]] + // fix@qf14 {{Replace with "isEmpty()"}} + // edit@qf14 [[sc=25;ec=38]]{{isEmpty()}} // problem in a nested expression - b = "abc".equals(s) || s.length() == 0; // Noncompliant + b = "abc".equals(s) || s.length() == 0; // Noncompliant [[quickfixes=qf15]] + // fix@qf15 {{Replace with "isEmpty()"}} + // edit@qf15 [[sc=30;ec=43]]{{isEmpty()}} b = s.length() == 1; b = s.length() > 3; diff --git a/java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java index 227439636f..883fdcfc38 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java @@ -20,15 +20,21 @@ package org.sonar.java.checks; import java.util.List; +import javax.annotation.Nullable; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.QuickFixHelper; import org.sonar.java.model.ExpressionUtils; import org.sonar.java.model.LiteralUtils; +import org.sonar.java.reporting.AnalyzerMessage; +import org.sonar.java.reporting.JavaQuickFix; +import org.sonar.java.reporting.JavaTextEdit; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaVersion; import org.sonar.plugins.java.api.JavaVersionAwareVisitor; import org.sonar.plugins.java.api.semantic.MethodMatchers; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.Tree.Kind; @@ -55,6 +61,19 @@ public class StringIsEmptyCheck extends IssuableSubscriptionVisitor implements J Kind.GREATER_THAN_OR_EQUAL_TO }; + private enum ComparisonType { + // for example, `s.length() == 0` + IS_EMPTY, + // for example, `s.length() > 0` + IS_NOT_EMPTY, + // for example, `s.length() > 80` + OTHER + } + + private static boolean isEmptinessCheck(ComparisonType comparisonType) { + return comparisonType == ComparisonType.IS_EMPTY || comparisonType == ComparisonType.IS_NOT_EMPTY; + } + // `String.isEmpty()` is available since Java 6. @Override public boolean isCompatibleWithJavaVersion(JavaVersion version) { @@ -78,29 +97,99 @@ public void visitNode(Tree tree) { boolean rightIsZero = LiteralUtils.isZero(right); boolean rightIsOne = LiteralUtils.isOne(right); - if ((isLengthCall(left) && isComparisonOnRight(bet, rightIsZero, rightIsOne)) - || (isLengthCall(right) && isComparisonOnLeft(leftIsZero, leftIsOne, bet))) { - reportIssue(tree, "Use isEmpty() to check whether a string is empty or not."); + final MethodInvocationTree lengthCall; + final ComparisonType comparisonType; + + // First try `s.length() OPERATOR VALUE`, then try `VALUE OPERATOR s.length()`. + MethodInvocationTree mit = getLengthCall(left); + if (mit != null) { + lengthCall = mit; + comparisonType = getComparisonOnRight(bet, rightIsZero, rightIsOne); + } else { + lengthCall = getLengthCall(right); + comparisonType = lengthCall != null ? getComparisonOnLeft(leftIsZero, leftIsOne, bet) : null; + } + + if (lengthCall != null && isEmptinessCheck(comparisonType)) { + QuickFixHelper + .newIssue(context) + .forRule(this) + .onTree(tree) + .withMessage("Use isEmpty() to check whether a string is empty or not.") + .withQuickFix(() -> getQuickFix(tree, lengthCall, comparisonType)) + .report(); + } + } + + private static JavaQuickFix getQuickFix(Tree comparisonExpression, MethodInvocationTree lengthInvocation, ComparisonType comparisonType) { + // There are two cases to deal with: + // s.length() OP CONST + // CONST OP s.length() + JavaQuickFix.Builder builder = JavaQuickFix.newQuickFix("Replace with \"isEmpty()\""); + + // Replace "[CONST OP]" with "!"/"". + AnalyzerMessage.TextSpan prefixSpan = AnalyzerMessage.textSpanBetween(comparisonExpression, true, lengthInvocation, false); + String prefixReplacement = comparisonType == ComparisonType.IS_NOT_EMPTY ? "!" : ""; + if (!prefixReplacement.isEmpty() || !prefixSpan.isEmpty()) { + builder.addTextEdit(JavaTextEdit.replaceTextSpan(prefixSpan, prefixReplacement)); } + + // Replace "length() [OP CONST]" with "isEmpty()". + IdentifierTree lengthIdentifier = ExpressionUtils.methodName(lengthInvocation); + builder.addTextEdit(JavaTextEdit.replaceBetweenTree( + lengthIdentifier, + true, + comparisonExpression, + true, + "isEmpty()" + )); + + return builder.build(); } - private static boolean isLengthCall(ExpressionTree tree) { - return tree instanceof MethodInvocationTree mit && LENGTH_METHOD.matches(mit); + @Nullable + private static MethodInvocationTree getLengthCall(ExpressionTree tree) { + if (tree instanceof MethodInvocationTree mit && LENGTH_METHOD.matches(mit)) { + return mit; + } + return null; } /** * Check the comparison for 
length() OP VALUE
. */ - private static boolean isComparisonOnRight(BinaryExpressionTree tree, boolean rightIsZero, boolean rightIsOne) { - return (tree.is(Kind.EQUAL_TO, Kind.LESS_THAN_OR_EQUAL_TO, Kind.NOT_EQUAL_TO, Kind.GREATER_THAN) && rightIsZero) - || (tree.is(Kind.LESS_THAN, Kind.GREATER_THAN_OR_EQUAL_TO) && rightIsOne); + private static ComparisonType getComparisonOnRight(BinaryExpressionTree tree, boolean rightIsZero, boolean rightIsOne) { + if (tree.is(Kind.EQUAL_TO, Kind.LESS_THAN_OR_EQUAL_TO) && rightIsZero) { + return ComparisonType.IS_EMPTY; + } + if (tree.is(Kind.NOT_EQUAL_TO, Kind.GREATER_THAN) && rightIsZero) { + return ComparisonType.IS_NOT_EMPTY; + } + if (tree.is(Kind.LESS_THAN) && rightIsOne) { + return ComparisonType.IS_EMPTY; + } + if (tree.is(Kind.GREATER_THAN_OR_EQUAL_TO) && rightIsOne) { + return ComparisonType.IS_NOT_EMPTY; + } + return ComparisonType.OTHER; } /** * Check the comparison for 
VALUE OP length()
. */ - private static boolean isComparisonOnLeft(boolean leftIsZero, boolean leftIsOne, BinaryExpressionTree tree) { - return (leftIsZero && tree.is(Kind.EQUAL_TO, Kind.GREATER_THAN_OR_EQUAL_TO, Kind.NOT_EQUAL_TO, Kind.LESS_THAN)) - || (leftIsOne && tree.is(Kind.GREATER_THAN, Kind.LESS_THAN_OR_EQUAL_TO)); + private static ComparisonType getComparisonOnLeft(boolean leftIsZero, boolean leftIsOne, BinaryExpressionTree tree) { + if (leftIsZero && tree.is(Kind.EQUAL_TO, Kind.GREATER_THAN_OR_EQUAL_TO)) { + return ComparisonType.IS_EMPTY; + } + if (leftIsZero && tree.is(Kind.NOT_EQUAL_TO, Kind.LESS_THAN)) { + return ComparisonType.IS_NOT_EMPTY; + } + if (leftIsOne && tree.is(Kind.GREATER_THAN)) { + return ComparisonType.IS_EMPTY; + } + if (leftIsOne && tree.is(Kind.LESS_THAN_OR_EQUAL_TO)) { + return ComparisonType.IS_NOT_EMPTY; + } + return ComparisonType.OTHER; } }