diff --git a/java-checks-test-sources/default/src/main/java/checks/StringIsBlankCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/StringIsBlankCheckSample.java new file mode 100644 index 00000000000..683746560b1 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/StringIsBlankCheckSample.java @@ -0,0 +1,106 @@ +package checks; + +public class StringIsBlankCheckSample { + public boolean sample(String s, StringBuilder sb) { + boolean b; + + // test `strip().length() == 0` and equivalent code + + b = s.strip().length() == 0; // Noncompliant {{Use "isBlank()" to check whether a "String" is blank or not.}} [[quickfixes=qf1]] + // fix@qf1 {{Replace with "isBlank()"}} + // edit@qf1 [[sc=11;ec=32]]{{isBlank()}} + + b = s.strip().length() <= 0; // Noncompliant [[quickfixes=qf2]] + // fix@qf2 {{Replace with "isBlank()"}} + // edit@qf2 [[sc=11;ec=32]]{{isBlank()}} + + b = s.strip().length() < 1; // Noncompliant [[quickfixes=qf3]] + // fix@qf3 {{Replace with "isBlank()"}} + // edit@qf3 [[sc=11;ec=31]]{{isBlank()}} + + // test `strip().length() != 0` and equivalent code + + b = s.strip().length() != 0; // Noncompliant [[quickfixes=qf4]] + // fix@qf4 {{Replace with "isBlank()"}} + // edit@qf4 [[sc=9;ec=9]]{{!}} + // edit@qf4 [[sc=11;ec=32]]{{isBlank()}} + + b = s.strip().length() > 0; // Noncompliant [[quickfixes=qf5]] + // fix@qf5 {{Replace with "isBlank()"}} + // edit@qf5 [[sc=9;ec=9]]{{!}} + // edit@qf5 [[sc=11;ec=31]]{{isBlank()}} + + b = s.strip().length() >= 1; // Noncompliant [[quickfixes=qf6]] + // fix@qf6 {{Replace with "isBlank()"}} + // edit@qf6 [[sc=9;ec=9]]{{!}} + // edit@qf6 [[sc=11;ec=32]]{{isBlank()}} + + // reversed order + + b = 0 == s.strip().length(); // Noncompliant [[quickfixes=qf7]] + // fix@qf7 {{Replace with "isBlank()"}} + // edit@qf7 [[sc=9;ec=14]]{{}} + // edit@qf7 [[sc=16;ec=32]]{{isBlank()}} + + b = 0 != s.strip().length(); // Noncompliant [[quickfixes=qf8]] + // fix@qf8 {{Replace with "isBlank()"}} + // edit@qf8 [[sc=9;ec=14]]{{!}} + // edit@qf8 [[sc=16;ec=32]]{{isBlank()}} + + // extra parentheses + + b = (s.strip().length()) == 0; // Noncompliant [[quickfixes=qf9]] + // fix@qf9 {{Replace with "isBlank()"}} + // edit@qf9 [[sc=9;ec=10]]{{}} + // edit@qf9 [[sc=12;ec=34]]{{isBlank()}} + + // the `strip()` call is not directly chained, so only emptiness can be simplified + b = (s.strip()).length() == 0; // Noncompliant {{Use "isEmpty()" to check whether a "String" is empty or not.}} [[quickfixes=qf10]] + // fix@qf10 {{Replace with "isEmpty()"}} + // edit@qf10 [[sc=21;ec=34]]{{isEmpty()}} + + // chained method calls + + b = s.toUpperCase().strip().length() == 0; // Noncompliant [[quickfixes=qf11]] + // fix@qf11 {{Replace with "isBlank()"}} + // edit@qf11 [[sc=25;ec=46]]{{isBlank()}} + + b = sb.toString().strip().length() == 0; // Noncompliant [[quickfixes=qf12]] + // fix@qf12 {{Replace with "isBlank()"}} + // edit@qf12 [[sc=23;ec=44]]{{isBlank()}} + + // `trim()` does not remove the same characters as `isBlank()` considers white space, + // so the message and the quick fix spell out what changes + + b = s.trim().length() == 0; // Noncompliant {{Use "isBlank()" to check whether a "String" is blank or not. Use this if Unicode whitespace should be treated as blank and control characters as non-blank.}} [[quickfixes=qf13]] + // fix@qf13 {{Replace with "isBlank()" (Unicode whitespace is treated as blank, control characters as non-blank)}} + // edit@qf13 [[sc=11;ec=31]]{{isBlank()}} + + b = s.trim().length() > 0; // Noncompliant [[quickfixes=qf14]] + // fix@qf14 {{Replace with "isBlank()" (Unicode whitespace is treated as blank, control characters as non-blank)}} + // edit@qf14 [[sc=9;ec=9]]{{!}} + // edit@qf14 [[sc=11;ec=30]]{{isBlank()}} + + b = 0 == s.trim().length(); // Noncompliant [[quickfixes=qf15]] + // fix@qf15 {{Replace with "isBlank()" (Unicode whitespace is treated as blank, control characters as non-blank)}} + // edit@qf15 [[sc=9;ec=14]]{{}} + // edit@qf15 [[sc=16;ec=31]]{{isBlank()}} + + b = s.toLowerCase().trim().length() == 0; // Noncompliant [[quickfixes=qf16]] + // fix@qf16 {{Replace with "isBlank()" (Unicode whitespace is treated as blank, control characters as non-blank)}} + // edit@qf16 [[sc=25;ec=45]]{{isBlank()}} + + b = s.trim().length() == 1; + b = s.trim().length() >= 8; + + b = s.strip().length() == 1; + b = s.strip().length() > 3; + b = s.strip().length() >= 8; + b = s.strip().length() == sb.length(); + + b = s.isBlank(); + b = !s.isBlank(); + + return b; + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/StringIsBlankCheckSample_java10.java b/java-checks-test-sources/default/src/main/java/checks/StringIsBlankCheckSample_java10.java new file mode 100644 index 00000000000..cece611f17c --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/StringIsBlankCheckSample_java10.java @@ -0,0 +1,29 @@ +package checks; + +public class StringIsBlankCheckSample_java10 { + public boolean sample(String s) { + boolean b; + + // `String.isBlank()` does not exist before Java 11, so these stay emptiness checks on the trimmed string + + b = s.strip().length() == 0; // Noncompliant {{Use "isEmpty()" to check whether a "String" is empty or not.}} [[quickfixes=qf1]] + // fix@qf1 {{Replace with "isEmpty()"}} + // edit@qf1 [[sc=19;ec=32]]{{isEmpty()}} + + b = s.strip().length() > 0; // Noncompliant {{Use "isEmpty()" to check whether a "String" is empty or not.}} [[quickfixes=qf2]] + // fix@qf2 {{Replace with "isEmpty()"}} + // edit@qf2 [[sc=9;ec=9]]{{!}} + // edit@qf2 [[sc=19;ec=31]]{{isEmpty()}} + + b = s.trim().length() == 0; // Noncompliant {{Use "isEmpty()" to check whether a "String" is empty or not.}} [[quickfixes=qf3]] + // fix@qf3 {{Replace with "isEmpty()"}} + // edit@qf3 [[sc=18;ec=31]]{{isEmpty()}} + + b = 0 == s.trim().length(); // Noncompliant {{Use "isEmpty()" to check whether a "String" is empty or not.}} [[quickfixes=qf4]] + // fix@qf4 {{Replace with "isEmpty()"}} + // edit@qf4 [[sc=9;ec=14]]{{}} + // edit@qf4 [[sc=23;ec=31]]{{isEmpty()}} + + return b; + } +} \ No newline at end of file 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 a48f8014bbc..69ab25d6abd 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 @@ -82,6 +82,36 @@ public boolean sample(String s, String t) { // fix@qf15 {{Replace with "isEmpty()"}} // edit@qf15 [[sc=30;ec=43]]{{isEmpty()}} + // zero written in a non-decimal base + b = s.length() == 0x0; // Noncompliant [[quickfixes=qf16]] + // fix@qf16 {{Replace with "isEmpty()"}} + // edit@qf16 [[sc=11;ec=26]]{{isEmpty()}} + + b = s.length() == 0X0; // Noncompliant [[quickfixes=qf17]] + // fix@qf17 {{Replace with "isEmpty()"}} + // edit@qf17 [[sc=11;ec=26]]{{isEmpty()}} + + b = s.length() == 00; // Noncompliant [[quickfixes=qf18]] + // fix@qf18 {{Replace with "isEmpty()"}} + // edit@qf18 [[sc=11;ec=25]]{{isEmpty()}} + + b = s.length() == 0b0; // Noncompliant [[quickfixes=qf19]] + // fix@qf19 {{Replace with "isEmpty()"}} + // edit@qf19 [[sc=11;ec=26]]{{isEmpty()}} + + b = s.length() == 0B0; // Noncompliant [[quickfixes=qf20]] + // fix@qf20 {{Replace with "isEmpty()"}} + // edit@qf20 [[sc=11;ec=26]]{{isEmpty()}} + + b = 0x0 != s.length(); // Noncompliant [[quickfixes=qf21]] + // fix@qf21 {{Replace with "isEmpty()"}} + // edit@qf21 [[sc=9;ec=16]]{{!}} + // edit@qf21 [[sc=18;ec=26]]{{isEmpty()}} + + b = s.length() == 0x1; + b = s.length() == 01; + b = s.length() == 0b1; + b = s.length() == 1; b = s.length() > 3; b = s.length() <= 10; 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 55cecf9441c..ce334a7395a 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 @@ -39,17 +39,41 @@ /** * Implement rule that 
CharSequence.length() == 0
should be replaced with - * 
CharSequence.isEmpty()
. + * 
CharSequence.isEmpty()
, and 
String.strip().length() == 0
or + * 
String.trim().length() == 0
with 
String.isBlank()
. */ @Rule(key = "S7158") public class StringIsEmptyCheck extends IssuableSubscriptionVisitor implements JavaVersionAwareVisitor { + private static final String JAVA_LANG_STRING = "java.lang.String"; + + // `String.isBlank()` is available since Java 11 + private static final int JAVA_11 = 11; + private static final MethodMatchers STRING_LENGTH_METHOD = MethodMatchers.create() - .ofTypes("java.lang.String") + .ofTypes(JAVA_LANG_STRING) .names("length") .addWithoutParametersMatcher() .build(); + private static final String TRIM_NAME = "trim"; + + private static final MethodMatchers STRING_WHITESPACE_REMOVAL_METHODS = MethodMatchers.create() + .ofTypes(JAVA_LANG_STRING) + .names("strip", TRIM_NAME) + .addWithoutParametersMatcher() + .build(); + + /** + * 
trim()
removes every character up to and including 
U+0020
, while 
isBlank()
relies on + * 
Character.isWhitespace
. They disagree on control characters below 
U+0020
and on Unicode white + * space above it, so replacing one with the other can change behaviour. + */ + private static final String TRIM_CAVEAT = + "Use this if Unicode whitespace should be treated as blank and control characters as non-blank."; + + private static final String TRIM_FIX_CAVEAT = " (Unicode whitespace is treated as blank, control characters as non-blank)"; + private static final MethodMatchers CHARSEQUENCE_LENGTH_METHOD = MethodMatchers.create() .ofSubTypes("java.lang.CharSequence") .names("length") @@ -117,21 +141,36 @@ public void visitNode(Tree tree) { } if (lengthCall != null && isEmptinessCheck(comparisonType)) { + MethodInvocationTree whitespaceRemovalCall = getWhitespaceRemovalCall(lengthCall); + QuickFixHelper .newIssue(context) .forRule(this) .onTree(tree) - .withMessage("Use \"isEmpty()\" to check whether a \"" + ownerName(lengthCall) + "\" is empty or not.") - .withQuickFix(() -> getQuickFix(tree, lengthCall, comparisonType)) + .withMessage(getMessage(lengthCall, whitespaceRemovalCall)) + .withQuickFix(() -> getQuickFix(tree, lengthCall, whitespaceRemovalCall, comparisonType)) .report(); } } - private static JavaQuickFix getQuickFix(Tree comparisonExpression, MethodInvocationTree lengthInvocation, ComparisonType comparisonType) { + private static String getMessage(MethodInvocationTree lengthCall, @Nullable MethodInvocationTree whitespaceRemovalCall) { + String adjective = whitespaceRemovalCall != null ? "blank" : "empty"; + String caveat = isTrimBased(whitespaceRemovalCall) ? (" " + TRIM_CAVEAT) : ""; + + return "Use \"" + replacementMethodName(whitespaceRemovalCall) + "\" to check whether a \"" + + ownerName(lengthCall) + "\" is " + adjective + " or not." + caveat; + } + + private static JavaQuickFix getQuickFix(Tree comparisonExpression, MethodInvocationTree lengthInvocation, + @Nullable MethodInvocationTree whitespaceRemovalCall, ComparisonType comparisonType) { + String replacedMethod = replacementMethodName(whitespaceRemovalCall); + MethodInvocationTree replacedInvocation = whitespaceRemovalCall != null ? whitespaceRemovalCall : lengthInvocation; + String fixDescription = "Replace with \"" + replacedMethod + "\"" + (isTrimBased(whitespaceRemovalCall) ? TRIM_FIX_CAVEAT : ""); + // There are two cases to deal with: // s.length() OP CONST // CONST OP s.length() - JavaQuickFix.Builder builder = JavaQuickFix.newQuickFix("Replace with \"isEmpty()\""); + JavaQuickFix.Builder builder = JavaQuickFix.newQuickFix(fixDescription); // Replace "[CONST OP]" with "!"/"". AnalyzerMessage.TextSpan prefixSpan = AnalyzerMessage.textSpanBetween(comparisonExpression, true, lengthInvocation, false); @@ -140,19 +179,27 @@ private static JavaQuickFix getQuickFix(Tree comparisonExpression, MethodInvocat builder.addTextEdit(JavaTextEdit.replaceTextSpan(prefixSpan, prefixReplacement)); } - // Replace "length() [OP CONST]" with "isEmpty()". - IdentifierTree lengthIdentifier = ExpressionUtils.methodName(lengthInvocation); + // Replace "length() [OP CONST]" with "isEmpty()", or "strip()/trim().length() [OP CONST]" with "isBlank()". + IdentifierTree replacedIdentifier = ExpressionUtils.methodName(replacedInvocation); builder.addTextEdit(JavaTextEdit.replaceBetweenTree( - lengthIdentifier, + replacedIdentifier, true, comparisonExpression, true, - "isEmpty()" + replacedMethod )); return builder.build(); } + private static String replacementMethodName(@Nullable MethodInvocationTree whitespaceRemovalCall) { + return whitespaceRemovalCall != null ? "isBlank()" : "isEmpty()"; + } + + private static boolean isTrimBased(@Nullable MethodInvocationTree whitespaceRemovalCall) { + return whitespaceRemovalCall != null && TRIM_NAME.equals(ExpressionUtils.methodName(whitespaceRemovalCall).name()); + } + private static String ownerName(MethodInvocationTree lengthCall) { if (lengthCall.methodSelect() instanceof MemberSelectExpressionTree sel) { return sel.expression().symbolType().name(); @@ -161,6 +208,21 @@ private static String ownerName(MethodInvocationTree lengthCall) { } } + /** + * @return the 
strip()
or 
trim()
invocation the given 
length()
call is applied to, or + * null when there is none, when it is not directly chained, or when 
String.isBlank()
is not available. + */ + @Nullable + private MethodInvocationTree getWhitespaceRemovalCall(MethodInvocationTree lengthCall) { + if (context.getJavaVersion().asInt() >= JAVA_11 + && lengthCall.methodSelect() instanceof MemberSelectExpressionTree sel + && sel.expression() instanceof MethodInvocationTree mit + && STRING_WHITESPACE_REMOVAL_METHODS.matches(mit)) { + return mit; + } + return null; + } + @Nullable private MethodInvocationTree getLengthCall(ExpressionTree tree) { if (tree instanceof MethodInvocationTree mit) { diff --git a/java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java index cb178a81bff..caf3f89ed31 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java @@ -41,6 +41,34 @@ void string_15() { .verifyIssues(); } + @Test + void blank_11() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/StringIsBlankCheckSample.java")) + .withCheck(new StringIsEmptyCheck()) + .withJavaVersion(11) + .verifyIssues(); + } + + @Test + void blank_11_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/StringIsBlankCheckSample.java")) + .withCheck(new StringIsEmptyCheck()) + .withJavaVersion(11) + .withoutSemantic() + .verifyIssues(); + } + + @Test + void blank_10() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/StringIsBlankCheckSample_java10.java")) + .withCheck(new StringIsEmptyCheck()) + .withJavaVersion(10) + .verifyIssues(); + } + @Test void charSequence_15() { CheckVerifier.newVerifier() diff --git a/java-frontend/src/main/java/org/sonar/java/model/LiteralUtils.java b/java-frontend/src/main/java/org/sonar/java/model/LiteralUtils.java index 289694d61cd..64a151d059f 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/LiteralUtils.java +++ b/java-frontend/src/main/java/org/sonar/java/model/LiteralUtils.java @@ -152,8 +152,19 @@ public static boolean isFalse(Tree tree) { return tree.is(Kind.BOOLEAN_LITERAL) && "false".equals(((LiteralTree) tree).value()); } + /** + * @return true when the expression is an int literal whose value is zero, in any base: + * decimal ({@code 0}), hexadecimal ({@code 0x0}), octal ({@code 00}) or binary ({@code 0b0}). + */ public static boolean isZero(ExpressionTree tree) { - return tree.is(Kind.INT_LITERAL) && "0".equals(((LiteralTree) tree).value()); + if (!tree.is(Kind.INT_LITERAL)) { + return false; + } + try { + return intLiteralValue((LiteralTree) tree) == 0; + } catch (NumberFormatException e) { + return false; + } } public static boolean isOne(ExpressionTree tree) { diff --git a/java-frontend/src/test/java/org/sonar/java/model/LiteralUtilsTest.java b/java-frontend/src/test/java/org/sonar/java/model/LiteralUtilsTest.java index 3b9776a052b..c9195a8e8db 100644 --- a/java-frontend/src/test/java/org/sonar/java/model/LiteralUtilsTest.java +++ b/java-frontend/src/test/java/org/sonar/java/model/LiteralUtilsTest.java @@ -22,6 +22,8 @@ import java.util.Optional; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.CompilationUnitTree; @@ -288,9 +290,10 @@ void isFalse_withExpectedValue_returnsTrue() { assertThat(LiteralUtils.isFalse(falseTree)).isTrue(); } - @Test - void isZero_withExpectedValue() { - ExpressionTree tree = getReturnExpression("int foo(){ return 0; }"); + @ParameterizedTest + @ValueSource(strings = {"0", "00", "0_0", "0x0", "0X0", "0x00", "0b0", "0B0", "0b0_0"}) + void isZero_withExpectedValue(String literal) { + ExpressionTree tree = getReturnExpression("int foo(){ return " + literal + "; }"); assertThat(LiteralUtils.isZero(tree)).isTrue(); } @@ -306,12 +309,11 @@ void isNegOne_withExpectedValue() { assertThat(LiteralUtils.isNegOne(tree)).isTrue(); } - @Test - void isZero_withUnexpectedValues() { - ExpressionTree intTree = getReturnExpression("int foo(){ return 5; }"); - assertThat(LiteralUtils.isZero(intTree)).isFalse(); - ExpressionTree boolTree = getReturnExpression("int foo(){ return true; }"); - assertThat(LiteralUtils.isZero(boolTree)).isFalse(); + @ParameterizedTest + @ValueSource(strings = {"5", "0x1", "01", "0b1", "true", "0xFFFF_FFFF_FFFF_FFFF_F", "0b1111_1111_1111_1111_1111_1111_1111_1111_1"}) + void isZero_withUnexpectedValues(String literal) { + ExpressionTree tree = getReturnExpression("int foo(){ return " + literal + "; }"); + assertThat(LiteralUtils.isZero(tree)).isFalse(); } @Test