Skip to content

SONARJAVA-6876 S2479 Reduce noise by suppressing on strings that are most likely generated - #6061

Open
lijun-chen-sonarsource wants to merge 1 commit into
masterfrom
lc/improve-S2479
Open

SONARJAVA-6876 S2479 Reduce noise by suppressing on strings that are most likely generated#6061
lijun-chen-sonarsource wants to merge 1 commit into
masterfrom
lc/improve-S2479

Conversation

@lijun-chen-sonarsource

@lijun-chen-sonarsource lijun-chen-sonarsource commented Aug 28, 2026

Copy link
Copy Markdown

Part of AT-82


Summary by Gitar

  • New helper:
    • Added GeneratedStringLiteralRecognizer to suppress string literals likely generated in Kotlin annotations
  • Check updates:
    • Updated ControlCharacterInLiteralCheck to ignore recognized generated string literals

This will update automatically on new commits.

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title S2479 Reduce noise by suppressing on strings that are most likely generated SONARJAVA-6876 S2479 Reduce noise by suppressing on strings that are most likely generated Aug 28, 2026
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6876

@sonarqube-next

Copy link
Copy Markdown
Contributor

Quality Gate failed Quality Gate failed

Failed conditions
2 New issues

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE SonarQube for IDE

Comment on lines +52 to +66
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ 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 πŸ‘ / πŸ‘Ž

Comment on lines +32 to +46
@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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ 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 πŸ‘ / πŸ‘Ž

@gitar-bot

gitar-bot Bot commented Aug 28, 2026

Copy link
Copy Markdown
Code Review πŸ‘ Approved with suggestions 0 resolved / 2 findings

Reduces noise in S2479 by adding GeneratedStringLiteralRecognizer to suppress string literals likely generated in Kotlin annotations, with ControlCharacterInLiteralCheck updated to ignore recognized generated literals.

Consider moving the isGenerated check behind the regex match to avoid walking the parent chain for every literal on the hot path. Also add a withoutSemantic() test case and move the shadowing scenario to a file without kotlin.Metadata imports so it validates in both semantic and non-semantic modes; a text-block case would also improve coverage.

πŸ’‘ 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

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())));
  }
}
πŸ’‘ 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 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();
}
πŸ€– Prompt for agents
Code Review: Reduces noise in S2479 by adding `GeneratedStringLiteralRecognizer` to suppress string literals likely generated in Kotlin annotations, with `ControlCharacterInLiteralCheck` updated to ignore recognized generated literals.
  
  Consider moving the `isGenerated` check behind the regex match to avoid walking the parent chain for every literal on the hot path. Also add a `withoutSemantic()` test case and move the shadowing scenario to a file without `kotlin.Metadata` imports so it validates in both semantic and non-semantic modes; a text-block case would also improve coverage.

1. πŸ’‘ Performance: isGenerated runs for every literal before the cheap regex check
   Files: 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

   `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.

   Fix (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())));
     }
   }

2. πŸ’‘ Quality: No withoutSemantic() test; shadowing case only covers semantic path
   Files: 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 `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.

   Fix (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();
   }

Options

Auto-apply is off β†’ Gitar will not commit updates to this branch.
Display: compact β†’ Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with πŸ‘ / πŸ‘Ž | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant