SONARJAVA-5622 extend S7158 to work with all CharSequence not just Srings #5196
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates rule S7158 to flag CharSequence.length() == 0 (and related comparisons) and suggest isEmpty(), not just for String but for all CharSequence starting in Java 15.
- Refactors
StringIsEmptyCheckto includeCharSequencematchers and version gating. - Adds a new test case targeting
CharSequencescenarios under Java 15. - Supplies a dedicated sample file with noncompliant and quickfix annotations for various
CharSequenceimplementations.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java | Added charSequence() test method to verify rules under Java 15 |
| java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java | Extended check to handle CharSequence, added version guard |
| java-checks-test-sources/default/src/main/java/checks/StringIsEmptyCheckSample.java | Introduced a charSequence method in the default sample |
| java-checks-test-sources/default/src/main/java/checks/CharSequenceIsEmptyCheckSample.java | New sample covering various CharSequence subtypes |
Comments suppressed due to low confidence (3)
java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java:35
- [nitpick] Consider renaming this test method to
testCharSequencefor consistency with other test names liketestOlderJavaVersionandtest.
void charSequence() {
java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java:89
- [nitpick] Update the Javadoc or inline comment here to explicitly mention that
CharSequence.isEmpty()is only available since Java 15, whileString.isEmpty()is available since Java 6.
@Override
public boolean isCompatibleWithJavaVersion(JavaVersion version) {
java-checks/src/test/java/org/sonar/java/checks/StringIsEmptyCheckTest.java:39
- Consider adding tests for Java versions 6 through 14 to confirm that
CharSequence.length()checks remain compliant (no false positives) on versions before Java 15.
.withJavaVersion(15)
| } | ||
|
|
||
| @Test | ||
| void charSequence() { |
There was a problem hiding this comment.
Let's also test on Java 8, because it's still popular and CharSequence doesn't have isEmpty.
java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java
Outdated
Show resolved
Hide resolved
java-checks/src/main/java/org/sonar/java/checks/StringIsEmptyCheck.java
Outdated
Show resolved
Hide resolved
separate String tests samples from other charSequence tests samples and test them for each java version
erwan-serandour
force-pushed
the
erwan/SONARJAVA-5622
branch
from
6d2dc02 to
f2accef
Compare
| import java.nio.CharBuffer; | ||
|
|
||
| public class CharSequenceIsEmptyCheckSample { | ||
|
|
There was a problem hiding this comment.
There are multiple occurrences of multiple blank lines in this file? Is this intentional?
| public boolean testStringBuilder(StringBuilder sb1, StringBuilder sb2) { | ||
| boolean b; | ||
|
|
||
| b = sb1.length() == 0; // Noncompliant {{Use "isEmpty()" to check whether a "AbstractStringBuilder" is empty or not.}} |
There was a problem hiding this comment.
This is surprising. Why is this an AbstractStringBuilder and not a StringBuilder?
There was a problem hiding this comment.
StringBuilder does not override the method, so length is implemented inside AbstractStringBuilder. I have an idea how to solve it.
| .forRule(this) | ||
| .onTree(tree) | ||
| .withMessage("Use \"isEmpty()\" to check whether a \"CharSequence\" is empty or not.") | ||
| .withMessage("Use \"isEmpty()\" to check whether a \""+lengthCall.methodSymbol().owner().name()+"\" is empty or not.") |
There was a problem hiding this comment.
Spaces around operators.
|
|
||
| @Test | ||
| void test() { | ||
| void stringBuilder() { |
There was a problem hiding this comment.
Shouldn't this be just "string"? Same for stringBuilder_15 case.
There was a problem hiding this comment.
yes, indeed. good catch
|
2 participants
SONARJAVA-5622