Skip to content

SONARJAVA-6757: Fix FP in S9149 for intentional hiding with deprecation annotations - #5924

Open
romainbrenguier wants to merge 2 commits into
masterfrom
romain/sonarjava-6757
Open

SONARJAVA-6757: Fix FP in S9149 for intentional hiding with deprecation annotations#5924
romainbrenguier wants to merge 2 commits into
masterfrom
romain/sonarjava-6757

Conversation

@romainbrenguier

Copy link
Copy Markdown
Contributor

Summary

  • Skip reporting S9149 when the hiding method is annotated with @Deprecated or @DoNotCall, as these indicate deliberate API design patterns rather than accidental hiding
  • This addresses 24 false positives (54.5% of FPs, 4.6% of total issues across 2 projects), primarily from Guava's ImmutableBiMap, ImmutableSortedSet, and similar classes

Test plan

  • Added compliant test cases for @Deprecated annotated hiding methods
  • Added compliant test cases for @DoNotCall annotated hiding methods
  • Added compliant test case for combined @Deprecated + @DoNotCall
  • Added noncompliant test case confirming unannotated hiding is still flagged
  • All unit tests pass (StaticMethodHidingCheckTest)
  • Ruling ITs may need expected file updates (Guava) since FPs are removed

🤖 Generated with Claude Code

…n annotations

Skip reporting when the hiding method is annotated with @deprecated or
@DonotCall, as these indicate deliberate API design patterns (preventing
misuse, guiding migration, enforcing type constraints).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hashicorp-vault-sonar-prod

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

Copy link
Copy Markdown
Contributor

SONARJAVA-6757

Comment on lines +82 to +85
private static boolean isIntentionalHiding(Symbol.MethodSymbol methodSymbol) {
return methodSymbol.metadata().isAnnotatedWith("java.lang.Deprecated")
|| methodSymbol.metadata().isAnnotatedWith("com.google.errorprone.annotations.DoNotCall");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Edge Case: @deprecated blanket-suppresses S9149, risking false negatives

isIntentionalHiding treats any @deprecated hiding method as intentional. @deprecated is applied for many reasons unrelated to method hiding (e.g. a method scheduled for removal that accidentally hides a parent static method), so this may suppress genuinely accidental hiding that developers still want flagged. This is a reasonable tradeoff to remove the Guava FPs, but consider whether @DonotCall alone (a much more specific signal) would suffice, or document this broadening in the rule metadata so users understand deprecated methods are now exempt.

Was this helpful? React with 👍 / 👎

@github-actions

Copy link
Copy Markdown
Contributor

Ruling needs updating. A fix PR has been created: #5926

Please review and merge it into your branch.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Ruling Diff Summary

Detected changes in 1 rule files: 23 issues removed, 0 issues added.

S9149 (java) on guava - 23 issues removed, 0 issues added

Removed src/com/google/common/collect/ContiguousSet.java (line 193)

       188 |    *
       189 |    * @throws UnsupportedOperationException always
       190 |    * @deprecated Use {@link #create}.
       191 |    */
       192 |   @Deprecated
>>>    193 |   public static <E> ImmutableSortedSet.Builder<E> builder() {
       194 |     throw new UnsupportedOperationException();
       195 |   }
       196 | }

Removed src/com/google/common/collect/ImmutableSortedMapFauxverideShim.java (line 37)

        32 |    * @throws UnsupportedOperationException always
        33 |    * @deprecated Use {@link ImmutableSortedMap#naturalOrder}, which offers
        34 |    *     better type-safety.
        35 |    */
        36 |   @Deprecated
>>>     37 |   public static <K, V> ImmutableSortedMap.Builder<K, V> builder() {
        38 |     throw new UnsupportedOperationException();
        39 |   }
        40 | 
        41 |   /**
        42 |    * Not supported. <b>You are attempting to create a map that may contain a

Removed src/com/google/common/collect/ImmutableSortedMapFauxverideShim.java (line 51)

        46 |    * @throws UnsupportedOperationException always
        47 |    * @deprecated <b>Pass a key of type {@code Comparable} to use {@link
        48 |    *     ImmutableSortedMap#of(Comparable, Object)}.</b>
        49 |    */
        50 |   @Deprecated
>>>     51 |   public static <K, V> ImmutableSortedMap<K, V> of(K k1, V v1) {
        52 |     throw new UnsupportedOperationException();
        53 |   }
        54 | 
        55 |   /**
        56 |    * Not supported. <b>You are attempting to create a map that may contain

Removed src/com/google/common/collect/ImmutableSortedMapFauxverideShim.java (line 65)

        60 |    * @throws UnsupportedOperationException always
        61 |    * @deprecated <b>Pass keys of type {@code Comparable} to use {@link
        62 |    *     ImmutableSortedMap#of(Comparable, Object, Comparable, Object)}.</b>
        63 |    */
        64 |   @Deprecated
>>>     65 |   public static <K, V> ImmutableSortedMap<K, V> of(K k1, V v1, K k2, V v2) {
        66 |     throw new UnsupportedOperationException();
        67 |   }
        68 | 
        69 |   /**
        70 |    * Not supported. <b>You are attempting to create a map that may contain

Removed src/com/google/common/collect/ImmutableSortedMultisetFauxverideShim.java (line 44)

        39 |    *
        40 |    * @throws UnsupportedOperationException always
        41 |    * @deprecated Use {@link ImmutableSortedMultiset#naturalOrder}, which offers better type-safety.
        42 |    */
        43 |   @Deprecated
>>>     44 |   public static <E> ImmutableSortedMultiset.Builder<E> builder() {
        45 |     throw new UnsupportedOperationException();
        46 |   }
        47 | 
        48 |   /**
        49 |    * Not supported. <b>You are attempting to create a multiset that may contain a non-{@code

Removed src/com/google/common/collect/ImmutableSortedMultisetFauxverideShim.java (line 58)

        53 |    * @throws UnsupportedOperationException always
        54 |    * @deprecated <b>Pass a parameter of type {@code Comparable} to use
        55 |    *             {@link ImmutableSortedMultiset#of(Comparable)}.</b>
        56 |    */
        57 |   @Deprecated
>>>     58 |   public static <E> ImmutableSortedMultiset<E> of(E element) {
        59 |     throw new UnsupportedOperationException();
        60 |   }
        61 | 
        62 |   /**
        63 |    * Not supported. <b>You are attempting to create a multiset that may contain a non-{@code

Removed src/com/google/common/collect/ImmutableSortedMultisetFauxverideShim.java (line 72)

        67 |    * @throws UnsupportedOperationException always
        68 |    * @deprecated <b>Pass the parameters of type {@code Comparable} to use
        69 |    *             {@link ImmutableSortedMultiset#of(Comparable, Comparable)}.</b>
        70 |    */
        71 |   @Deprecated
>>>     72 |   public static <E> ImmutableSortedMultiset<E> of(E e1, E e2) {
        73 |     throw new UnsupportedOperationException();
        74 |   }
        75 | 
        76 |   /**
        77 |    * Not supported. <b>You are attempting to create a multiset that may contain a non-{@code

Removed src/com/google/common/collect/ImmutableSortedSetFauxverideShim.java (line 46)

        41 |    * @throws UnsupportedOperationException always
        42 |    * @deprecated Use {@link ImmutableSortedSet#naturalOrder}, which offers
        43 |    *     better type-safety.
        44 |    */
        45 |   @Deprecated
>>>     46 |   public static <E> ImmutableSortedSet.Builder<E> builder() {
        47 |     throw new UnsupportedOperationException();
        48 |   }
        49 | 
        50 |   /**
        51 |    * Not supported. <b>You are attempting to create a set that may contain a

Removed src/com/google/common/collect/ImmutableSortedSetFauxverideShim.java (line 60)

        55 |    * @throws UnsupportedOperationException always
        56 |    * @deprecated <b>Pass a parameter of type {@code Comparable} to use {@link
        57 |    *     ImmutableSortedSet#of(Comparable)}.</b>
        58 |    */
        59 |   @Deprecated
>>>     60 |   public static <E> ImmutableSortedSet<E> of(E element) {
        61 |     throw new UnsupportedOperationException();
        62 |   }
        63 | 
        64 |   /**
        65 |    * Not supported. <b>You are attempting to create a set that may contain a

Removed src/com/google/common/collect/ImmutableSortedSetFauxverideShim.java (line 74)

        69 |    * @throws UnsupportedOperationException always
        70 |    * @deprecated <b>Pass the parameters of type {@code Comparable} to use {@link
        71 |    *     ImmutableSortedSet#of(Comparable, Comparable)}.</b>
        72 |    */
        73 |   @Deprecated
>>>     74 |   public static <E> ImmutableSortedSet<E> of(E e1, E e2) {
        75 |     throw new UnsupportedOperationException();
        76 |   }
        77 | 
        78 |   /**
        79 |    * Not supported. <b>You are attempting to create a set that may contain a

@gitar-bot

gitar-bot Bot commented Aug 14, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Updates S9149 to skip reporting intentional static method hiding when annotated with @Deprecated or @DoNotCall. Consider refining the @Deprecated check to ensure it doesn't mask true false negatives.

💡 Edge Case: @Deprecated blanket-suppresses S9149, risking false negatives

📄 java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java:82-85

isIntentionalHiding treats any @Deprecated hiding method as intentional. @Deprecated is applied for many reasons unrelated to method hiding (e.g. a method scheduled for removal that accidentally hides a parent static method), so this may suppress genuinely accidental hiding that developers still want flagged. This is a reasonable tradeoff to remove the Guava FPs, but consider whether @DoNotCall alone (a much more specific signal) would suffice, or document this broadening in the rule metadata so users understand deprecated methods are now exempt.

🤖 Prompt for agents
Code Review: Updates S9149 to skip reporting intentional static method hiding when annotated with @Deprecated or @DoNotCall. Consider refining the @Deprecated check to ensure it doesn't mask true false negatives.

1. 💡 Edge Case: @Deprecated blanket-suppresses S9149, risking false negatives
   Files: java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java:82-85

   isIntentionalHiding treats any @Deprecated hiding method as intentional. @Deprecated is applied for many reasons unrelated to method hiding (e.g. a method scheduled for removal that accidentally hides a parent static method), so this may suppress genuinely accidental hiding that developers still want flagged. This is a reasonable tradeoff to remove the Guava FPs, but consider whether @DoNotCall alone (a much more specific signal) would suffice, or document this broadening in the rule metadata so users understand deprecated methods are now exempt.

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

@sonarqube-next

Copy link
Copy Markdown

@romainbrenguier
romainbrenguier marked this pull request as ready for review August 14, 2026 08:03
}

private static boolean isIntentionalHiding(Symbol.MethodSymbol methodSymbol) {
return methodSymbol.metadata().isAnnotatedWith("java.lang.Deprecated")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It could be worth considering additional common decorators with similar meaning:

  • com.google.errorprone.annotations.InlineMe
  • org.jetbrains.annotations.ApiStatus.Obsolete
  • org.jetbrains.annotations.ApiStatus.ScheduledForRemoval
  • kotlin.Deprecated

Also, have you considered updating the RSPEC to document these exceptions?

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.

2 participants