From 8333a7d757e0fd434c6fc93ef941aaa97e2903e7 Mon Sep 17 00:00:00 2001 From: Paul O'Reilly Date: Fri, 20 Sep 2024 09:47:11 +0100 Subject: [PATCH] SONARJAVA-5111 S5838 Improve quickfix directly to isEmpty() --- .../resources/autoscan/diffs/diff_S5838.json | 4 +- .../AssertJChainSimplificationCheckTest.java | 6 +-- ...nSimplificationCheckTest_ListQuickFix.java | 46 +++++++++++++++++++ .../AssertJChainSimplificationIndex.java | 27 ++++++++++- .../AssertJChainSimplificationCheckTest.java | 9 ++++ 5 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest_ListQuickFix.java diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5838.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5838.json index 1be71c6cf41..f4da0ff8366 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5838.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5838.json @@ -1,6 +1,6 @@ { "ruleKey": "S5838", "hasTruePositives": false, - "falseNegatives": 274, + "falseNegatives": 280, "falsePositives": 0 -} \ No newline at end of file +} diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest.java index 8e4482bd0fc..2ccb04cf725 100644 --- a/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest.java +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest.java @@ -232,7 +232,7 @@ void stringRelatedAssertionChains() { assertThat(getString().trim()).isNotEmpty(); // Noncompliant {{Use assertThat(actual).isNotBlank() instead.}} assertThat(getString().trim()).isNotEqualTo(""); // Noncompliant {{Use assertThat(actual).isNotBlank() instead.}} - assertThat(getString().length()).isEqualTo(0); // Noncompliant {{Use isZero() instead.}} + assertThat(getString().length()).isEqualTo(0); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} assertThat(getString().length()).isEqualTo(12); // Noncompliant {{Use assertThat(actual).hasSize(expected) instead.}} assertThat(getString().length()).isEqualTo(x.length()); // Noncompliant {{Use assertThat(actual).hasSameSizeAs(expected) instead.}} assertThat(getString().length()).isEqualTo(getArray().length); // Noncompliant {{Use assertThat(actual).hasSize(expected) instead.}} @@ -258,7 +258,7 @@ void arrayRelatedAssertionChains() { int i = 12; MyClassWithLength myClassWithLength = new MyClassWithLength(); - assertThat(getArray().length).isEqualTo(0); // Noncompliant {{Use isZero() instead.}} + assertThat(getArray().length).isEqualTo(0); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} assertThat(getArray().length).isZero(); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} assertThat(getArray().length).isEqualTo(i); // Noncompliant {{Use assertThat(actual).hasSize(expected) instead.}} assertThat(getArray().length).isPositive(); // Noncompliant {{Use assertThat(actual).isNotEmpty() instead.}} @@ -380,7 +380,7 @@ void fileRelatedAssertionChains() { assertThat(getFile().listFiles()).isNotEmpty(); // Noncompliant {{Use assertThat(actual).isNotEmptyDirectory() instead.}} // We report only step by step, not the final transformation possible - assertThat(getFile().list().length).isEqualTo(0); // Noncompliant {{Use isZero() instead.}} + assertThat(getFile().list().length).isEqualTo(0); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} assertThat(getFile().list().length).isZero(); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} assertThat(getFile().list()).isEmpty(); // Noncompliant {{Use assertThat(actual).isEmptyDirectory() instead.}} assertThat(getFile()).isEmptyDirectory(); // Compliant, 3 iterations to reach a nice assertion diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest_ListQuickFix.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest_ListQuickFix.java new file mode 100644 index 00000000000..e0119f4d7d1 --- /dev/null +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertJChainSimplificationCheckTest_ListQuickFix.java @@ -0,0 +1,46 @@ +package checks.tests; + +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +public class AssertJChainSimplificationCheckTest_ListQuickFix { + + List stringsList = new ArrayList<>(); + String[] stringArray = new String[0]; + String string = new String(); + + // test https://sonarsource.atlassian.net/browse/SONARJAVA-5111 + void contextFreeQuickFixes() { + + assertThat(stringsList).isEmpty(); // Compliant + assertThat(stringsList.hashCode()).isEqualTo(0); // Noncompliant {{Use isZero() instead.}} + + assertThat(stringsList.size()).isEqualTo(0); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} [[quickfixes=qf_context1]] +// ^^^^^^^^^ + // fix@qf_context1 {{Use "assertThat(actual).isEmpty()"}} + // edit@qf_context1 [[sc=27;ec=34]] {{}} + // edit@qf_context1 [[sc=36;ec=48]] {{isEmpty()}} + + assertThat(string).isEmpty(); // Compliant + assertThat(string.hashCode()).isEqualTo(0); // Noncompliant {{Use isZero() instead.}} + + assertThat(string.length()).isEqualTo(0); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} [[quickfixes=qf_context2]] +// ^^^^^^^^^ + // fix@qf_context2 {{Use "assertThat(actual).isEmpty()"}} + // edit@qf_context2 [[sc=22;ec=31]] {{}} + // edit@qf_context2 [[sc=33;ec=45]] {{isEmpty()}} + + assertThat(stringArray).isEmpty(); // Compliant + assertThat(stringArray.hashCode()).isEqualTo(0); // Noncompliant {{Use isZero() instead.}} + + assertThat(stringArray.length).isEqualTo(0); // Noncompliant {{Use assertThat(actual).isEmpty() instead.}} [[quickfixes=qf_context3]] +// ^^^^^^^^^ + // fix@qf_context3 {{Use "assertThat(actual).isEmpty()"}} + // edit@qf_context3 [[sc=27;ec=34]] {{}} + // edit@qf_context3 [[sc=36;ec=48]] {{isEmpty()}} + + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/AssertJChainSimplificationIndex.java b/java-checks/src/main/java/org/sonar/java/checks/tests/AssertJChainSimplificationIndex.java index 57f89289d73..d8a54cf4f91 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/tests/AssertJChainSimplificationIndex.java +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/AssertJChainSimplificationIndex.java @@ -147,6 +147,9 @@ private AssertJChainSimplificationIndex() { withSubjectArgumentCondition(LiteralUtils::isTrue, AssertJChainSimplificationIndex::isNotObject, "isTrue()"), withSubjectArgumentCondition(LiteralUtils::isFalse, AssertJChainSimplificationIndex::isNotObject, "isFalse()"), withSubjectArgumentCondition(LiteralUtils::isEmptyString, AssertJChainSimplificationIndex::isNotObject, "isEmpty()"), + withSubjectArgumentCondition(AssertJChainSimplificationIndex::isZeroIntOrLong, AssertJChainSimplificationIndex::isStringLength, msgWithActual(IS_EMPTY)), + withSubjectArgumentCondition(AssertJChainSimplificationIndex::isZeroIntOrLong, AssertJChainSimplificationIndex::isCollectionSize, msgWithActual(IS_EMPTY)), + withSubjectArgumentCondition(AssertJChainSimplificationIndex::isZeroIntOrLong, AssertJChainSimplificationIndex::isArrayLength, msgWithActual(IS_EMPTY)), withSubjectArgumentCondition(AssertJChainSimplificationIndex::isZeroIntOrLong, AssertJChainSimplificationIndex::isNotObject, "isZero()"), methodCallInSubject(Matchers.TO_STRING, msgWithActualCustom("hasToString", "expectedString")), methodCallInSubject(predicateArg -> hasMethodCallAsArg(predicateArg, Matchers.HASH_CODE), Matchers.HASH_CODE, msgWithActualExpected("hasSameHashCodeAs")), @@ -435,7 +438,29 @@ public static boolean isNegOneIntOrLong(ExpressionTree tree) { private static boolean isArrayLength(ExpressionTree expression) { if (expression.is(Tree.Kind.MEMBER_SELECT)) { MemberSelectExpressionTree memberSelectExpressionTree = (MemberSelectExpressionTree) expression; - return memberSelectExpressionTree.expression().symbolType().isArray() && LENGTH.equals(memberSelectExpressionTree.identifier().name()); + if (memberSelectExpressionTree.expression().symbolType().isArray()) { + return LENGTH.equals(memberSelectExpressionTree.identifier().name()); + } + } + return false; + } + + private static boolean isCollectionSize(ExpressionTree expression) { + if (expression.is(Tree.Kind.METHOD_INVOCATION)) { + MethodInvocationTree invocation = (MethodInvocationTree) expression; + if (invocation.methodSelect().is(Tree.Kind.MEMBER_SELECT)) { + return Matchers.COLLECTION_SIZE.matches(invocation); + } + } + return false; + } + + private static boolean isStringLength(ExpressionTree expression) { + if (expression.is(Tree.Kind.METHOD_INVOCATION)) { + MethodInvocationTree invocation = (MethodInvocationTree) expression; + if (invocation.methodSelect().is(Tree.Kind.MEMBER_SELECT)) { + return Matchers.STRING_LENGTH.matches(invocation); + } } return false; } diff --git a/java-checks/src/test/java/org/sonar/java/checks/tests/AssertJChainSimplificationCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/tests/AssertJChainSimplificationCheckTest.java index eeae119356c..76243b6273f 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/tests/AssertJChainSimplificationCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/tests/AssertJChainSimplificationCheckTest.java @@ -49,4 +49,13 @@ void test_quick_fixes() { .withCheck(new AssertJChainSimplificationCheck()) .verifyIssues(); } + + @Test + void test_list_quick_fixes() { + CheckVerifier.newVerifier() + .onFile(testCodeSourcesPath("checks/tests/AssertJChainSimplificationCheckTest_ListQuickFix.java")) + .withCheck(new AssertJChainSimplificationCheck()) + .verifyIssues(); + } + }