SONARJAVA-6876 S2479 Reduce noise by suppressing on strings that are most likely generated - #6061
SONARJAVA-6876 S2479 Reduce noise by suppressing on strings that are most likely generated#6061lijun-chen-sonarsource wants to merge 1 commit into
Conversation
|
| public static boolean isGenerated(LiteralTree literal) { | ||
| if (!literal.is(Tree.Kind.STRING_LITERAL, Tree.Kind.TEXT_BLOCK)) { | ||
| return false; | ||
| } | ||
|
|
||
| Tree annotationArgument = literal; | ||
| while (annotationArgument.parent() != null && !annotationArgument.parent().is(Tree.Kind.ARGUMENTS)) { | ||
| annotationArgument = annotationArgument.parent(); | ||
| } | ||
|
|
||
| Tree arguments = annotationArgument.parent(); | ||
| if (arguments == null || !(arguments.parent() instanceof AnnotationTree annotation)) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
π‘ Performance: isGenerated runs for every literal before the cheap regex check
isGenerated is invoked as the first statement of visitNode, so for every string/char literal/text block in every analysed file it walks the parent chain up to the compilation-unit root (the while loop only stops on an ARGUMENTS parent, which the vast majority of literals never have), and in the unresolved-type case it additionally runs concatenate plus a full import scan up to three times. The suppression is only needed for the tiny fraction of literals that actually contain a control character, so moving the call behind matcher.find() yields identical behaviour with strictly less work on the hot path of a rule that subscribes to all literals.
Only consult the recognizer when an issue is about to be reported:
@Override
public void visitNode(Tree tree) {
LiteralTree literal = (LiteralTree) tree;
String literalValue = LiteralUtils.getAsStringValue(literal);
Matcher matcher;
if (allowTabsInTextBlocks && tree.is(Tree.Kind.TEXT_BLOCK)) {
matcher = CONTROL_CHARACTERS_WITHOUT_TABS_PATTERN.matcher(literalValue);
} else {
matcher = CONTROL_CHARACTERS_PATTERN.matcher(literalValue);
}
if (matcher.find() && !GeneratedStringLiteralRecognizer.isGenerated(literal)) {
reportIssue(literal, String.format(MESSAGE_FORMAT, literalValue.codePointAt(matcher.start())));
}
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with π / π
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath(TEST_FILE)) | ||
| .withCheck(new TestCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void test_without_dependencies() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath(TEST_FILE)) | ||
| .withCheck(new TestCheck()) | ||
| .withClassPath(List.of()) | ||
| .verifyIssues(); |
There was a problem hiding this comment.
π‘ Quality: No withoutSemantic() test; shadowing case only covers semantic path
The new test class only runs with the full classpath and with withClassPath(List.of()), not with the repo-mandated withoutSemantic(). InternalCheckVerifier.scanFiles skips enableSemanticWithProjectClasspath when withoutSemantic is set, so in that mode even the source-declared nested @interface Metadata has an unknown symbolType; isAnnotationType then falls back to simple-name matching and finds import kotlin.Metadata in the same file, so ShadowedMetadataSample.Annotated (line 38, marked // Compliant) would be classified as generated. The shadowing scenario the sample claims to cover is therefore only validated on the resolved-type path. Add the withoutSemantic() test and put the shadowing case in a file that does not import kotlin.Metadata so it is meaningful in both modes; also add a text-block case, since isGenerated explicitly accepts Tree.Kind.TEXT_BLOCK but no text block appears in the sample.
Add the conventional without-semantic test and move the shadowing case to its own sample file that does not import kotlin.Metadata:
@Test
void test_without_semantic() {
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath(TEST_FILE))
.withCheck(new TestCheck())
.withoutSemantic()
.verifyIssues();
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with π / π
Code Review π Approved with suggestions 0 resolved / 2 findingsReduces noise in S2479 by adding Consider moving the π‘ Performance: isGenerated runs for every literal before the cheap regex checkπ java-checks/src/main/java/org/sonar/java/checks/ControlCharacterInLiteralCheck.java:68-82 π java-checks/src/main/java/org/sonar/java/checks/helpers/GeneratedStringLiteralRecognizer.java:52-66
Only consult the recognizer when an issue is about to be reportedπ‘ Quality: No withoutSemantic() test; shadowing case only covers semantic pathπ java-checks/src/test/java/org/sonar/java/checks/helpers/GeneratedStringLiteralRecognizerTest.java:32-46 π java-checks-test-sources/default/src/main/java/checks/helpers/GeneratedStringLiteralRecognizerSample.java:3 π java-checks-test-sources/default/src/main/java/checks/helpers/GeneratedStringLiteralRecognizerSample.java:32-41 The new test class only runs with the full classpath and with Add the conventional without-semantic test and move the shadowing case to its own sample file that does not import kotlin.Metadataπ€ Prompt for agentsOptionsAuto-apply is off β Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with π / π | Gitar |




Part of AT-82
Summary by Gitar
GeneratedStringLiteralRecognizerto suppress string literals likely generated in Kotlin annotationsControlCharacterInLiteralCheckto ignore recognized generated string literalsThis will update automatically on new commits.